Bug 1564131 - re-enable syncing of builtin themes. r=rpl a=RyanVM
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 18 Jul 2019 01:25:50 +0000
changeset 544633 046a7bd8885499ad1ba280dd8533a9bdacb06cf5
parent 544632 57d5f2ae8afd6e492cf0cf9dd6d62e2fa0b83cfb
child 544634 ec6c6890c684785000c5559c805f308ed427c94c
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrpl, RyanVM
bugs1564131
milestone69.0
Bug 1564131 - re-enable syncing of builtin themes. r=rpl a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D37837
browser/app/profile/firefox.js
services/sync/modules/engines/prefs.js
services/sync/tests/unit/test_prefs_store.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1224,16 +1224,17 @@ pref("services.sync.prefs.sync.browser.u
 pref("services.sync.prefs.sync.browser.urlbar.suggest.history", true);
 pref("services.sync.prefs.sync.browser.urlbar.suggest.openpage", true);
 pref("services.sync.prefs.sync.browser.urlbar.suggest.searches", true);
 pref("services.sync.prefs.sync.dom.disable_open_during_load", true);
 pref("services.sync.prefs.sync.dom.disable_window_flip", true);
 pref("services.sync.prefs.sync.dom.disable_window_move_resize", true);
 pref("services.sync.prefs.sync.dom.event.contextmenu.enabled", true);
 pref("services.sync.prefs.sync.extensions.update.enabled", true);
+pref("services.sync.prefs.sync.extensions.activeThemeID", true);
 pref("services.sync.prefs.sync.intl.accept_languages", true);
 pref("services.sync.prefs.sync.layout.spellcheckDefault", true);
 pref("services.sync.prefs.sync.media.autoplay.default", true);
 pref("services.sync.prefs.sync.media.eme.enabled", true);
 pref("services.sync.prefs.sync.network.cookie.cookieBehavior", true);
 pref("services.sync.prefs.sync.network.cookie.lifetimePolicy", true);
 pref("services.sync.prefs.sync.network.cookie.thirdparty.sessionOnly", true);
 pref("services.sync.prefs.sync.permissions.default.image", true);
--- a/services/sync/modules/engines/prefs.js
+++ b/services/sync/modules/engines/prefs.js
@@ -26,16 +26,22 @@ const { SCORE_INCREMENT_XLARGE } = Chrom
 const { CommonUtils } = ChromeUtils.import(
   "resource://services-common/utils.js"
 );
 
 XPCOMUtils.defineLazyGetter(this, "PREFS_GUID", () =>
   CommonUtils.encodeBase64URL(Services.appinfo.ID)
 );
 
+ChromeUtils.defineModuleGetter(
+  this,
+  "AddonManager",
+  "resource://gre/modules/AddonManager.jsm"
+);
+
 // In bug 1538015, we decided that it isn't always safe to allow all "incoming"
 // preferences to be applied locally. So we have introduced another preference,
 // which if false (the default) will ignore all incoming preferences which don't
 // already have the "control" preference locally set. If this new
 // preference is set to true, then we continue our old behavior of allowing all
 // preferences to be updated, even those which don't already have a local
 // "control" pref.
 const PREF_SYNC_PREFS_ARBITRARY =
@@ -195,16 +201,20 @@ PrefStore.prototype = {
           ? this._prefs.get(pref, null)
           : null;
       }
     }
     return values;
   },
 
   _setAllPrefs(values) {
+    const selectedThemeIDPref = "extensions.activeThemeID";
+    let selectedThemeIDBefore = this._prefs.get(selectedThemeIDPref, null);
+    let selectedThemeIDAfter = selectedThemeIDBefore;
+
     // Update 'services.sync.prefs.sync.foo.pref' before 'foo.pref', otherwise
     // _isSynced returns false when 'foo.pref' doesn't exist (e.g., on a new device).
     let prefs = Object.keys(values).sort(
       a => -a.indexOf(PREF_SYNC_PREFS_PREFIX)
     );
     for (let pref of prefs) {
       let value = values[pref];
       if (!this._isSynced(pref)) {
@@ -245,27 +255,65 @@ PrefStore.prototype = {
         continue;
       }
 
       if (typeof value == "string" && UNSYNCABLE_URL_REGEXP.test(value)) {
         this._log.trace(`Skipping incoming unsyncable url for pref: ${pref}`);
         continue;
       }
 
-      if (value == null) {
-        // Pref has gone missing. The best we can do is reset it.
-        this._prefs.reset(pref);
-      } else {
-        try {
-          this._prefs.set(pref, value);
-        } catch (ex) {
-          this._log.trace(`Failed to set pref: ${pref}`, ex);
-        }
+      switch (pref) {
+        // Some special prefs we don't want to set directly.
+        case selectedThemeIDPref:
+          selectedThemeIDAfter = value;
+          break;
+
+        // default is to just set the pref
+        default:
+          if (value == null) {
+            // Pref has gone missing. The best we can do is reset it.
+            this._prefs.reset(pref);
+          } else {
+            try {
+              this._prefs.set(pref, value);
+            } catch (ex) {
+              this._log.trace(`Failed to set pref: ${pref}`, ex);
+            }
+          }
       }
     }
+    // Themes are a little messy. Themes which have been installed are handled
+    // by the addons engine - but default themes aren't seen by that engine.
+    // So if there's a new default theme ID and that ID corresponds to a
+    // system addon, then we arrange to enable that addon here.
+    if (selectedThemeIDBefore != selectedThemeIDAfter) {
+      this._maybeEnableBuiltinTheme(selectedThemeIDAfter).catch(e => {
+        this._log.error("Failed to maybe update the default theme", e);
+      });
+    }
+  },
+
+  async _maybeEnableBuiltinTheme(themeId) {
+    let addon = null;
+    try {
+      addon = await AddonManager.getAddonByID(themeId);
+    } catch (ex) {
+      this._log.trace(
+        `There's no addon with ID '${themeId} - it can't be a builtin theme`
+      );
+      return;
+    }
+    if (addon && addon.isBuiltin && addon.type == "theme") {
+      this._log.trace(`Enabling builtin theme '${themeId}'`);
+      await addon.enable();
+    } else {
+      this._log.trace(
+        `Have incoming theme ID of '${themeId}' but it's not a builtin theme`
+      );
+    }
   },
 
   async getAllIDs() {
     /* We store all prefs in just one WBO, with just one GUID */
     let allprefs = {};
     allprefs[PREFS_GUID] = true;
     return allprefs;
   },
--- a/services/sync/tests/unit/test_prefs_store.js
+++ b/services/sync/tests/unit/test_prefs_store.js
@@ -6,45 +6,62 @@ const { Preferences } = ChromeUtils.impo
 );
 const { PrefRec } = ChromeUtils.import(
   "resource://services-sync/engines/prefs.js"
 );
 const { Service } = ChromeUtils.import("resource://services-sync/service.js");
 
 const PREFS_GUID = CommonUtils.encodeBase64URL(Services.appinfo.ID);
 
+const DEFAULT_THEME_ID = "default-theme@mozilla.org";
+const COMPACT_THEME_ID = "firefox-compact-light@mozilla.org";
+
 AddonTestUtils.init(this);
 AddonTestUtils.createAppInfo(
   "xpcshell@tests.mozilla.org",
   "XPCShell",
   "1",
   "1.9.2"
 );
 AddonTestUtils.overrideCertDB();
-AddonTestUtils.awaitPromise(AddonTestUtils.promiseStartupManager());
-
-function makePersona(id) {
-  return {
-    id: id || Math.random().toString(),
-    name: Math.random().toString(),
-    headerURL: "http://localhost:1234/a",
-  };
-}
 
 add_task(async function run_test() {
   _("Test fixtures.");
+  // Part of this test ensures the default theme, via the preference
+  // extensions.activeThemeID, is synced correctly - so we do a little
+  // addons initialization to allow this to work.
+
+  // Enable application scopes to ensure the builtin theme is going to
+  // be installed as part of the the addon manager startup.
+  Preferences.set("extensions.enabledScopes", AddonManager.SCOPE_APPLICATION);
+  await AddonTestUtils.promiseStartupManager();
+
+  // Install another built-in theme.
+  await AddonManager.installBuiltinAddon("resource:///modules/themes/light/");
+
+  const defaultThemeAddon = await AddonManager.getAddonByID(DEFAULT_THEME_ID);
+  ok(defaultThemeAddon, "Got an addon wrapper for the default theme");
+
+  const otherThemeAddon = await AddonManager.getAddonByID(COMPACT_THEME_ID);
+  ok(otherThemeAddon, "Got an addon wrapper for the compact theme");
+
+  await otherThemeAddon.enable();
+
   // read our custom prefs file before doing anything.
   Services.prefs.readDefaultPrefsFromFile(
     do_get_file("prefs_test_prefs_store.js")
   );
 
   let engine = Service.engineManager.get("prefs");
   let store = engine._store;
   let prefs = new Preferences();
   try {
+    _("Expect the compact light theme to be active");
+    Assert.strictEqual(prefs.get("extensions.activeThemeID"), COMPACT_THEME_ID);
+
     _("The GUID corresponds to XUL App ID.");
     let allIDs = await store.getAllIDs();
     let ids = Object.keys(allIDs);
     Assert.equal(ids.length, 1);
     Assert.equal(ids[0], PREFS_GUID);
     Assert.ok(allIDs[PREFS_GUID]);
 
     Assert.ok(await store.itemExists(PREFS_GUID));
@@ -137,16 +154,17 @@ add_task(async function run_test() {
     );
     // And a pref without a local control pref but one that's incoming.
     prefs.set(
       "testing.deleted-with-incoming-control-pref",
       "I'm deleted-with-incoming-control-pref"
     );
     record = new PrefRec("prefs", PREFS_GUID);
     record.value = {
+      "extensions.activeThemeID": DEFAULT_THEME_ID,
       "testing.int": 42,
       "testing.string": "im in ur prefs",
       "testing.bool": false,
       "testing.deleted-without-control-pref": null,
       "testing.deleted-with-local-control-pref": null,
       "testing.deleted-with-incoming-control-pref": null,
       "services.sync.prefs.sync.testing.deleted-with-incoming-control-pref": true,
       "testing.somepref": "im a new pref from other device",
@@ -155,16 +173,19 @@ add_task(async function run_test() {
       // we consider unsyncable.
       "testing.synced.url": "blob:ebeb707a-502e-40c6-97a5-dd4bda901463",
       // Make sure we can replace the unsynced URL with a valid URL.
       "testing.unsynced.url": "https://www.example.com/2",
       // Make sure our "master control pref" is ignored.
       "services.sync.prefs.dangerously_allow_arbitrary": true,
       "services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary": true,
     };
+
+    const onceAddonEnabled = AddonTestUtils.promiseAddonEvent("onEnabled");
+
     await store.update(record);
     Assert.strictEqual(prefs.get("testing.int"), 42);
     Assert.strictEqual(prefs.get("testing.string"), "im in ur prefs");
     Assert.strictEqual(prefs.get("testing.bool"), false);
     Assert.strictEqual(
       prefs.get("testing.deleted-without-control-pref"),
       "I'm deleted-without-control-pref"
     );
@@ -196,16 +217,26 @@ add_task(async function run_test() {
     );
     Assert.strictEqual(
       prefs.get(
         "services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary"
       ),
       undefined
     );
 
+    await onceAddonEnabled;
+    ok(
+      !defaultThemeAddon.userDisabled,
+      "the default theme should have been enabled"
+    );
+    ok(
+      otherThemeAddon.userDisabled,
+      "the compact theme should have been disabled"
+    );
+
     _("Only the current app's preferences are applied.");
     record = new PrefRec("prefs", "some-fake-app");
     record.value = {
       "testing.int": 98,
     };
     await store.update(record);
     Assert.equal(prefs.get("testing.int"), 42);
   } finally {