From c2ae84a7b4dce2a0364f38e6929094fea47b0a27 Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Tue, 20 Oct 2020 15:28:28 +0000 Subject: [PATCH] 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 | 16 +- .../components/migration/MigrationUtils.jsm | 9 ++ browser/components/places/PlacesUIUtils.jsm | 26 ++++ .../tests/browser/browser_import_button.js | 143 +++++++++++++++++- 4 files changed, 187 insertions(+), 7 deletions(-) diff --git a/browser/components/BrowserGlue.jsm b/browser/components/BrowserGlue.jsm index 86512a9ade81..eeb53adab732 100644 --- a/browser/components/BrowserGlue.jsm +++ b/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(); } }, }, diff --git a/browser/components/migration/MigrationUtils.jsm b/browser/components/migration/MigrationUtils.jsm index 80c51a944a76..8d695f900170 100644 --- a/browser/components/migration/MigrationUtils.jsm +++ b/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); diff --git a/browser/components/places/PlacesUIUtils.jsm b/browser/components/places/PlacesUIUtils.jsm index 3d4c31093bfa..16c65461f786 100644 --- a/browser/components/places/PlacesUIUtils.jsm +++ b/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. diff --git a/browser/components/places/tests/browser/browser_import_button.js b/browser/components/places/tests/browser/browser_import_button.js index ae10eb706247..e52ef126641c 100644 --- a/browser/components/places/tests/browser/browser_import_button.js +++ b/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." + ); });