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)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: kats, Assigned: kats)
References
Details
(Keywords: feature, Whiteboard: [gfx-noted][e10s-45-uplift])
Attachments
(1 file)
(deleted),
patch
|
botond
:
review+
Sylvestre
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•9 years ago
|
Alias: all-aboard-apz
Updated•9 years ago
|
Whiteboard: [gfx-noted]
Depends on: 1193557
Updated•9 years ago
|
Depends on: apz-parallax
Assignee | ||
Updated•9 years ago
|
No longer depends on: apz-parallax
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
My understanding (based on comment 0) is that APZ won't even go to Aurora until the dependencies of this bug are fixed.
Assignee | ||
Updated•9 years ago
|
Summary: Let APZ ride the train → Let APZ on desktop ride the train
Depends on: 1211777
Depends on: 1217627
Updated•9 years ago
|
Depends on: fuzzy-text
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
Comment on attachment 8702668 [details] [diff] [review]
Patch
Review of attachment 8702668 [details] [diff] [review]:
-----------------------------------------------------------------
Exciting times!
Attachment #8702668 -
Flags: review?(botond) → review+
Comment 6•9 years ago
|
||
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:
--- → ?
Comment 8•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Assignee | ||
Comment 9•9 years ago
|
||
All dependencies fixed too \o/
Comment 10•9 years ago
|
||
Can we uplift this to current Aurora, so that beta45 gets it?
Flags: needinfo?(bugmail.mozilla)
Comment 11•9 years ago
|
||
(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)
Assignee | ||
Comment 12•9 years ago
|
||
(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)
Comment 13•9 years ago
|
||
Removing ni from Vasilica since kats has pretty much clarified things here.
Flags: needinfo?(vasilica.mihasca)
Updated•9 years ago
|
Whiteboard: [gfx-noted] → [gfx-noted][e10s-45-uplift]
Comment 14•9 years ago
|
||
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 15•9 years ago
|
||
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-
Comment 16•9 years ago
|
||
Felipe, approx how many users would be impacted by this?
Flags: needinfo?(felipc)
Comment 17•9 years ago
|
||
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)
Comment 18•9 years ago
|
||
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)
Assignee | ||
Comment 19•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
Kats, can you share the bug we should expect to see reported if we accept this in beta?
Thanks
Flags: needinfo?(sledru)
Assignee | ||
Comment 22•9 years ago
|
||
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).
Assignee | ||
Comment 23•9 years ago
|
||
Oh, also RTL scrollbar issues and issues with windowed flash plugins (bug 1152049)
Comment 24•9 years ago
|
||
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•9 years ago
|
||
(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)
Assignee | ||
Comment 27•9 years ago
|
||
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.
Assignee | ||
Comment 28•9 years ago
|
||
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)
Assignee | ||
Comment 30•9 years ago
|
||
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.
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.
Description
•