1309589 - Cleanup old loop.* preferences in profiles after Firefox Hello removal

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Mark Banner (:standard8)]

Firefox Hello set a number of preferences in profiles for those that were using it.

To save prefs.js getting ever bigger, we should clear these out.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1309589 - Cleanup old loop.* preferences in profiles after Firefox Hello removal.

Review commit: https://reviewboard.mozilla.org/r/85238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85238/

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8800257 [details]
Bug 1309589 - Cleanup old loop.* preferences in profiles after Firefox Hello removal.

https://reviewboard.mozilla.org/r/85238/#review84106

I couldn't be more pleased with that unit test! It's a great example for any future nsBrowserGlue.js addition.
Thanks!

::: browser/components/nsBrowserGlue.js:2109
(Diff revision 1)
>          Services.prefs.clearUserPref(kOldSafeBrowsingPref);
>        }
>      }
>  
> +    if (currentUIVersion < 41) {
> +      let Preferences = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;

nit: not that it matters much, but you *can* make this a `const` too.

::: browser/components/nsBrowserGlue.js:2110
(Diff revision 1)
>        }
>      }
>  
> +    if (currentUIVersion < 41) {
> +      let Preferences = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
> +      Preferences.resetBranch("loop.");

Ah! <3 Preferences.jsm

::: browser/components/tests/unit/head.js:5
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var Ci = Components.interfaces;

nits: Can you make this `const {interfaces: Ci, classes: Cc, results: Cr, utils: Cu} = Components;`

and keep `gProfD` as a const?

::: browser/components/tests/unit/test_browserGlue_migration_loop_cleanup.js:5
(Diff revision 1)
> +const UI_VERSION = 41;
> +const TOPIC_BROWSERGLUE_TEST = "browser-glue-test";
> +const TOPICDATA_BROWSERGLUE_TEST = "force-ui-migration";
> +
> +var gBrowserGlue = Components.classes["@mozilla.org/browser/browserglue;1"]

nit: `Cc` and `Ci` can now be used here.

::: browser/components/tests/unit/test_browserGlue_migration_loop_cleanup.js:9
(Diff revision 1)
> +
> +var gBrowserGlue = Components.classes["@mozilla.org/browser/browserglue;1"]
> +                     .getService(Components.interfaces.nsIObserver);
> +
> +add_task(function* setup() {
> +  Services.prefs.setIntPref("browser.migration.version", UI_VERSION - 1);

nit: This particular setup step doesn't need to be in a separate task, but you can execute it right away.

::: browser/components/tests/unit/test_browserGlue_migration_loop_cleanup.js:17
(Diff revision 1)
> +add_task(function* test_check_cleanup_loop_prefs() {
> +  Services.prefs.setBoolPref("loop.createdRoom", true);
> +  Services.prefs.setBoolPref("loop1.createdRoom", true);
> +  Services.prefs.setBoolPref("loo.createdRoom", true);
> +
> +  // Simulate a migration

nit: missing dot.

::: browser/components/tests/unit/test_browserGlue_migration_loop_cleanup.js:22
(Diff revision 1)
> +  // Simulate a migration
> +  gBrowserGlue.observe(null, TOPIC_BROWSERGLUE_TEST, TOPICDATA_BROWSERGLUE_TEST);
> +
> +  Assert.throws(() => {
> +    Services.prefs.getBoolPref("loop.createdRoom")
> +  }, /NS_ERROR_UNEXPECTED/, "should have cleared old loop preference 'loop.createdRoom'");

nit: for one-line arrow functions you can omit the braces. If you keep the braces, please add a semi-colon at the end of this statement.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8800257 [details]
Bug 1309589 - Cleanup old loop.* preferences in profiles after Firefox Hello removal.

https://reviewboard.mozilla.org/r/85238/#review84106

> nits: Can you make this `const {interfaces: Ci, classes: Cc, results: Cr, utils: Cu} = Components;`
> 
> and keep `gProfD` as a const?

Note: gProfD was left as var, as one of the test files imports from elsewhere, which also defines gProfD.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8800257 [details]
Bug 1309589 - Cleanup old loop.* preferences in profiles after Firefox Hello removal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85238/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c07923f57466
Cleanup old loop.* preferences in profiles after Firefox Hello removal. r=mikedeboer

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c07923f57466