Popups with remote browsers currently do not render correctly on any platform. Since support for composited popups is marginal, at best, at the moment, we only want to enable the features we need for these popups when they actually contain remote content. This bug adds an initial stopgap implementation, primarily for testing purposes, that force enables compositing for remote popups, despite the UI quirks it causes.

Comment on attachment 8868666 [details] Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X. https://reviewboard.mozilla.org/r/140258/#review143604 ::: widget/cocoa/nsCocoaWindow.mm:481 (Diff revision 1) > [mWindow setLevel:kCGDesktopWindowLevelKey]; > } > > if (mWindowType == eWindowType_popup) { > SetPopupWindowLevel(); > - [mWindow setHasShadow:YES]; > + [mWindow setHasShadow:mDropShadow]; I think this is going to be overwritten almost immediately by nsCocoaWindow::SetWindowShadowStyle. I'd prefer to keep ignoring the initData->mDropShadow field and only respect the -moz-window-shadow property. Does this change actually make a difference? I'd expect the window to not have a shadow either way, due to the accelerated content.

Comment on attachment 8868662 [details] Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten https://reviewboard.mozilla.org/r/140250/#review143606 ::: layout/xul/nsMenuPopupFrame.cpp:324 (Diff revision 1) > nsIAtom *tag = nullptr; > if (parentContent && parentContent->IsXULElement()) > tag = parentContent->NodeInfo()->NameAtom(); > widgetData.mHasRemoteContent = remote; > widgetData.mSupportTranslucency = mode == eTransparencyTransparent; > - widgetData.mDropShadow = !(mode == eTransparencyTransparent || tag == nsGkAtoms::menulist); > + widgetData.mDropShadow = !(remote || mode == eTransparencyTransparent || tag == nsGkAtoms::menulist); Does this change make a difference on Windows?

Comment on attachment 8868666 [details] Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X. https://reviewboard.mozilla.org/r/140258/#review143604 > I think this is going to be overwritten almost immediately by nsCocoaWindow::SetWindowShadowStyle. I'd prefer to keep ignoring the initData->mDropShadow field and only respect the -moz-window-shadow property. > > Does this change actually make a difference? I'd expect the window to not have a shadow either way, due to the accelerated content. The window definitely has a drop shadow without this change. I'm still waiting for a build to finish so I can test with it. The MacBook that I use for testing OS-X is extremely slow.

Comment on attachment 8868662 [details] Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten https://reviewboard.mozilla.org/r/140250/#review143606 > Does this change make a difference on Windows? I believe so, but I'm not certain. I don't have a Windows machine to test on at the moment (still waiting for an order), but someone else tested for me: https://usercontent.irccloud-cdn.com/file/Z8XSLfqA/jk.png That build was with transparency disabled on all platforms, so there may be a drop shadow hidden behind the opaque areas. I'm not sure what the results are with it enabled, but either way they should be good enough for testing. This won't be enabled by default until good UI support.

Comment on attachment 8868663 [details] Bug 1365660: Part 4 - Only enable APZ for popups which contain remote content. https://reviewboard.mozilla.org/r/140252/#review143618

Comment on attachment 8868661 [details] Bug 1365660: Part 2 - Add a HasRemoteContent flag to popup widgets that contain remote browsers. .schouten https://reviewboard.mozilla.org/r/140248/#review143620

Comment on attachment 8868666 [details] Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X. https://reviewboard.mozilla.org/r/140258/#review143604 > The window definitely has a drop shadow without this change. I'm still waiting for a build to finish so I can test with it. The MacBook that I use for testing OS-X is extremely slow. You're right, it doesn't make a difference. So I guess we should just remove the mDropShadow properly entirely (since it seems to actually be overridden by CSS on all platforms), and just disable drop shadows for composited popups until we get them working properly.

I would prefer that, yes.

Comment on attachment 8868660 [details] Bug 1365660: Part 1 - Add a remote="true" attribute to popups with remote browsers. https://reviewboard.mozilla.org/r/140246/#review143668

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #11) > Comment on attachment 8868662 [details] > Bug 1365660: Part 3 - Disable transparency (where necessary) and drop > shadows for popups which contain remote content. .schouten > > https://reviewboard.mozilla.org/r/140250/#review143606 > > > Does this change make a difference on Windows? > > I believe so, but I'm not certain. I don't have a Windows machine to test on > at the moment (still waiting for an order), but someone else tested for me: > > https://usercontent.irccloud-cdn.com/file/Z8XSLfqA/jk.png > > That build was with transparency disabled on all platforms, so there may be > a drop shadow hidden behind the opaque areas. I'm not sure what the results > are with it enabled, but either way they should be good enough for testing. > This won't be enabled by default until good UI support. Zombie tested again with the latest build that didn't disable transparently, and apparently drop shadows are not disabled, but the popup appears to display without any visual issues: https://usercontent.irccloud-cdn.com/file/LdAV2FgG/Untitled3.png

Comment on attachment 8868662 [details] Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten https://reviewboard.mozilla.org/r/140250/#review144236 ::: commit-message-400c8:3 (Diff revision 2) > +Drop shadows are currently not handled correctly for animated, composited > +popups on Windows and OS-X. The shadows are rendered, but are not correctly > +updated to match the final popup size. > + > +Ideally, we should support drop shadows for these popups correctly, but in the > +interim, disabling them gives better results. This part of the commit message seems no longer relevant. ::: layout/xul/nsMenuPopupFrame.cpp:308 (Diff revision 2) > } > > bool remote = HasRemoteContent(); > > nsTransparencyMode mode = nsLayoutUtils::GetFrameTransparency(this, this); > +#ifdef MOZ_WIDGET_GTK Please annotate this with a bug number for a bug that tracks fixing this properly.

Comment on attachment 8868666 [details] Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X. https://reviewboard.mozilla.org/r/140258/#review144238 ::: widget/cocoa/nsChildView.mm:1428 (Diff revision 2) > + if (HasRemoteContent()) > + return true; Please add braces. I know this file isn't very consistent with respect to that rule, but we should follow the style guide on new code. ::: widget/cocoa/nsCocoaWindow.h:364 (Diff revision 2) > > nsresult CreateNativeWindow(const NSRect &aRect, > nsBorderStyle aBorderStyle, > bool aRectIsFrameRect); > - nsresult CreatePopupContentView(const LayoutDeviceIntRect &aRect); > + nsresult CreatePopupContentView(const LayoutDeviceIntRect &aRect, > + nsWidgetInitData* aInitData); Is this change still needed?

Comment on attachment 8868666 [details] Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X. https://reviewboard.mozilla.org/r/140258/#review144238 > Is this change still needed? Yes, so that we can propagate the mHasRemoteContent flag to the content view, where it's actually checked.

Comment on attachment 8868666 [details] Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X. https://reviewboard.mozilla.org/r/140258/#review144238 > Yes, so that we can propagate the mHasRemoteContent flag to the content view, where it's actually checked. Ah, makes sense.

Comment on attachment 8868661 [details] Bug 1365660: Part 2 - Add a HasRemoteContent flag to popup widgets that contain remote browsers. .schouten https://reviewboard.mozilla.org/r/140248/#review144314

Comment on attachment 8868662 [details] Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten https://reviewboard.mozilla.org/r/140250/#review144316 I agree with Markus.

Comment on attachment 8868665 [details] Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. .schouten https://reviewboard.mozilla.org/r/140256/#review144318 ::: widget/windows/nsWindow.cpp:7172 (Diff revision 2) > nsWindow::ShouldUseOffMainThreadCompositing() > { > // We don't currently support using an accelerated layer manager with > // transparent windows so don't even try. I'm also not sure if we even > // want to support this case. See bug 593471 > - if (mTransparencyMode == eTransparencyTransparent) { > + if (!HasRemoteContent() && mTransparencyMode == eTransparencyTransparent) { This causes assertions right now. See my and Jimm's comments in bug 1356317.

(In reply to Bas Schouten (:bas.schouten) from comment #29) > Comment on attachment 8868665 [details] > Bug 1365660: Part 5b - Enable compositing for popups with remote content on > Windows. .schouten > > https://reviewboard.mozilla.org/r/140256/#review144318 > > ::: widget/windows/nsWindow.cpp:7172 > (Diff revision 2) > > nsWindow::ShouldUseOffMainThreadCompositing() > > { > > // We don't currently support using an accelerated layer manager with > > // transparent windows so don't even try. I'm also not sure if we even > > // want to support this case. See bug 593471 > > - if (mTransparencyMode == eTransparencyTransparent) { > > + if (!HasRemoteContent() && mTransparencyMode == eTransparencyTransparent) { > > This causes assertions right now. See my and Jimm's comments in bug 1356317. I saw, thanks. We'll leave OOP extensions disabled by default on Windows until that bug is fixed.

Comment on attachment 8868664 [details] Bug 1365660: Part 5a - Enable compositing for popups with remote content on Linux. It's been a while since I last touched this code, redirecting to karlt.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e53d393c722f451c0e8dca642dbb1ace5331f6f Bug 1365660: Part 1 - Add a remote="true" attribute to popups with remote browsers. r=mixedpuppy https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe30fc6158c4dcb6c9fe2bc9bff35674ed9b247 Bug 1365660: Part 2 - Add a HasRemoteContent flag to popup widgets that contain remote browsers. r=kats,bas https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4b09616e235659ac4b6570c99f5a771d2d9206 Bug 1365660: Part 3 - Disable transparency (where necessary) for popups which contain remote content. r=mstange,bas https://hg.mozilla.org/integration/mozilla-inbound/rev/6b38e46d1a8b5cde3885c753919c36115ba9439c Bug 1365660: Part 4 - Only enable APZ for popups which contain remote content. r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/38f3bef8b8651935cb3d9eac0ff5d93da87bee3d Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X. r=mstange

https://hg.mozilla.org/mozilla-central/rev/5e53d393c722 https://hg.mozilla.org/mozilla-central/rev/cfe30fc6158c https://hg.mozilla.org/mozilla-central/rev/cb4b09616e23 https://hg.mozilla.org/mozilla-central/rev/6b38e46d1a8b https://hg.mozilla.org/mozilla-central/rev/38f3bef8b865

Bas, is there a reason that we can't land the Windows portion of this before bug 1356317 is fixed, as long as remote popups are disabled behind a hidden pref? It would be really helpful for QA, since they want to get as much testing done for this as possible, as soon as they can.

Comment on attachment 8868662 [details] Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten https://reviewboard.mozilla.org/r/140250/#review144236 > Please annotate this with a bug number for a bug that tracks fixing this properly. This was annotated with bug 630346, which is closed: https://hg.mozilla.org/mozilla-central/rev/cb4b09616e235659ac4b6570c99f5a771d2d9206#l1.18 The review asked for a bug that tracks fixing this. Note also that bug 630346 was about GL layers. OMTC on Linux does not use GL layers without about:config changes. Since changes for bug 408284, which was fixed after bug 630346, shaping is necessary for translucency only when the window manager does not composite windows. I don't know whether or not there are good reasons to avoid OMTC for translucency when there is a compositing window manager. I'm not clear whether the black you see is merely due to skipping the shaping code required with non-compositing window manager, or whether there are additional problems with the OMTC compositor in Gecko.

Comment on attachment 8868664 [details] Bug 1365660: Part 5a - Enable compositing for popups with remote content on Linux. https://reviewboard.mozilla.org/r/140254/#review145768 ::: widget/gtk/nsWindow.cpp:6429 (Diff revision 2) > +nsWindow::ShouldUseOffMainThreadCompositing() > +{ > + return (nsBaseWidget::ShouldUseOffMainThreadCompositing() && > + (HasRemoteContent() || GetTransparencyMode() != eTransparencyTransparent)); Doesn't GetTransparencyMode() return eTransparencyTransparent when HasRemoteContent() due to this? http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/xul/nsMenuPopupFrame.cpp#315 That would make the HasRemoteContent() check here unnecessary? As the HasRemoteContent() test seems to be merely a workaround to avoid unresolved issues, I'd prefer not to add the unnecessary workaround.

Comment on attachment 8868665 [details] Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. .schouten https://reviewboard.mozilla.org/r/140256/#review147108 ::: widget/windows/nsWindow.cpp:7172 (Diff revision 2) > nsWindow::ShouldUseOffMainThreadCompositing() > { > // We don't currently support using an accelerated layer manager with > // transparent windows so don't even try. I'm also not sure if we even > // want to support this case. See bug 593471 > - if (mTransparencyMode == eTransparencyTransparent) { > + if (!HasRemoteContent() && mTransparencyMode == eTransparencyTransparent) { I'd like this to be explicitly blocked by a pref, so that even if remote=true would be specified this won't cause OMTC to be used for these windows on release. So for example: if (!(HasRemoteContent() && IsPrefSet(OOP)) && mTransparencyMode == eTransparencyTransparent) {

Comment on attachment 8868665 [details] Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. .schouten https://reviewboard.mozilla.org/r/140256/#review147108 > I'd like this to be explicitly blocked by a pref, so that even if remote=true would be specified this won't cause OMTC to be used for these windows on release. > > So for example: > > if (!(HasRemoteContent() && IsPrefSet(OOP)) && mTransparencyMode == eTransparencyTransparent) { This attribute only ever exists when remote WebExtensions are enabled, and if it needs to be switched off, I'd rather remote extensions also be switched off, rather than having only one or the other enabled. I suppose I could add another pref and unconditionally disable remote extensions unless it's enabled, though, if you think it's worth it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a54f3cba5c24d4a12fe3029282fcab8a7b32a3e8 Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. r=bas

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #38) > Comment on attachment 8868665 [details] > Bug 1365660: Part 5b - Enable compositing for popups with remote content on > Windows. .schouten > > https://reviewboard.mozilla.org/r/140256/#review147108 > > > I'd like this to be explicitly blocked by a pref, so that even if remote=true would be specified this won't cause OMTC to be used for these windows on release. > > > > So for example: > > > > if (!(HasRemoteContent() && IsPrefSet(OOP)) && mTransparencyMode == eTransparencyTransparent) { > > This attribute only ever exists when remote WebExtensions are enabled, and > if it needs to be switched off, I'd rather remote extensions also be > switched off, rather than having only one or the other enabled. > > I suppose I could add another pref and unconditionally disable remote > extensions unless it's enabled, though, if you think it's worth it. That is fine with me as well, whatever you think is the preferred route. I just don't want someone at the front-end to be able to add a remote attribute that makes this codepath be used without it ever going passed a widget/gfx person for review.

Can we close this bug and move any follow-up work to another bug? It's confusing to leave this open when the patches landed months ago.

Comment on attachment 8868664 [details] Bug 1365660: Part 5a - Enable compositing for popups with remote content on Linux. https://reviewboard.mozilla.org/r/140254/#review145768 > Doesn't GetTransparencyMode() return eTransparencyTransparent when HasRemoteContent() due to this? > http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/xul/nsMenuPopupFrame.cpp#315 > > That would make the HasRemoteContent() check here unnecessary? > > As the HasRemoteContent() test seems to be merely a workaround to avoid unresolved issues, I'd prefer not to add the unnecessary workaround. The HasRemoteContent() condition makes more sense if nsMenuPopupFrame::CreateWidgetForView() does not force remote popups to eTransparencyOpaque. A comment would be helpful to explain that having black in place of transparent areas on systems with non-compositing window managers is preferable to the remote content not drawing at all, which is what happens with basic layers.