1365660 - Add special handling for popups which will contain remote browsers

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P3)

Description

[Kris Maglione [:kmag]]

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 1

[Kris Maglione [:kmag]]

Attachment: Bug 1365660: Part 1 - Add a remote="true" attribute to popups with remote browsers.

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.

Review commit: https://reviewboard.mozilla.org/r/140246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140246/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1365660: Part 2 - Add a HasRemoteContent flag to popup widgets that contain remote browsers. .schouten

There are several behaviors that we only need to apply to popups which are
known to contain remote content. And since those behaviors can cause issues
elsewhere, we need to be able to identify popups which should contain remote
content, and treat them specially, where appropriate.

Review commit: https://reviewboard.mozilla.org/r/140248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140248/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten

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.

Transparency is not handled correctly for composited popups on any platform,
but works with varying degrees of success on some platforms, for some popups.
Oddly, out of the three main desktop platforms, Linux seems to handle it the
best, so long as we render the popup as opaque, and let the platform
compositor handle the transparency.

Review commit: https://reviewboard.mozilla.org/r/140250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140250/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1365660: Part 4 - Only enable APZ for popups which contain remote content.

We only ever need to enable APZ for popups which contain remote content. In
theory, enabling it for other popups shouldn't hurt, but having it enabled
adds overhead that we'd rather avoid, and causes painting issues under some
circumstances.

Ideally, the painting issues should be fixed, but disabling APZ is a good
short term workaround, and we should try to avoid the unnecessary overhead
either way.

Review commit: https://reviewboard.mozilla.org/r/140252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140252/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1365660: Part 5a - Enable compositing for popups with remote content on Linux.

Review commit: https://reviewboard.mozilla.org/r/140254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140254/

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. .schouten

Review commit: https://reviewboard.mozilla.org/r/140256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140256/

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X.

Review commit: https://reviewboard.mozilla.org/r/140258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140258/

Comment 8

[Markus Stange [:mstange]]

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 9

[Markus Stange [:mstange]]

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 10

[Kris Maglione [:kmag]]

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 11

[Kris Maglione [:kmag]]

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 12

[Kartikaya Gupta (email:[e-mail])]

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 13

[Kartikaya Gupta (email:[e-mail])]

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 14

[Kris Maglione [:kmag]]

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.

Comment 15

[Markus Stange [:mstange]]

I would prefer that, yes.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8868662 [details]
Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140250/diff/1-2/

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8868663 [details]
Bug 1365660: Part 4 - Only enable APZ for popups which contain remote content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140252/diff/1-2/

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8868664 [details]
Bug 1365660: Part 5a - Enable compositing for popups with remote content on Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140254/diff/1-2/

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8868665 [details]
Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. .schouten

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140256/diff/1-2/

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8868666 [details]
Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140258/diff/1-2/

Comment 21

[Shane Caraveo (:mixedpuppy)]

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

Comment 22

[Kris Maglione [:kmag]]

(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 23

[Markus Stange [:mstange]]

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 24

[Markus Stange [:mstange]]

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 25

[Kris Maglione [:kmag]]

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 26

[Markus Stange [:mstange]]

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 27

[Bas Schouten (:bas.schouten)]

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 28

[Bas Schouten (:bas.schouten)]

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 29

[Bas Schouten (:bas.schouten)]

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.

Comment 30

[Kris Maglione [:kmag]]

(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 31

[Marco Castelluccio [:marco]]

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.

Comment 32

[Kris Maglione [:kmag]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Kris Maglione [:kmag]]

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 35

[Karl Tomlinson (:karlt)]

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 36

[Karl Tomlinson (:karlt)]

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 37

[Bas Schouten (:bas.schouten)]

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 38

[Kris Maglione [:kmag]]

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.

Comment 39

[Kris Maglione [:kmag]]

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

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a54f3cba5c24

Comment 41

[Bas Schouten (:bas.schouten)]

(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.

Comment 42

[Kartikaya Gupta (email:[e-mail])]

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 43

[Karl Tomlinson (:karlt)]

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.