Bug 1665389 - create bookmarks on the toolbar by default, r=mak,jaws

This adds a pref containing a parent GUID, a lazy pref getter that validates
that GUID (asynchronously), and starts using the pref from the
PlacesCommandHook.

It also sets the future default (toolbar) into firefox.js, and overrides
that on the default branch when the new 2020 bookmarks pref is not set.

Finally, it sets the pref to the unfiled default for existing profiles
with a migration. If we end up delaying shipping, we'll need to
update that migration - but I don't see a way around that.

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

browser/app/profile/firefox.js

@@ -601,6 +601,18 @@ pref("browser.bookmarks.max_backups",             15);
 // Whether menu should close after Ctrl-click, middle-click, etc.
 pref("browser.bookmarks.openInTabClosesMenu", true);
 
+// Where new bookmarks go by default.
+// Use PlacesUIUtils.defaultParentGuid to read this; do NOT read the pref
+// directly.
+// The pref is ignored if the browser.toolbars.bookmarks.2h2020 pref is false,
+// in which case bookmarks always go in the "Other bookmarks" folder.
+// The value is one of:
+// - a bookmarks guid
+// - "toolbar", "menu" or "unfiled" for those folders.
+// If we use the pref but the value isn't any of these, we'll fall back to
+// the bookmarks toolbar as a default.
+pref("browser.bookmarks.defaultLocation", "toolbar");
+
 // Scripts & Windows prefs
 pref("dom.disable_open_during_load",              true);
 pref("javascript.options.showInConsole",          true);

browser/base/content/browser-places.js

@@ -468,7 +468,9 @@ var PlacesCommandHook = {
     let isNewBookmark = !info;
     let showEditUI = !isNewBookmark || StarUI.showForNewBookmarks;
     if (isNewBookmark) {
-      let parentGuid = PlacesUtils.bookmarks.unfiledGuid;
+      // This is async because we have to validate the guid
+      // coming from prefs.
+      let parentGuid = await PlacesUIUtils.defaultParentGuid;
       info = { url, parentGuid };
       // Bug 1148838 - Make this code work for full page plugins.
       let charset = null;

browser/components/BrowserGlue.jsm

@@ -3317,7 +3317,7 @@ BrowserGlue.prototype = {
   _migrateUI: function BG__migrateUI() {
     // Use an increasing number to keep track of the current migration state.
     // Completely unrelated to the current Firefox release number.
-    const UI_VERSION = 103;
+    const UI_VERSION = 104;
     const BROWSER_DOCURL = AppConstants.BROWSER_CHROME_URL;
 
     if (!Services.prefs.prefHasUserValue("browser.migration.version")) {
@@ -3983,6 +3983,15 @@ BrowserGlue.prototype = {
       );
     }
 
+    // For existing profiles, continue putting bookmarks in the
+    // "other bookmarks" folder.
+    if (currentUIVersion < 104) {
+      Services.prefs.setCharPref(
+        "browser.bookmarks.defaultLocation",
+        "unfiled"
+      );
+    }
+
     // Update the migration version.
     Services.prefs.setIntPref("browser.migration.version", UI_VERSION);
   },

browser/components/places/PlacesUIUtils.jsm

@@ -1457,6 +1457,11 @@ var PlacesUIUtils = {
     Services.obs.addObserver(obs, "Migration:ItemAfterMigrate");
     Services.obs.addObserver(obs, "Migration:ItemError");
   },
+
+  get _nonPrefDefaultParentGuid() {
+    let { unfiledGuid, toolbarGuid } = PlacesUtils.bookmarks;
+    return this._2020h2bookmarks ? toolbarGuid : unfiledGuid;
+  },
 };
 
 // These are lazy getters to avoid importing PlacesUtils immediately.
@@ -1506,6 +1511,36 @@ XPCOMUtils.defineLazyPreferenceGetter(
   7
 );
 
+XPCOMUtils.defineLazyPreferenceGetter(
+  PlacesUIUtils,
+  "_2020h2bookmarks",
+  "browser.toolbars.bookmarks.2h2020",
+  false
+);
+
+XPCOMUtils.defineLazyPreferenceGetter(
+  PlacesUIUtils,
+  "defaultParentGuid",
+  "browser.bookmarks.defaultLocation",
+  "", // Avoid eagerly loading PlacesUtils.
+  null,
+  prefValue => {
+    if (!PlacesUIUtils._2020h2bookmarks) {
+      return PlacesUtils.bookmarks.unfiledGuid;
+    }
+    if (!prefValue) {
+      return PlacesUIUtils._nonPrefDefaultParentGuid;
+    }
+    if (["toolbar", "menu", "unfiled"].includes(prefValue)) {
+      return PlacesUtils.bookmarks[prefValue + "Guid"];
+    }
+    return PlacesUtils.bookmarks
+      .fetch({ guid: prefValue })
+      .then(bm => bm.guid)
+      .catch(() => PlacesUIUtils._nonPrefDefaultParentGuid);
+  }
+);
+
 /**
  * Determines if an unwrapped node can be moved.
  *

browser/components/places/tests/browser/browser.ini

@@ -58,6 +58,7 @@ skip-if = (verify && debug && (os == 'win' || os == 'mac'))
 [browser_controller_onDrop.js]
 [browser_copy_query_without_tree.js]
 [browser_cutting_bookmarks.js]
+[browser_default_bookmark_location.js]
 [browser_drag_bookmarks_on_toolbar.js]
 [browser_enable_toolbar_sidebar.js]
 skip-if = (verify && debug && (os == 'mac' || os == 'linux'))

browser/components/places/tests/browser/browser_bookmarkProperties_folderSelection.js

@@ -41,14 +41,21 @@ add_task(async function test_selectChoose() {
   let menuList = win.document.getElementById("editBMPanel_folderMenuList");
   let folderTreeRow = win.document.getElementById("editBMPanel_folderTreeRow");
 
+  let expectedFolder = gBookmarksToolbar2h2020
+    ? "BookmarksToolbarFolderTitle"
+    : "OtherBookmarksFolderTitle";
+  let expectedGuid = gBookmarksToolbar2h2020
+    ? PlacesUtils.bookmarks.toolbarGuid
+    : PlacesUtils.bookmarks.unfiledGuid;
+
   Assert.equal(
     menuList.label,
-    PlacesUtils.getString("OtherBookmarksFolderTitle"),
+    PlacesUtils.getString(expectedFolder),
     "Should have the other bookmarks folder selected by default"
   );
   Assert.equal(
     menuList.getAttribute("selectedGuid"),
-    PlacesUtils.bookmarks.unfiledGuid,
+    expectedGuid,
     "Should have the correct default guid selected"
   );
   Assert.equal(
@@ -80,12 +87,12 @@ add_task(async function test_selectChoose() {
 
   Assert.equal(
     menuList.getAttribute("selectedGuid"),
-    PlacesUtils.bookmarks.unfiledGuid,
+    expectedGuid,
     "Should still have the correct selected guid"
   );
   Assert.equal(
     menuList.label,
-    PlacesUtils.getString("OtherBookmarksFolderTitle"),
+    PlacesUtils.getString(expectedFolder),
     "Should have kept the same menu label"
   );
 

browser/components/places/tests/browser/browser_bookmark_popup.js

@@ -54,7 +54,7 @@ async function test_bookmarks_popup({
     try {
       if (!isNewBookmark) {
         await PlacesUtils.bookmarks.insert({
-          parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+          parentGuid: await PlacesUIUtils.defaultParentGuid,
           url: TEST_URL,
           title: "Home Page",
         });
@@ -127,6 +127,7 @@ async function test_bookmarks_popup({
         );
       }
 
+      let defaultLocation = await PlacesUIUtils.defaultParentGuid;
       let bookmarkRemovedPromise = Promise.resolve();
       if (isBookmarkRemoved) {
         bookmarkRemovedPromise = PlacesTestUtils.waitForNotification(
@@ -134,8 +135,7 @@ async function test_bookmarks_popup({
           events =>
             events.some(
               event =>
-                event.parentGuid == PlacesUtils.bookmarks.unfiledGuid &&
-                TEST_URL == event.url
+                event.parentGuid == defaultLocation && TEST_URL == event.url
             ),
           "places"
         );

browser/components/places/tests/browser/browser_default_bookmark_location.js

@@ -0,0 +1,87 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const TEST_URL = "about:about";
+let bookmarkPanel;
+let folders;
+let win;
+
+add_task(async function setup() {
+  await PlacesUtils.bookmarks.eraseEverything();
+
+  win = await BrowserTestUtils.openNewBrowserWindow();
+  await BrowserTestUtils.openNewForegroundTab({
+    gBrowser: win.gBrowser,
+    opening: TEST_URL,
+    waitForStateStop: true,
+  });
+
+  let oldTimeout = win.StarUI._autoCloseTimeout;
+  // Make the timeout something big, so it doesn't interact badly with tests.
+  win.StarUI._autoCloseTimeout = 6000000;
+
+  win.StarUI._createPanelIfNeeded();
+  bookmarkPanel = win.document.getElementById("editBookmarkPanel");
+  bookmarkPanel.setAttribute("animate", false);
+
+  registerCleanupFunction(async () => {
+    bookmarkPanel = null;
+    win.StarUI._autoCloseTimeout = oldTimeout;
+    await BrowserTestUtils.closeWindow(win);
+    win = null;
+    await PlacesUtils.bookmarks.eraseEverything();
+  });
+});
+
+/**
+ * Helper to check the selected folder is correct.
+ */
+async function checkSelection() {
+  // Open folder selector.
+  let menuList = win.document.getElementById("editBMPanel_folderMenuList");
+
+  let expectedFolder = win.gBookmarksToolbar2h2020
+    ? "BookmarksToolbarFolderTitle"
+    : "OtherBookmarksFolderTitle";
+  Assert.equal(
+    menuList.label,
+    PlacesUtils.getString(expectedFolder),
+    `Should have ${expectedFolder} selected by default`
+  );
+  Assert.equal(
+    menuList.getAttribute("selectedGuid"),
+    await PlacesUIUtils.defaultParentGuid,
+    "Should have the correct default guid selected"
+  );
+
+  let hiddenPromise = promisePopupHidden(
+    win.document.getElementById("editBookmarkPanel")
+  );
+  // Confirm and close the dialog.
+  win.document.getElementById("editBookmarkPanelRemoveButton").click();
+  await hiddenPromise;
+}
+
+/**
+ * Verify that bookmarks created with the star button go to the default
+ * bookmark location.
+ */
+add_task(async function test_star_location() {
+  await clickBookmarkStar(win);
+  await checkSelection();
+});
+
+/**
+ * Verify that bookmarks created with the shortcut go to the default bookmark
+ * location.
+ */
+add_task(async function test_shortcut_location() {
+  let shownPromise = promisePopupShown(
+    win.document.getElementById("editBookmarkPanel")
+  );
+  win.document.getElementById("Browser:AddBookmarkAs").doCommand();
+  await shownPromise;
+  await checkSelection();
+});