Bug 785225 - Part 2: Minimize Engines singleton usage; r=rnewman
authorGregory Szorc <gps@mozilla.com>
Wed, 29 Aug 2012 14:43:40 -0700
changeset 111049 bc3e055a34a1bd859433af0d5eac54a9f8777aae
parent 111048 e10e5cf8fafa03f6a391f36f2defd109479b84e1
child 111050 79eba94e1758fb441ec2742aed30b0adaaa13be4
push id93
push usernmatsakis@mozilla.com
push dateWed, 31 Oct 2012 21:26:57 +0000
reviewersrnewman
bugs785225
milestone18.0a1
Bug 785225 - Part 2: Minimize Engines singleton usage; r=rnewman Weave.Engines is no longer exported. Service now exposes an EngineManager instance, which is the new recommended way to get at the engine manager. Service was updated to reference the internal instance.
browser/base/content/browser-places.js
browser/base/content/sync/aboutSyncTabs.js
browser/base/content/sync/progress.js
browser/base/content/sync/quota.js
browser/base/content/sync/setup.js
services/sync/modules/engines.js
services/sync/modules/main.js
services/sync/modules/service.js
services/sync/modules/stages/enginesync.js
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -647,18 +647,19 @@ HistoryMenu.prototype = {
     if (Weave.Status.checkSetup() == Weave.CLIENT_NOT_CONFIGURED ||
         Weave.Svc.Prefs.get("firstSync", "") == "notReady") {
       menuitem.setAttribute("hidden", true);
       return;
     }
 
     // The tabs engine might never be inited (if services.sync.registerEngines
     // is modified), so make sure we avoid undefined errors.
-    let enabled = Weave.Service.isLoggedIn && Weave.Engines.get("tabs") &&
-                  Weave.Engines.get("tabs").enabled;
+    let enabled = Weave.Service.isLoggedIn &&
+                  Weave.Service.engineManager.get("tabs") &&
+                  Weave.Service.engineManager.get("tabs").enabled;
     menuitem.setAttribute("disabled", !enabled);
     menuitem.setAttribute("hidden", false);
 #endif
   },
 
   toggleRestoreLastSession: function PHM_toggleRestoreLastSession() {
     let restoreItem = this._rootElt.getElementsByClassName("restoreLastSession")[0];
 
--- a/browser/base/content/sync/aboutSyncTabs.js
+++ b/browser/base/content/sync/aboutSyncTabs.js
@@ -125,17 +125,17 @@ let RemoteTabViewer = {
                                        , type: "folder"
                                        , URIList: URIs
                                        , hiddenRows: [ "description" ]
                                        }, window.top);
     }
   },
 
   _generateTabList: function() {
-    let engine = Weave.Engines.get("tabs");
+    let engine = Weave.Service.engineManager.get("tabs");
     let list = this._tabsList;
 
     // clear out existing richlistitems
     let count = list.getRowCount();
     if (count > 0) {
       for (let i = count - 1; i >= 0; i--)
         list.removeItemAt(i);
     }
@@ -210,17 +210,17 @@ let RemoteTabViewer = {
         return false;
     }
 
     // if Clients hasn't synced yet this session, need to sync it as well
     if (Weave.Clients.lastSync == 0)
       Weave.Clients.sync();
 
     // Force a sync only for the tabs engine
-    let engine = Weave.Engines.get("tabs");
+    let engine = Weave.Service.engineManager.get("tabs");
     engine.lastModified = null;
     engine.sync();
     Services.prefs.setIntPref("services.sync.lastTabFetch",
                               Math.floor(Date.now() / 1000));
 
     return true;
   },
 
--- a/browser/base/content/sync/progress.js
+++ b/browser/base/content/sync/progress.js
@@ -47,17 +47,17 @@ function onEngineSync(subject, topic, da
   // and evaluate how many engines are enabled when the first "real" engine
   // syncs.
   if (data == "clients") {
     return;
   }
 
   if (!gCounter &&
       Services.prefs.getPrefType("services.sync.firstSync") != Ci.nsIPrefBranch.PREF_INVALID) {
-    gProgressBar.max = Weave.Engines.getEnabled().length;
+    gProgressBar.max = Weave.Service.engineManager.getEnabled().length;
   }
 
   gCounter += 1;
   gProgressBar.setAttribute("value", gCounter);
 }
 
 function onServiceSync(subject, topic, data) {
   // To address the case where 0 engines are synced, we will fill the
--- a/browser/base/content/sync/quota.js
+++ b/browser/base/content/sync/quota.js
@@ -65,17 +65,17 @@ let gSyncQuota = {
       this._quota_req.abort();
     }
     return true;
   },
 
   onAccept: function onAccept() {
     let engines = gUsageTreeView.getEnginesToDisable();
     for each (let engine in engines) {
-      Weave.Engines.get(engine).enabled = false;
+      Weave.Service.engineManager.get(engine).enabled = false;
     }
     if (engines.length) {
       // The 'Weave' object will disappear once the window closes.
       let Service = Weave.Service;
       Weave.Utils.nextTick(function() { Service.sync(); });
     }
     return this.onCancel();
   },
@@ -95,17 +95,17 @@ let gUsageTreeView = {
   /*
    * Internal data structures underlaying the tree.
    */
   _collections: [],
   _byname: {},
 
   init: function init() {
     let retrievingLabel = gSyncQuota.bundle.getString("quota.retrieving.label");
-    for each (let engine in Weave.Engines.getEnabled()) {
+    for each (let engine in Weave.Service.engineManager.getEnabled()) {
       if (this._ignored[engine.name])
         continue;
 
       // Some engines use the same pref, which means they can only be turned on
       // and off together. We need to combine them here as well.
       let existing = this._byname[engine.prefName];
       if (existing) {
         existing.engines.push(engine.name);
--- a/browser/base/content/sync/setup.js
+++ b/browser/base/content/sync/setup.js
@@ -865,17 +865,17 @@ var gSyncSetup = {
     switch (desc) {
       case 1:
         if (this._case1Setup)
           break;
 
         let places_db = PlacesUtils.history
                                    .QueryInterface(Ci.nsPIPlacesDatabase)
                                    .DBConnection;
-        if (Weave.Engines.get("history").enabled) {
+        if (Weave.Service.engineManager.get("history").enabled) {
           let daysOfHistory = 0;
           let stm = places_db.createStatement(
             "SELECT ROUND(( " +
               "strftime('%s','now','localtime','utc') - " +
               "( " +
                 "SELECT visit_date FROM moz_historyvisits " +
                 "ORDER BY visit_date ASC LIMIT 1 " +
                 ")/1000000 " +
@@ -888,17 +888,17 @@ var gSyncSetup = {
             PluralForm.get(daysOfHistory,
                            this._stringBundle.GetStringFromName("historyDaysCount.label"))
                       .replace("%S", daysOfHistory)
                       .replace("#1", daysOfHistory);
         } else {
           document.getElementById("historyCount").hidden = true;
         }
 
-        if (Weave.Engines.get("bookmarks").enabled) {
+        if (Weave.Service.engineManager.get("bookmarks").enabled) {
           let bookmarks = 0;
           let stm = places_db.createStatement(
             "SELECT count(*) AS bookmarks " +
             "FROM moz_bookmarks b " +
             "LEFT JOIN moz_bookmarks t ON " +
             "b.parent = t.id WHERE b.type = 1 AND t.parent <> :tag");
           stm.params.tag = PlacesUtils.tagsFolderId;
           if (stm.executeStep())
@@ -908,29 +908,29 @@ var gSyncSetup = {
             PluralForm.get(bookmarks,
                            this._stringBundle.GetStringFromName("bookmarksCount.label"))
                       .replace("%S", bookmarks)
                       .replace("#1", bookmarks);
         } else {
           document.getElementById("bookmarkCount").hidden = true;
         }
 
-        if (Weave.Engines.get("passwords").enabled) {
+        if (Weave.Service.engineManager.get("passwords").enabled) {
           let logins = Services.logins.getAllLogins({});
           // Support %S for historical reasons (see bug 600141)
           document.getElementById("passwordCount").value =
             PluralForm.get(logins.length,
                            this._stringBundle.GetStringFromName("passwordsCount.label"))
                       .replace("%S", logins.length)
                       .replace("#1", logins.length);
         } else {
           document.getElementById("passwordCount").hidden = true;
         }
 
-        if (!Weave.Engines.get("prefs").enabled) {
+        if (!Weave.Service.engineManager.get("prefs").enabled) {
           document.getElementById("prefsWipe").hidden = true;
         }
 
         this._case1Setup = true;
         break;
       case 2:
         if (this._case2Setup)
           break;
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -1,19 +1,21 @@
 /* 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/. */
 
-const EXPORTED_SYMBOLS = ['Engines', 'Engine', 'SyncEngine',
-                          'Tracker', 'Store'];
+const EXPORTED_SYMBOLS = [
+  "Engines",
+  "Engine",
+  "SyncEngine",
+  "Tracker",
+  "Store"
+];
 
-const Cc = Components.classes;
-const Ci = Components.interfaces;
-const Cr = Components.results;
-const Cu = Components.utils;
+const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
 
 Cu.import("resource://services-common/async.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-common/observers.js");
 Cu.import("resource://services-sync/identity.js");
 Cu.import("resource://services-common/log4moz.js");
 Cu.import("resource://services-sync/resource.js");
@@ -346,31 +348,29 @@ Store.prototype = {
    * can be thought of as clearing out all state and restoring the "new
    * browser" state.
    */
   wipe: function Store_wipe() {
     throw "override wipe in a subclass";
   }
 };
 
-
-// Singleton service, holds registered engines
-
+// TODO remove singleton (bug 785225).
 XPCOMUtils.defineLazyGetter(this, "Engines", function() {
-  return new EngineManagerSvc();
+  return new EngineManager();
 });
 
-function EngineManagerSvc() {
+function EngineManager() {
   this._engines = {};
   this._log = Log4Moz.repository.getLogger("Sync.EngineManager");
   this._log.level = Log4Moz.Level[Svc.Prefs.get(
     "log.logger.service.engines", "Debug")];
 }
-EngineManagerSvc.prototype = {
-  get: function EngMgr_get(name) {
+EngineManager.prototype = {
+  get: function get(name) {
     // Return an array of engines if we have an array of names
     if (Array.isArray(name)) {
       let engines = [];
       name.forEach(function(name) {
         let engine = this.get(name);
         if (engine)
           engines.push(engine);
       }, this);
@@ -380,32 +380,34 @@ EngineManagerSvc.prototype = {
     let engine = this._engines[name];
     if (!engine) {
       this._log.debug("Could not get engine: " + name);
       if (Object.keys)
         this._log.debug("Engines are: " + JSON.stringify(Object.keys(this._engines)));
     }
     return engine;
   },
-  getAll: function EngMgr_getAll() {
+
+  getAll: function getAll() {
     return [engine for ([name, engine] in Iterator(Engines._engines))];
   },
-  getEnabled: function EngMgr_getEnabled() {
+
+  getEnabled: function getEnabled() {
     return this.getAll().filter(function(engine) engine.enabled);
   },
-  
+
   /**
    * Register an Engine to the service. Alternatively, give an array of engine
    * objects to register.
    *
    * @param engineObject
    *        Engine object used to get an instance of the engine
    * @return The engine object if anything failed
    */
-  register: function EngMgr_register(engineObject) {
+  register: function register(engineObject) {
     if (Array.isArray(engineObject))
       return engineObject.map(this.register, this);
 
     try {
       let engine = new engineObject();
       let name = engine.name;
       if (name in this._engines)
         this._log.error("Engine '" + name + "' is already registered!");
@@ -419,22 +421,23 @@ EngineManagerSvc.prototype = {
       name = name.name || "";
 
       let out = "Could not initialize engine '" + name + "': " + mesg;
       this._log.error(out);
 
       return engineObject;
     }
   },
-  unregister: function EngMgr_unregister(val) {
+
+  unregister: function unregister(val) {
     let name = val;
     if (val instanceof Engine)
       name = val.name;
     delete this._engines[name];
-  }
+  },
 };
 
 function Engine(name) {
   this.Name = name || "Unnamed";
   this.name = name.toLowerCase();
 
   this._notify = Utils.notify("weave:engine:");
   this._log = Log4Moz.repository.getLogger("Sync.Engine." + this.Name);
--- a/services/sync/modules/main.js
+++ b/services/sync/modules/main.js
@@ -3,17 +3,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const EXPORTED_SYMBOLS = ['Weave'];
 
 let Weave = {};
 Components.utils.import("resource://services-sync/constants.js", Weave);
 let lazies = {
   "record.js":            ["CollectionKeys"],
-  "engines.js":           ['Engines', 'Engine', 'SyncEngine', 'Store'],
+  "engines.js":           ['Engine', 'SyncEngine', 'Store'],
   "engines/addons.js":    ["AddonsEngine"],
   "engines/bookmarks.js": ['BookmarksEngine', 'BookmarksSharingManager'],
   "engines/clients.js":   ["Clients"],
   "engines/forms.js":     ["FormEngine"],
   "engines/history.js":   ["HistoryEngine"],
   "engines/prefs.js":     ["PrefsEngine"],
   "engines/passwords.js": ["PasswordEngine"],
   "engines/tabs.js":      ["TabEngine"],
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -59,16 +59,20 @@ WeaveSvc.prototype = {
   _identity: Weave.Identity,
 
   userBaseURL: null,
   infoURL: null,
   storageURL: null,
   metaURL: null,
   cryptoKeyURL: null,
 
+  get enabledEngineNames() {
+    return [e.name for each (e in this.engineManager.getEnabled())];
+  },
+
   get serverURL() Svc.Prefs.get("serverURL"),
   set serverURL(value) {
     // Only do work if it's actually changing
     if (value == this.serverURL)
       return;
 
     // A new server most likely uses a different cluster, so clear that
     Svc.Prefs.set("serverURL", value);
@@ -385,26 +389,29 @@ WeaveSvc.prototype = {
     oldPref.resetBranch("");
     Svc.Prefs.set("migrated", true);
   },
 
   /**
    * Register the built-in engines for certain applications
    */
   _registerEngines: function _registerEngines() {
+    // TODO Singleton (bug 785225).
+    this.engineManager = Engines;
+
     let engines = [];
     // Applications can provide this preference (comma-separated list)
     // to specify which engines should be registered on startup.
     let pref = Svc.Prefs.get("registerEngines");
     if (pref) {
       engines = pref.split(",");
     }
 
     // Grab the actual engines and register them
-    Engines.register(engines.map(function onItem(name) {
+    this.engineManager.register(engines.map(function onItem(name) {
       return Weave[name + "Engine"];
     }));
   },
 
   QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
                                          Ci.nsISupportsWeakReference]),
 
   // nsIObserver
@@ -776,17 +783,17 @@ WeaveSvc.prototype = {
     this._identity.syncKey = null;
     Status.login = LOGIN_FAILED_NO_PASSPHRASE;
     this.logout();
     Svc.Obs.notify("weave:service:start-over");
 
     // Deletion doesn't make sense if we aren't set up yet!
     if (this.clusterURL != "") {
       // Clear client-specific data from the server, including disabled engines.
-      for each (let engine in [Clients].concat(Engines.getAll())) {
+      for each (let engine in [Clients].concat(this.engineManager.getAll())) {
         try {
           engine.removeClientData();
         } catch(ex) {
           this._log.warn("Deleting client data for " + engine.name + " failed:"
                          + Utils.exceptionStr(ex));
         }
       }
     } else {
@@ -1195,19 +1202,18 @@ WeaveSvc.prototype = {
       // If we got into a race condition, we'll abort the sync this way, too.
       // That's fine. We'll just wait till the next sync. The client that we're
       // racing is probably busy uploading stuff right now anyway.
       throw resp;
     }
     Records.set(this.metaURL, meta);
 
     // Wipe everything we know about except meta because we just uploaded it
-    let collections = [Clients].concat(Engines.getAll()).map(function(engine) {
-      return engine.name;
-    });
+    let engines = [Clients].concat(this.engineManager.getAll());
+    let collections = [engine.name for each (engine in engines)];
 
     // Generate, upload, and download new keys. Do this last so we don't wipe
     // them...
     this.generateNewSymmetricKeys();
   },
 
   /**
    * Wipe user data from the server.
@@ -1270,21 +1276,21 @@ WeaveSvc.prototype = {
    *        Array of engine names to wipe. If not given, all engines are used.
    */
   wipeClient: function wipeClient(engines) {
     // If we don't have any engines, reset the service and wipe all engines
     if (!engines) {
       // Clear out any service data
       this.resetService();
 
-      engines = [Clients].concat(Engines.getAll());
+      engines = [Clients].concat(this.engineManager.getAll());
     }
     // Convert the array of names into engines
     else {
-      engines = Engines.get(engines);
+      engines = this.engineManager.get(engines);
     }
 
     // Fully wipe each engine if it's able to decrypt data
     for each (let engine in engines) {
       if (engine.canDecrypt()) {
         engine.wipeClient();
       }
     }
@@ -1346,21 +1352,21 @@ WeaveSvc.prototype = {
    */
   resetClient: function resetClient(engines) {
     this._catch(function doResetClient() {
       // If we don't have any engines, reset everything including the service
       if (!engines) {
         // Clear out any service data
         this.resetService();
 
-        engines = [Clients].concat(Engines.getAll());
+        engines = [Clients].concat(this.engineManager.getAll());
       }
       // Convert the array of names into engines
       else {
-        engines = Engines.get(engines);
+        engines = this.engineManager.get(engines);
       }
 
       // Have each engine drop any temporary meta data
       for each (let engine in engines) {
         engine.resetClient();
       }
     })();
   },
--- a/services/sync/modules/stages/enginesync.js
+++ b/services/sync/modules/stages/enginesync.js
@@ -74,17 +74,17 @@ EngineSynchronizer.prototype = {
       infoURL += "?v=" + WEAVE_VERSION;
       Svc.Prefs.set("lastPing", now);
     }
 
     // Figure out what the last modified time is for each collection
     let info = this.service._fetchInfo(infoURL);
 
     // Convert the response to an object and read out the modified times
-    for (let engine of [Clients].concat(Engines.getAll())) {
+    for (let engine of [Clients].concat(this.service.engineManager.getAll())) {
       engine.lastModified = info.obj[engine.name] || 0;
     }
 
     if (!(this.service._remoteSetup(info))) {
       this.onComplete(new Error("Aborting sync, remote setup failed"));
       return;
     }
 
@@ -96,23 +96,23 @@ EngineSynchronizer.prototype = {
       this._log.warn("Client engine sync failed. Aborting.");
       this.onComplete(null);
       return;
     }
 
     // Wipe data in the desired direction if necessary
     switch (Svc.Prefs.get("firstSync")) {
       case "resetClient":
-        this.service.resetClient(Engines.getEnabled().map(function(e) e.name));
+        this.service.resetClient(this.service.enabledEngineNames);
         break;
       case "wipeClient":
-        this.service.wipeClient(Engines.getEnabled().map(function(e) e.name));
+        this.service.wipeClient(this.service.enabledEngineNames);
         break;
       case "wipeRemote":
-        this.service.wipeRemote(Engines.getEnabled().map(function(e) e.name));
+        this.service.wipeRemote(this.service.enabledEngineNames);
         break;
     }
 
     if (Clients.localCommands) {
       try {
         if (!(Clients.processIncomingCommands())) {
           Status.sync = ABORT_SYNC_COMMAND;
           this.onComplete(new Error("Processed command aborted sync."));
@@ -141,17 +141,17 @@ EngineSynchronizer.prototype = {
       this._log.debug("Updating enabled engines failed: " +
                       Utils.exceptionStr(ex));
       ErrorHandler.checkServerError(ex);
       this.onComplete(ex);
       return;
     }
 
     try {
-      for each (let engine in Engines.getEnabled()) {
+      for each (let engine in this.service.engineManager.getEnabled()) {
         // If there's any problems with syncing the engine, report the failure
         if (!(this._syncEngine(engine)) || Status.enforceBackoff) {
           this._log.info("Aborting sync for failure in " + engine.name);
           break;
         }
       }
 
       // If _syncEngine fails for a 401, we might not have a cluster URL here.
@@ -221,29 +221,29 @@ EngineSynchronizer.prototype = {
     if ((SyncScheduler.numClients <= 1) &&
         ([e for (e in meta.payload.engines) if (e != "clients")].length == 0)) {
       this._log.info("One client and no enabled engines: not touching local engine status.");
       return;
     }
 
     this.service._ignorePrefObserver = true;
 
-    let enabled = [eng.name for each (eng in Engines.getEnabled())];
+    let enabled = this.service.enabledEngineNames;
     for (let engineName in meta.payload.engines) {
       if (engineName == "clients") {
         // Clients is special.
         continue;
       }
       let index = enabled.indexOf(engineName);
       if (index != -1) {
         // The engine is enabled locally. Nothing to do.
         enabled.splice(index, 1);
         continue;
       }
-      let engine = Engines.get(engineName);
+      let engine = this.service.engineManager.get(engineName);
       if (!engine) {
         // The engine doesn't exist locally. Nothing to do.
         continue;
       }
 
       if (Svc.Prefs.get("engineStatusChanged." + engine.prefName, false)) {
         // The engine was disabled locally. Wipe server data and
         // disable it everywhere.
@@ -255,17 +255,17 @@ EngineSynchronizer.prototype = {
         // The engine was enabled remotely. Enable it locally.
         this._log.trace(engineName + " engine was enabled remotely.");
         engine.enabled = true;
       }
     }
 
     // Any remaining engines were either enabled locally or disabled remotely.
     for each (let engineName in enabled) {
-      let engine = Engines.get(engineName);
+      let engine = this.service.engineManager.get(engineName);
       if (Svc.Prefs.get("engineStatusChanged." + engine.prefName, false)) {
         this._log.trace("The " + engineName + " engine was enabled locally.");
       } else {
         this._log.trace("The " + engineName + " engine was disabled remotely.");
         engine.enabled = false;
       }
     }