1178298 - (all-aboard-apz) Let APZ on desktop ride the train

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

I plan on enabling APZ on nightly only soon. Once that happens this bug will track the things that need to be fixed before we will let it ride the train to Aurora.

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Bug 1205459 causes major problems for developer tools, I know this bug is specifically about launching apz in release - should we log a separate bug for enabling apz in aurora/dev edition?

Comment 2

[Botond Ballo [:botond]]

My understanding (based on comment 0) is that APZ won't even go to Aurora until the dependencies of this bug are fixed.

Comment 3

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

What Botond said :)

Comment 4

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

Attachment: Patch

Considering we're going to let APZ ride on 46 we might as well get this landed. Any outstanding dependencies can be uplifted to 46 if they don't land in this cycle.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8702668 [details] [diff] [review]
Patch

Review of attachment 8702668 [details] [diff] [review]:
-----------------------------------------------------------------

Exciting times!

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Big improvement for users
[Suggested wording]: Async Pan/Zoom
[Links (documentation, blog post, etc)]: do we have a blog post?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84479159751f

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/84479159751f

Comment 9

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

All dependencies fixed too \o/

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Can we uplift this to current Aurora, so that beta45 gets it?

Comment 11

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to :Felipe Gomes (needinfo me!) from comment #10)
> Can we uplift this to current Aurora, so that beta45 gets it?

So I believe we want to this because of the experiment that we intend to run for e10s in Beta 45. However, do we have a sense of the quality of the feature at this point? Is it good enough for Beta?

Adding Vasilica to provide some input on this.

Comment 12

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

(In reply to :Felipe Gomes (needinfo me!) from comment #10)
> Can we uplift this to current Aurora, so that beta45 gets it?

I just responded to your email. I'll post it here as well for posterity:

We fixed a whole slew of APZ-related things in 46; those fixes
are not in 45. So if you enable APZ on 45 (either aurora or for the
beta experiment, or both) you will get a "bad" APZ experience, lacking
fixes for a number of things. So:

- If you're planning on letting e10s ride to release on 45, then I
don't think it makes sense to turn on APZ on 45 at all, because the
experiment will give you results for "bad" APZ whereas the release
population will see no APZ at all.
- If the experiment is just a short-term thing (1 or 2 betas), then
I'm ok with turning on APZ for the 45 beta experiment to gather some
data, but keep in mind that the APZ experience will be better in 46.
- If the experiment is for the entire length of the 45 beta, and you
really want to have the "bad" APZ experience enabled, then I won't
really object but I'm not sure there's much value in doing that.
Instead, I would suggest that you leave it off for the 45 experiment
on beta. That will give you data on how e10s works without APZ. The 46
beta will have APZ enabled anyway, and that will give you data on how
e10s works *with* APZ. If it turns out the 46 beta has a bunch of
critical APZ bugs, then we can turn off APZ on 46 with more confidence
since we tested the e10s without APZ combo on 45.

Regardless of what we go with, I'd rather not remove the #ifdef on 45
aurora until very late in the cycle, because I don't want to subject
our aurora population to the "bad" APZ - it will probably just result
in a bunch of bugs being filed which are already fixed in 46. If you
want APZ in the 45 beta experiment I'd wait to remove the ifdef until
just before or just after the next merge.

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing ni from Vasilica since kats has pretty much clarified things here.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8702668 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: This is necessary to allow apz to be enabled during the e10s beta45 experiment. APZ is only active for users of e10s, so this change has no effect for non-e10s users on beta or when this gets to release.
[User impact if declined]: The testing done during the experiment wouldn't be useful as everyone would be testing something far from what we plan on shipping.
[Describe test coverage new/current, TreeHerder]: I imagine apz has tests..
[Risks and why]: There are some known apz bugs on 45 that have been fixed on 46 but are too risky to be uplifted, so the users will be exposed to them. The consensus is that this is better than not testing apz at all with e10s.
[String/UUID change made/needed]: none

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8702668 [details] [diff] [review]
Patch

I am sorry but I cannot accept this uplift. Comment #12 suggests that we already know that the experience will be bad for users. We have a few users on the beta channel: I don't want to give them a bad experience. And we are planning to target an important percentage of users with the experiment.

Moreover, uplifting a new important feature from nightly to beta is not the proper way to do things.

Comment 16

[Erin Lancaster [:elan]]

Felipe, approx how many users would be impacted by this?

Comment 17

[:Felipe Gomes (needinfo for replies!)]

45% of the users will be part of the experiment, but a number of them won't be enabled due to a11y. So I'd say approx. 40%.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Here is the list of bugs that are fixed in 46 (where we plan to enable APZ for release) and not fixed in 45: http://tinyurl.com/jmosybc

In that list, only bug 1230693 concerns me, and Kats has already suggested that we uplift that to 45 anyway. If anyone sees anything else in that list that concerns them, please flag it.

Sylvestre, my point here is that the "bad" that Kats refers to in comment 12 is mostly polish issues and shouldn't drive people away from beta any more than they'll be driven away in 46 when we plan to enable it. Based on that, can you reconsider your uplift approval decision?

Comment 19

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

I agree with what Brad said. Although the list of bugs is scary-looking I don't think they will actually force users to stop using Firefox, specially if they have a way to turn off e10s/APZ. We will probably get a flood of bug reports for things that are already fixed in 46 though.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #18)
> In that list, only bug 1230693 concerns me, and Kats has already suggested
> that we uplift that to 45 anyway. If anyone sees anything else in that list
> that concerns them, please flag it.

Bug 1168263 was backed out from Aurora45 as part of the massive preserve-3d backout (https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d), so Aurora shouldn't be affected by bug 1230693 at this point. I intend to verify the various deps affected by that backout later today.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Kats, can you share the bug we should expect to see reported if we accept this in beta?
Thanks

Comment 22

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

The list brad linked to in comment 18 probably covers the sorts of bugs we'd expect to see filed. Specifically, there will likely be bugs about page elements jittering or getting out of sync temporarily as you scroll, crashes/distorted rendering on some HiDPI displays or super-long pages, high-ish amounts of checkerboarding, and blurry fonts (bug 1122916 had the most amount of dupes when APZ was enabled on nightly).

Comment 23

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

Oh, also RTL scrollbar issues and issues with windowed flash plugins (bug 1152049)

Comment 24

[Sylvestre Ledru [:Sylvestre]]

The issues you describe seems pretty serious and could badly reflect on the image of Firefox.
40% of the beta population is way too much people to do an experiment which will decrease the quality of Firefox... Especially when we know the issues upfront.

So, please wait 46 to experiment e10s with APZ.

Comment 25

[Avi Halachmi (:avih)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> So, please wait 46 to experiment e10s with APZ.

FWIW, this will also allow us to compare synchronous scroll with and without e10s on 45 (this probe appeared first on 45). Otherwise, if APZ was to get enabled in 45, then the biggest trigger for scrolls (mouse wheel) would have used APZ rather than synchronous.

So that's another useful thing when postponing APZ to 46.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Noting for 46 aurora as Asynchronous Panning and Zooming 
kats, how about a link to https://staktrace.com/spout/entry.php?id=834?  
Or, better, can you post something on a mozilla.org blog that I can link to from the release notes?

Comment 27

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

Sorry for the delayed response. I'll draft a blog post for hacks.m.o, which seems like an appropriate place to put this announcement. Assuming APZ stays on the 46 train, should the article be posted when 46 goes to beta? To release? Or anytime is fine?

Leaving ni on me until I get it written.

Comment 28

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

There's a hacks blog post up now: https://hacks.mozilla.org/2016/02/smoother-scrolling-in-firefox-46-with-apz/ - feel free to link to that in the release notes.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks! I updated the link in the release notes.

Comment 30

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

I filed a new metabug for APZ release blockers (bug 1254668) since cpeterson said it was confusing that this bug was fixed. Any open release blockers should block that instead.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

This is also now out of the 46 relnotes, but I will put it back if APZ goes to release!