Closed Bug 1073607 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
35.3
Tracking Status
firefox33 --- fixed
firefox34 --- verified
firefox35 --- unaffected

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Keywords: verifyme)

Attachments

(1 file, 1 obsolete file)

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.
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
Taking this for the upcoming iteration.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Flags: needinfo?(mmucci)
Assigned for IT 35.3
Flags: needinfo?(mmucci)
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?
Attachment #8497458 - Flags: review?(jaws)
QA Contact: cornel.ionce
QA Contact: cornel.ionce → bogdan.maris
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?
(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?
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.
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?
(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 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.
Attachment #8497458 - Flags: review?(jaws) → review+
Updated with the new list of locales per email, and switched to ifndef NIGHTLY_BUILD so the code runs on aurora (d'oh).
Attachment #8497458 - Attachment is obsolete: true
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?
Attachment #8500099 - Flags: review+
Attachment #8500099 - Flags: approval-mozilla-aurora?
Flags: needinfo?(dolske)
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.
Attachment #8500099 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(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 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/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).
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: verifyme
Resolution: --- → FIXED
(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? :-)
Target Milestone: --- → Firefox 34
(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.
Status: RESOLVED → VERIFIED
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.
Whiteboard: [fixed-alder]
You need to log in before you can comment on or make changes to this bug.