Bug 1289906 - Part 1: Use an observer to know when the user signs into sync after an undo. r=markh, a=lizzard
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 22 Aug 2016 12:39:39 +0100
changeset 347866 0fe247649cf4fccc9bb86ea1b3896ee451c2d32b
parent 347865 656a6b1bdec9749882809999daa4cdc740e7054a
child 347867 ee6808c347afdf6a341bdc8992fe297764546daf
push id6389
push userraliiev@mozilla.com
push dateMon, 19 Sep 2016 13:38:22 +0000
treeherdermozilla-beta@01d67bfe6c81 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, lizzard
bugs1289906
milestone50.0a2
Bug 1289906 - Part 1: Use an observer to know when the user signs into sync after an undo. r=markh, a=lizzard MozReview-Commit-ID: BPjUVvYdsRG
browser/components/migration/AutoMigrate.jsm
browser/components/migration/tests/unit/test_automigration.js
toolkit/components/telemetry/Histograms.json
--- a/browser/components/migration/AutoMigrate.jsm
+++ b/browser/components/migration/AutoMigrate.jsm
@@ -19,16 +19,18 @@ const kAutoMigrateLastUndoPromptDateMsPr
 const kAutoMigrateDaysToOfferUndoPref = "browser.migrate.automigrate.daysToOfferUndo";
 
 const kPasswordManagerTopic = "passwordmgr-storage-changed";
 const kPasswordManagerTopicTypes = new Set([
   "addLogin",
   "modifyLogin",
 ]);
 
+const kSyncTopic = "fxaccounts:onlogin";
+
 const kNotificationId = "abouthome-automigration-undo";
 
 Cu.import("resource:///modules/MigrationUtils.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
@@ -55,33 +57,37 @@ const AutoMigrate = {
                      getService(Ci.nsIXULChromeRegistry).
                      getSelectedLocale("browser") == "en-US";
     if (this.enabled) {
       this.maybeInitUndoObserver();
     }
   },
 
   maybeInitUndoObserver() {
-    // Check synchronously (NB: canUndo is async) if we really need
-    // to do this:
-    if (!this.getUndoRange()) {
+    if (!this.canUndo()) {
       return;
     }
-    // Now register places and password observers:
+    // Now register places, password and sync observers:
     this.onItemAdded = this.onItemMoved = this.onItemChanged =
       this.removeUndoOption;
     PlacesUtils.addLazyBookmarkObserver(this, true);
-    Services.obs.addObserver(this, kPasswordManagerTopic, true);
+    for (let topic of [kSyncTopic, kPasswordManagerTopic]) {
+      Services.obs.addObserver(this, topic, true);
+    }
   },
 
   observe(subject, topic, data) {
-    // As soon as any login gets added or modified, disable undo:
-    // (Note that this ignores logins being removed as that doesn't
-    //  impair the 'undo' functionality of the import.)
-    if (kPasswordManagerTopicTypes.has(data)) {
+    if (topic == kPasswordManagerTopic) {
+      // As soon as any login gets added or modified, disable undo:
+      // (Note that this ignores logins being removed as that doesn't
+      //  impair the 'undo' functionality of the import.)
+      if (kPasswordManagerTopicTypes.has(data)) {
+        this.removeUndoOption();
+      }
+    } else if (topic == kSyncTopic) {
       this.removeUndoOption();
     }
   },
 
   /**
    * Automatically pick a migrator and resources to migrate,
    * then migrate those and start up.
    *
@@ -199,34 +205,23 @@ const AutoMigrate = {
     }
     if (!finish || !start) {
       return null;
     }
     return [new Date(start), new Date(finish)];
   },
 
   canUndo() {
-    if (!this.getUndoRange()) {
-      return Promise.resolve(false);
-    }
-    // Return a promise resolving to false if we're signed into sync, resolve
-    // to true otherwise.
-    let {fxAccounts} = Cu.import("resource://gre/modules/FxAccounts.jsm", {});
-    return fxAccounts.getSignedInUser().then(user => {
-      if (user) {
-        Services.telemetry.getHistogramById("FX_STARTUP_MIGRATION_CANT_UNDO_BECAUSE_SYNC").add(true);
-      }
-      return !user;
-    }, () => Promise.resolve(true));
+    return !!this.getUndoRange();
   },
 
   undo: Task.async(function* () {
     let histogram = Services.telemetry.getHistogramById("FX_STARTUP_MIGRATION_AUTOMATED_IMPORT_UNDO");
     histogram.add(0);
-    if (!(yield this.canUndo())) {
+    if (!this.canUndo()) {
       histogram.add(5);
       throw new Error("Can't undo!");
     }
 
     histogram.add(10);
 
     yield PlacesUtils.bookmarks.eraseEverything();
     histogram.add(15);
@@ -254,22 +249,28 @@ const AutoMigrate = {
     histogram.add(25);
     this.removeUndoOption();
     histogram.add(30);
   }),
 
   removeUndoOption() {
     // Remove observers, and ensure that exceptions doing so don't break
     // removing the pref.
-    try {
-      Services.obs.removeObserver(this, kPasswordManagerTopic);
-    } catch (ex) {}
+    for (let topic of [kSyncTopic, kPasswordManagerTopic]) {
+      try {
+        Services.obs.removeObserver(this, topic);
+      } catch (ex) {
+        Cu.reportError("Error removing observer for " + topic + ": " + ex);
+      }
+    }
     try {
       PlacesUtils.removeLazyBookmarkObserver(this);
-    } catch (ex) {}
+    } catch (ex) {
+      Cu.reportError("Error removing lazy bookmark observer: " + ex);
+    }
     Services.prefs.clearUserPref(kAutoMigrateStartedPref);
     Services.prefs.clearUserPref(kAutoMigrateFinishedPref);
     Services.prefs.clearUserPref(kAutoMigrateBrowserPref);
 
     let browserWindows = Services.wm.getEnumerator("navigator:browser");
     while (browserWindows.hasMoreElements()) {
       let win = browserWindows.getNext();
       if (!win.closed) {
@@ -295,65 +296,63 @@ const AutoMigrate = {
   getLocalizedString(str, args) {
     if (args === undefined)
       return getBundle().GetStringFromName(str);
     return getBundle().formatStringFromName(
       str, args, args.length);
   },
 
   maybeShowUndoNotification(target) {
-    this.canUndo().then(canUndo => {
-      // The tab might have navigated since we requested the undo state:
-      if (!canUndo || target.currentURI.spec != "about:home" ||
-          !Preferences.get(kUndoUIEnabledPref, false)) {
-        return;
-      }
-      let win = target.ownerGlobal;
-      let notificationBox = win.gBrowser.getNotificationBox(target);
-      if (!notificationBox || notificationBox.getNotificationWithValue("abouthome-automigration-undo")) {
-        return;
-      }
+    // The tab might have navigated since we requested the undo state:
+    if (!this.canUndo() || target.currentURI.spec != "about:home" ||
+        !Preferences.get(kUndoUIEnabledPref, false)) {
+      return;
+    }
+    let win = target.ownerGlobal;
+    let notificationBox = win.gBrowser.getNotificationBox(target);
+    if (!notificationBox || notificationBox.getNotificationWithValue("abouthome-automigration-undo")) {
+      return;
+    }
 
-      // At this stage we're committed to show the prompt - unless we shouldn't,
-      // in which case we remove the undo prefs (which will cause canUndo() to
-      // return false from now on.):
-      if (!this.shouldStillShowUndoPrompt()) {
-        this.removeUndoOption();
-        return;
-      }
+    // At this stage we're committed to show the prompt - unless we shouldn't,
+    // in which case we remove the undo prefs (which will cause canUndo() to
+    // return false from now on.):
+    if (!this.shouldStillShowUndoPrompt()) {
+      this.removeUndoOption();
+      return;
+    }
 
-      let browserName = this.getBrowserUsedForMigration();
-      let message;
-      if (browserName) {
-        message = this.getLocalizedString("automigration.undo.message",
-                                                    [browserName]);
-      } else {
-        message = this.getLocalizedString("automigration.undo.unknownBrowserMessage");
-      }
+    let browserName = this.getBrowserUsedForMigration();
+    let message;
+    if (browserName) {
+      message = this.getLocalizedString("automigration.undo.message",
+                                                  [browserName]);
+    } else {
+      message = this.getLocalizedString("automigration.undo.unknownBrowserMessage");
+    }
 
-      let buttons = [
-        {
-          label: this.getLocalizedString("automigration.undo.keep.label"),
-          accessKey: this.getLocalizedString("automigration.undo.keep.accesskey"),
-          callback: () => {
-            this.removeUndoOption();
-          },
+    let buttons = [
+      {
+        label: this.getLocalizedString("automigration.undo.keep.label"),
+        accessKey: this.getLocalizedString("automigration.undo.keep.accesskey"),
+        callback: () => {
+          this.removeUndoOption();
         },
-        {
-          label: this.getLocalizedString("automigration.undo.dontkeep.label"),
-          accessKey: this.getLocalizedString("automigration.undo.dontkeep.accesskey"),
-          callback: () => {
-            this.undo();
-          },
+      },
+      {
+        label: this.getLocalizedString("automigration.undo.dontkeep.label"),
+        accessKey: this.getLocalizedString("automigration.undo.dontkeep.accesskey"),
+        callback: () => {
+          this.undo();
         },
-      ];
-      notificationBox.appendNotification(
-        message, kNotificationId, null, notificationBox.PRIORITY_INFO_HIGH, buttons
-      );
-    });
+      },
+    ];
+    notificationBox.appendNotification(
+      message, kNotificationId, null, notificationBox.PRIORITY_INFO_HIGH, buttons
+    );
   },
 
   shouldStillShowUndoPrompt() {
     let today = new Date();
     // Round down to midnight:
     today = new Date(today.getFullYear(), today.getMonth(), today.getDate());
     // We store the unix timestamp corresponding to midnight on the last day
     // on which we prompted. Fetch that and compare it to today's date.
--- a/browser/components/migration/tests/unit/test_automigration.js
+++ b/browser/components/migration/tests/unit/test_automigration.js
@@ -160,17 +160,17 @@ add_task(function* checkUndoPrecondition
   Assert.deepEqual(gShimmedMigrator._migrateArgs, [expectedTypes, "startup", null],
                    "migrate called with 'null' as a profile");
 
   yield migrationFinishedPromise;
   Assert.ok(Preferences.has("browser.migrate.automigrate.started"),
             "Should have set start time pref");
   Assert.ok(Preferences.has("browser.migrate.automigrate.finished"),
             "Should have set finish time pref");
-  Assert.ok((yield AutoMigrate.canUndo()), "Should be able to undo migration");
+  Assert.ok(AutoMigrate.canUndo(), "Should be able to undo migration");
 
   let [beginRange, endRange] = AutoMigrate.getUndoRange();
   let stringRange = `beginRange: ${beginRange}; endRange: ${endRange}`;
   Assert.ok(beginRange <= endRange,
             "Migration should have started before or when it ended " + stringRange);
 
   yield AutoMigrate.undo();
   Assert.ok(true, "Should be able to finish an undo cycle.");
@@ -221,17 +221,17 @@ add_task(function* checkUndoRemoval() {
   visits.root.containerOpen = false;
 
   // Now set finished pref:
   let endTime = "" + Date.now();
   Preferences.set("browser.migrate.automigrate.started", startTime);
   Preferences.set("browser.migrate.automigrate.finished", endTime);
 
   // Verify that we can undo, then undo:
-  Assert.ok(yield AutoMigrate.canUndo(), "Should be possible to undo migration");
+  Assert.ok(AutoMigrate.canUndo(), "Should be possible to undo migration");
   yield AutoMigrate.undo();
 
   // Check that the undo removed the history visits:
   visits = PlacesUtils.history.executeQuery(query, opts);
   visits.root.containerOpen = true;
   Assert.equal(visits.root.childCount, 0, "Should have no more visits");
   visits.root.containerOpen = false;
 
@@ -254,35 +254,35 @@ add_task(function* checkUndoRemoval() {
 add_task(function* checkUndoDisablingByBookmarksAndPasswords() {
   let startTime = "" + Date.now();
   Services.prefs.setCharPref("browser.migrate.automigrate.started", startTime);
   // Now set finished pref:
   let endTime = "" + (Date.now() + 1000);
   Services.prefs.setCharPref("browser.migrate.automigrate.finished", endTime);
   AutoMigrate.maybeInitUndoObserver();
 
-  ok((yield AutoMigrate.canUndo()), "Should be able to undo.");
+  ok(AutoMigrate.canUndo(), "Should be able to undo.");
 
   // Insert a login and check that that disabled undo.
   let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
   login.init("www.mozilla.org", "http://www.mozilla.org", null, "user", "pass", "userEl", "passEl");
   Services.logins.addLogin(login);
 
-  ok(!(yield AutoMigrate.canUndo()), "Should no longer be able to undo.");
+  ok(!AutoMigrate.canUndo(), "Should no longer be able to undo.");
   Services.prefs.setCharPref("browser.migrate.automigrate.started", startTime);
   Services.prefs.setCharPref("browser.migrate.automigrate.finished", endTime);
-  ok((yield AutoMigrate.canUndo()), "Should be able to undo.");
+  ok(AutoMigrate.canUndo(), "Should be able to undo.");
   AutoMigrate.maybeInitUndoObserver();
 
   // Insert a bookmark and check that that disabled undo.
   yield PlacesUtils.bookmarks.insert({
     parentGuid: PlacesUtils.bookmarks.toolbarGuid,
     url: "http://www.example.org/",
     title: "Some example bookmark",
   });
-  ok(!(yield AutoMigrate.canUndo()), "Should no longer be able to undo.");
+  ok(!AutoMigrate.canUndo(), "Should no longer be able to undo.");
 
   try {
     Services.logins.removeAllLogins();
   } catch (ex) {}
   yield PlacesUtils.bookmarks.eraseEverything();
 });
 
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -4344,24 +4344,16 @@
     "bug_numbers": [1283565],
     "alert_emails": ["gijs@mozilla.com"],
     "expires_in_version": "53",
     "kind": "enumerated",
     "n_values": 31,
     "releaseChannelCollection": "opt-out",
     "description": "Where undo of the automatic migration was attempted, indicates to what degree we succeeded to undo. 0 means we started to undo, 5 means we bailed out from the undo because it was not possible to complete it (there was nothing to undo or the user was signed in to sync). All higher values indicate progression through the undo sequence, with 30 indicating we finished the undo without exceptions in the middle."
   },
-  "FX_STARTUP_MIGRATION_CANT_UNDO_BECAUSE_SYNC": {
-    "bug_numbers": [1283565],
-    "alert_emails": ["gijs@mozilla.com"],
-    "expires_in_version": "53",
-    "kind": "boolean",
-    "releaseChannelCollection": "opt-out",
-    "description": "Where undo of the automatic migration was requested and denied because the user was logged into sync."
-  },
   "FX_STARTUP_MIGRATION_DATA_RECENCY": {
     "bug_numbers": [1276694],
     "alert_emails": ["gijs@mozilla.com"],
     "expires_in_version": "53",
     "keyed": true,
     "kind": "exponential",
     "n_buckets": 50,
     "high": 8760,