1154459 - Turn on APZ for Windows for one nightly

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: disable failing tests

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: flip apz on for windows

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

The idea is to back out both of these patches after they have made it to one nightly.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

No chance in combining everything into a single patch?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffde08dc5ceb

Comment 11

[Phil Ringnalda (:philor)]

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.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Thanks, I will look at that talos crash.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

This version takes care not to erase any fuzzy[-ifs], checks for winWidget as well, and fixes some ordering problems. Sending to try shortly.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41be76009f70

Comment 17

[Markus Stange [:mstange]]

(In reply to David Anderson [:dvander] from comment #15)
> (In reply to Kartikaya Gupta (email:[e-mail]) 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 18

[Pulsebot]

https://hg.mozilla.org/mozilla-central/rev/17aad8f83237

Comment 19

[Markus Stange [:mstange]]

Oh, okay then.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

[Carsten Book [:Tomcat]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 ?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Backed out on m-c (but I typo'd the bug number):

https://hg.mozilla.org/mozilla-central/rev/570bb53a3e7b

Comment 23

[Kartikaya Gupta (email:[e-mail])]

And I guess we can mark this fixed since the experiment is done. Any regressions reported by users should block this bug.

Comment 24

[Vaibhav (:vaibhav1994)]

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.

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

(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.