Bug 1146068 (part 1) - fix scheduler to match readinglist sync engine implementation. r=adw
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 25 Mar 2015 16:28:18 +1100
changeset 264386 c12a9bd2cc2be89715046881c4504e973b3ee4b6
parent 264385 fbeeabe755e755a9641ea9bd5657daba8e79b2b4
child 264387 497bcc0a0143e9f5ea0c93cc09257aa92e6e490c
push id4718
push userraliiev@mozilla.com
push dateMon, 11 May 2015 18:39:53 +0000
treeherdermozilla-beta@c20c4ef55f08 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1146068
milestone39.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1146068 (part 1) - fix scheduler to match readinglist sync engine implementation. r=adw
browser/components/readinglist/Scheduler.jsm
browser/components/readinglist/test/xpcshell/test_scheduler.js
--- a/browser/components/readinglist/Scheduler.jsm
+++ b/browser/components/readinglist/Scheduler.jsm
@@ -3,94 +3,92 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict;"
 
 const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
+Cu.import('resource://gre/modules/Task.jsm');
 
 
 XPCOMUtils.defineLazyModuleGetter(this, 'LogManager',
   'resource://services-common/logmanager.js');
 
 XPCOMUtils.defineLazyModuleGetter(this, 'Log',
   'resource://gre/modules/Log.jsm');
 
 XPCOMUtils.defineLazyModuleGetter(this, 'Preferences',
   'resource://gre/modules/Preferences.jsm');
 
 XPCOMUtils.defineLazyModuleGetter(this, 'setTimeout',
   'resource://gre/modules/Timer.jsm');
 XPCOMUtils.defineLazyModuleGetter(this, 'clearTimeout',
   'resource://gre/modules/Timer.jsm');
 
-Cu.import('resource://gre/modules/Task.jsm');
+// The main readinglist module.
+XPCOMUtils.defineLazyModuleGetter(this, 'ReadingList',
+  'resource:///modules/readinglist/ReadingList.jsm');
+
+// The "engine"
+XPCOMUtils.defineLazyModuleGetter(this, 'Sync',
+  'resource:///modules/readinglist/Sync.jsm');
+
 
 this.EXPORTED_SYMBOLS = ["ReadingListScheduler"];
 
 // A list of "external" observer topics that may cause us to change when we
 // sync.
 const OBSERVERS = [
   // We don't sync when offline and restart when online.
   "network:offline-status-changed",
   // FxA notifications also cause us to check if we should sync.
   "fxaccounts:onverified",
-  // When something notices a local change to an item.
-  "readinglist:item-changed",
   // some notifications the engine might send if we have been requested to backoff.
   "readinglist:backoff-requested",
   // request to sync now
   "readinglist:user-sync",
 
 ];
 
-///////// A temp object until we get our "engine"
-let engine = {
-  ERROR_AUTHENTICATION: "authentication error",
-  sync: Task.async(function* () {
-  }),
-}
-
 let prefs = new Preferences("readinglist.scheduler.");
 
 // A helper to manage our interval values.
 let intervals = {
   // Getters for our intervals.
   _fixupIntervalPref(prefName, def) {
     // All pref values are seconds, but we return ms.
     return prefs.get(prefName, def) * 1000;
   },
 
   // How long after startup do we do an initial sync?
-  get initial() this._fixupIntervalPref("initial", 20), // 20 seconds.
+  get initial() this._fixupIntervalPref("initial", 10), // 10 seconds.
   // Every interval after the first.
   get schedule() this._fixupIntervalPref("schedule", 2 * 60 * 60), // 2 hours
-  // After we've been told an item has changed
-  get dirty() this._fixupIntervalPref("dirty", 2 * 60), // 2 mins
   // After an error
   get retry() this._fixupIntervalPref("retry", 2 * 60), // 2 mins
 };
 
 // This is the implementation, but it's not exposed directly.
-function InternalScheduler() {
+function InternalScheduler(readingList = null) {
   // oh, I don't know what logs yet - let's guess!
   let logs = [
     "browserwindow.syncui",
     "FirefoxAccounts",
     "readinglist.api",
     "readinglist.serverclient",
     "readinglist.sync",
   ];
 
   this._logManager = new LogManager("readinglist.", logs, "readinglist");
   this.log = Log.repository.getLogger("readinglist.scheduler");
   this.log.info("readinglist scheduler created.")
   this.state = this.STATE_OK;
+  this.readingList = readingList || ReadingList; // hook point for tests.
 
   // don't this.init() here, but instead at the module level - tests want to
   // add hooks before it is called.
 }
 
 InternalScheduler.prototype = {
   // When the next scheduled sync should happen.  If we can sync, there will
   // be a timer set to fire then. If we can't sync there will not be a timer,
@@ -100,34 +98,55 @@ InternalScheduler.prototype = {
   // schedule a new timer before this.
   _backoffUntil: 0,
   // Our current timer.
   _timer: null,
   // Our timer fires a promise - _timerRunning is true until it resolves or
   // rejects.
   _timerRunning: false,
   // Our sync engine - XXX - maybe just a callback?
-  _engine: engine,
+  _engine: Sync,
 
   // Our state variable and constants.
   state: null,
   STATE_OK: "ok",
   STATE_ERROR_AUTHENTICATION: "authentication error",
   STATE_ERROR_OTHER: "other error",
 
   init() {
     this.log.info("scheduler initialzing");
+    this._setupRLListener();
     this._observe = this.observe.bind(this);
     for (let notification of OBSERVERS) {
       Services.obs.addObserver(this._observe, notification, false);
     }
     this._nextScheduledSync = Date.now() + intervals.initial;
     this._setupTimer();
   },
 
+  _setupRLListener() {
+    let maybeSync = () => {
+      if (this._timerRunning) {
+        // If a sync is currently running it is possible it will miss the change
+        // just made, so tell the timer the next sync should be 1 ms after
+        // it completes (we don't use zero as that has special meaning re backoffs)
+        this._maybeReschedule(1);
+      } else {
+        // Do the sync now.
+        this._syncNow();
+      }
+    };
+    let listener = {
+      onItemAdded: maybeSync,
+      onItemUpdated: maybeSync,
+      onItemDeleted: maybeSync,
+    }
+    this.readingList.addListener(listener);
+  },
+
   // Note: only called by tests.
   finalize() {
     this.log.info("scheduler finalizing");
     this._clearTimer();
     for (let notification of OBSERVERS) {
       Services.obs.removeObserver(this._observe, notification);
     }
     this._observe = null;
@@ -143,19 +162,16 @@ InternalScheduler.prototype = {
           this.log.warn("Backoff request had non-numeric value", data);
           return;
         }
         this.log.info("Received a request to backoff for ${} seconds", interval);
         this._backoffUntil = Date.now() + interval * 1000;
         this._maybeReschedule(0);
         break;
       }
-      case "readinglist:local:dirty":
-        this._maybeReschedule(intervals.dirty);
-        break;
       case "readinglist:user-sync":
         this._syncNow();
         break;
       case "fxaccounts:onverified":
         // If we were in an authentication error state, reset that now.
         if (this.state == this.STATE_ERROR_AUTHENTICATION) {
           this.state = this.STATE_OK;
         }
@@ -236,35 +252,35 @@ InternalScheduler.prototype = {
       return;
     }
     let now = Date.now();
     if (!this._nextScheduledSync) {
       this._nextScheduledSync = now + delay;
     }
     // If there is something currently scheduled before the requested delay,
     // keep the existing value (eg, if we have a timer firing in 1 second, and
-    // get a "dirty" notification that says we should sync in 2 seconds, we
-    // keep the 1 second value)
+    // get a notification that says we should sync in 2 seconds, we keep the 1
+    // second value)
     this._nextScheduledSync = Math.min(this._nextScheduledSync, now + delay);
     // But we still need to honor a backoff.
     this._nextScheduledSync = Math.max(this._nextScheduledSync, this._backoffUntil);
     // And always create a new timer next time _setupTimer is called.
     this._clearTimer();
   },
 
   // callback for when the timer fires.
   _doSync() {
     this.log.debug("starting sync");
     this._timer = null;
     this._timerRunning = true;
     // flag that there's no new schedule yet, so a request coming in while
     // we are running does the right thing.
     this._nextScheduledSync = 0;
     Services.obs.notifyObservers(null, "readinglist:sync:start", null);
-    this._engine.sync().then(() => {
+    this._engine.start().then(() => {
       this.log.info("Sync completed successfully");
       // Write a pref in the same format used to services/sync to indicate
       // the last success.
       prefs.set("lastSync", new Date().toString());
       this.state = this.STATE_OK;
       this._logManager.resetFileLog(this._logManager.REASON_SUCCESS);
       Services.obs.notifyObservers(null, "readinglist:sync:finish", null);
       return intervals.schedule;
@@ -294,16 +310,21 @@ InternalScheduler.prototype = {
       this._timer = null;
     }
   },
 
   // A function to "sync now", but not allowing it to start if one is
   // already running, and rescheduling the timer.
   // To call this, just send a "readinglist:user-sync" notification.
   _syncNow() {
+    if (!prefs.get("enabled")) {
+      this.log.info("syncNow() but syncing is disabled - ignoring");
+      return;
+    }
+
     if (this._timerRunning) {
       this.log.info("syncNow() but a sync is already in progress - ignoring");
       return;
     }
     this._clearTimer();
     this._doSync();
   },
 
@@ -328,22 +349,22 @@ let ReadingListScheduler = {
   get STATE_ERROR_AUTHENTICATION() internalScheduler.STATE_ERROR_AUTHENTICATION,
   get STATE_ERROR_OTHER() internalScheduler.STATE_ERROR_OTHER,
 
   get state() internalScheduler.state,
 };
 
 // These functions are exposed purely for tests, which manage to grab them
 // via a BackstagePass.
-function createTestableScheduler() {
+function createTestableScheduler(readingList) {
   // kill the "real" scheduler as we don't want it listening to notifications etc.
   if (internalScheduler) {
     internalScheduler.finalize();
     internalScheduler = null;
   }
   // No .init() call - that's up to the tests after hooking.
-  return new InternalScheduler();
+  return new InternalScheduler(readingList);
 }
 
 // mochitests want the internal state of the real scheduler for various things.
 function getInternalScheduler() {
   return internalScheduler;
 }
--- a/browser/components/readinglist/test/xpcshell/test_scheduler.js
+++ b/browser/components/readinglist/test/xpcshell/test_scheduler.js
@@ -21,28 +21,40 @@ function promiseObserver(topic) {
     let obs = (subject, topic, data) => {
       Services.obs.removeObserver(obs, topic);
       resolve(data);
     }
     Services.obs.addObserver(obs, topic, false);
   });
 }
 
+function ReadingListMock() {
+  this.listener = null;
+}
+
+ReadingListMock.prototype = {
+  addListener(listener) {
+    ok(!this.listener, "mock only expects 1 listener");
+    this.listener = listener;
+  },
+}
+
 function createScheduler(options) {
   // avoid typos in the test and other footguns in the options.
   let allowedOptions = ["expectedDelay", "expectNewTimer", "syncFunction"];
   for (let key of Object.keys(options)) {
     if (allowedOptions.indexOf(key) == -1) {
       throw new Error("Invalid option " + key);
     }
   }
-  let scheduler = createTestableScheduler();
+  let rlMock = new ReadingListMock();
+  let scheduler = createTestableScheduler(rlMock);
   // make our hooks
   let syncFunction = options.syncFunction || Promise.resolve;
-  scheduler._engine.sync = syncFunction;
+  scheduler._engine.start = syncFunction;
   // we expect _setTimeout to be called *twice* - first is the initial sync,
   // and there's no need to test the delay used for that. options.expectedDelay
   // is to check the *subsequent* timer.
   let numCalls = 0;
   scheduler._setTimeout = function(delay) {
     ++numCalls;
     print("Test scheduler _setTimeout call number " + numCalls + " with delay=" + delay);
     switch (numCalls) {
@@ -85,16 +97,37 @@ add_task(function* testSuccess() {
   ];
   // New delay should be "as regularly scheduled".
   prefs.set("schedule", 100);
   let scheduler = createScheduler({expectedDelay: 100});
   yield Promise.all(allNotifications);
   scheduler.finalize();
 });
 
+// Test that if we get a reading list notification while we are syncing we
+// immediately start a new one when it complets.
+add_task(function* testImmediateResyncWhenChangedDuringSync() {
+  // promises which resolve once we've got all the expected notifications.
+  let allNotifications = [
+    promiseObserver("readinglist:sync:start"),
+    promiseObserver("readinglist:sync:finish"),
+  ];
+  prefs.set("schedule", 100);
+  // New delay should be "immediate".
+  let scheduler = createScheduler({
+    expectedDelay: 0,
+    syncFunction: () => {
+      // we are now syncing - pretend the readinglist has an item change
+      scheduler.readingList.listener.onItemAdded();
+      return Promise.resolve();
+    }});
+  yield Promise.all(allNotifications);
+  scheduler.finalize();
+});
+
 add_task(function* testOffline() {
   let scheduler = createScheduler({expectNewTimer: false});
   Services.io.offline = true;
   ok(!scheduler._canSync(), "_canSync is false when offline.")
   ok(!scheduler._timer, "there is no current timer while offline.")
   Services.io.offline = false;
   ok(scheduler._canSync(), "_canSync is true when online.")
   ok(scheduler._timer, "there is a new timer when back online.")