From d5bbddcc3e1070d2c2c87739afde673ebd4ba7a2 Mon Sep 17 00:00:00 2001 From: Marco Castelluccio Date: Mon, 31 Jul 2017 20:18:57 +0200 Subject: [PATCH] Bug 1357517 - Remove Preferences.jsm usage from some browser/extensions/ modules. r=Gijs --HG-- extra : rebase_source : 88c4cec30759e18d77e7e492747a58364ad82474 --- .../extensions/activity-stream/bootstrap.js | 19 ++--- .../clicktoplay-rollout/bootstrap.js | 30 ++++---- browser/extensions/e10srollout/bootstrap.js | 64 ++++++++--------- browser/extensions/onboarding/bootstrap.js | 44 ++++++++---- .../onboarding/content/onboarding.js | 30 ++++---- .../lib/PreferenceExperiments.jsm | 70 ++++++++++++++----- .../lib/ShieldRecipeClient.jsm | 46 ++++++++---- .../browser/browser_PreferenceExperiments.js | 60 +++++++++------- 8 files changed, 221 insertions(+), 142 deletions(-) diff --git a/browser/extensions/activity-stream/bootstrap.js b/browser/extensions/activity-stream/bootstrap.js index 7ea0423634e5..64284787400b 100644 --- a/browser/extensions/activity-stream/bootstrap.js +++ b/browser/extensions/activity-stream/bootstrap.js @@ -7,13 +7,12 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components; Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.importGlobalProperties(["fetch"]); -XPCOMUtils.defineLazyModuleGetter(this, "Preferences", - "resource://gre/modules/Preferences.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm"); const ACTIVITY_STREAM_ENABLED_PREF = "browser.newtabpage.activity-stream.enabled"; const BROWSER_READY_NOTIFICATION = "sessionstore-windows-restored"; +const PREF_CHANGED_TOPIC = "nsPref:changed"; const REASON_SHUTDOWN_ON_PREF_CHANGE = "PREF_OFF"; const REASON_STARTUP_ON_PREF_CHANGE = "PREF_ON"; const RESOURCE_BASE = "resource://activity-stream"; @@ -84,10 +83,9 @@ function uninit(reason) { /** * onPrefChanged - handler for changes to ACTIVITY_STREAM_ENABLED_PREF * - * @param {bool} isEnabled Determines whether Activity Stream is enabled */ -function onPrefChanged(isEnabled) { - if (isEnabled) { +function onPrefChanged() { + if (Services.prefs.getBoolPref(ACTIVITY_STREAM_ENABLED_PREF, false)) { init(REASON_STARTUP_ON_PREF_CHANGE); } else { uninit(REASON_SHUTDOWN_ON_PREF_CHANGE); @@ -101,10 +99,10 @@ function onBrowserReady() { waitingForBrowserReady = false; // Listen for changes to the pref that enables Activity Stream - Preferences.observe(ACTIVITY_STREAM_ENABLED_PREF, onPrefChanged); + Services.prefs.addObserver(ACTIVITY_STREAM_ENABLED_PREF, observe); // Only initialize if the pref is true - if (Preferences.get(ACTIVITY_STREAM_ENABLED_PREF)) { + if (Services.prefs.getBoolPref(ACTIVITY_STREAM_ENABLED_PREF, false)) { init(startupReason); } } @@ -119,6 +117,11 @@ function observe(subject, topic, data) { // Avoid running synchronously during this event that's used for timing Services.tm.dispatchToMainThread(() => onBrowserReady()); break; + case PREF_CHANGED_TOPIC: + if (data == ACTIVITY_STREAM_ENABLED_PREF) { + onPrefChanged(); + } + break; } } @@ -152,7 +155,7 @@ this.shutdown = function shutdown(data, reason) { Services.obs.removeObserver(observe, BROWSER_READY_NOTIFICATION); } else { // Stop listening to the pref that enables Activity Stream - Preferences.ignore(ACTIVITY_STREAM_ENABLED_PREF, onPrefChanged); + Services.prefs.removeObserver(ACTIVITY_STREAM_ENABLED_PREF, observe); } // Unload any add-on modules that might might have been imported diff --git a/browser/extensions/clicktoplay-rollout/bootstrap.js b/browser/extensions/clicktoplay-rollout/bootstrap.js index 6ccf98618283..18ca7a3ec4b3 100644 --- a/browser/extensions/clicktoplay-rollout/bootstrap.js +++ b/browser/extensions/clicktoplay-rollout/bootstrap.js @@ -6,7 +6,7 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components; -Cu.import("resource://gre/modules/Preferences.jsm"); +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/UpdateUtils.jsm"); Cu.import("resource://gre/modules/AppConstants.jsm"); @@ -46,13 +46,13 @@ function defineCohort() { return; } - let cohort = Preferences.get(PREF_COHORT_NAME); + let cohort = Services.prefs.getStringPref(PREF_COHORT_NAME); if (!cohort) { // The cohort has not been defined yet: this is the first // time that we're running. Let's see if the user has // a non-default setting to avoid changing it. - let currentPluginState = Preferences.get(PREF_FLASH_STATE); + let currentPluginState = Services.prefs.getIntPref(PREF_FLASH_STATE); switch (currentPluginState) { case Ci.nsIPluginTag.STATE_CLICKTOPLAY: cohort = "early-adopter-ctp"; @@ -81,8 +81,8 @@ function defineCohort() { if (userSample < testThreshold) { cohort = "test"; - let defaultPrefs = new Preferences({defaultBranch: true}); - defaultPrefs.set(PREF_FLASH_STATE, Ci.nsIPluginTag.STATE_CLICKTOPLAY); + let defaultPrefs = Services.prefs.getDefaultBranch(""); + defaultPrefs.setIntPref(PREF_FLASH_STATE, Ci.nsIPluginTag.STATE_CLICKTOPLAY); } else if (userSample >= 1.0 - testThreshold) { cohort = "control"; } else { @@ -105,21 +105,19 @@ function defineCohort() { } function getUserSample() { - let prefValue = Preferences.get(PREF_COHORT_SAMPLE, undefined); - let value = 0.0; + let prefType = Services.prefs.getPrefType(PREF_COHORT_SAMPLE); - if (typeof(prefValue) == "string") { - value = parseFloat(prefValue, 10); - return value; + if (prefType == Ci.nsIPrefBranch.PREF_STRING) { + return parseFloat(Services.prefs.getStringPref(PREF_COHORT_SAMPLE), 10); } - value = Math.random(); - Preferences.set(PREF_COHORT_SAMPLE, value.toString().substr(0, 8)); + let value = Math.random(); + Services.prefs.setStringPref(PREF_COHORT_SAMPLE, value.toString().substr(0, 8)); return value; } function setCohort(cohortName) { - Preferences.set(PREF_COHORT_NAME, cohortName); + Services.prefs.setStringPref(PREF_COHORT_NAME, cohortName); TelemetryEnvironment.setExperimentActive("clicktoplay-rollout", cohortName); try { @@ -130,10 +128,10 @@ function setCohort(cohortName) { } function watchForPrefChanges() { - Preferences.observe(PREF_FLASH_STATE, function prefWatcher() { - let currentCohort = Preferences.get(PREF_COHORT_NAME, "unknown"); + Services.prefs.addObserver(PREF_FLASH_STATE, function prefWatcher() { + let currentCohort = Services.prefs.getStringPref(PREF_COHORT_NAME, "unknown"); setCohort(`user-changed-from-${currentCohort}`); - Preferences.ignore(PREF_FLASH_STATE, prefWatcher); + Services.prefs.removeObserver(PREF_FLASH_STATE, prefWatcher); }); } diff --git a/browser/extensions/e10srollout/bootstrap.js b/browser/extensions/e10srollout/bootstrap.js index b0358dac8939..514d2bf75dce 100644 --- a/browser/extensions/e10srollout/bootstrap.js +++ b/browser/extensions/e10srollout/bootstrap.js @@ -6,7 +6,6 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components; -Cu.import("resource://gre/modules/Preferences.jsm"); Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/UpdateUtils.jsm"); Cu.import("resource://gre/modules/AppConstants.jsm"); @@ -111,13 +110,13 @@ function defineCohort() { let addonPolicy = "unknown"; if (updateChannel in ADDON_ROLLOUT_POLICY) { addonPolicy = ADDON_ROLLOUT_POLICY[updateChannel]; - Preferences.set(PREF_E10S_ADDON_POLICY, addonPolicy); + Services.prefs.setStringPref(PREF_E10S_ADDON_POLICY, addonPolicy); // This is also the proper place to set the blocklist pref // in case it is necessary. - Preferences.set(PREF_E10S_ADDON_BLOCKLIST, ""); + Services.prefs.setStringPref(PREF_E10S_ADDON_BLOCKLIST, ""); } else { - Preferences.reset(PREF_E10S_ADDON_POLICY); + Services.prefs.clearUserPref(PREF_E10S_ADDON_POLICY); } let userOptedOut = optedOut(); @@ -125,7 +124,7 @@ function defineCohort() { let disqualified = (Services.appinfo.multiprocessBlockPolicy != 0); let testThreshold = TEST_THRESHOLD[updateChannel]; let testGroup = (getUserSample(false) < testThreshold); - let hasNonExemptAddon = Preferences.get(PREF_E10S_HAS_NONEXEMPT_ADDON, false); + let hasNonExemptAddon = Services.prefs.getBoolPref(PREF_E10S_HAS_NONEXEMPT_ADDON, false); let temporaryDisqualification = getTemporaryDisqualification(); let temporaryQualification = getTemporaryQualification(); @@ -154,24 +153,24 @@ function defineCohort() { // to activate e10s because the backend doesn't know about it. E10S_STATUS // here will be accumulated as "2 - Disabled", which is fine too. setCohort(`temp-disqualified-${temporaryDisqualification}`); - Preferences.reset(PREF_TOGGLE_E10S); - Preferences.reset(PREF_E10S_PROCESSCOUNT + ".web"); + Services.prefs.clearUserPref(PREF_TOGGLE_E10S); + Services.prefs.clearUserPref(PREF_E10S_PROCESSCOUNT + ".web"); } else if (!disqualified && testThreshold < 1.0 && temporaryQualification != "") { // Users who are qualified for e10s and on channels where some population // would not receive e10s can be pushed into e10s anyway via a temporary // qualification which overrides the user sample value when non-empty. setCohort(`temp-qualified-${temporaryQualification}`); - Preferences.set(PREF_TOGGLE_E10S, true); + Services.prefs.setBoolPref(PREF_TOGGLE_E10S, true); eligibleForMulti = true; } else if (testGroup) { setCohort(`${cohortPrefix}test`); - Preferences.set(PREF_TOGGLE_E10S, true); + Services.prefs.setBoolPref(PREF_TOGGLE_E10S, true); eligibleForMulti = true; } else { setCohort(`${cohortPrefix}control`); - Preferences.reset(PREF_TOGGLE_E10S); - Preferences.reset(PREF_E10S_PROCESSCOUNT + ".web"); + Services.prefs.clearUserPref(PREF_TOGGLE_E10S); + Services.prefs.clearUserPref(PREF_E10S_PROCESSCOUNT + ".web"); } // Now determine if this user should be in the e10s-multi experiment. @@ -189,7 +188,7 @@ function defineCohort() { !eligibleForMulti || userOptedIn.multi || disqualified) { - Preferences.reset(PREF_E10S_PROCESSCOUNT + ".web"); + Services.prefs.clearUserPref(PREF_E10S_PROCESSCOUNT + ".web"); return; } @@ -211,7 +210,7 @@ function defineCohort() { setCohort(`${cohortPrefix}multiBucket${sampleName}`); // NB: Coerce sampleName to an integer because this is an integer pref. - Preferences.set(PREF_E10S_PROCESSCOUNT + ".web", +sampleName); + Services.prefs.setIntPref(PREF_E10S_PROCESSCOUNT + ".web", +sampleName); break; } } @@ -225,27 +224,26 @@ function uninstall() { function getUserSample(multi) { let pref = multi ? (PREF_COHORT_SAMPLE + ".multi") : PREF_COHORT_SAMPLE; - let prefValue = Preferences.get(pref, undefined); - let value = 0.0; + let prefType = Services.prefs.getPrefType(pref); - if (typeof(prefValue) == "string") { - value = parseFloat(prefValue, 10); - return value; + if (prefType == Ci.nsIPrefBranch.PREF_STRING) { + return parseFloat(Services.prefs.getStringPref(pref), 10); } - if (typeof(prefValue) == "number") { + let value = 0.0; + if (prefType == Ci.nsIPrefBranch.PREF_INT) { // convert old integer value - value = prefValue / 100; + value = Services.prefs.getIntPref(pref) / 100; } else { value = Math.random(); } - Preferences.set(pref, value.toString().substr(0, 8)); + Services.prefs.setStringPref(pref, value.toString().substr(0, 8)); return value; } function setCohort(cohortName) { - Preferences.set(PREF_COHORT_NAME, cohortName); + Services.prefs.setStringPref(PREF_COHORT_NAME, cohortName); if (cohortName != "unsupportedChannel") { TelemetryEnvironment.setExperimentActive("e10sCohort", cohortName); } @@ -257,10 +255,10 @@ function setCohort(cohortName) { } function optedIn() { - let e10s = Preferences.get(PREF_E10S_OPTED_IN, false) || - Preferences.get(PREF_E10S_FORCE_ENABLED, false); - let multi = Preferences.isSet(PREF_E10S_PROCESSCOUNT) || - !Preferences.get(PREF_USE_DEFAULT_PERF_SETTINGS, true); + let e10s = Services.prefs.getBoolPref(PREF_E10S_OPTED_IN, false) || + Services.prefs.getBoolPref(PREF_E10S_FORCE_ENABLED, false); + let multi = Services.prefs.prefHasUserValue(PREF_E10S_PROCESSCOUNT) || + !Services.prefs.getBoolPref(PREF_USE_DEFAULT_PERF_SETTINGS, true); return { e10s, multi }; } @@ -268,10 +266,10 @@ function optedOut() { // Users can also opt-out by toggling back the pref to false. // If they reset the pref instead they might be re-enabled if // they are still part of the threshold. - let e10s = Preferences.get(PREF_E10S_FORCE_DISABLED, false) || - (Preferences.isSet(PREF_TOGGLE_E10S) && - Preferences.get(PREF_TOGGLE_E10S) == false); - let multi = Preferences.get(PREF_E10S_MULTI_OPTOUT, 0) >= + let e10s = Services.prefs.getBoolPref(PREF_E10S_FORCE_DISABLED, false) || + (Services.prefs.prefHasUserValue(PREF_TOGGLE_E10S) && + Services.prefs.getBoolPref(PREF_TOGGLE_E10S) == false); + let multi = Services.prefs.getIntPref(PREF_E10S_MULTI_OPTOUT, 0) >= Services.appinfo.E10S_MULTI_EXPERIMENT; return { e10s, multi }; } @@ -298,7 +296,7 @@ function getTemporaryQualification() { // system. If this pref is set, then it means the user has opened DevTools at // some point in time. const PREF_OPENED_DEVTOOLS = "devtools.telemetry.tools.opened.version"; - let hasOpenedDevTools = Preferences.isSet(PREF_OPENED_DEVTOOLS); + let hasOpenedDevTools = Services.prefs.prefHasUserValue(PREF_OPENED_DEVTOOLS); if (hasOpenedDevTools) { return "devtools"; } @@ -307,6 +305,6 @@ function getTemporaryQualification() { } function getAddonsDisqualifyForMulti() { - return Preferences.get("extensions.e10sMultiBlocksEnabling", false) && - Preferences.get("extensions.e10sMultiBlockedByAddons", false); + return Services.prefs.getBoolPref("extensions.e10sMultiBlocksEnabling", false) && + Services.prefs.getBoolPref("extensions.e10sMultiBlockedByAddons", false); } diff --git a/browser/extensions/onboarding/bootstrap.js b/browser/extensions/onboarding/bootstrap.js index cabf6b855848..aa97f066a82a 100644 --- a/browser/extensions/onboarding/bootstrap.js +++ b/browser/extensions/onboarding/bootstrap.js @@ -5,26 +5,26 @@ /* globals APP_STARTUP, ADDON_INSTALL */ "use strict"; -const {utils: Cu} = Components; +const {utils: Cu, interfaces: Ci} = Components; Cu.import("resource://gre/modules/XPCOMUtils.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "OnboardingTourType", "resource://onboarding/modules/OnboardingTourType.jsm"); -XPCOMUtils.defineLazyModuleGetter(this, "Preferences", - "resource://gre/modules/Preferences.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts", "resource://gre/modules/FxAccounts.jsm"); +const {PREF_STRING, PREF_BOOL, PREF_INT} = Ci.nsIPrefBranch; + const BROWSER_READY_NOTIFICATION = "browser-delayed-startup-finished"; const BROWSER_SESSION_STORE_NOTIFICATION = "sessionstore-windows-restored"; const PREF_WHITELIST = [ - "browser.onboarding.enabled", - "browser.onboarding.hidden", - "browser.onboarding.notification.finished", - "browser.onboarding.notification.prompt-count", - "browser.onboarding.notification.last-time-of-changing-tour-sec", - "browser.onboarding.notification.tour-ids-queue" + ["browser.onboarding.enabled", PREF_BOOL], + ["browser.onboarding.hidden", PREF_BOOL], + ["browser.onboarding.notification.finished", PREF_BOOL], + ["browser.onboarding.notification.prompt-count", PREF_INT], + ["browser.onboarding.notification.last-time-of-changing-tour-sec", PREF_INT], + ["browser.onboarding.notification.tour-ids-queue", PREF_STRING], ]; [ @@ -37,7 +37,7 @@ const PREF_WHITELIST = [ "onboarding-tour-search", "onboarding-tour-singlesearch", "onboarding-tour-sync", -].forEach(tourId => PREF_WHITELIST.push(`browser.onboarding.tour.${tourId}.completed`)); +].forEach(tourId => PREF_WHITELIST.push([`browser.onboarding.tour.${tourId}.completed`, PREF_BOOL])); let waitingForBrowserReady = true; @@ -53,8 +53,28 @@ let waitingForBrowserReady = true; **/ function setPrefs(prefs) { prefs.forEach(pref => { - if (PREF_WHITELIST.includes(pref.name)) { - Preferences.set(pref.name, pref.value); + let prefObj = PREF_WHITELIST.find(([name, ]) => name == pref.name); + if (!prefObj) { + return; + } + + let [name, type] = prefObj; + + switch (type) { + case PREF_BOOL: + Services.prefs.setBoolPref(name, pref.value); + break; + + case PREF_INT: + Services.prefs.setIntPref(name, pref.value); + break; + + case PREF_STRING: + Services.prefs.setStringPref(name, pref.value); + break; + + default: + throw new TypeError(`Unexpected type (${type}) for preference ${name}.`) } }); } diff --git a/browser/extensions/onboarding/content/onboarding.js b/browser/extensions/onboarding/content/onboarding.js index f202d148ada2..2db0f42f86d0 100644 --- a/browser/extensions/onboarding/content/onboarding.js +++ b/browser/extensions/onboarding/content/onboarding.js @@ -7,8 +7,8 @@ "use strict"; const {classes: Cc, interfaces: Ci, utils: Cu} = Components; +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource://gre/modules/Services.jsm"); -Cu.import("resource://gre/modules/Preferences.jsm"); const ONBOARDING_CSS_URL = "resource://onboarding/onboarding.css"; const ABOUT_HOME_URL = "about:home"; @@ -421,14 +421,14 @@ class Onboarding { }); }); for (let [name, callback] of this._prefsObserved) { - Preferences.observe(name, callback); + Services.prefs.addObserver(name, callback); } } _clearPrefObserver() { if (this._prefsObserved) { for (let [name, callback] of this._prefsObserved) { - Preferences.ignore(name, callback); + Services.prefs.removeObserver(name, callback); } this._prefsObserved = null; } @@ -537,7 +537,7 @@ class Onboarding { } isTourCompleted(tourId) { - return Preferences.get(`browser.onboarding.tour.${tourId}.completed`, false); + return Services.prefs.getBoolPref(`browser.onboarding.tour.${tourId}.completed`, false); } setToursCompleted(tourIds) { @@ -564,12 +564,12 @@ class Onboarding { } _muteNotificationOnFirstSession() { - if (Preferences.isSet("browser.onboarding.notification.tour-ids-queue")) { + if (Services.prefs.prefHasUserValue("browser.onboarding.notification.tour-ids-queue")) { // There is a queue. We had prompted before, this must not be the 1st session. return false; } - let muteDuration = Preferences.get("browser.onboarding.notification.mute-duration-on-first-session-ms"); + let muteDuration = Services.prefs.getIntPref("browser.onboarding.notification.mute-duration-on-first-session-ms"); if (muteDuration == 0) { // Don't mute when this is set to 0 on purpose. return false; @@ -577,7 +577,7 @@ class Onboarding { // Reuse the `last-time-of-changing-tour-sec` to save the time that // we try to prompt on the 1st session. - let lastTime = 1000 * Preferences.get("browser.onboarding.notification.last-time-of-changing-tour-sec", 0); + let lastTime = 1000 * Services.prefs.getIntPref("browser.onboarding.notification.last-time-of-changing-tour-sec", 0); if (lastTime <= 0) { this.sendMessageToChrome("set-prefs", [{ name: "browser.onboarding.notification.last-time-of-changing-tour-sec", @@ -589,14 +589,14 @@ class Onboarding { } _isTimeForNextTourNotification() { - let promptCount = Preferences.get("browser.onboarding.notification.prompt-count", 0); - let maxCount = Preferences.get("browser.onboarding.notification.max-prompt-count-per-tour"); + let promptCount = Services.prefs.getIntPref("browser.onboarding.notification.prompt-count", 0); + let maxCount = Services.prefs.getIntPref("browser.onboarding.notification.max-prompt-count-per-tour"); if (promptCount >= maxCount) { return true; } - let lastTime = 1000 * Preferences.get("browser.onboarding.notification.last-time-of-changing-tour-sec", 0); - let maxTime = Preferences.get("browser.onboarding.notification.max-life-time-per-tour-ms"); + let lastTime = 1000 * Services.prefs.getIntPref("browser.onboarding.notification.last-time-of-changing-tour-sec", 0); + let maxTime = Services.prefs.getIntPref("browser.onboarding.notification.max-life-time-per-tour-ms"); if (lastTime && Date.now() - lastTime >= maxTime) { return true; } @@ -624,8 +624,8 @@ class Onboarding { _getNotificationQueue() { let queue = ""; - if (Preferences.isSet("browser.onboarding.notification.tour-ids-queue")) { - queue = Preferences.get("browser.onboarding.notification.tour-ids-queue"); + if (Services.prefs.prefHasUserValue("browser.onboarding.notification.tour-ids-queue")) { + queue = Services.prefs.getStringPref("browser.onboarding.notification.tour-ids-queue"); } else { // For each tour, it only gets 2 chances to prompt with notification // (each chance includes 8 impressions or 5-days max life time) @@ -645,7 +645,7 @@ class Onboarding { } showNotification() { - if (Preferences.get("browser.onboarding.notification.finished", false)) { + if (Services.prefs.getBoolPref("browser.onboarding.notification.finished", false)) { return; } @@ -710,7 +710,7 @@ class Onboarding { value: queue.join(",") }); } else { - let promptCount = Preferences.get(PROMPT_COUNT_PREF, 0); + let promptCount = Services.prefs.getIntPref(PROMPT_COUNT_PREF, 0); params.push({ name: PROMPT_COUNT_PREF, value: promptCount + 1 diff --git a/browser/extensions/shield-recipe-client/lib/PreferenceExperiments.jsm b/browser/extensions/shield-recipe-client/lib/PreferenceExperiments.jsm index 7b19233bf783..b3ad42d527bf 100644 --- a/browser/extensions/shield-recipe-client/lib/PreferenceExperiments.jsm +++ b/browser/extensions/shield-recipe-client/lib/PreferenceExperiments.jsm @@ -58,7 +58,6 @@ XPCOMUtils.defineLazyModuleGetter(this, "CleanupManager", "resource://shield-rec XPCOMUtils.defineLazyModuleGetter(this, "JSONFile", "resource://gre/modules/JSONFile.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "LogManager", "resource://shield-recipe-client/lib/LogManager.jsm"); -XPCOMUtils.defineLazyModuleGetter(this, "Preferences", "resource://gre/modules/Preferences.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "TelemetryEnvironment", "resource://gre/modules/TelemetryEnvironment.jsm"); this.EXPORTED_SYMBOLS = ["PreferenceExperiments"]; @@ -71,14 +70,15 @@ const PREFERENCE_TYPE_MAP = { integer: Services.prefs.PREF_INT, }; -const DefaultPreferences = new Preferences({defaultBranch: true}); +const UserPreferences = Services.prefs; +const DefaultPreferences = Services.prefs.getDefaultBranch(""); /** * Enum storing Preference modules for each type of preference branch. * @enum {Object} */ const PreferenceBranchType = { - user: Preferences, + user: UserPreferences, default: DefaultPreferences, }; @@ -101,6 +101,41 @@ const log = LogManager.getLogger("preference-experiments"); let experimentObservers = new Map(); CleanupManager.addCleanupHandler(() => PreferenceExperiments.stopAllObservers()); +function getPref(prefBranch, prefName, prefType, defaultVal) { + switch (prefType) { + case "boolean": + return prefBranch.getBoolPref(prefName, defaultVal); + + case "string": + return prefBranch.getStringPref(prefName, defaultVal); + + case "integer": + return prefBranch.getIntPref(prefName, defaultVal); + + default: + throw new TypeError(`Unexpected preference type (${prefType}) for ${prefName}.`); + } +} + +function setPref(prefBranch, prefName, prefType, prefValue) { + switch (prefType) { + case "boolean": + prefBranch.setBoolPref(prefName, prefValue); + break; + + case "string": + prefBranch.setStringPref(prefName, prefValue); + break; + + case "integer": + prefBranch.setIntPref(prefName, prefValue); + break; + + default: + throw new TypeError(`Unexpected preference type (${prefType}) for ${prefName}.`); + } +} + this.PreferenceExperiments = { /** * Set the default preference value for active experiments that use the @@ -110,11 +145,11 @@ this.PreferenceExperiments = { for (const experiment of await this.getAllActive()) { // Set experiment default preferences, since they don't persist between restarts if (experiment.preferenceBranchType === "default") { - DefaultPreferences.set(experiment.preferenceName, experiment.preferenceValue); + setPref(DefaultPreferences, experiment.preferenceName, experiment.preferenceType, experiment.preferenceValue); } // Check that the current value of the preference is still what we set it to - if (Preferences.get(experiment.preferenceName, undefined) !== experiment.preferenceValue) { + if (getPref(UserPreferences, experiment.preferenceName, experiment.preferenceType) !== experiment.preferenceValue) { // if not, stop the experiment, and skip the remaining steps log.info(`Stopping experiment "${experiment.name}" because its value changed`); await this.stop(experiment.name, false); @@ -125,7 +160,7 @@ this.PreferenceExperiments = { TelemetryEnvironment.setExperimentActive(experiment.name, experiment.branch); // Watch for changes to the experiment's preference - this.startObserver(experiment.name, experiment.preferenceName, experiment.preferenceValue); + this.startObserver(experiment.name, experiment.preferenceName, experiment.preferenceType, experiment.preferenceValue); } }, @@ -207,7 +242,7 @@ this.PreferenceExperiments = { preferenceName, preferenceValue, preferenceType, - previousPreferenceValue: preferences.get(preferenceName, undefined), + previousPreferenceValue: getPref(preferences, preferenceName, preferenceType), preferenceBranchType, }; @@ -225,8 +260,8 @@ this.PreferenceExperiments = { ); } - preferences.set(preferenceName, preferenceValue); - PreferenceExperiments.startObserver(name, preferenceName, preferenceValue); + setPref(preferences, preferenceName, preferenceType, preferenceValue); + PreferenceExperiments.startObserver(name, preferenceName, preferenceType, preferenceValue); store.data[name] = experiment; store.saveSoon(); @@ -242,7 +277,7 @@ this.PreferenceExperiments = { * @throws {Error} * If an observer for the named experiment is already active. */ - startObserver(experimentName, preferenceName, preferenceValue) { + startObserver(experimentName, preferenceName, preferenceType, preferenceValue) { log.debug(`PreferenceExperiments.startObserver(${experimentName})`); if (experimentObservers.has(experimentName)) { @@ -253,7 +288,8 @@ this.PreferenceExperiments = { const observerInfo = { preferenceName, - observer(newValue) { + observer() { + let newValue = getPref(UserPreferences, preferenceName, preferenceType); if (newValue !== preferenceValue) { PreferenceExperiments.stop(experimentName, false) .catch(Cu.reportError); @@ -261,7 +297,7 @@ this.PreferenceExperiments = { }, }; experimentObservers.set(experimentName, observerInfo); - Preferences.observe(preferenceName, observerInfo.observer); + Services.prefs.addObserver(preferenceName, observerInfo.observer); }, /** @@ -288,7 +324,7 @@ this.PreferenceExperiments = { } const {preferenceName, observer} = experimentObservers.get(experimentName); - Preferences.ignore(preferenceName, observer); + Services.prefs.removeObserver(preferenceName, observer); experimentObservers.delete(experimentName); }, @@ -298,7 +334,7 @@ this.PreferenceExperiments = { stopAllObservers() { log.debug("PreferenceExperiments.stopAllObservers()"); for (const {preferenceName, observer} of experimentObservers.values()) { - Preferences.ignore(preferenceName, observer); + Services.prefs.removeObserver(preferenceName, observer); } experimentObservers.clear(); }, @@ -352,15 +388,15 @@ this.PreferenceExperiments = { } if (resetValue) { - const {preferenceName, previousPreferenceValue, preferenceBranchType} = experiment; + const {preferenceName, preferenceType, previousPreferenceValue, preferenceBranchType} = experiment; const preferences = PreferenceBranchType[preferenceBranchType]; if (previousPreferenceValue !== undefined) { - preferences.set(preferenceName, previousPreferenceValue); + setPref(preferences, preferenceName, preferenceType, previousPreferenceValue); } else { // This does nothing if we're on the default branch, which is fine. The // preference will be reset on next restart, and most preferences should // have had a default value set before the experiment anyway. - preferences.reset(preferenceName); + preferences.clearUserPref(preferenceName); } } diff --git a/browser/extensions/shield-recipe-client/lib/ShieldRecipeClient.jsm b/browser/extensions/shield-recipe-client/lib/ShieldRecipeClient.jsm index 7498b8e1d2c2..cadbac2eeea7 100644 --- a/browser/extensions/shield-recipe-client/lib/ShieldRecipeClient.jsm +++ b/browser/extensions/shield-recipe-client/lib/ShieldRecipeClient.jsm @@ -3,9 +3,8 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ "use strict"; -const {utils: Cu} = Components; +const {utils: Cu, interfaces: Ci} = Components; Cu.import("resource://gre/modules/Services.jsm"); -Cu.import("resource://gre/modules/Preferences.jsm"); Cu.import("resource://gre/modules/Log.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); @@ -20,6 +19,8 @@ XPCOMUtils.defineLazyModuleGetter(this, "PreferenceExperiments", this.EXPORTED_SYMBOLS = ["ShieldRecipeClient"]; +const {PREF_STRING, PREF_BOOL, PREF_INT} = Ci.nsIPrefBranch; + const REASONS = { APP_STARTUP: 1, // The application is starting up. APP_SHUTDOWN: 2, // The application is shutting down. @@ -32,14 +33,14 @@ const REASONS = { }; const PREF_BRANCH = "extensions.shield-recipe-client."; const DEFAULT_PREFS = { - api_url: "https://normandy.cdn.mozilla.net/api/v1", - dev_mode: false, - enabled: true, - startup_delay_seconds: 300, - "logging.level": Log.Level.Warn, - user_id: "", - run_interval_seconds: 86400, // 24 hours - first_run: true, + api_url: ["https://normandy.cdn.mozilla.net/api/v1", PREF_STRING], + dev_mode: [false, PREF_BOOL], + enabled: [true, PREF_BOOL], + startup_delay_seconds: [300, PREF_INT], + "logging.level": [Log.Level.Warn, PREF_INT], + user_id: ["", PREF_STRING], + run_interval_seconds: [86400, PREF_INT], // 24 hours + first_run: [true, PREF_BOOL], }; const PREF_DEV_MODE = "extensions.shield-recipe-client.dev_mode"; const PREF_LOGGING_LEVEL = PREF_BRANCH + "logging.level"; @@ -57,9 +58,9 @@ this.ShieldRecipeClient = { // Setup logging and listen for changes to logging prefs LogManager.configure(Services.prefs.getIntPref(PREF_LOGGING_LEVEL)); - Preferences.observe(PREF_LOGGING_LEVEL, LogManager.configure); + Services.prefs.addObserver(PREF_LOGGING_LEVEL, LogManager.configure); CleanupManager.addCleanupHandler( - () => Preferences.ignore(PREF_LOGGING_LEVEL, LogManager.configure), + () => Services.prefs.removeObserver(PREF_LOGGING_LEVEL, LogManager.configure), ); log = LogManager.getLogger("bootstrap"); @@ -79,11 +80,26 @@ this.ShieldRecipeClient = { }, setDefaultPrefs() { - for (const [key, val] of Object.entries(DEFAULT_PREFS)) { + for (const [key, [val, type]] of Object.entries(DEFAULT_PREFS)) { const fullKey = PREF_BRANCH + key; // If someone beat us to setting a default, don't overwrite it. - if (!Preferences.isSet(fullKey)) { - Preferences.set(fullKey, val); + if (!Services.prefs.prefHasUserValue(fullKey)) { + switch (type) { + case PREF_BOOL: + Services.prefs.setBoolPref(fullKey, val); + break; + + case PREF_INT: + Services.prefs.setIntPref(fullKey, val); + break; + + case PREF_STRING: + Services.prefs.setStringPref(fullKey, val); + break; + + default: + throw new TypeError(`Unexpected type (${type}) for preference ${fullKey}.`) + } } } }, diff --git a/browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js b/browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js index 63756fbc1f9c..b67a86d90951 100644 --- a/browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js +++ b/browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js @@ -97,7 +97,7 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc }); ok("test" in experiments, "start saved the experiment"); ok( - startObserver.calledWith("test", "fake.preference", "newvalue"), + startObserver.calledWith("test", "fake.preference", "string", "newvalue"), "start registered an observer", ); @@ -144,7 +144,7 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc preferenceBranchType: "user", }); ok( - startObserver.calledWith("test", "fake.preference", "newvalue"), + startObserver.calledWith("test", "fake.preference", "string", "newvalue"), "start registered an observer", ); @@ -194,9 +194,9 @@ add_task(withMockPreferences(async function(mockPreferences) { // startObserver should throw if an observer for the experiment is already // active. add_task(withMockExperiments(async function() { - PreferenceExperiments.startObserver("test", "fake.preference", "newvalue"); + PreferenceExperiments.startObserver("test", "fake.preference", "string", "newvalue"); Assert.throws( - () => PreferenceExperiments.startObserver("test", "another.fake", "othervalue"), + () => PreferenceExperiments.startObserver("test", "another.fake", "string", "othervalue"), "startObserver threw due to a conflicting active observer", ); PreferenceExperiments.stopAllObservers(); @@ -205,26 +205,34 @@ add_task(withMockExperiments(async function() { // startObserver should register an observer that calls stop when a preference // changes from its experimental value. add_task(withMockExperiments(withMockPreferences(async function(mockExperiments, mockPreferences) { - const stop = sinon.stub(PreferenceExperiments, "stop"); - mockPreferences.set("fake.preference", "startvalue"); + let tests = [ + ["string", "startvalue", "experimentvalue", "newvalue"], + ["boolean", false, true, false], + ["integer", 1, 2, 42], + ]; - // NOTE: startObserver does not modify the pref - PreferenceExperiments.startObserver("test", "fake.preference", "experimentvalue"); + for (let [type, startvalue, experimentvalue, newvalue] of tests) { + const stop = sinon.stub(PreferenceExperiments, "stop"); + mockPreferences.set("fake.preference" + type, startvalue); - // Setting it to the experimental value should not trigger the call. - Preferences.set("fake.preference", "experimentvalue"); - ok(!stop.called, "Changing to the experimental pref value did not trigger the observer"); + // NOTE: startObserver does not modify the pref + PreferenceExperiments.startObserver("test" + type, "fake.preference" + type, type, experimentvalue); - // Setting it to something different should trigger the call. - Preferences.set("fake.preference", "newvalue"); - ok(stop.called, "Changing to a different value triggered the observer"); + // Setting it to the experimental value should not trigger the call. + Preferences.set("fake.preference" + type, experimentvalue); + ok(!stop.called, "Changing to the experimental pref value did not trigger the observer"); - PreferenceExperiments.stopAllObservers(); - stop.restore(); + // Setting it to something different should trigger the call. + Preferences.set("fake.preference" + type, newvalue); + ok(stop.called, "Changing to a different value triggered the observer"); + + PreferenceExperiments.stopAllObservers(); + stop.restore(); + } }))); add_task(withMockExperiments(async function testHasObserver() { - PreferenceExperiments.startObserver("test", "fake.preference", "experimentValue"); + PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentValue"); ok(await PreferenceExperiments.hasObserver("test"), "hasObserver detects active observers"); ok( @@ -248,7 +256,7 @@ add_task(withMockExperiments(withMockPreferences(async function(mockExperiments, const stop = sinon.stub(PreferenceExperiments, "stop"); mockPreferences.set("fake.preference", "startvalue"); - PreferenceExperiments.startObserver("test", "fake.preference", "experimentvalue"); + PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue"); PreferenceExperiments.stopObserver("test"); // Setting the preference now that the observer is stopped should not call @@ -259,7 +267,7 @@ add_task(withMockExperiments(withMockPreferences(async function(mockExperiments, // Now that the observer is stopped, start should be able to start a new one // without throwing. try { - PreferenceExperiments.startObserver("test", "fake.preference", "experimentvalue"); + PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue"); } catch (err) { ok(false, "startObserver did not throw an error for an observer that was already stopped"); } @@ -274,8 +282,8 @@ add_task(withMockExperiments(withMockPreferences(async function(mockExperiments, mockPreferences.set("fake.preference", "startvalue"); mockPreferences.set("other.fake.preference", "startvalue"); - PreferenceExperiments.startObserver("test", "fake.preference", "experimentvalue"); - PreferenceExperiments.startObserver("test2", "other.fake.preference", "experimentvalue"); + PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue"); + PreferenceExperiments.startObserver("test2", "other.fake.preference", "string", "experimentvalue"); PreferenceExperiments.stopAllObservers(); // Setting the preference now that the observers are stopped should not call @@ -287,8 +295,8 @@ add_task(withMockExperiments(withMockPreferences(async function(mockExperiments, // Now that the observers are stopped, start should be able to start new // observers without throwing. try { - PreferenceExperiments.startObserver("test", "fake.preference", "experimentvalue"); - PreferenceExperiments.startObserver("test2", "other.fake.preference", "experimentvalue"); + PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue"); + PreferenceExperiments.startObserver("test2", "other.fake.preference", "string", "experimentvalue"); } catch (err) { ok(false, "startObserver did not throw an error for an observer that was already stopped"); } @@ -349,7 +357,7 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc previousPreferenceValue: "oldvalue", preferenceBranchType: "default", }); - PreferenceExperiments.startObserver("test", "fake.preference", "experimentvalue"); + PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue"); await PreferenceExperiments.stop("test"); ok(stopObserver.calledWith("test"), "stop removed an observer"); @@ -377,7 +385,7 @@ add_task(withMockExperiments(withMockPreferences(async function(experiments, moc previousPreferenceValue: "oldvalue", preferenceBranchType: "user", }); - PreferenceExperiments.startObserver("test", "fake.preference", "experimentvalue"); + PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue"); await PreferenceExperiments.stop("test"); ok(stopObserver.calledWith("test"), "stop removed an observer"); @@ -654,7 +662,7 @@ add_task(withMockExperiments(withMockPreferences(async function testInitRegister await PreferenceExperiments.init(); ok( - startObserver.calledWith("test", "fake.preference", "experiment value"), + startObserver.calledWith("test", "fake.preference", "string", "experiment value"), "init registered an observer", );