Details
- Reviewers
edgar emilio gstoll smaug - Group Reviewers
dom-core win-reviewers - Commits
- rFIREFOXAUTOLAND8bf1985cc5d1: Bug 1858562 - Part 3: Platform specific Document Picture-in-Picture adjustments.
- Bugzilla Bug ID
- 1858562
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
Event Timeline
Code analysis found 5 defects in diff 1123845:
- 1 defect found by eslint (Mozlint)
- 4 defects found by wptlint-gecko (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1123845.
Code analysis found 5 defects in diff 1125413:
- 1 defect found by eslint (Mozlint)
- 4 defects found by wptlint-gecko (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1125413.
Code analysis found 5 defects in diff 1126815:
- 1 defect found by eslint (Mozlint)
- 4 defects found by wptlint-gecko (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1126815.
| widget/gtk/nsWindow.cpp | ||
|---|---|---|
| 5084 | if (mPIPWindow && aEvent->button == 3) { probably. | |
| 6431 | Can we add some helpers in nsIWidget to deal with these consistently maybe? In the context of this patch, it's easy to see that mAlwaysOnTop && TopLevel means document pip, but I feel it'd be clearer with an bool IsDocumentPip() const { return mAlwaysOnTop && mWindowType == WindowType::Toplevel; }. Then this would be if (mAlwaysOnTop && !IsDocumentPip()) { or so. | |
I guess browser_ignore_document_pip.js could have been in a separate patch.
| browser/components/sessionstore/test/browser_ignore_document_pip.js | ||
|---|---|---|
| 25 ↗ | (On Diff #1126815) | What is this commented out line? |
| browser/components/sessionstore/test/browser_ignore_document_pip.js | ||
|---|---|---|
| 25 ↗ | (On Diff #1126815) | I saw that tests usually flush the browser, but doing that for PiP raised some error. I'm a bit concerned the test might not actually ensure that the PiP doesn't end up in session store. Maybe someone a sessionstore reviewer knows, otherwise I'll look around the code a bit more. |
| widget/gtk/nsWindow.cpp | ||
| 5084 | Thanks, didn't notice that member! | |
| 6431 | Would it make sense to let widget know directly that this is Document Picture-in-Picture? It feels a bit fragile to rely on TopLevel. I'm unfamiliar with widget or fronted code, maybe it would help to have a nsIWebBrowserChrome flag for this that is propagated to widgetInitData here and to AppWindow here? For now I added mDocumentPIP by replicating the way mIsPIPWindow is set. | |
Code analysis found 3 defects in diff 1128513:
- 3 defects found by wptlint-gecko (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
The analysis task source-test-mozlint-eslint failed, but we could not detect any defect.
Please check this task manually.
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1128513.
| browser/components/sessionstore/test/browser_ignore_document_pip.js | ||
|---|---|---|
| 25 ↗ | (On Diff #1126815) | I tried to pull down your stack but it seemed busted to me.
Seems to be a problem with pulling down D265289 specifically. I'd recommend running
I haven't really done any reading about this feature, but it sounds like it's supposed to open a specialized window. I think it makes sense not to want to persist that special window in a session because the parent tab does not necessarily get loaded automatically on session restore. If you flush all of the windows and get session state and all you have in session state is the parent window, then I'd be satisfied. If this patch stack gets to a point where it can be pulled down cleanly, I can do some manual testing if you'd like additional support. |
| browser/components/sessionstore/test/browser_ignore_document_pip.js | ||
|---|---|---|
| 25 ↗ | (On Diff #1126815) |
Perhaps you included the child of D265289? That one is filed as sec-sensitive, I CC'ed you.
Document PiP is basically a specialized popup. But thanks for the recommendation, the test fails now. I did suspect the PiP needs to be filtered out in the sessionstore code. I'll likely need to add some identifier to that window, also for other browser code. |
I split the changes to browser code into a separate patch as they became a bit more. I'm sorry that this means the test browser/components/sessionstore/test/browser_ignore_document_pip.js moved to a different part.
| widget/gtk/nsWindow.cpp | ||
|---|---|---|
| 6431 | Yes, it appeared quite helpful to have AppWindow propagate the information about being a document PiP further. I didn't add a flag for this, but relying on the document PiP specific flag combination seems ok for now. | |
Code analysis found 3 defects in diff 1132516:
- 3 defects found by wptlint-gecko (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1132516.
| widget/gtk/nsWindow.cpp | ||
|---|---|---|
| 5082 | Could we please rename mIsPIPWindow to mIsMediaPIPWindow or something. Otherwise it is very confusing whether it is true when mDocumentPIP is true. | |
Looking at this, I'm considering replacing mPIPWindow with an enum in a separate patch
| widget/gtk/nsWindow.cpp | ||
|---|---|---|
| 6430–6431 | Try is looking mostly green, except that the document PiP window doesn't receive focus on windows yet, so something is wrong in this file. | |
| widget/InitData.h | ||
|---|---|---|
| 103 | I still think there should be some enum for window type. It is confusing to have mMediaPiPWindow and mDocumentPiPWindow | |