Bug 1289906 - part 1: use an observer to know when the user signs into sync after an undo, r=markh
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 22 Aug 2016 12:39:39 +0100
changeset 310676 e848a9e1ad6aa2b50ffc772bb77b19c73c1e11b6
parent 310675 121b51876d3c45830ff603102e0c60c822a57538
child 310677 e1ba26444126ce1195bdb297d683166a5738f982
push id80944
push usergijskruitbosch@gmail.com
push dateTue, 23 Aug 2016 12:13:39 +0000
treeherdermozilla-inbound@315763ca22c1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1289906
milestone51.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 1289906 - part 1: use an observer to know when the user signs into sync after an undo, r=markh 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
@@ -18,16 +18,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");
@@ -41,33 +43,37 @@ const AutoMigrate = {
   init() {
     this.enabled = Preferences.get(kAutoMigrateEnabledPref, false);
     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.
    *
@@ -185,34 +191,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);
@@ -240,22 +235,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) {
@@ -274,64 +275,63 @@ const AutoMigrate = {
     let browserId = Services.prefs.getCharPref(kAutoMigrateBrowserPref);
     if (browserId) {
       return MigrationUtils.getBrowserName(browserId);
     }
     return null;
   },
 
   maybeShowUndoNotification(target) {
-    this.canUndo().then(canUndo => {
-      // The tab might have navigated since we requested the undo state:
-      if (!canUndo || target.currentURI.spec != "about:home") {
-        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") {
+      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 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;
+    }
 
-      let browserName = this.getBrowserUsedForMigration();
-      let message;
-      if (browserName) {
-        message = MigrationUtils.getLocalizedString("automigration.undo.message",
-                                                    [browserName]);
-      } else {
-        message = MigrationUtils.getLocalizedString("automigration.undo.unknownBrowserMessage");
-      }
+    let browserName = this.getBrowserUsedForMigration();
+    let message;
+    if (browserName) {
+      message = MigrationUtils.getLocalizedString("automigration.undo.message",
+                                                  [browserName]);
+    } else {
+      message = MigrationUtils.getLocalizedString("automigration.undo.unknownBrowserMessage");
+    }
 
-      let buttons = [
-        {
-          label: MigrationUtils.getLocalizedString("automigration.undo.keep.label"),
-          accessKey: MigrationUtils.getLocalizedString("automigration.undo.keep.accesskey"),
-          callback: () => {
-            this.removeUndoOption();
-          },
+    let buttons = [
+      {
+        label: MigrationUtils.getLocalizedString("automigration.undo.keep.label"),
+        accessKey: MigrationUtils.getLocalizedString("automigration.undo.keep.accesskey"),
+        callback: () => {
+          this.removeUndoOption();
         },
-        {
-          label: MigrationUtils.getLocalizedString("automigration.undo.dontkeep.label"),
-          accessKey: MigrationUtils.getLocalizedString("automigration.undo.dontkeep.accesskey"),
-          callback: () => {
-            this.undo();
-          },
+      },
+      {
+        label: MigrationUtils.getLocalizedString("automigration.undo.dontkeep.label"),
+        accessKey: MigrationUtils.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
@@ -4652,24 +4652,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,