Bug 1331888 - immediately remove (and don't reshow) notification bars once user chooses to undo, r=dao
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 18 Jan 2017 11:07:47 +0000
changeset 377196 276202b206839a1c2563408fbcfa969272d929d8
parent 377195 f3db78764c55ca4d581002c99a2430e7f068660b
child 377197 7261ee7e3e67c62531ca1e745da5e7b714b5d4ab
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdao
bugs1331888
milestone53.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 1331888 - immediately remove (and don't reshow) notification bars once user chooses to undo, r=dao The automated test verifies that we remove the notification bars immediately. Unfortunately, I couldn't think of a way to verify we won't allow reshowing the notification bar while an undo is ongoing, because for that to happen I'd need to get one to show while an undo is ongoing, which isn't reliably possible in an automated test. MozReview-Commit-ID: EYHNIPEaOOo
browser/components/migration/AutoMigrate.jsm
browser/components/migration/tests/browser/browser.ini
browser/components/migration/tests/browser/browser_undo_notification.js
browser/components/migration/tests/browser/browser_undo_notification_multiple_dismissal.js
--- a/browser/components/migration/AutoMigrate.jsm
+++ b/browser/components/migration/AutoMigrate.jsm
@@ -171,20 +171,24 @@ const AutoMigrate = {
       return suggestedProfile;
     }
     if (profiles && profiles.length > 1) {
       throw new Error("Don't know how to pick a profile when more than 1 profile is present.");
     }
     return profiles ? profiles[0] : null;
   },
 
+  _pendingUndoTasks: false,
   canUndo: Task.async(function* () {
     if (this._savingPromise) {
       yield this._savingPromise;
     }
+    if (this._pendingUndoTasks) {
+      return false;
+    }
     let fileExists = false;
     try {
       fileExists = yield OS.File.exists(kUndoStateFullPath);
     } catch (ex) {
       Cu.reportError(ex);
     }
     return fileExists;
   }),
@@ -192,16 +196,18 @@ const AutoMigrate = {
   undo: Task.async(function* () {
     let histogram = Services.telemetry.getHistogramById("FX_STARTUP_MIGRATION_AUTOMATED_IMPORT_UNDO");
     histogram.add(0);
     if (!(yield this.canUndo())) {
       histogram.add(5);
       throw new Error("Can't undo!");
     }
 
+    this._pendingUndoTasks = true;
+    this._removeNotificationBars();
     histogram.add(10);
 
     let readPromise = OS.File.read(kUndoStateFullPath, {
       encoding: "utf-8",
       compression: "lz4",
     });
     let stateData = this._dejsonifyUndoState(yield readPromise);
     yield this._removeUnchangedBookmarks(stateData.get("bookmarks"));
@@ -213,41 +219,46 @@ const AutoMigrate = {
     yield this._removeUnchangedLogins(stateData.get("logins"));
     histogram.add(25);
 
     // This is async, but no need to wait for it.
     NewTabUtils.links.populateCache(() => {
       NewTabUtils.allPages.update();
     }, true);
 
-    this.removeUndoOption(this.UNDO_REMOVED_REASON_UNDO_USED);
+    this._purgeUndoState(this.UNDO_REMOVED_REASON_UNDO_USED);
     histogram.add(30);
   }),
 
-  removeUndoOption(reason) {
-    // We don't wait for the off-main-thread removal to complete. OS.File will
-    // ensure it happens before shutdown.
-    OS.File.remove(kUndoStateFullPath, {ignoreAbsent: true});
-
-    let migrationBrowser = Preferences.get(kAutoMigrateBrowserPref, "unknown");
-    Services.prefs.clearUserPref(kAutoMigrateBrowserPref);
-
+  _removeNotificationBars() {
     let browserWindows = Services.wm.getEnumerator("navigator:browser");
     while (browserWindows.hasMoreElements()) {
       let win = browserWindows.getNext();
       if (!win.closed) {
         for (let browser of win.gBrowser.browsers) {
           let nb = win.gBrowser.getNotificationBox(browser);
           let notification = nb.getNotificationWithValue(kNotificationId);
           if (notification) {
             nb.removeNotification(notification);
           }
         }
       }
     }
+  },
+
+  _purgeUndoState(reason) {
+    // We don't wait for the off-main-thread removal to complete. OS.File will
+    // ensure it happens before shutdown.
+    OS.File.remove(kUndoStateFullPath, {ignoreAbsent: true}).then(() => {
+      this._pendingUndoTasks = false;
+    });
+
+    let migrationBrowser = Preferences.get(kAutoMigrateBrowserPref, "unknown");
+    Services.prefs.clearUserPref(kAutoMigrateBrowserPref);
+
     let histogram =
       Services.telemetry.getKeyedHistogramById("FX_STARTUP_MIGRATION_UNDO_REASON");
     histogram.add(migrationBrowser, reason);
   },
 
   getBrowserUsedForMigration() {
     let browserId = Services.prefs.getCharPref(kAutoMigrateBrowserPref);
     if (browserId) {
@@ -273,17 +284,18 @@ const AutoMigrate = {
     if (!notificationBox || notificationBox.getNotificationWithValue(kNotificationId)) {
       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(this.UNDO_REMOVED_REASON_OFFER_EXPIRED);
+      this._purgeUndoState(this.UNDO_REMOVED_REASON_OFFER_EXPIRED);
+      this._removeNotificationBars();
       return;
     }
 
     let browserName = this.getBrowserUsedForMigration();
     let message;
     if (browserName) {
       message = MigrationUtils.getLocalizedString("automigration.undo.message",
                                                   [browserName]);
@@ -291,17 +303,18 @@ const AutoMigrate = {
       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(this.UNDO_REMOVED_REASON_OFFER_REJECTED);
+          this._purgeUndoState(this.UNDO_REMOVED_REASON_OFFER_REJECTED);
+          this._removeNotificationBars();
         },
       },
       {
         label: MigrationUtils.getLocalizedString("automigration.undo.dontkeep.label"),
         accessKey: MigrationUtils.getLocalizedString("automigration.undo.dontkeep.accesskey"),
         callback: () => {
           this.undo();
         },
--- a/browser/components/migration/tests/browser/browser.ini
+++ b/browser/components/migration/tests/browser/browser.ini
@@ -1,1 +1,2 @@
 [browser_undo_notification.js]
+[browser_undo_notification_multiple_dismissal.js]
--- a/browser/components/migration/tests/browser/browser_undo_notification.js
+++ b/browser/components/migration/tests/browser/browser_undo_notification.js
@@ -1,19 +1,17 @@
 "use strict";
 
 let scope = {};
 Cu.import("resource:///modules/AutoMigrate.jsm", scope);
 let oldCanUndo = scope.AutoMigrate.canUndo;
 let oldUndo = scope.AutoMigrate.undo;
 registerCleanupFunction(function() {
-  Cu.reportError("Cleaning up");
   scope.AutoMigrate.canUndo = oldCanUndo;
   scope.AutoMigrate.undo = oldUndo;
-  Cu.reportError("Cleaned up");
 });
 
 const kExpectedNotificationId = "automigration-undo";
 
 add_task(function* autoMigrationUndoNotificationShows() {
   let getNotification = browser =>
     gBrowser.getNotificationBox(browser).getNotificationWithValue(kExpectedNotificationId);
 
new file mode 100644
--- /dev/null
+++ b/browser/components/migration/tests/browser/browser_undo_notification_multiple_dismissal.js
@@ -0,0 +1,122 @@
+"use strict";
+
+
+const kExpectedNotificationId = "automigration-undo";
+
+/**
+ * Pretend we can undo something, trigger a notification, pick the undo option,
+ * and verify that the notifications are all dismissed immediately.
+ */
+add_task(function* checkNotificationsDismissed() {
+  yield SpecialPowers.pushPrefEnv({set: [
+    ["browser.migrate.automigrate.enabled", true],
+    ["browser.migrate.automigrate.ui.enabled", true],
+  ]});
+  let getNotification = browser =>
+    gBrowser.getNotificationBox(browser).getNotificationWithValue(kExpectedNotificationId);
+
+  Services.prefs.setCharPref("browser.migrate.automigrate.browser", "someunknownbrowser");
+
+  let {guid, lastModified} = yield PlacesUtils.bookmarks.insert(
+    {title: "Some imported bookmark", parentGuid: PlacesUtils.bookmarks.toolbarGuid, url: "http://www.example.com"}
+  );
+
+  let testUndoData = {
+    visits: [],
+    bookmarks: [{guid, lastModified: lastModified.getTime()}],
+    logins: [],
+  };
+  let path = OS.Path.join(OS.Constants.Path.profileDir, "initialMigrationMetadata.jsonlz4");
+  registerCleanupFunction(() => {
+    return OS.File.remove(path, {ignoreAbsent: true});
+  });
+  yield OS.File.writeAtomic(path, JSON.stringify(testUndoData), {
+    encoding: "utf-8",
+    compression: "lz4",
+    tmpPath: path + ".tmp",
+  });
+
+  let firstTab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:home", false);
+  if (!getNotification(firstTab.linkedBrowser)) {
+    info(`Notification not immediately present on first tab, waiting for it.`);
+    yield BrowserTestUtils.waitForNotificationBar(gBrowser, firstTab.linkedBrowser, kExpectedNotificationId);
+  }
+  let secondTab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:home", false);
+  if (!getNotification(secondTab.linkedBrowser)) {
+    info(`Notification not immediately present on second tab, waiting for it.`);
+    yield BrowserTestUtils.waitForNotificationBar(gBrowser, secondTab.linkedBrowser, kExpectedNotificationId);
+  }
+
+  // Create a listener for the removal in the first tab, and a listener for bookmarks removal,
+  // then click 'Don't keep' in the second tab, and verify that the notification is removed
+  // before we start removing bookmarks.
+  let haveRemovedBookmark = false;
+  let bmObserver;
+  let bookmarkRemovedPromise = new Promise(resolve => {
+    bmObserver = {
+      onItemRemoved(itemId, parentId, index, itemType, uri, removedGuid) {
+        if (guid == removedGuid) {
+          haveRemovedBookmark = true;
+          resolve();
+        }
+      },
+    };
+    PlacesUtils.bookmarks.addObserver(bmObserver, false);
+    registerCleanupFunction(() => PlacesUtils.bookmarks.removeObserver(bmObserver));
+  });
+
+  let firstTabNotificationRemovedPromise = new Promise(resolve => {
+    let notification = getNotification(firstTab.linkedBrowser);
+    // Save this reference because notification.parentNode will be null once it's removed.
+    let notificationBox = notification.parentNode;
+    let mut = new MutationObserver(mutations => {
+      // Yucky, but we have to detect either the removal via animation (with marginTop)
+      // or when the element is removed. We can't just detect the element being removed
+      // because this happens asynchronously (after the animation) and so it'd race
+      // with the rest of the undo happening.
+      for (let mutation of mutations) {
+        if (mutation.target == notification && mutation.attributeName == "style" &&
+            parseInt(notification.style.marginTop, 10) < 0) {
+          ok(!haveRemovedBookmark, "Should not have removed bookmark yet");
+          mut.disconnect();
+          resolve();
+          return;
+        }
+        if (mutation.target == notificationBox && mutation.removedNodes.length &&
+            mutation.removedNodes[0] == notification) {
+          ok(!haveRemovedBookmark, "Should not have removed bookmark yet");
+          mut.disconnect();
+          resolve();
+          return;
+        }
+      }
+    });
+    mut.observe(notification.parentNode, {childList: true});
+    mut.observe(notification, {attributes: true});
+  });
+
+  let prefResetPromise = new Promise(resolve => {
+    const kObservedPref = "browser.migrate.automigrate.browser";
+    let obs = () => {
+      Services.prefs.removeObserver(kObservedPref, obs);
+      ok(!Services.prefs.prefHasUserValue(kObservedPref),
+         "Pref should have been reset");
+      resolve();
+    };
+    Services.prefs.addObserver(kObservedPref, obs, false);
+  });
+
+  // Click "Don't keep" button:
+  let notificationToActivate = getNotification(secondTab.linkedBrowser);
+  notificationToActivate.querySelector("button:not(.notification-button-default)").click();
+  info("Waiting for notification to be removed in first (background) tab");
+  yield firstTabNotificationRemovedPromise;
+  info("Waiting for bookmark to be removed");
+  yield bookmarkRemovedPromise;
+  info("Waiting for prefs to be reset");
+  yield prefResetPromise;
+
+  info("Removing spare tabs");
+  yield BrowserTestUtils.removeTab(firstTab);
+  yield BrowserTestUtils.removeTab(secondTab);
+});