900564 - Remove 'double tap to reflow' from settings

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Readability, defect)

Description

[Ian Barlow (:ibarlow)]

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?

Comment 1

[Karen Rudnitski [:kar]]

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

Comment 2

[Scott Johnson (:jwir3)]

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

Comment 3

[Scott Johnson (:jwir3)]

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.

Comment 4

[Scott Johnson (:jwir3)]

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.

Comment 5

[Karen Rudnitski [:kar]]

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.

Comment 6

[Scott Johnson (:jwir3)]

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

Comment 7

[Scott Johnson (:jwir3)]

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

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 9

[Scott Johnson (:jwir3)]

(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?)

Comment 10

[Scott Johnson (:jwir3)]

Attachment: b900564-releasebranch

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

Comment 11

[Karen Rudnitski [:kar]]

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 12

[Mark Finkle (:mfinkle) (use needinfo?)]

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 13

[Scott Johnson (:jwir3)]

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.

Comment 14

[Scott Johnson (:jwir3)]

Attachment: b900564-general

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 15

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 18

[Scott Johnson (:jwir3)]

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

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 21

[bhavana bajaj [:bajaj]]

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 22

[bhavana bajaj [:bajaj]]

Comment on attachment 785029 [details] [diff] [review]
b900564-releasebranch

Approving for beta given comment #17

Comment 23

[Scott Johnson (:jwir3)]

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

Comment 24

[Aaron Train [:aaronmt]]

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

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 26

[Chenxia Liu [:liuche]]

Attachment: Patch: Remove "text reflow" in release + beta

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.

Comment 27

[Chenxia Liu [:liuche]]

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

Comment 28

[Chenxia Liu [:liuche]]

Attachment: Part 1: Remove text reflow from RELEASE_BUILD

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

Comment 29

[Chenxia Liu [:liuche]]

Attachment: Part 2: tests for conditional settings

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 30

[Geoff Brown [:gbrown]]

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

Comment 31

[Chenxia Liu [:liuche]]

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

Comment 32

[Chenxia Liu [:liuche]]

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.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 34

[Aaron Train [:aaronmt]]

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

Comment 35

[Chenxia Liu [:liuche]]

Attachment: Aurora patch: remove text reflow from release builds and tests for release builds v1

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 36

[Chenxia Liu [:liuche]]

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

Comment 37

[Alex Keybl [:akeybl]]

Awaiting review before uplift to Aurora 25.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd2d818af48a

Comment 39

[u421692]

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

Comment 40

[Ioana Chiorean]

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