1073607 - Add clever prefs system to enable/disable panic button on per-locale basis for 33, but always enable in 34 beta

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

For:

- the anniversary release:
  -- button disabled by default
  -- enabled by default on locales with translations including en-US
  -- locale list could/should be a pref
- 34 beta
  -- button enabled by default
  -- second pref to toggle listening to the locale list (as on 33) so that QA can test the locale blocking/enabling mechanism

I'm splitting this off from bug 1069300 so we can work on finalizing the feature there ASAP so l10n can actually try it when localizing. We have a bit more time to get this bit right, and it should be a pretty limited-scope patch.

Comment 1

[:Gijs (he/him)]

Taking this for the upcoming iteration.

Comment 2

[Marco Mucci [:MarcoM]]

Assigned for IT 35.3

Comment 3

[:Gijs (he/him)]

Attachment: add magical pref system for panic button,

To test on Nightly, s/ifdef/ifndef/ . I went with a space-separated list because it's easier to hardcode in prefs.js (no quote escaping as you would for JSON, which also requires double quotes) and is less exception-happy than JSON. I kept the try...catch because exceptions here are pretty fatal (you get a non-functioning browser). Perhaps more of this should be in a try catch? Not sure. In any case, this works, as far as I can tell. What do you think?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8497458 [details] [diff] [review]
add magical pref system for panic button,

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +963,5 @@
>  #endif
>  
> +let isPanicButtonEnabled = Services.prefs.getBoolPref("privacy.panicButton.enabled");
> +#ifdef RELEASE_BUILD
> +if (Services.prefs.getBoolPref("privacy.panicButton.useLocaleList")) {

Whatever we're using to set the .useLocaleList=true, why not just set the panicButton.enabled=true? That would allow us to not need this .useLocaleList pref, right?

Otherwise, do we even need the .useLocaleList pref, could we just check to see if the browserLocale is in the enabledLocales array?

Comment 5

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Comment on attachment 8497458 [details] [diff] [review]
> add magical pref system for panic button,
> 
> Review of attachment 8497458 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/CustomizableWidgets.jsm
> @@ +963,5 @@
> >  #endif
> >  
> > +let isPanicButtonEnabled = Services.prefs.getBoolPref("privacy.panicButton.enabled");
> > +#ifdef RELEASE_BUILD
> > +if (Services.prefs.getBoolPref("privacy.panicButton.useLocaleList")) {
> 
> Whatever we're using to set the .useLocaleList=true, why not just set the
> panicButton.enabled=true? That would allow us to not need this
> .useLocaleList pref, right?
> 
> Otherwise, do we even need the .useLocaleList pref, could we just check to
> see if the browserLocale is in the enabledLocales array?


I think the basic premises I had here were:
1) we need a killswitch for the feature. That's the .enabled pref
2) above that, we want a way to have it enabled per-locale on beta. That's the list.
3) above that, we want QE to be able to test (2) on aurora. Hence the third pref, which is off by default on aurora (so everyone on aurora can continue to use it) and can easily be turned on by QE / localizers.

While we could reuse the first pref, I think the current implementation is clearer, and I don't think the cost of the extra pref here is important. Do you disagree, and/or do you have suggestions on how I could make this clearer in the patch?

Comment 6

[Justin Dolske [:Dolske]]

I think the current patch's pref makes sense to me -- a global killswitch (.enabled), and a per-local killswitch that can will disable the feature even when .enabled is true.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

The part that I find confusing is that when .useLocaleList=false the feature is disabled. When I read it, .useLocaleList=false implies that the list will be ignored and we will use whatever language is available (and falling back to en-US if none is provided). So maybe a better name would remove this confusion?

Comment 8

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> The part that I find confusing is that when .useLocaleList=false the feature
> is disabled.

No? When useLocaleList is false, the code acts the same as if RELEASE_BUILD isn't defined... meaning the feature being enabled/disabled depends purely on the .enabled pref. So:

> When I read it, .useLocaleList=false implies that the list will
> be ignored and we will use whatever language is available (and falling back
> to en-US if none is provided).

this is what's happening with the patch, I think? What am I missing?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8497458 [details] [diff] [review]
add magical pref system for panic button,

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

> isPanicButtonEnabled = isPanicButtonEnabled && enabledLocales.indexOf(browserLocale) != -1;

Oh sorry, I was referencing this line, but was clearly confused. I had forgot about the line above that checks to see if useLocaleList=true (line 967) and I think I mentally replaced the `isPanicButtonEnabled` with a check for useLocaleList==true. This is fine then.

Comment 10

[:Gijs (he/him)]

Attachment: add magical pref system for panic button,

Updated with the new list of locales per email, and switched to ifndef NIGHTLY_BUILD so the code runs on aurora (d'oh).

Comment 11

[:Gijs (he/him)]

Comment on attachment 8500099 [details] [diff] [review]
add magical pref system for panic button,

Dolske, does it make sense to land this directly on Aurora or do you think we should land on Nightly first?

Comment 12

[Axel Hecht [:Pike]]

So, my take on this would be to only land this on 33.

For 34, we'll have a different locale list, at least. We'll also have this localized, or partial localizations anyway, probably, so the value of doing a locale list on aurora would be so-so.

On top of that, aurora is the channel where localizers are working, so disabling the button will make testing hard.

Leaving the needinfo on dolske to confirm or disagree.

Comment 13

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch from comment #11)

> Dolske, does it make sense to land this directly on Aurora or do you think
> we should land on Nightly first?

My understanding was that this should (must) land on 33.0.1 with .useLocaleList == true, and that we'd also like to land it on 34 with .useLocaleList == false. The only purpose of the 34 landing is to allow earlier testing of it (by QA or localizers who flip that to true and perhaps edit .enabledLocales as needed).

It doesn't need to land on 35+, since it's primarily just a short-term thing for 33.

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 8500099 [details] [diff] [review]
add magical pref system for panic button,

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

::: browser/app/profile/firefox.js
@@ +553,5 @@
>  pref("privacy.sanitize.migrateFx3Prefs",    false);
>  
>  pref("privacy.panicButton.enabled",         true);
> +pref("privacy.panicButton.useLocaleList",   false);
> +pref("privacy.panicButton.enabledLocales",  "ast da de en-GB en-US es-AR es-CL es-ES es-MX fi fr fy-NL hu it ja ko lv nb-NO nn-NO pa-IN pl pt-BR rm ru sk sl zh-TW");

Can we add

he
ja-JP-mac

Comment 15

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/95cf39085d65

Once this has built, to verify:

1) toggle the privacy.panicButton.useLocaleList pref to true in about:config
2) verify that the locales in privacy.panicButton.enabledLocales pref get a button, and the ones not in there do not (e.g. French and en-US should work, nl (Dutch) is not in the list so shouldn't).
(you can set the pref to check if the list works, too, obviously - although for both of these prefs, a restart is required to make things work).

Comment 16

[:Gijs (he/him)]

(In reply to Axel Hecht [:Pike] from comment #14)
> Comment on attachment 8500099 [details] [diff] [review]
> add magical pref system for panic button,
> 
> Review of attachment 8500099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/profile/firefox.js
> @@ +553,5 @@
> >  pref("privacy.sanitize.migrateFx3Prefs",    false);
> >  
> >  pref("privacy.panicButton.enabled",         true);
> > +pref("privacy.panicButton.useLocaleList",   false);
> > +pref("privacy.panicButton.enabledLocales",  "ast da de en-GB en-US es-AR es-CL es-ES es-MX fi fr fy-NL hu it ja ko lv nb-NO nn-NO pa-IN pl pt-BR rm ru sk sl zh-TW");
> 
> Can we add
> 
> he
> ja-JP-mac

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/7b9f27b04e41

Considering people can change the prefs themselves, can we hold further update until we (have) uplift(ed) to 33? :-)

Comment 17

[Bogdan Maris, Desktop QA]

(In reply to :Gijs Kruitbosch from comment #15)
> remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/95cf39085d65
> 
> Once this has built, to verify:
> 
> 1) toggle the privacy.panicButton.useLocaleList pref to true in about:config
> 2) verify that the locales in privacy.panicButton.enabledLocales pref get a
> button, and the ones not in there do not (e.g. French and en-US should work,
> nl (Dutch) is not in the list so shouldn't).
> (you can set the pref to check if the list works, too, obviously - although
> for both of these prefs, a restart is required to make things work).

We tested on Windows 7 64bit, Windows 8.1 64bit, Mac OS X 10.9.5 and Ubuntu 14.04 32bit all the locales from 'privacy.panicButton.enabledLocales' and selected a few that are not in that list. The behaviour of the prefs are as expected. Information about our testing can be found in the etherpad: https://etherpad.mozilla.org/ForgetButton-Prefs

I will go ahead and close hit as Verified since it has reached it`s target milestone, will leave verifyme for verification in Firefox 33 when the moment comes.

Comment 18

[Justin Dolske [:Dolske]]

Landed in alder: https://hg.mozilla.org/projects/alder/rev/1599c644f54f

This is a rollup of https://hg.mozilla.org/releases/mozilla-aurora/rev/95cf39085d65 and https://hg.mozilla.org/releases/mozilla-aurora/rev/7b9f27b04e41 (comment 15 and comment 16). I also flipped the privacy.panicButton.useLocaleList to true, since that's what we want for release.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/1599c644f54f