Closed Bug 1154459 Opened 5 years ago Closed 5 years ago

Turn on APZ for Windows for one nightly

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch disable failing tests (obsolete) — Splinter Review
Attachment #8592459 - Flags: review?(bugmail.mozilla)
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)
Attached patch flip apz on for windows (obsolete) — Splinter Review
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)
The idea is to back out both of these patches after they have made it to one nightly.
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 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)
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
Attached patch patch (obsolete) — Splinter Review
Attachment #8592459 - Attachment is obsolete: true
Attachment #8592467 - Attachment is obsolete: true
Attachment #8592937 - Flags: review?(bugmail.mozilla)
Attachment #8592937 - Flags: review?(bugmail.mozilla) → review+
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.
Thanks, I will look at that talos crash.
No longer depends on: 1154739
Attached patch v2Splinter Review
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 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+
(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.
(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?
Oh, okay then.
Blocks: 1157090
(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)
Backed out on m-c (but I typo'd the bug number):

https://hg.mozilla.org/mozilla-central/rev/570bb53a3e7b
Flags: needinfo?(dvander)
And I guess we can mark this fixed since the experiment is done. Any regressions reported by users should block this bug.
No longer blocks: 1157090
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Depends on: 1157090
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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.
(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.
Depends on: 1157070
Depends on: 1157560
Depends on: 1157579
You need to log in before you can comment on or make changes to this bug.