From e6e4165603b8f29013646b7f586e1ceb9761ce64 Mon Sep 17 00:00:00 2001 From: Matthew Noorenberghe Date: Mon, 4 Dec 2017 15:24:02 -0800 Subject: [PATCH] Bug 1415692 - Show the bookmark toolbar in new profiles with > 3 bookmarks on it. r=Gijs,mak MozReview-Commit-ID: C3tmqIrt5ak --HG-- extra : rebase_source : 936b8cab5de587e2dde708b94f8be597f9c83e6a --- .../base/content/test/performance/browser.ini | 3 ++ .../tests/marionette/test_refresh_firefox.py | 26 +++++++++++-- browser/components/nsBrowserGlue.js | 38 +++++++++++++++++++ browser/components/tests/browser/browser.ini | 1 + ...ser_default_bookmark_toolbar_visibility.js | 18 +++++++++ dom/events/test/mochitest.ini | 3 ++ 6 files changed, 85 insertions(+), 4 deletions(-) create mode 100644 browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js diff --git a/browser/base/content/test/performance/browser.ini b/browser/base/content/test/performance/browser.ini index 1b0ab728fb2c..ae95dd95793f 100644 --- a/browser/base/content/test/performance/browser.ini +++ b/browser/base/content/test/performance/browser.ini @@ -5,6 +5,9 @@ # set during early startup to have an impact as a canvas will be used by # startupRecorder.js 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 gfx.canvas.willReadFrequently.enable=true support-files = diff --git a/browser/components/migration/tests/marionette/test_refresh_firefox.py b/browser/components/migration/tests/marionette/test_refresh_firefox.py index fca0e2c4ce07..c5f8a4648c1f 100644 --- a/browser/components/migration/tests/marionette/test_refresh_firefox.py +++ b/browser/components/migration/tests/marionette/test_refresh_firefox.py @@ -44,7 +44,7 @@ class TestFirefoxRefresh(MarionetteTestCase): 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]; @@ -52,6 +52,14 @@ class TestFirefoxRefresh(MarionetteTestCase): 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. @@ -200,7 +208,7 @@ class TestFirefoxRefresh(MarionetteTestCase): # 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), {}, {}); @@ -208,6 +216,14 @@ class TestFirefoxRefresh(MarionetteTestCase): """, 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 => { @@ -378,18 +394,20 @@ class TestFirefoxRefresh(MarionetteTestCase): 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() diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js index d114cf7e1cdf..9ed97cc9ca97 100644 --- a/browser/components/nsBrowserGlue.js +++ b/browser/components/nsBrowserGlue.js @@ -1746,6 +1746,35 @@ BrowserGlue.prototype = { 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; @@ -1757,6 +1786,15 @@ BrowserGlue.prototype = { } 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; } diff --git a/browser/components/tests/browser/browser.ini b/browser/components/tests/browser/browser.ini index d5b9a651a7e4..e99fef91dd8b 100644 --- a/browser/components/tests/browser/browser.ini +++ b/browser/components/tests/browser/browser.ini @@ -4,3 +4,4 @@ skip-if = !updater reason = test depends on update channel [browser_contentpermissionprompt.js] +[browser_default_bookmark_toolbar_visibility.js] diff --git a/browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js b/browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js new file mode 100644 index 000000000000..0e79967171a9 --- /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"); +}); diff --git a/dom/events/test/mochitest.ini b/dom/events/test/mochitest.ini index 2f52688cc3f4..66bff1d57681 100644 --- a/dom/events/test/mochitest.ini +++ b/dom/events/test/mochitest.ini @@ -1,4 +1,7 @@ [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