Closed Bug 1370131 Opened 3 years ago Closed 2 years ago

Support OMTC on panels with shadows

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox57 --- wontfix
firefox60 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Whiteboard: tpi:+)

Attachments

(3 files)

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.
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.
Depends on: 1341913
Blocks: 1352075
No longer blocks: 1352075
Priority: -- → P2
Whiteboard: tpi:+
Depends on: 1371476
This only affects OS X so not a blocker for 56.
(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
Flags: needinfo?(kmaglione+bmo)
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.
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.
Flags: needinfo?(kmaglione+bmo)
(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 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.
Attachment #8952882 - Flags: review?(kmaglione+bmo) → review+
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).
Blocks: 1385403
No longer blocks: webext-oop
(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.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/cd1a68c40c3b
Allow shadows on accelerated popups. r=kmag
(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.
https://hg.mozilla.org/mozilla-central/rev/cd1a68c40c3b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Victor, can you confirm here once all the testing is complete for OOP to be flipped on for Beta 61? Thanks!
Flags: needinfo?(vcarciu)
See Also: → 1406533
Removing dependency on bug 1341913 because that's only a perf improvement which doesn't block this bug from shipping.
No longer depends on: 1341913
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(vcarciu)
You need to log in before you can comment on or make changes to this bug.