Closed
Bug 1154459
Opened 10 years ago
Closed 10 years ago
Turn on APZ for Windows for one nightly
Categories
(Core :: Panning and Zooming, defect)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(1 file, 3 obsolete files)
52.07 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
We'd like to turn on APZ for Windows for one nightly to get feedback on what sort of stuff breaks. There are some test failures still, but they're getting more and more obscure. We'll flip these tests off, then back on after the nightly build has gone out.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8592459 -
Flags: review?(bugmail.mozilla)
Comment 2•10 years ago
|
||
Comment on attachment 8592459 [details] [diff] [review]
disable failing tests
Review of attachment 8592459 [details] [diff] [review]:
-----------------------------------------------------------------
I'd like the pref flip to happen as part of this patch as well, and please add a commit message that clearly indicates it will be backed out after a day.
Attachment #8592459 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 3•10 years ago
|
||
The WillHandleWheelEvent change is a quick hack to only use APZ if e10s is on by default. I'd like to get a more robust version of this later, if I can figure out how to tell the widget that it was created as an e10s window.
Attachment #8592467 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 4•10 years ago
|
||
The idea is to back out both of these patches after they have made it to one nightly.
Comment 5•10 years ago
|
||
No chance in combining everything into a single patch?
If you're on the main thread, it would be better to call mozilla::BrowserTabsRemoteAutostart() here rather than trying to check the prefs individually. There are some cases where the prefs are set but we still don't enable e10s.
Comment 7•10 years ago
|
||
Comment on attachment 8592467 [details] [diff] [review]
flip apz on for windows
Review of attachment 8592467 [details] [diff] [review]:
-----------------------------------------------------------------
Yeah, dvander and I discussed this in person - we agreed to (1) have the widget code check for BrowserTabsRemoteAutostart and not create the mAPZC if that function returns true and (2) add a flag on the scroll events which gets set when the APZCTreeManager receives it, and then the ESM would skip handling of that event if the flag is set. That handles some other weird cases such as a user setting e10s on by default, opening a non-e10s window, and then trying to scroll comboboxes, or something along those lines where WillHandleWheelEvent incorrectly returns true.
Attachment #8592467 -
Flags: review?(bugmail.mozilla)
Comment 8•10 years ago
|
||
In the widget code we should also check for mRequireOffMainThreadCompositing and not create a tree manager if that is false, ifdef windows. That should prevent non-e10s windows from getting apzc.
http://mxr.mozilla.org/mozilla-central/source/widget/nsWidgetInitData.h?rev=59a30471542b#128
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8592459 -
Attachment is obsolete: true
Attachment #8592467 -
Attachment is obsolete: true
Attachment #8592937 -
Flags: review?(bugmail.mozilla)
Updated•10 years ago
|
Attachment #8592937 -
Flags: review?(bugmail.mozilla) → review+
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe20e721ddf
The proximate cause of the backout was that you muffed https://hg.mozilla.org/integration/mozilla-inbound/diff/ffde08dc5ceb/layout/reftests/bugs/reftest.list#l1.145, dropping the pref and the fuzzy so that it started failing on !Windows, and because APZ crashes Windows talos svgr, https://treeherder.mozilla.org/logviewer.html#?job_id=9034479&repo=mozilla-inbound.
But I also question the wisdom of those sweeping skip-if(asyncPanZoom) instead of what you mean, skip-if(asyncPanZoom&&winWidget), which are setting us up to break something on other APZ platforms while this is in. There are other APZ platforms, aren't there?
And I also question integration branches as a way to ship something "in one nightly." I might or might not have merged that to m-c before the Monday morning nightly, and then a revert of it, also landed on m-i, might or might not have been merged to m-c before the Tuesday nightly. With a little sheriff coordination, you could instead land on m-c (after getting an actual full complete and actually green try run) after the last merge from integration branches on a weekday, and then get backed out after nightlies start, before the next merge around to integration branches, and thus both be assured of just one nightly and also not have to worry about people landing on top of you and either complicating the backout or busting the backout by thinking that APZ was actually enabled on Windows and having to adapt their patches to that (or just busting things for APZ and not knowing because of the sweeping skip-if(asyncPanZoom)).
Or to do things truly properly, you could land more careful test annotations on m-i, not only (asyncPanZoom&&winWidget) but only using skip-if when you have to, for crashes and timeouts, and using fails-if for failures and fuzzy-if for partial failures, and once those are in and merged around only then land the pref flip directly on m-c.
Assignee | ||
Comment 12•10 years ago
|
||
Thanks, I will look at that talos crash.
Assignee | ||
Comment 13•10 years ago
|
||
This version takes care not to erase any fuzzy[-ifs], checks for winWidget as well, and fixes some ordering problems. Sending to try shortly.
Attachment #8592937 -
Attachment is obsolete: true
Attachment #8594990 -
Flags: review?(bugmail.mozilla)
Comment 14•10 years ago
|
||
Comment on attachment 8594990 [details] [diff] [review]
v2
Review of attachment 8594990 [details] [diff] [review]:
-----------------------------------------------------------------
Please post a link to the try run when you have it, and coordinate with sheriffs to get this landed in a nightly and then backed out.
::: layout/reftests/layers/reftest.list
@@ +12,5 @@
> skip != pull-background-animated-position-3.html about:blank # Fails because PaintedLayer item assignment doesn't recognize overflow:hidden clips
> skip != pull-background-animated-position-4.html about:blank # Fails because PaintedLayer item assignment and background pulling don't recognize overflow:hidden clips
> skip != pull-background-animated-position-5.html about:blank # Fails because ownLayer bounds don't anticipate changes of animated contents, but doesn't fail with event regions
> +skip-if(!asyncPanZoom) skip-if(asyncPanZoom) != pull-background-displayport-1.html about:blank
> +skip-if(!asyncPanZoom) skip-if(asyncPanZoom) != pull-background-displayport-2.html about:blank
Should these also be skip-if(asyncPanZoom&&winWidget) ? Or just comment them out since they're skipped both with and without asyncPanZoom.
Attachment #8594990 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Comment on attachment 8594990 [details] [diff] [review]
>
> ::: layout/reftests/layers/reftest.list
> @@ +12,5 @@
> > skip != pull-background-animated-position-3.html about:blank # Fails because PaintedLayer item assignment doesn't recognize overflow:hidden clips
> > skip != pull-background-animated-position-4.html about:blank # Fails because PaintedLayer item assignment and background pulling don't recognize overflow:hidden clips
> > skip != pull-background-animated-position-5.html about:blank # Fails because ownLayer bounds don't anticipate changes of animated contents, but doesn't fail with event regions
> > +skip-if(!asyncPanZoom) skip-if(asyncPanZoom) != pull-background-displayport-1.html about:blank
> > +skip-if(!asyncPanZoom) skip-if(asyncPanZoom) != pull-background-displayport-2.html about:blank
>
> Should these also be skip-if(asyncPanZoom&&winWidget) ? Or just comment them
> out since they're skipped both with and without asyncPanZoom.
They're known failures with APZ - we should probably just mark them as skip on central. I'll see what Markus wants to do with them for now.
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
(In reply to David Anderson [:dvander] from comment #15)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> > Comment on attachment 8594990 [details] [diff] [review]
> >
> > ::: layout/reftests/layers/reftest.list
> > @@ +12,5 @@
> > > skip != pull-background-animated-position-3.html about:blank # Fails because PaintedLayer item assignment doesn't recognize overflow:hidden clips
> > > skip != pull-background-animated-position-4.html about:blank # Fails because PaintedLayer item assignment and background pulling don't recognize overflow:hidden clips
> > > skip != pull-background-animated-position-5.html about:blank # Fails because ownLayer bounds don't anticipate changes of animated contents, but doesn't fail with event regions
> > > +skip-if(!asyncPanZoom) skip-if(asyncPanZoom) != pull-background-displayport-1.html about:blank
> > > +skip-if(!asyncPanZoom) skip-if(asyncPanZoom) != pull-background-displayport-2.html about:blank
> >
> > Should these also be skip-if(asyncPanZoom&&winWidget) ? Or just comment them
> > out since they're skipped both with and without asyncPanZoom.
>
> They're known failures with APZ - we should probably just mark them as skip
> on central. I'll see what Markus wants to do with them for now.
There's a patch in bug 1148515 now that should fix them. In fact, with that patch, you shouldn't need any changes to layout/reftests/layers/reftest.list at all. Can you test whether these tests pass with that patch and APZ + event regions turned on on Windows locally?
Comment 19•10 years ago
|
||
Oh, okay then.
Looks like https://treeherder.mozilla.org/logviewer.html#?job_id=1372413&repo=mozilla-central started happening intermittently-but-frequently once this patch landed.
Comment 21•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Comment on attachment 8594990 [details] [diff] [review]
> v2
>
> Review of attachment 8594990 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Please post a link to the try run when you have it, and coordinate with
> sheriffs to get this landed in a nightly and then backed out.
>
nightlys are building so can this go out ?
Flags: needinfo?(dvander)
Comment 22•10 years ago
|
||
Backed out on m-c (but I typo'd the bug number):
https://hg.mozilla.org/mozilla-central/rev/570bb53a3e7b
Flags: needinfo?(dvander)
Comment 23•10 years ago
|
||
And I guess we can mark this fixed since the experiment is done. Any regressions reported by users should block this bug.
Updated•10 years ago
|
Target Milestone: --- → mozilla40
Comment 24•10 years ago
|
||
This generated a good amount of regressions on windows: http://alertmanager.allizom.org:8080/alerts.html?rev=17aad8f83237&showAll=1&testIndex=0&platIndex=0
Since it is backed out its ok for now, just something to keep in mind when it is deployed in future.
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Vaibhav (:vaibhav1994) from comment #24)
> This generated a good amount of regressions on windows:
> http://alertmanager.allizom.org:8080/alerts.
> html?rev=17aad8f83237&showAll=1&testIndex=0&platIndex=0
>
> Since it is backed out its ok for now, just something to keep in mind when
> it is deployed in future.
tscroll doesn't measure anything with APZ enabled, so once APZ lands we should just remove or disable it.
Paint regressions are expected, it's something we'll have to take (within a certain range) for APZ when it comes. APZ has to paint a larger area of the screen in order to work. There's no way around this.
You need to log in
before you can comment on or make changes to this bug.
Description
•