Bug 1665442 - remove the import button once the user successfully imports bookmarks, r=jaws

Any import of bookmarks that attempts to insert at least 1 bookmark successfully
will be treated as a success. This avoids discounting imports where one or more
bookmarks could not be imported due to issues with the URL, especially because
the user has no obvious remedial options - retrying is unlikely to fix things.

Differential Revision: https://phabricator.services.mozilla.com/D93768

browser/components/BrowserGlue.jsm

@@ -2426,7 +2426,6 @@ BrowserGlue.prototype = {
    * to the other ones scheduled together.
    */
   _scheduleStartupIdleTasks() {
-    let isNewProfile = this._isNewProfile;
     const idleTasks = [
       // It's important that SafeBrowsing is initialized reasonably
       // early, so we use a maximum timeout for it.
@@ -2639,9 +2638,9 @@ BrowserGlue.prototype = {
 
       // Add the import button if this is the first startup.
       {
-        task: () => {
+        task: async () => {
           if (
-            isNewProfile &&
+            this._isNewProfile &&
             Services.prefs.getBoolPref(
               "browser.toolbars.bookmarks.2h2020",
               false
@@ -2649,7 +2648,16 @@ BrowserGlue.prototype = {
             // Not in automation: the button changes CUI state, breaking tests
             !Cu.isInAutomation
           ) {
-            PlacesUIUtils.maybeAddImportButton();
+            await PlacesUIUtils.maybeAddImportButton();
+          }
+
+          if (
+            Services.prefs.getBoolPref(
+              "browser.bookmarks.addedImportButton",
+              false
+            )
+          ) {
+            PlacesUIUtils.removeImportButtonWhenImportSucceeds();
           }
         },
       },

browser/components/migration/MigrationUtils.jsm

@@ -1108,6 +1108,15 @@ var MigrationUtils = Object.seal({
     history: 0,
   },
 
+  getImportedCount(type) {
+    if (!this._importQuantities.hasOwnProperty(type)) {
+      throw new Error(
+        `Unknown import data type "${type}" passed to getImportedCount`
+      );
+    }
+    return this._importQuantities[type];
+  },
+
   insertBookmarkWrapper(bookmark) {
     this._importQuantities.bookmarks++;
     let insertionPromise = PlacesUtils.bookmarks.insert(bookmark);

browser/components/places/PlacesUIUtils.jsm

@@ -19,6 +19,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   AppConstants: "resource://gre/modules/AppConstants.jsm",
   BrowserWindowTracker: "resource:///modules/BrowserWindowTracker.jsm",
   CustomizableUI: "resource:///modules/CustomizableUI.jsm",
+  MigrationUtils: "resource:///modules/MigrationUtils.jsm",
   OpenInTabsUtils: "resource:///modules/OpenInTabsUtils.jsm",
   PlacesTransactions: "resource://gre/modules/PlacesTransactions.jsm",
   PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
@@ -1424,8 +1425,33 @@ var PlacesUIUtils = {
         CustomizableUI.AREA_BOOKMARKS,
         0
       );
+      Services.prefs.setBoolPref("browser.bookmarks.addedImportButton", true);
     }
   },
+
+  removeImportButtonWhenImportSucceeds() {
+    // If the user (re)moved the button, clear the pref and stop worrying about
+    // moving the item.
+    let placement = CustomizableUI.getPlacementOfWidget("import-button");
+    if (placement?.area != CustomizableUI.AREA_BOOKMARKS) {
+      Services.prefs.clearUserPref("browser.bookmarks.addedImportButton");
+      return;
+    }
+    // Otherwise, wait for a successful migration:
+    let obs = (subject, topic, data) => {
+      if (
+        data == Ci.nsIBrowserProfileMigrator.BOOKMARKS &&
+        MigrationUtils.getImportedCount("bookmarks") > 0
+      ) {
+        CustomizableUI.removeWidgetFromArea("import-button");
+        Services.prefs.clearUserPref("browser.bookmarks.addedImportButton");
+        Services.obs.removeObserver(obs, "Migration:ItemAfterMigrate");
+        Services.obs.removeObserver(obs, "Migration:ItemError");
+      }
+    };
+    Services.obs.addObserver(obs, "Migration:ItemAfterMigrate");
+    Services.obs.addObserver(obs, "Migration:ItemError");
+  },
 };
 
 // These are lazy getters to avoid importing PlacesUtils immediately.

browser/components/places/tests/browser/browser_import_button.js

@@ -3,6 +3,8 @@
 
 "use strict";
 
+const kPref = "browser.bookmarks.addedImportButton";
+
 /**
  * Verify that we add the import button only if there aren't enough bookmarks
  * in the toolbar.
@@ -22,6 +24,7 @@ add_task(async function test_bookmark_import_button() {
   );
   await PlacesUIUtils.maybeAddImportButton();
   ok(document.getElementById("import-button"), "Button should be added.");
+  is(Services.prefs.getBoolPref(kPref), true, "Pref should be set.");
 
   CustomizableUI.reset();
 
@@ -36,9 +39,15 @@ add_task(async function test_bookmark_import_button() {
       })
     )
   );
-  registerCleanupFunction(async () =>
-    Promise.all(bookmarks.map(b => PlacesUtils.bookmarks.remove(b.guid)))
-  );
+
+  // Ensure we remove items after this task, or worst-case after this test
+  // file has completed.
+  let removeAllBookmarks = () => {
+    let removals = bookmarks.map(b => PlacesUtils.bookmarks.remove(b.guid));
+    bookmarks = [];
+    return Promise.all(removals);
+  };
+  registerCleanupFunction(removeAllBookmarks);
 
   await PlacesUIUtils.maybeAddImportButton();
   ok(
@@ -48,4 +57,132 @@ add_task(async function test_bookmark_import_button() {
 
   // Just in case, for future tests we run:
   CustomizableUI.reset();
+
+  await removeAllBookmarks();
+});
+
+/**
+ * Verify the button gets removed when we import bookmarks successfully.
+ */
+add_task(async function test_bookmark_import_button_removal() {
+  let bookmarkCount = PlacesUtils.getChildCountForFolder(
+    PlacesUtils.bookmarks.toolbarGuid
+  );
+  Assert.less(bookmarkCount, 3, "we should start with less than 3 bookmarks");
+
+  ok(
+    !document.getElementById("import-button"),
+    "Shouldn't have button to start with."
+  );
+  await PlacesUIUtils.maybeAddImportButton();
+  ok(document.getElementById("import-button"), "Button should be added.");
+  is(Services.prefs.getBoolPref(kPref), true, "Pref should be set.");
+
+  PlacesUIUtils.removeImportButtonWhenImportSucceeds();
+
+  Services.obs.notifyObservers(
+    null,
+    "Migration:ItemAfterMigrate",
+    Ci.nsIBrowserProfileMigrator.BOOKMARKS
+  );
+
+  is(
+    Services.prefs.getBoolPref(kPref, false),
+    true,
+    "Pref should stay without import."
+  );
+  ok(document.getElementById("import-button"), "Button should still be there.");
+
+  // OK, actually add some bookmarks:
+  MigrationUtils._importQuantities.bookmarks = 5;
+  Services.obs.notifyObservers(
+    null,
+    "Migration:ItemAfterMigrate",
+    Ci.nsIBrowserProfileMigrator.BOOKMARKS
+  );
+
+  is(Services.prefs.prefHasUserValue(kPref), false, "Pref should be removed.");
+  ok(
+    !document.getElementById("import-button"),
+    "Button should have been removed."
+  );
+
+  // Reset this, otherwise subsequent tests are going to have a bad time.
+  MigrationUtils._importQuantities.bookmarks = 0;
+});
+
+/**
+ * Check that if the user removes the button, the next startup
+ * we clear the pref and stop monitoring to remove the item.
+ */
+add_task(async function test_bookmark_import_button_removal_cleanup() {
+  let bookmarkCount = PlacesUtils.getChildCountForFolder(
+    PlacesUtils.bookmarks.toolbarGuid
+  );
+  Assert.less(bookmarkCount, 3, "we should start with less than 3 bookmarks");
+
+  ok(
+    !document.getElementById("import-button"),
+    "Shouldn't have button to start with."
+  );
+  await PlacesUIUtils.maybeAddImportButton();
+  ok(document.getElementById("import-button"), "Button should be added.");
+  is(Services.prefs.getBoolPref(kPref), true, "Pref should be set.");
+
+  // Simulate the user removing the item.
+  CustomizableUI.removeWidgetFromArea("import-button");
+
+  // We'll call this next startup:
+  PlacesUIUtils.removeImportButtonWhenImportSucceeds();
+
+  // And it should clean up the pref:
+  is(Services.prefs.prefHasUserValue(kPref), false, "Pref should be removed.");
+});
+
+/**
+ * Check that if migration (silently) errors, we still remove the button
+ * _if_ we imported any bookmarks.
+ */
+add_task(async function test_bookmark_import_button_errors() {
+  let bookmarkCount = PlacesUtils.getChildCountForFolder(
+    PlacesUtils.bookmarks.toolbarGuid
+  );
+  Assert.less(bookmarkCount, 3, "we should start with less than 3 bookmarks");
+
+  ok(
+    !document.getElementById("import-button"),
+    "Shouldn't have button to start with."
+  );
+  await PlacesUIUtils.maybeAddImportButton();
+  ok(document.getElementById("import-button"), "Button should be added.");
+  is(Services.prefs.getBoolPref(kPref), true, "Pref should be set.");
+
+  PlacesUIUtils.removeImportButtonWhenImportSucceeds();
+
+  Services.obs.notifyObservers(
+    null,
+    "Migration:ItemError",
+    Ci.nsIBrowserProfileMigrator.BOOKMARKS
+  );
+
+  is(
+    Services.prefs.getBoolPref(kPref, false),
+    true,
+    "Pref should stay when fatal error happens."
+  );
+  ok(document.getElementById("import-button"), "Button should still be there.");
+
+  // OK, actually add some bookmarks:
+  MigrationUtils._importQuantities.bookmarks = 5;
+  Services.obs.notifyObservers(
+    null,
+    "Migration:ItemError",
+    Ci.nsIBrowserProfileMigrator.BOOKMARKS
+  );
+
+  is(Services.prefs.prefHasUserValue(kPref), false, "Pref should be removed.");
+  ok(
+    !document.getElementById("import-button"),
+    "Button should have been removed."
+  );
 });