Bug 1131413 (part 2) - add readinglist support to browser-syncui.js. r=adw
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 26 Feb 2015 18:48:11 +1100
changeset 230875 9fd3d9330f824d1df6b448e73103d304f4b5dc31
parent 230874 d41af7cc8362e8923163267fba3da88a8f42f7ec
child 230876 c756b24020c408d02d76c11394c89ffc7a1941f7
push id11510
push usermhammond@skippinet.com.au
push dateThu, 26 Feb 2015 07:51:02 +0000
treeherderfx-team@9fd3d9330f82 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1131413
milestone39.0a1
Bug 1131413 (part 2) - add readinglist support to browser-syncui.js. r=adw
browser/base/content/browser-syncui.js
browser/base/content/browser.js
browser/base/content/test/general/browser_syncui.js
services/sync/modules/browserid_identity.js
--- a/browser/base/content/browser-syncui.js
+++ b/browser/base/content/browser-syncui.js
@@ -6,33 +6,45 @@ Cu.import("resource://gre/modules/XPCOMU
 
 #ifdef MOZ_SERVICES_CLOUDSYNC
 XPCOMUtils.defineLazyModuleGetter(this, "CloudSync",
                                   "resource://gre/modules/CloudSync.jsm");
 #else
 let CloudSync = null;
 #endif
 
+XPCOMUtils.defineLazyModuleGetter(this, "ReadingListScheduler",
+                                  "resource:///modules/readinglist/Scheduler.jsm");
+
 // gSyncUI handles updating the tools menu and displaying notifications.
 let gSyncUI = {
   _obs: ["weave:service:sync:start",
+         "weave:service:sync:finish",
+         "weave:service:sync:error",
          "weave:service:quota:remaining",
          "weave:service:setup-complete",
          "weave:service:login:start",
          "weave:service:login:finish",
+         "weave:service:login:error",
          "weave:service:logout:finish",
          "weave:service:start-over",
          "weave:service:start-over:finish",
          "weave:ui:login:error",
          "weave:ui:sync:error",
          "weave:ui:sync:finish",
          "weave:ui:clear-error",
+
+         "readinglist:sync:start",
+         "readinglist:sync:finish",
+         "readinglist:sync:error",
   ],
 
   _unloaded: false,
+  // The number of "active" syncs - while this is non-zero, our button will spin
+  _numActiveSyncTasks: 0,
 
   init: function () {
     Cu.import("resource://services-common/stringbundle.js");
 
     // Proceed to set up the UI if Sync has already started up.
     // Otherwise we'll do it when Sync is firing up.
     let xps = Components.classes["@mozilla.org/weave/service;1"]
                                 .getService(Components.interfaces.nsISupports)
@@ -90,148 +102,182 @@ let gSyncUI = {
     // notificationbox will listen to observers from now on.
     Services.obs.removeObserver(this, "weave:notification:added");
     let idx = this._obs.indexOf("weave:notification:added");
     if (idx >= 0) {
       this._obs.splice(idx, 1);
     }
   },
 
-  _needsSetup: function SUI__needsSetup() {
+  _needsSetup() {
     // We want to treat "account needs verification" as "needs setup". So
     // "reach in" to Weave.Status._authManager to check whether we the signed-in
     // user is verified.
     // Referencing Weave.Status spins a nested event loop to initialize the
     // authManager, so this should always return a value directly.
     // This only applies to fxAccounts-based Sync.
-    if (Weave.Status._authManager._signedInUser) {
-      // If we have a signed in user already, and that user is not verified,
-      // revert to the "needs setup" state.
-      if (!Weave.Status._authManager._signedInUser.verified) {
-        return true;
-      }
+    if (Weave.Status._authManager._signedInUser !== undefined) {
+      // So we are using Firefox accounts - in this world, checking Sync isn't
+      // enough as reading list may be configured but not Sync.
+      // We consider ourselves setup if we have a verified user.
+      // XXX - later we should consider checking preferences to ensure at least
+      // one engine is enabled?
+      return !Weave.Status._authManager._signedInUser ||
+             !Weave.Status._authManager._signedInUser.verified;
     }
 
+    // So we are using legacy sync, and reading-list isn't supported for such
+    // users, so check sync itself.
     let firstSync = "";
     try {
       firstSync = Services.prefs.getCharPref("services.sync.firstSync");
     } catch (e) { }
 
     return Weave.Status.checkSetup() == Weave.CLIENT_NOT_CONFIGURED ||
            firstSync == "notReady";
   },
 
   _loginFailed: function () {
-    return Weave.Status.login == Weave.LOGIN_FAILED_LOGIN_REJECTED;
+    return Weave.Status.login == Weave.LOGIN_FAILED_LOGIN_REJECTED ||
+           ReadingListScheduler.state == ReadingListScheduler.STATE_ERROR_AUTHENTICATION;
   },
 
   updateUI: function SUI_updateUI() {
     let needsSetup = this._needsSetup();
     let loginFailed = this._loginFailed();
 
     // Start off with a clean slate
     document.getElementById("sync-reauth-state").hidden = true;
     document.getElementById("sync-setup-state").hidden = true;
     document.getElementById("sync-syncnow-state").hidden = true;
 
     if (CloudSync && CloudSync.ready && CloudSync().adapters.count) {
       document.getElementById("sync-syncnow-state").hidden = false;
     } else if (loginFailed) {
       document.getElementById("sync-reauth-state").hidden = false;
+      this.showLoginError();
     } else if (needsSetup) {
       document.getElementById("sync-setup-state").hidden = false;
     } else {
       document.getElementById("sync-syncnow-state").hidden = false;
     }
 
     if (!gBrowser)
       return;
 
     let syncButton = document.getElementById("sync-button");
-    if (syncButton) {
-      syncButton.removeAttribute("status");
-    }
-    let panelHorizontalButton = document.getElementById("PanelUI-fxa-status");
-    if (panelHorizontalButton) {
-      panelHorizontalButton.removeAttribute("syncstatus");
-    }
-
     if (needsSetup && syncButton)
       syncButton.removeAttribute("tooltiptext");
 
     this._updateLastSyncTime();
   },
 
 
   // Functions called by observers
-  onActivityStart: function SUI_onActivityStart() {
+  onActivityStart() {
     if (!gBrowser)
       return;
 
-    let button = document.getElementById("sync-button");
-    if (button) {
-      button.setAttribute("status", "active");
+    this.log.debug("onActivityStart with numActive", this._numActiveSyncTasks);
+    if (++this._numActiveSyncTasks == 1) {
+      let button = document.getElementById("sync-button");
+      if (button) {
+        button.setAttribute("status", "active");
+      }
+      button = document.getElementById("PanelUI-fxa-status");
+      if (button) {
+        button.setAttribute("syncstatus", "active");
+      }
     }
-    button = document.getElementById("PanelUI-fxa-status");
-    if (button) {
-      button.setAttribute("syncstatus", "active");
+  },
+
+  onActivityStop() {
+    if (!gBrowser)
+      return;
+    this.log.debug("onActivityStop with numActive", this._numActiveSyncTasks);
+    if (--this._numActiveSyncTasks) {
+      if (this._numActiveSyncTasks < 0) {
+        // This isn't particularly useful (it seems more likely we'll set a
+        // "start" without a "stop" meaning it forever remains > 0) but it
+        // might offer some value...
+        this.log.error("mismatched onActivityStart/Stop calls",
+                       new Error("active=" + this._numActiveSyncTasks));
+      }
+      return; // active tasks are still ongoing...
+    }
+
+    let syncButton = document.getElementById("sync-button");
+    if (syncButton) {
+      syncButton.removeAttribute("status");
+    }
+    let panelHorizontalButton = document.getElementById("PanelUI-fxa-status");
+    if (panelHorizontalButton) {
+      panelHorizontalButton.removeAttribute("syncstatus");
     }
   },
 
   onLoginFinish: function SUI_onLoginFinish() {
     // Clear out any login failure notifications
     let title = this._stringBundle.GetStringFromName("error.login.title");
     this.clearError(title);
   },
 
   onSetupComplete: function SUI_onSetupComplete() {
     this.onLoginFinish();
   },
 
   onLoginError: function SUI_onLoginError() {
+    // Note: This is used for *both* Sync and ReadingList login errors.
     // if login fails, any other notifications are essentially moot
     Weave.Notifications.removeAll();
 
     // if we haven't set up the client, don't show errors
     if (this._needsSetup()) {
       this.updateUI();
       return;
     }
     // if we are still waiting for the identity manager to initialize, don't show errors
     if (Weave.Status.login == Weave.LOGIN_FAILED_NOT_READY) {
       this.updateUI();
       return;
     }
+    this.showLoginError();
+    this.updateUI();
+  },
 
+  showLoginError() {
+    // Note: This is used for *both* Sync and ReadingList login errors.
     let title = this._stringBundle.GetStringFromName("error.login.title");
 
     let description;
-    if (Weave.Status.sync == Weave.PROLONGED_SYNC_FAILURE) {
+    if (Weave.Status.sync == Weave.PROLONGED_SYNC_FAILURE ||
+        this.isProlongedReadingListError()) {
+      this.log.debug("showLoginError has a prolonged login error");
       // Convert to days
       let lastSync =
         Services.prefs.getIntPref("services.sync.errorhandler.networkFailureReportTimeout") / 86400;
       description =
         this._stringBundle.formatStringFromName("error.sync.prolonged_failure", [lastSync], 1);
     } else {
       let reason = Weave.Utils.getErrorString(Weave.Status.login);
       description =
         this._stringBundle.formatStringFromName("error.sync.description", [reason], 1);
+      this.log.debug("showLoginError has a non-prolonged error", reason);
     }
 
     let buttons = [];
     buttons.push(new Weave.NotificationButton(
       this._stringBundle.GetStringFromName("error.login.prefs.label"),
       this._stringBundle.GetStringFromName("error.login.prefs.accesskey"),
       function() { gSyncUI.openPrefs(); return true; }
     ));
 
     let notification = new Weave.Notification(title, description, null,
                                               Weave.Notifications.PRIORITY_WARNING, buttons);
     Weave.Notifications.replaceTitle(notification);
-    this.updateUI();
   },
 
   onLogout: function SUI_onLogout() {
     this.updateUI();
   },
 
   onStartOver: function SUI_onStartOver() {
     this.clearError();
@@ -266,16 +312,17 @@ let gSyncUI = {
   doSync: function SUI_doSync() {
     let needsSetup = this._needsSetup();
 
     if (!needsSetup) {
       setTimeout(function () Weave.Service.errorHandler.syncAndReportErrors(), 0);
     }
 
     Services.obs.notifyObservers(null, "cloudsync:user-sync", null);
+    Services.obs.notifyObservers(null, "readinglist:user-sync", null);
   },
 
   handleToolbarButton: function SUI_handleStatusbarButton() {
     if (this._needsSetup())
       this.openSetup();
     else
       this.doSync();
   },
@@ -362,45 +409,115 @@ let gSyncUI = {
       return;
 
     let syncButton = document.getElementById("sync-button");
     if (!syncButton)
       return;
 
     let lastSync;
     try {
-      lastSync = Services.prefs.getCharPref("services.sync.lastSync");
+      lastSync = new Date(Services.prefs.getCharPref("services.sync.lastSync"));
+    }
+    catch (e) { };
+    // and reading-list time - we want whatever one is the most recent.
+    try {
+      let lastRLSync = new Date(Services.prefs.getCharPref("readinglist.scheduler.lastSync"));
+      if (!lastSync || lastRLSync > lastSync) {
+        lastSync = lastRLSync;
+      }
     }
     catch (e) { };
     if (!lastSync || this._needsSetup()) {
       syncButton.removeAttribute("tooltiptext");
       return;
     }
 
     // Show the day-of-week and time (HH:MM) of last sync
-    let lastSyncDate = new Date(lastSync).toLocaleFormat("%a %H:%M");
+    let lastSyncDateString = lastSync.toLocaleFormat("%a %H:%M");
     let lastSyncLabel =
-      this._stringBundle.formatStringFromName("lastSync2.label", [lastSyncDate], 1);
+      this._stringBundle.formatStringFromName("lastSync2.label", [lastSyncDateString], 1);
 
     syncButton.setAttribute("tooltiptext", lastSyncLabel);
   },
 
   clearError: function SUI_clearError(errorString) {
     Weave.Notifications.removeAll(errorString);
     this.updateUI();
   },
 
   onSyncFinish: function SUI_onSyncFinish() {
     let title = this._stringBundle.GetStringFromName("error.sync.title");
 
     // Clear out sync failures on a successful sync
     this.clearError(title);
   },
 
+  // Return true if the reading-list is in a "prolonged" error state. That
+  // engine doesn't impose what that means, so calculate it here. For
+  // consistency, we just use the sync prefs.
+  isProlongedReadingListError() {
+    let lastSync, threshold, prolonged;
+    try {
+      lastSync = new Date(Services.prefs.getCharPref("readinglist.scheduler.lastSync"));
+      threshold = new Date(Date.now() - Services.prefs.getIntPref("services.sync.errorhandler.networkFailureReportTimeout"));
+      prolonged = lastSync <= threshold;
+    } catch (ex) {
+      // no pref, assume not prolonged.
+      prolonged = false;
+    }
+    this.log.debug("isProlongedReadingListError has last successful sync at ${lastSync}, threshold is ${threshold}, prolonged=${prolonged}",
+                   {lastSync, threshold, prolonged});
+    return prolonged;
+  },
+
+  onRLSyncError() {
+    // Like onSyncError, but from the reading-list engine.
+    // However, the current UX around Sync is that error notifications should
+    // generally *not* be seen as they typically aren't actionable - so only
+    // authentication errors (which require user action) and "prolonged" errors
+    // (which technically aren't actionable, but user really should know anyway)
+    // are shown.
+    this.log.debug("onRLSyncError with readingList state", ReadingListScheduler.state);
+    if (ReadingListScheduler.state == ReadingListScheduler.STATE_ERROR_AUTHENTICATION) {
+      this.onLoginError();
+      return;
+    }
+    // If it's not prolonged there's nothing to do.
+    if (!this.isProlongedReadingListError()) {
+      this.log.debug("onRLSyncError has a non-authentication, non-prolonged error, so not showing any error UI");
+      return;
+    }
+    // So it's a prolonged error.
+    // Unfortunate duplication from below...
+    this.log.debug("onRLSyncError has a prolonged error");
+    let title = this._stringBundle.GetStringFromName("error.sync.title");
+    // XXX - this is somewhat wrong - we are reporting the threshold we consider
+    // to be prolonged, not how long it actually has been. (ie, lastSync below
+    // is effectively constant) - bit it too is copied from below.
+    let lastSync =
+      Services.prefs.getIntPref("services.sync.errorhandler.networkFailureReportTimeout") / 86400;
+    let description =
+      this._stringBundle.formatStringFromName("error.sync.prolonged_failure", [lastSync], 1);
+    let priority = Weave.Notifications.PRIORITY_INFO;
+    let buttons = [
+      new Weave.NotificationButton(
+        this._stringBundle.GetStringFromName("error.sync.tryAgainButton.label"),
+        this._stringBundle.GetStringFromName("error.sync.tryAgainButton.accesskey"),
+        function() { gSyncUI.doSync(); return true; }
+      ),
+    ];
+    let notification =
+      new Weave.Notification(title, description, null, priority, buttons);
+    Weave.Notifications.replaceTitle(notification);
+
+    this.updateUI();
+  },
+
   onSyncError: function SUI_onSyncError() {
+    this.log.debug("onSyncError");
     let title = this._stringBundle.GetStringFromName("error.sync.title");
 
     if (Weave.Status.login != Weave.LOGIN_SUCCEEDED) {
       this.onLoginError();
       return;
     }
 
     let description;
@@ -413,17 +530,19 @@ let gSyncUI = {
     } else {
       let error = Weave.Utils.getErrorString(Weave.Status.sync);
       description =
         this._stringBundle.formatStringFromName("error.sync.description", [error], 1);
     }
     let priority = Weave.Notifications.PRIORITY_WARNING;
     let buttons = [];
 
-    // Check if the client is outdated in some way
+    // Check if the client is outdated in some way (but note: we've never in the
+    // past, and probably never will, bump the relevent version numbers, so
+    // this is effectively dead code!)
     let outdated = Weave.Status.sync == Weave.VERSION_OUT_OF_DATE;
     for (let [engine, reason] in Iterator(Weave.Status.engines))
       outdated = outdated || reason == Weave.VERSION_OUT_OF_DATE;
 
     if (outdated) {
       description = this._stringBundle.GetStringFromName(
         "error.sync.needUpdate.description");
       buttons.push(new Weave.NotificationButton(
@@ -463,47 +582,61 @@ let gSyncUI = {
     let notification =
       new Weave.Notification(title, description, null, priority, buttons);
     Weave.Notifications.replaceTitle(notification);
 
     this.updateUI();
   },
 
   observe: function SUI_observe(subject, topic, data) {
+    this.log.debug("observed", topic);
     if (this._unloaded) {
       Cu.reportError("SyncUI observer called after unload: " + topic);
       return;
     }
 
     // Unwrap, just like Svc.Obs, but without pulling in that dependency.
     if (subject && typeof subject == "object" &&
         ("wrappedJSObject" in subject) &&
         ("observersModuleSubjectWrapper" in subject.wrappedJSObject)) {
       subject = subject.wrappedJSObject.object;
     }
 
+    // First handle "activity" only.
     switch (topic) {
       case "weave:service:sync:start":
+      case "weave:service:login:start":
+      case "readinglist:sync:start":
         this.onActivityStart();
         break;
+      case "weave:service:sync:finish":
+      case "weave:service:sync:error":
+      case "weave:service:login:finish":
+      case "weave:service:login:error":
+      case "readinglist:sync:finish":
+      case "readinglist:sync:error":
+        this.onActivityStop();
+        break;
+    }
+    // Now non-activity state (eg, enabled, errors, etc)
+    // Note that sync uses the ":ui:" notifications for errors because sync.
+    // ReadingList has no such concept (yet?; hopefully the :error is enough!)
+    switch (topic) {
       case "weave:ui:sync:finish":
         this.onSyncFinish();
         break;
       case "weave:ui:sync:error":
         this.onSyncError();
         break;
       case "weave:service:quota:remaining":
         this.onQuotaNotice();
         break;
       case "weave:service:setup-complete":
         this.onSetupComplete();
         break;
-      case "weave:service:login:start":
-        this.onActivityStart();
-        break;
       case "weave:service:login:finish":
         this.onLoginFinish();
         break;
       case "weave:ui:login:error":
         this.onLoginError();
         break;
       case "weave:service:logout:finish":
         this.onLogout();
@@ -518,16 +651,23 @@ let gSyncUI = {
         this.initUI();
         break;
       case "weave:notification:added":
         this.initNotifications();
         break;
       case "weave:ui:clear-error":
         this.clearError();
         break;
+
+      case "readinglist:sync:error":
+        this.onRLSyncError();
+        break;
+      case "readinglist:sync:finish":
+        this.clearError();
+        break;
     }
   },
 
   QueryInterface: XPCOMUtils.generateQI([
     Ci.nsIObserver,
     Ci.nsISupportsWeakReference
   ])
 };
@@ -535,8 +675,11 @@ let gSyncUI = {
 XPCOMUtils.defineLazyGetter(gSyncUI, "_stringBundle", function() {
   //XXXzpao these strings should probably be moved from /services to /browser... (bug 583381)
   //        but for now just make it work
   return Cc["@mozilla.org/intl/stringbundle;1"].
          getService(Ci.nsIStringBundleService).
          createBundle("chrome://weave/locale/services/sync.properties");
 });
 
+XPCOMUtils.defineLazyGetter(gSyncUI, "log", function() {
+  return Log.repository.getLogger("browserwindow.syncui");
+});
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -32,16 +32,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "GMPInstallManager",
                                   "resource://gre/modules/GMPInstallManager.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "NewTabUtils",
                                   "resource://gre/modules/NewTabUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "ContentSearch",
                                   "resource:///modules/ContentSearch.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "AboutHome",
                                   "resource:///modules/AboutHome.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Log",
+                                  "resource://gre/modules/Log.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "Favicons",
                                    "@mozilla.org/browser/favicon-service;1",
                                    "mozIAsyncFavicons");
 XPCOMUtils.defineLazyServiceGetter(this, "gDNSService",
                                    "@mozilla.org/network/dns-service;1",
                                    "nsIDNSService");
 
 const nsIWebNavigation = Ci.nsIWebNavigation;
--- a/browser/base/content/test/general/browser_syncui.js
+++ b/browser/base/content/test/general/browser_syncui.js
@@ -1,18 +1,24 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
+let {Log} = Cu.import("resource://gre/modules/Log.jsm", {});
 let {Weave} = Cu.import("resource://services-sync/main.js", {});
 let {Notifications} = Cu.import("resource://services-sync/notifications.js", {});
+// The BackStagePass allows us to get this test-only non-exported function.
+let {getInternalScheduler} = Cu.import("resource:///modules/readinglist/Scheduler.jsm", {});
 
 let stringBundle = Cc["@mozilla.org/intl/stringbundle;1"]
                    .getService(Ci.nsIStringBundleService)
                    .createBundle("chrome://weave/locale/services/sync.properties");
 
+// ensure test output sees log messages.
+Log.repository.getLogger("browserwindow.syncui").addAppender(new Log.DumpAppender());
+
 function promiseObserver(topic) {
   return new Promise(resolve => {
     let obs = (subject, topic, data) => {
       Services.obs.removeObserver(obs, topic);
       resolve(subject);
     }
     Services.obs.addObserver(obs, topic, false);
   });
@@ -23,23 +29,20 @@ add_task(function* prepare() {
                               .getService(Components.interfaces.nsISupports)
                               .wrappedJSObject;
   yield xps.whenLoaded();
   // mock out the "_needsSetup()" function so we don't short-circuit.
   let oldNeedsSetup = window.gSyncUI._needsSetup;
   window.gSyncUI._needsSetup = () => false;
   registerCleanupFunction(() => {
     window.gSyncUI._needsSetup = oldNeedsSetup;
-    // this test leaves the tab focused which can cause browser_tabopen_reflows
-    // to fail, so re-select the URLBar.
-    gURLBar.select();
   });
 });
 
-add_task(function* testProlongedError() {
+add_task(function* testProlongedSyncError() {
   let promiseNotificationAdded = promiseObserver("weave:notification:added");
   Assert.equal(Notifications.notifications.length, 0, "start with no notifications");
 
   // Pretend we are in the "prolonged error" state.
   Weave.Status.sync = Weave.PROLONGED_SYNC_FAILURE;
   Weave.Status.login = Weave.LOGIN_SUCCEEDED;
   Services.obs.notifyObservers(null, "weave:ui:sync:error", null);
 
@@ -51,60 +54,191 @@ add_task(function* testProlongedError() 
   // Now pretend we just had a successful sync - the error notification should go away.
   let promiseNotificationRemoved = promiseObserver("weave:notification:removed");
   Weave.Status.sync = Weave.STATUS_OK;
   Services.obs.notifyObservers(null, "weave:ui:sync:finish", null);
   yield promiseNotificationRemoved;
   Assert.equal(Notifications.notifications.length, 0, "no notifications left");
 });
 
-add_task(function* testLoginError() {
+add_task(function* testProlongedRLError() {
+  let promiseNotificationAdded = promiseObserver("weave:notification:added");
+  Assert.equal(Notifications.notifications.length, 0, "start with no notifications");
+
+  // Pretend the reading-list is in the "prolonged error" state.
+  let longAgo = new Date(Date.now() - 100 * 24 * 60 * 60 * 1000); // 100 days ago.
+  Services.prefs.setCharPref("readinglist.scheduler.lastSync", longAgo.toString());
+  getInternalScheduler().state = ReadingListScheduler.STATE_ERROR_OTHER;
+  Services.obs.notifyObservers(null, "readinglist:sync:start", null);
+  Services.obs.notifyObservers(null, "readinglist:sync:error", null);
+
+  let subject = yield promiseNotificationAdded;
+  let notification = subject.wrappedJSObject.object; // sync's observer abstraction is abstract!
+  Assert.equal(notification.title, stringBundle.GetStringFromName("error.sync.title"));
+  Assert.equal(Notifications.notifications.length, 1, "exactly 1 notification");
+
+  // Now pretend we just had a successful sync - the error notification should go away.
+  let promiseNotificationRemoved = promiseObserver("weave:notification:removed");
+  Services.prefs.setCharPref("readinglist.scheduler.lastSync", Date.now().toString());
+  Services.obs.notifyObservers(null, "readinglist:sync:start", null);
+  Services.obs.notifyObservers(null, "readinglist:sync:finish", null);
+  yield promiseNotificationRemoved;
+  Assert.equal(Notifications.notifications.length, 0, "no notifications left");
+});
+
+add_task(function* testSyncLoginError() {
   let promiseNotificationAdded = promiseObserver("weave:notification:added");
   Assert.equal(Notifications.notifications.length, 0, "start with no notifications");
 
   // Pretend we are in the "prolonged error" state.
   Weave.Status.sync = Weave.LOGIN_FAILED;
   Weave.Status.login = Weave.LOGIN_FAILED_LOGIN_REJECTED;
   Services.obs.notifyObservers(null, "weave:ui:sync:error", null);
 
   let subject = yield promiseNotificationAdded;
   let notification = subject.wrappedJSObject.object; // sync's observer abstraction is abstract!
   Assert.equal(notification.title, stringBundle.GetStringFromName("error.login.title"));
   Assert.equal(Notifications.notifications.length, 1, "exactly 1 notification");
+
   // Now pretend we just had a successful login - the error notification should go away.
   Weave.Status.sync = Weave.STATUS_OK;
   Weave.Status.login = Weave.LOGIN_SUCCEEDED;
   let promiseNotificationRemoved = promiseObserver("weave:notification:removed");
   Services.obs.notifyObservers(null, "weave:service:login:start", null);
   Services.obs.notifyObservers(null, "weave:service:login:finish", null);
   yield promiseNotificationRemoved;
   Assert.equal(Notifications.notifications.length, 0, "no notifications left");
 });
 
-function testButtonActions(startNotification, endNotification) {
+add_task(function* testRLLoginError() {
+  let promiseNotificationAdded = promiseObserver("weave:notification:added");
+  Assert.equal(Notifications.notifications.length, 0, "start with no notifications");
+
+  // Pretend RL is in an auth error state
+  getInternalScheduler().state = ReadingListScheduler.STATE_ERROR_AUTHENTICATION;
+  Services.obs.notifyObservers(null, "readinglist:sync:start", null);
+  Services.obs.notifyObservers(null, "readinglist:sync:error", null);
+
+  let subject = yield promiseNotificationAdded;
+  let notification = subject.wrappedJSObject.object; // sync's observer abstraction is abstract!
+  Assert.equal(notification.title, stringBundle.GetStringFromName("error.login.title"));
+  Assert.equal(Notifications.notifications.length, 1, "exactly 1 notification");
+
+  // Now pretend we just had a successful sync - the error notification should go away.
+  getInternalScheduler().state = ReadingListScheduler.STATE_OK;
+  let promiseNotificationRemoved = promiseObserver("weave:notification:removed");
+  Services.obs.notifyObservers(null, "readinglist:sync:start", null);
+  Services.obs.notifyObservers(null, "readinglist:sync:finish", null);
+  yield promiseNotificationRemoved;
+  Assert.equal(Notifications.notifications.length, 0, "no notifications left");
+});
+
+// Here we put readinglist into an "authentication error" state (should see
+// the error bar reflecting this), then report a prolonged error from Sync (an
+// infobar to reflect the sync error should replace it), then resolve the sync
+// error - the authentication error from readinglist should remain.
+add_task(function* testRLLoginErrorRemains() {
+  let promiseNotificationAdded = promiseObserver("weave:notification:added");
+  Assert.equal(Notifications.notifications.length, 0, "start with no notifications");
+
+  // Pretend RL is in an auth error state
+  getInternalScheduler().state = ReadingListScheduler.STATE_ERROR_AUTHENTICATION;
+  Services.obs.notifyObservers(null, "readinglist:sync:start", null);
+  Services.obs.notifyObservers(null, "readinglist:sync:error", null);
+
+  let subject = yield promiseNotificationAdded;
+  let notification = subject.wrappedJSObject.object; // sync's observer abstraction is abstract!
+  Assert.equal(notification.title, stringBundle.GetStringFromName("error.login.title"));
+  Assert.equal(Notifications.notifications.length, 1, "exactly 1 notification");
+
+  // Now Sync into a prolonged auth error state.
+  promiseNotificationAdded = promiseObserver("weave:notification:added");
+  Weave.Status.sync = Weave.PROLONGED_SYNC_FAILURE;
+  Weave.Status.login = Weave.LOGIN_FAILED_LOGIN_REJECTED;
+  Services.obs.notifyObservers(null, "weave:ui:sync:error", null);
+  subject = yield promiseNotificationAdded;
+  // still exactly 1 notification with the "login" title.
+  notification = subject.wrappedJSObject.object;
+  Assert.equal(notification.title, stringBundle.GetStringFromName("error.login.title"));
+  Assert.equal(Notifications.notifications.length, 1, "exactly 1 notification");
+
+  // Resolve the sync problem.
+  promiseNotificationAdded = promiseObserver("weave:notification:added");
+  Weave.Status.sync = Weave.STATUS_OK;
+  Weave.Status.login = Weave.LOGIN_SUCCEEDED;
+  Services.obs.notifyObservers(null, "weave:ui:sync:finish", null);
+
+  // Expect one notification - the RL login problem.
+  subject = yield promiseNotificationAdded;
+  // still exactly 1 notification with the "login" title.
+  notification = subject.wrappedJSObject.object;
+  Assert.equal(notification.title, stringBundle.GetStringFromName("error.login.title"));
+  Assert.equal(Notifications.notifications.length, 1, "exactly 1 notification");
+
+  // and cleanup - resolve the readinglist error.
+  getInternalScheduler().state = ReadingListScheduler.STATE_OK;
+  let promiseNotificationRemoved = promiseObserver("weave:notification:removed");
+  Services.obs.notifyObservers(null, "readinglist:sync:start", null);
+  Services.obs.notifyObservers(null, "readinglist:sync:finish", null);
+  yield promiseNotificationRemoved;
+  Assert.equal(Notifications.notifications.length, 0, "no notifications left");
+});
+
+function checkButtonsStatus(shouldBeActive) {
   let button = document.getElementById("sync-button");
-  Assert.ok(button, "button exists");
   let panelbutton = document.getElementById("PanelUI-fxa-status");
-  Assert.ok(panelbutton, "panel button exists");
+  if (shouldBeActive) {
+    Assert.equal(button.getAttribute("status"), "active");
+    Assert.equal(panelbutton.getAttribute("syncstatus"), "active");
+  } else {
+    Assert.ok(!button.hasAttribute("status"));
+    Assert.ok(!panelbutton.hasAttribute("syncstatus"));
+  }
+}
 
+function testButtonActions(startNotification, endNotification) {
+  checkButtonsStatus(false);
   // pretend a sync is starting.
   Services.obs.notifyObservers(null, startNotification, null);
-  Assert.equal(button.getAttribute("status"), "active");
-  Assert.equal(panelbutton.getAttribute("syncstatus"), "active");
-
+  checkButtonsStatus(true);
+  // and has stopped
   Services.obs.notifyObservers(null, endNotification, null);
-  Assert.ok(!button.hasAttribute("status"));
-  Assert.ok(!panelbutton.hasAttribute("syncstatus"));
+  checkButtonsStatus(false);
 }
 
 add_task(function* testButtonActivities() {
   // add the Sync button to the panel so we can get it!
   CustomizableUI.addWidgetToArea("sync-button", CustomizableUI.AREA_PANEL);
   // check the button's functionality
   yield PanelUI.show();
   try {
     testButtonActions("weave:service:login:start", "weave:service:login:finish");
-    testButtonActions("weave:service:sync:start", "weave:ui:sync:finish");
+    testButtonActions("weave:service:login:start", "weave:service:login:error");
+
+    testButtonActions("weave:service:sync:start", "weave:service:sync:finish");
+    testButtonActions("weave:service:sync:start", "weave:service:sync:error");
+
+    testButtonActions("readinglist:sync:start", "readinglist:sync:finish");
+    testButtonActions("readinglist:sync:start", "readinglist:sync:error");
+
+    // and ensure the counters correctly handle multiple in-flight syncs
+    Services.obs.notifyObservers(null, "weave:service:sync:start", null);
+    checkButtonsStatus(true);
+    Services.obs.notifyObservers(null, "readinglist:sync:start", null);
+    checkButtonsStatus(true);
+    Services.obs.notifyObservers(null, "readinglist:sync:finish", null);
+    // sync is still going...
+    checkButtonsStatus(true);
+    // another reading list starts
+    Services.obs.notifyObservers(null, "readinglist:sync:start", null);
+    checkButtonsStatus(true);
+    // The initial sync stops.
+    Services.obs.notifyObservers(null, "weave:service:sync:finish", null);
+    // RL is still going...
+    checkButtonsStatus(true);
+    // RL finishes with an error, so no longer active.
+    Services.obs.notifyObservers(null, "readinglist:sync:error", null);
+    checkButtonsStatus(false);
   } finally {
     PanelUI.hide();
     CustomizableUI.removeWidgetFromArea("sync-button");
   }
 });
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -612,17 +612,17 @@ this.BrowserIDManager.prototype = {
           this._authFailureReason = LOGIN_FAILED_NETWORK_ERROR;
         }
         // this._authFailureReason being set to be non-null in the above if clause
         // ensures we are in the correct currentAuthState, and
         // this._shouldHaveSyncKeyBundle being true ensures everything that cares knows
         // that there is no authentication dance still under way.
         this._shouldHaveSyncKeyBundle = true;
         Weave.Status.login = this._authFailureReason;
-        Services.obs.notifyObservers(null, "weave:service:login:error", null);
+        Services.obs.notifyObservers(null, "weave:ui:login:error", null);
         throw err;
       });
   },
 
   // Returns a promise that is resolved when we have a valid token for the
   // current user stored in this._token.  When resolved, this._token is valid.
   _ensureValidToken: function() {
     if (this.hasValidToken()) {