Closed Bug 1309589 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: standard8, Assigned: standard8)

Details

Attachments

(1 file)

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 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.
Attachment #8800257 - Flags: review?(mdeboer) → review+
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.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c07923f57466
Cleanup old loop.* preferences in profiles after Firefox Hello removal. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/c07923f57466
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.