Reflow on zoom simply isn't performing the way we want it to, across all of our release channels on Android. In fact, it is performing so poorly and unreliably, that even shipping it as a pref'd off feature in Firefox is no longer an acceptable approach. I think it's time for us to make a difficult decision, and that is that this feature is not ready for users, and shouldn't be something that can be enabled as easily as it can be now. I am proposing that for now we remove the item from Settings, across all our release channels. For the purposes of continued development and testing, perhaps it can be left as a pref in about:config, or an add-on that puts the pref back in Settings for those who want it. Or, we could even leave it on in Nightly but not uplift it. I want to be clear that I support having this feature in our browser, as it's something that I know a small but enthusiastic set of our users are asking for. But if it doesn't work yet, it shouldn't be in Firefox yet. As I understand it, the Chrome team recently shared a test with us that indicated just how large the gap in reflow performance is between Chrome and Firefox. Perhaps there is something we can learn from the approach they took?

Speaking with Mark and Brad over IRC, we can now make use of flags to keep this visible in settings for specified release channels. I support the decision to remove this option from the settings menu from the commerical, GA channel. I also believe that it should still be visible in the settings menu for both nightly and aurora channels. I think the big debate is whether it is visible for our beta users. I personally believe that having it in beta but defaulted off is acceptable provided we don't regress in performance (we are currently seeing different behaviour between nightly and beta).

(In reply to Ian Barlow (:ibarlow) from comment #0) > As I understand it, the Chrome team recently shared a test with us that > indicated just how large the gap in reflow performance is between Chrome and > Firefox. Perhaps there is something we can learn from the approach they took? FYI - This isn't a test for reflow-on-zoom, but rather general reflow/painting performance for us compared to chrome on android.

This is probably a good decision to remove from current release, beta, and maybe aurora channels, as most of the bugfixes that went into reflow-on-zoom were for Firefox 24 and 25, but I think we should leave it in Nightly at the very least.

After speaking with ibarlow on IRC, it sounds like we should remove reflow-on-zoom from the current release channel at the very least (i.e. not ship in Fx22), and probably from beta (Fx 23) as well. Ian reports that the major blocker for shipping with current Nightly (when Fx25 comes through the channels), is problems with flickering caused by successive repaints after the reflow-on-zoom happens. This is tracked by bug 878935.

Scott - how would you feel about leaving it in beta but defaulted off? It *is* something our users have been asking about and it does show we're serious about it. But I don't know if that's more work, less work, or whether it would sit there for a truly long period of time before it would finally be ready for GA.

(In reply to Karen Rudnitski [:kar] from comment #5) > Scott - how would you feel about leaving it in beta but defaulted off? It > *is* something our users have been asking about and it does show we're > serious about it. But I don't know if that's more work, less work, or > whether it would sit there for a truly long period of time before it would > finally be ready for GA. I'm ok with leaving it in beta but defaulted to off. I think we should probably think about uplifting some of the positioning fixes that were made in 24, though, if we do that.

By the way, the reflow/drawing performance test ibarlow mentioned in the description is bug 900165.

Tracking for Fx23. It sounds like we are wanting to use the RELEASE_BUILD flag to control this Setting. That might mean Beta and Release builds will not contain the setting. I'm not sure exactly what RELEASE_BUILD controls. It's dubious that we'll respin Fx23 to get this landed. Fx24 might be the highest we go.

(In reply to Mark Finkle (:mfinkle) from comment #8) > Tracking for Fx23. It sounds like we are wanting to use the RELEASE_BUILD > flag to control this Setting. That might mean Beta and Release builds will > not contain the setting. I'm not sure exactly what RELEASE_BUILD controls. > > It's dubious that we'll respin Fx23 to get this landed. Fx24 might be the > highest we go. Well, if we're just removing it from the menu, does it really need a respin? (Perhaps I don't understand how release builds work... does the final beta actually just become the release, or is rebuilt on merge day?)

Remove the reflow-on-zoom preference from the settings menu for mozilla-release.

Can someone confirm that this will therefore be removed from the GA channel, or both the GA & Beta channel (based on flag functionality). Thanks!

Comment on attachment 785029 [details] [diff] [review] b900564-releasebranch Looks OK for GA alone. We still need a patch for m-c that enables for Nightly, but removes for other channels.

Comment on attachment 785029 [details] [diff] [review] b900564-releasebranch [Approval Request Comment] Bug caused by (feature/regressing bug #): reflow-on-zoom User impact if declined: User-visible preference that enables reflow-on-zoom feature, not yet ready for beta consumption. Testing completed (on m-c, etc.): No testing completed yet. Risk to taking this patch (and alternatives if risky): Not risky - it simply removes preferences from the main preference screen on Firefox for android. String or IDL/UUID changes made by this patch: None.

I think this is the solution that makes it disabled on the release and beta branches. Unfortunately, my custom builds all have "default" as the MOZ_UPDATE_CHANNEL variable, so I'm not 100% sure. Mark, if you could look this over (or pass it to someone who is better suited if you don't think that's you), I'd appreciate it.

Comment on attachment 787577 [details] [diff] [review] b900564-general In general this looks right. But we don't want to use MOZ_UPDATE_CHANNEL. We want to use NIGHTLY_BUILD instead. In order to do that, we need to add NIGHTLY_BUILD to AppConstants.java.in

If we want this in Nightly and Aurora, we would add RELEASE_BUILD to AppConstants.java.in too and use it.

(In reply to Scott Johnson (:jwir3) from comment #13) > Comment on attachment 785029 [details] [diff] [review] > b900564-releasebranch > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): reflow-on-zoom > User impact if declined: User-visible preference that enables reflow-on-zoom > feature, not yet ready for beta consumption. > Testing completed (on m-c, etc.): No testing completed yet. > Risk to taking this patch (and alternatives if risky): Not risky - it simply > removes preferences from the main preference screen on Firefox for android. > String or IDL/UUID changes made by this patch: None. Note to Release Management: We are fine with this landing in beta 2. We do not need a respin of beta 1 for this patch. It can wait.

(In reply to Mark Finkle (:mfinkle) from comment #16) > If we want this in Nightly and Aurora, we would add RELEASE_BUILD to > AppConstants.java.in too and use it. So I'm a bit confused about comment 16 and comment 15. We only need one of RELEASE_BUILD or NIGHTLY_BUILD, correct? If we want it in aurora and nightly, then we want to use RELEASE_BUILD, but if we only want it in nightly, then we want NIGHTLY_BUILD. Am I understanding that correctly?

(In reply to Scott Johnson (:jwir3) from comment #18) > (In reply to Mark Finkle (:mfinkle) from comment #16) > > If we want this in Nightly and Aurora, we would add RELEASE_BUILD to > > AppConstants.java.in too and use it. > > So I'm a bit confused about comment 16 and comment 15. We only need one of > RELEASE_BUILD or NIGHTLY_BUILD, correct? If we want it in aurora and > nightly, then we want to use RELEASE_BUILD, but if we only want it in > nightly, then we want NIGHTLY_BUILD. Am I understanding that correctly? You are understanding it correctly.

More info: Bug 888982 just landed the RELEASE_BUILD flag in AppConstants.java.in on fx-team. It should appear on m-c and inbound soon.

Aaron, can you please help verify this by grabbing the beta builds once the patch lands(before we even goto build for beta 2) given there is no testing done on this.

Comment on attachment 785029 [details] [diff] [review] b900564-releasebranch Approving for beta given comment #17

Pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/bfd9bfa0505f

Manual tests will need updating; either disabled or reflective of changes for the targeted release.

Assigning to Chenxia since she is doing the same thing for the Geolocation data collection. (bug 903535)

This patch removes the text reflow UI from release channels and updates the tests. Not sure about the test style, and having #ifdefs there. Try build (I think this is a beta build, but we'll see!): https://tbpl.mozilla.org/?tree=Try&rev=8f1dfb65ccef Just to remind me, this will need to be uplifted to aurora as well.

Last build didn't manage to make a beta build - another try build to try to verify this on a beta-ish build, for this bug + Automatic updates (bug 906339): https://tbpl.mozilla.org/?tree=Try&rev=8e1023fe869f

I wasn't able to ever get a beta/release to build correctly, so this is kind fo untested on release builds.

This adds tests for conditional settings based on the Gecko build flags in AppConstants, using reflection. It makes things a little less clean and separated because we can no longer use immutable arrays. In addition to handling text reflow based on RELEASE_BUILD, this patch also handles Crash Reporter and Telemetry. The patch for bug 906339 will handle Automatic updates. Try build: https://tbpl.mozilla.org/?tree=Try&rev=8e1023fe869f

Comment on attachment 794770 [details] [diff] [review] Part 2: tests for conditional settings > It makes things a little less clean I think it's fine this way, but you might be able to make it prettier using some combination of array initialization and Arrays.asList(). http://stackoverflow.com/questions/2760995/arraylist-initialization-equivalent-to-array-initialization

UI: https://hg.mozilla.org/integration/fx-team/rev/93742ae69c40 Tests: https://hg.mozilla.org/integration/fx-team/rev/7b5fe881c650

Aaron or philo, if you could find some way to spin these patches as a beta/release build, please verify that in Settings > Customize, "Text reflow" is not present; also, that the tests for a beta spin of these do not fail.

https://hg.mozilla.org/mozilla-central/rev/93742ae69c40 https://hg.mozilla.org/mozilla-central/rev/7b5fe881c650

(In reply to Chenxia Liu [:liuche] from comment #32) > Aaron or philo, if you could find some way to spin these patches as a > beta/release build, please verify that in Settings > Customize, "Text > reflow" is not present; also, that the tests for a beta spin of these do not > fail. Nightly will let us determine that.

Instead of porting over all the tests, this patch just uses an #ifndef RELEASE_BUILD for tests. Try build: https://tbpl.mozilla.org/?tree=Try&rev=fbcda47b0335

Comment on attachment 799189 [details] [diff] [review] Aurora patch: remove text reflow from release builds and tests for release builds v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): Restricting text reflow to non-release builds User impact if declined: text reflow will be visible in 25 Testing completed (on m-c, etc.): all green try aurora build (https://tbpl.mozilla.org/?tree=Try&rev=fbcda47b0335) Risk to taking this patch (and alternatives if risky): low, adds a conditional UI check and updates a test String or IDL/UUID changes made by this patch: none

Awaiting review before uplift to Aurora 25.

Verified as fixed on: Device: LG Nexus 4 (Android 4.2.2) Build: Firefox 25 Beta 8 / Aurora 26.0a2 (2013-10-16)

TCs updated accordingly: https://moztrap.mozilla.org/manage/case/10549/ https://moztrap.mozilla.org/manage/case/8225/ https://moztrap.mozilla.org/manage/case/8127/