Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. r=Gijs,mak a=jcristau
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Mon, 04 Dec 2017 15:24:02 -0800
changeset 445252 3c0c4b0d577fa418f3a7cc755d129ddb130f1d4c
parent 445251 2ba2da4d0553fe825b5df66a31a1170236e95883
child 445253 2b215d1288330380fdb3dfc6102032e83cd2e5a2
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, jcristau
bugs1415692
milestone58.0
Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. r=Gijs,mak a=jcristau 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
dom/events/test/mochitest.ini
--- 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 = 59;
     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");
+});
--- a/dom/events/test/mochitest.ini
+++ b/dom/events/test/mochitest.ini
@@ -1,9 +1,12 @@
 [DEFAULT]
+# Skip migration work in BG__migrateUI for browser_startup.js since it increases
+# the occurrence of the leak reported in bug 1398563 with test_bug1327798.html.
+prefs = browser.migration.version=9999999
 skip-if = toolkit == 'android' #CRASH_DUMP, RANDOM
 support-files =
   bug226361_iframe.xhtml
   bug299673.js
   bug322588-popup.html
   bug426082.html
   bug545268.html
   bug574663.html