1370131 - Support OMTC on panels with shadows

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, enhancement, P2)

Description

[Markus Stange [:mstange]]

With remote webextensions (bug 1190679), we can have arrow panels that contain remote browsers. Those require off-main-thread compositing to be displayed. And on Mac, OMTC requires the use of an OpenGL context. But OpenGL compositing currently breaks the window shadow.

The work in bug 1291457 will make this easier to fix.

Comment 1

[Markus Stange [:mstange]]

We'll want to fix bug 1341913 before we enable accelerated compositing for all popups, because at the moment, doing so would make opening these windows slower.

Comment 2

[Andy McKay]

This only affects OS X so not a blocker for 56.

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1370131 - Allow shadows on accelerated popups.

It looks like the work in bug 1291457 was all that was necessary to get working
window shadows for accelerated popups. It seems that macOS is able to compute
the correct shadow style consistently for whatever is drawn in the
NSOpenGLContext during the first paint of the window (inside the drawRect call
during the orderFront call that opens the window).

We only ran into problems when we animated the contents of the window in a way
that affects the shadow style; we're no longer doing that after bug 1291457.

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

Comment 4

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #0)
> The work in bug 1291457 will make this easier to fix.

In fact, it seems that bug 1291457 was all that was needed to fix this completely!

It looks like none of the complicated work I had in mind for this bug is actually necessary. Unlike what I thought, it looks like we don't actually have to draw the popup shape on the main thread in order to get working shadows; all we need to do is to draw the correct shape during the first paint of the window, and that's already happening ever since bug 1291457 landed. 

I've triggered a tryserver build with this patch and will test it on both 10.9 and 10.12 once it's complete, by flipping extensions.webextensions.remote to true, installing the Gecko Profiler add-on, and opening and closing its panel a bunch of times.

Kmag, do you have any other tests in mind that I should run? This review request is mostly a "please test" request. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=265adfea9b62f962f4552d30ed7977b9ee53a841

Comment 5

[Markus Stange [:mstange]]

I tested the try build on 10.9 and the window did not have a shadow :(
So it seems like we do need to do more work on this in order to support older versions of macOS.

Comment 6

[Kris Maglione [:kmag]]

Sorry, this is still on my TODO list. It's just been a while since I last used my Mac development environment, and it needs updates/context switch.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Markus Stange [:mstange] from comment #5)
> I tested the try build on 10.9 and the window did not have a shadow :(
> So it seems like we do need to do more work on this in order to support
> older versions of macOS.

I can live with not having a shadow on older versions of OS-X as long as we have proper rendering of remote layer trees, and don't have a broken/non-updated-on-resize shadow.

Comment 8

[Kris Maglione [:kmag]]

Attachment: pageAction screenshot

Comment 9

[Kris Maglione [:kmag]]

Attachment: browserAction screenshot

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8952882 [details]
Bug 1370131 - Allow shadows on accelerated popups.

https://reviewboard.mozilla.org/r/222114/#review231150

With this change, I occasionally get about a 1px gap at the bottom of the popup with no shadow (see attached screenshots). This seems to happen about 1 time in 10. We should probably file a follow-up bug for that, but I don't think it should be a blocker at this point.

Comment 11

[:shell escalante]

sent via email to involved people (Markus, Kris, Jimm, ddurst, krupa, Stephen P):

When this bug lands, are we ready to test Out Of Process webextensions on Macs, based on https://bugzil.la/1406531#c4? 

Is this bug trying to land in 60 or 61?  Unless there is a strong driver for 60, 61 would be better for QA timing.

I need to file a PIrequest for testing, because the areas that would show up with issues are cross-team (GFX / add-ons).

Comment 12

[Markus Stange [:mstange]]

(In reply to :shell escalante from comment #11)
> When this bug lands, are we ready to test Out Of Process webextensions on
> Macs, based on https://bugzil.la/1406531#c4? 

Yes we are. Bug 1385403 has a patch for that.

> Is this bug trying to land in 60 or 61?  Unless there is a strong driver for
> 60, 61 would be better for QA timing.

I don't have an opinion on this, but bug 1385403 landed just now, and I trust Jim to know more about the timing requirements than myself.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd1a68c40c3b
Allow shadows on accelerated popups. r=kmag

Comment 14

[Markus Stange [:mstange]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #10)
> With this change, I occasionally get about a 1px gap at the bottom of the
> popup with no shadow (see attached screenshots). This seems to happen about
> 1 time in 10. We should probably file a follow-up bug for that, but I don't
> think it should be a blocker at this point.

I agree. I've filed bug 1443920 about it.

Comment 15

[Stefan Hindli [:stefan_hindli]]

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

Comment 16

[krupa raj [:krupa--use this to needinfo]]

Victor, can you confirm here once all the testing is complete for OOP to be flipped on for Beta 61? Thanks!

Comment 17

[Markus Stange [:mstange]]

Removing dependency on bug 1341913 because that's only a perf improvement which doesn't block this bug from shipping.

Comment 18

[Victor Carciu]

I tested the bug on MacOS 10.13, using Gecko Profiler add-on and no issues were observed. Also I tested using MacOS 10.9 for shttps://bugzilla.mozilla.org/show_bug.cgi?id=1370131#c7 and no other issues than the absence of shadow were found.

Both dependencies for this bug are marked as "qe-verify-" so I will mark this one as verified too.