Closed Bug 1178298 (all-aboard-apz) Opened 9 years ago Closed 9 years ago

Let APZ on desktop ride the train

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed
relnote-firefox --- 46+

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: feature, Whiteboard: [gfx-noted][e10s-45-uplift])

Attachments

(1 file)

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.
Alias: all-aboard-apz
Whiteboard: [gfx-noted]
Depends on: 1178745
Depends on: 1178382
Depends on: 1184343
Depends on: 1187804
Depends on: 1187990
No longer depends on: 1167773
No longer depends on: 1168487
Depends on: 1189972
Depends on: 1190105
Depends on: 1190112
No longer depends on: 1157834
No longer depends on: 1012752
Depends on: 1192419
Depends on: 1192919
Depends on: 1195526
No longer depends on: 1195526
Depends on: 1197312
Depends on: 1197654
Depends on: 1200158
Depends on: 1200778
Depends on: apz-parallax
Depends on: 1201889
No longer depends on: apz-parallax
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?
Flags: needinfo?(bugmail.mozilla)
My understanding (based on comment 0) is that APZ won't even go to Aurora until the dependencies of this bug are fixed.
What Botond said :)
Flags: needinfo?(bugmail.mozilla)
Depends on: 1200580
Summary: Let APZ ride the train → Let APZ on desktop ride the train
Depends on: 1198021
Depends on: 1208829
Depends on: 1209100
No longer depends on: 1209100
Depends on: 1209964
Depends on: 1213324
Depends on: 1211506
Depends on: 1214170
Depends on: 1216488
No longer depends on: 1217627
No longer depends on: 1201192
No longer depends on: 1212020
No longer depends on: 1211777
Depends on: 1216924
Depends on: 1152049
Depends on: 1193055
No longer depends on: 1188430
No longer depends on: 1188624
Depends on: 1227971
Depends on: 1228133
No longer depends on: 1193969
No longer depends on: 1197654
No longer depends on: 1201996
No longer depends on: 1209058
No longer depends on: 1230228
No longer depends on: 1160601
Depends on: apz-talos
No longer depends on: 1216924
No longer depends on: 1193781
No longer depends on: 1213073
Depends on: 1232184
No longer depends on: 1150941
No longer depends on: apz-talos
Attached patch Patch (deleted) — — Splinter Review
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.
Assignee: nobody → bugmail.mozilla
Attachment #8702668 - Flags: review?(botond)
Comment on attachment 8702668 [details] [diff] [review] Patch Review of attachment 8702668 [details] [diff] [review]: ----------------------------------------------------------------- Exciting times!
Attachment #8702668 - Flags: review?(botond) → review+
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?
relnote-firefox: --- → ?
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
All dependencies fixed too \o/
Can we uplift this to current Aurora, so that beta45 gets it?
Flags: needinfo?(bugmail.mozilla)
(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.
Flags: needinfo?(vasilica.mihasca)
(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.
Flags: needinfo?(bugmail.mozilla)
Removing ni from Vasilica since kats has pretty much clarified things here.
Flags: needinfo?(vasilica.mihasca)
Whiteboard: [gfx-noted] → [gfx-noted][e10s-45-uplift]
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
Attachment #8702668 - Flags: approval-mozilla-aurora?
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.
Attachment #8702668 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Felipe, approx how many users would be impacted by this?
Flags: needinfo?(felipc)
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%.
Flags: needinfo?(felipc)
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?
Flags: needinfo?(sledru)
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.
(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.
Kats, can you share the bug we should expect to see reported if we accept this in beta? Thanks
Flags: needinfo?(sledru)
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).
Oh, also RTL scrollbar issues and issues with windowed flash plugins (bug 1152049)
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.
(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.
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?
Flags: needinfo?(bugmail.mozilla)
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.
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.
Flags: needinfo?(bugmail.mozilla) → needinfo?(lhenry)
Thanks! I updated the link in the release notes.
Flags: needinfo?(lhenry)
Depends on: 1252822
Depends on: 1252947
Depends on: 1227799
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.
No longer depends on: 1227799, 1247854, 1252822, 1252947
This is also now out of the 46 relnotes, but I will put it back if APZ goes to release!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: