Closed
Bug 1370131
Opened 7 years ago
Closed 7 years ago
Support OMTC on panels with shadows
Categories
(Core :: Widget: Cocoa, enhancement, P2)
Core
Widget: Cocoa
Tracking
()
VERIFIED
FIXED
mozilla60
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.
Assignee | ||
Comment 1•7 years ago
|
||
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
Updated•7 years ago
|
Priority: -- → P2
Whiteboard: tpi:+
Comment 2•7 years ago
|
||
This only affects OS X so not a blocker for 56.
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
(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
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 5•7 years ago
|
||
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•7 years ago
|
||
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)
Comment 7•7 years ago
|
||
(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•7 years ago
|
||
Comment 9•7 years ago
|
||
Comment 10•7 years ago
|
||
mozreview-review |
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+
Comment 11•7 years ago
|
||
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).
Updated•7 years ago
|
Assignee | ||
Comment 12•7 years ago
|
||
(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•7 years ago
|
||
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/cd1a68c40c3b
Allow shadows on accelerated popups. r=kmag
Assignee | ||
Comment 14•7 years ago
|
||
(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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 16•7 years ago
|
||
Victor, can you confirm here once all the testing is complete for OOP to be flipped on for Beta 61? Thanks!
Flags: needinfo?(vcarciu)
Updated•7 years ago
|
Assignee | ||
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
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
Updated•7 years ago
|
Flags: needinfo?(vcarciu)
You need to log in
before you can comment on or make changes to this bug.
Description
•