Bug 1289906 - part 1: use an observer to know when the user signs into sync after an undo, r?markh draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 22 Aug 2016 12:39:39 +0100
changeset 403905 81f166d1a62dd4886b6203a05f1ab79f7eafb79a
parent 403904 3b0f290f1326950d6753fd5709f3b6c7d3542ea5
child 403906 1b9357ec4e78fc7b1b6ec6e81144e13b9abfe870
push id27041
push usergijskruitbosch@gmail.com
push dateMon, 22 Aug 2016 11:53:01 +0000
reviewersmarkh
bugs1289906
milestone51.0a1
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,21 @@ const kAutoMigrateLastUndoPromptDateMsPr
 const kAutoMigrateDaysToOfferUndoPref = "browser.migrate.automigrate.daysToOfferUndo";
 
 const kPasswordManagerTopic = "passwordmgr-storage-changed";
 const kPasswordManagerTopicTypes = new Set([
   "addLogin",
   "modifyLogin",
 ]);
 
+const kSyncTopics = new Set([
+  "weave:service:setup-complete",
+  "weave:service:login:finish",
+]);
+
 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 +46,39 @@ 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:
     this.onItemAdded = this.onItemMoved = this.onItemChanged =
       this.removeUndoOption;
     PlacesUtils.addLazyBookmarkObserver(this, true);
     Services.obs.addObserver(this, kPasswordManagerTopic, true);
+    // And sync ones:
+    for (let topic of kSyncTopics) {
+      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 (kSyncTopics.has(topic)) {
       this.removeUndoOption();
     }
   },
 
   /**
    * Automatically pick a migrator and resources to migrate,
    * then migrate those and start up.
    *
@@ -185,34 +196,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);
@@ -241,16 +241,21 @@ const AutoMigrate = {
     this.removeUndoOption();
     histogram.add(30);
   }),
 
   removeUndoOption() {
     // Remove observers, and ensure that exceptions doing so don't break
     // removing the pref.
     try {
+      for (let topic of kSyncTopics) {
+        Services.obs.removeObserver(this, kSyncTopics);
+      }
+    } catch (ex) {}
+    try {
       Services.obs.removeObserver(this, kPasswordManagerTopic);
     } catch (ex) {}
     try {
       PlacesUtils.removeLazyBookmarkObserver(this);
     } catch (ex) {}
     Services.prefs.clearUserPref(kAutoMigrateStartedPref);
     Services.prefs.clearUserPref(kAutoMigrateFinishedPref);
     Services.prefs.clearUserPref(kAutoMigrateBrowserPref);
@@ -274,64 +279,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,