Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. r=Gijs,mak
☠☠ backed out by aabfc14671b5 ☠ ☠
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Fri, 10 Nov 2017 15:04:22 -0800
changeset 444626 d3c111ae8e200b6f02e04db1a27d94061db293c9
parent 444625 06c7f50b73aead72a059557d20acfc0c666d6b37
child 444627 985ed7674b221d955cc33db91464ec039a8c8bea
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, mak
bugs1415692
milestone58.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 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. r=Gijs,mak MozReview-Commit-ID: C3tmqIrt5ak
browser/base/content/test/performance/browser.ini
browser/components/migration/tests/marionette/test_refresh_firefox.py
browser/components/nsBrowserGlue.js
browser/components/tests/browser/browser.ini
browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js
--- a/browser/base/content/test/performance/browser.ini
+++ b/browser/base/content/test/performance/browser.ini
@@ -1,10 +1,13 @@
 [DEFAULT]
 prefs =
+  # Skip migration work in BG__migrateUI for browser_startup.js since it isn't
+  # representative of common startup.
+  browser.migration.version=9999999
   browser.startup.record=true
 support-files =
   head.js
 [browser_appmenu_reflows.js]
 skip-if = asan || debug # Bug 1382809, bug 1369959
 [browser_favicon_load.js]
 [browser_startup.js]
 [browser_startup_content.js]
--- a/browser/components/migration/tests/marionette/test_refresh_firefox.py
+++ b/browser/components/migration/tests/marionette/test_refresh_firefox.py
@@ -39,24 +39,32 @@ class TestFirefoxRefresh(MarionetteTestC
             arguments[0],
             arguments[1],
             "username",
             "password"
           );
           Services.logins.addLogin(myLogin)
         """, script_args=(self._username, self._password))
 
-    def createBookmark(self):
+    def createBookmarkInMenu(self):
         self.marionette.execute_script("""
           let url = arguments[0];
           let title = arguments[1];
           PlacesUtils.bookmarks.insertBookmark(PlacesUtils.bookmarks.bookmarksMenuFolder,
             makeURI(url), 0, title);
         """, script_args=(self._bookmarkURL, self._bookmarkText))
 
+    def createBookmarksOnToolbar(self):
+        self.marionette.execute_script("""
+          for (let i = 1; i <= 5; i++) {
+            PlacesUtils.bookmarks.insertBookmark(PlacesUtils.toolbarFolderId,
+              makeURI(`about:rights?p=${i}`), 0, `Bookmark ${i}`);
+          }
+        """)
+
     def createHistory(self):
         error = self.runAsyncCode("""
           // Copied from PlacesTestUtils, which isn't available in Marionette tests.
           let didReturn;
           PlacesUtils.asyncHistory.updatePlaces(
             [{title: arguments[1], uri: makeURI(arguments[0]), visits: [{
                 transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
                 visitDate: (Date.now() - 5000) * 1000,
@@ -195,24 +203,32 @@ class TestFirefoxRefresh(MarionetteTestC
         self.assertEqual(loginInfo[0]['password'], self._password)
 
         loginCount = self.marionette.execute_script("""
           return Services.logins.getAllLogins().length;
         """)
         # Note that we expect 2 logins - one from us, one from sync.
         self.assertEqual(loginCount, 2, "No other logins are present")
 
-    def checkBookmark(self):
+    def checkBookmarkInMenu(self):
         titleInBookmarks = self.marionette.execute_script("""
           let url = arguments[0];
           let bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(makeURI(url), {}, {});
           return bookmarkIds.length == 1 ? PlacesUtils.bookmarks.getItemTitle(bookmarkIds[0]) : "";
         """, script_args=(self._bookmarkURL,))
         self.assertEqual(titleInBookmarks, self._bookmarkText)
 
+    def checkBookmarkToolbarVisibility(self):
+        toolbarVisible = self.marionette.execute_script("""
+          const BROWSER_DOCURL = "chrome://browser/content/browser.xul";
+          let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
+          return xulStore.getValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed")
+        """)
+        self.assertEqual(toolbarVisible, "false")
+
     def checkHistory(self):
         historyResult = self.runAsyncCode("""
           PlacesUtils.history.fetch(arguments[0]).then(pageInfo => {
             if (!pageInfo) {
               marionetteScriptFinished("No visits found");
             } else {
               marionetteScriptFinished(pageInfo);
             }
@@ -373,28 +389,30 @@ class TestFirefoxRefresh(MarionetteTestC
         self.assertEqual(result["accountData"]["keyFetchToken"], "top-secret");
         if hasMigrated:
           # This test doesn't actually configure sync itself, so the username
           # pref only exists after migration.
           self.assertEqual(result["prefUsername"], "test@test.com");
 
     def checkProfile(self, hasMigrated=False):
         self.checkPassword()
-        self.checkBookmark()
+        self.checkBookmarkInMenu()
         self.checkHistory()
         self.checkFormHistory()
         self.checkFormAutofill()
         self.checkCookie()
         self.checkSync(hasMigrated);
         if hasMigrated:
+            self.checkBookmarkToolbarVisibility()
             self.checkSession()
 
     def createProfileData(self):
         self.savePassword()
-        self.createBookmark()
+        self.createBookmarkInMenu()
+        self.createBookmarksOnToolbar()
         self.createHistory()
         self.createFormHistory()
         self.createFormAutofill()
         self.createCookie()
         self.createSession()
         self.createSync()
 
     def setUpScriptData(self):
--- a/browser/components/nsBrowserGlue.js
+++ b/browser/components/nsBrowserGlue.js
@@ -1738,27 +1738,65 @@ BrowserGlue.prototype = {
     let clickCallback = (subject, topic, data) => {
       if (topic != "alertclickcallback")
         return;
       this._openPreferences("sync", { origin: "doorhanger" });
     };
     this.AlertsService.showAlertNotification(null, title, body, true, null, clickCallback);
   },
 
+  /**
+   * Uncollapses PersonalToolbar if its collapsed status is not
+   * persisted, and user customized it or changed default bookmarks.
+   *
+   * If the user does not have a persisted value for the toolbar's
+   * "collapsed" attribute, try to determine whether it's customized.
+   */
+  _maybeToggleBookmarkToolbarVisibility() {
+    const BROWSER_DOCURL = "chrome://browser/content/browser.xul";
+    const NUM_TOOLBAR_BOOKMARKS_TO_UNHIDE = 3;
+    let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
+
+    if (!xulStore.hasValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed")) {
+      // We consider the toolbar customized if it has more than NUM_TOOLBAR_BOOKMARKS_TO_UNHIDE
+      // children, or if it has a persisted currentset value.
+      let toolbarIsCustomized = xulStore.hasValue(BROWSER_DOCURL, "PersonalToolbar", "currentset");
+      let getToolbarFolderCount = () => {
+        let toolbarFolder = PlacesUtils.getFolderContents(PlacesUtils.toolbarFolderId).root;
+        let toolbarChildCount = toolbarFolder.childCount;
+        toolbarFolder.containerOpen = false;
+        return toolbarChildCount;
+      };
+
+      if (toolbarIsCustomized || getToolbarFolderCount() > NUM_TOOLBAR_BOOKMARKS_TO_UNHIDE) {
+        xulStore.setValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed", "false");
+      }
+    }
+  },
+
   // eslint-disable-next-line complexity
   _migrateUI: function BG__migrateUI() {
     const UI_VERSION = 58;
     const BROWSER_DOCURL = "chrome://browser/content/browser.xul";
 
     let currentUIVersion;
     if (Services.prefs.prefHasUserValue("browser.migration.version")) {
       currentUIVersion = Services.prefs.getIntPref("browser.migration.version");
     } else {
       // This is a new profile, nothing to migrate.
       Services.prefs.setIntPref("browser.migration.version", UI_VERSION);
+
+      try {
+        // New profiles may have existing bookmarks (imported from another browser or
+        // copied into the profile) and we want to show the bookmark toolbar for them
+        // in some cases.
+        this._maybeToggleBookmarkToolbarVisibility();
+      } catch (ex) {
+        Cu.reportError(ex);
+      }
       return;
     }
 
     if (currentUIVersion >= UI_VERSION)
       return;
 
     let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
 
--- a/browser/components/tests/browser/browser.ini
+++ b/browser/components/tests/browser/browser.ini
@@ -1,6 +1,7 @@
 [DEFAULT]
 
 [browser_bug538331.js]
 skip-if = !updater
 reason = test depends on update channel
 [browser_contentpermissionprompt.js]
+[browser_default_bookmark_toolbar_visibility.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js
@@ -0,0 +1,18 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Test _maybeToggleBookmarkToolbarVisibility() code running for new profiles.
+ * Ensure that the bookmarks toolbar is hidden in a default configuration.
+ * If new default bookmarks are added to the toolbar then the threshold of > 3
+ * in NUM_TOOLBAR_BOOKMARKS_TO_UNHIDE may need to be adjusted there.
+ */
+add_task(async function test_default_bookmark_toolbar_visibility() {
+  const BROWSER_DOCURL = "chrome://browser/content/browser.xul";
+  let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
+
+  is(xulStore.getValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed"), "",
+     "Check that @collapsed isn't persisted");
+  ok(document.getElementById("PersonalToolbar").collapsed,
+     "The bookmarks toolbar should be collapsed by default");
+});