Bug 1553510 - Don't compute whether or not to show the Bookmarks Toolbar for new profiles until Places has finished initting. r=MattN
authorMike Conley <mconley@mozilla.com>
Thu, 07 Nov 2019 21:41:04 +0000
changeset 501175 df2ba9ebafc9181cbac08044596bdaf311c28fe5
parent 501174 c179bdbb0d2ac61df8672c7baa31e2ecb797cd05
child 501176 fe7a4ae3b3849ea16323e1832d9b96063b95df56
push id114168
push userdluca@mozilla.com
push dateSun, 10 Nov 2019 03:08:55 +0000
treeherdermozilla-inbound@33f64c1ef3e4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1553510
milestone72.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 1553510 - Don't compute whether or not to show the Bookmarks Toolbar for new profiles until Places has finished initting. r=MattN This causes BrowserGlue to wait until Places has notified that it's initted before checking to see whether or not the Bookmarks Toolbar should be shown. Also, if it turns out that the Bookmarks Toolbar is shown, we now use CustomizableUI to do this, which means that the Bookmarks Toolbar will be made visible on all windows after the check is run - not just new windows. Differential Revision: https://phabricator.services.mozilla.com/D51701
browser/components/BrowserGlue.jsm
browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js
--- a/browser/components/BrowserGlue.jsm
+++ b/browser/components/BrowserGlue.jsm
@@ -15,16 +15,22 @@ const { AppConstants } = ChromeUtils.imp
 );
 
 ChromeUtils.defineModuleGetter(
   this,
   "ActorManagerParent",
   "resource://gre/modules/ActorManagerParent.jsm"
 );
 
+ChromeUtils.defineModuleGetter(
+  this,
+  "CustomizableUI",
+  "resource:///modules/CustomizableUI.jsm"
+);
+
 XPCOMUtils.defineLazyServiceGetter(
   this,
   "PushService",
   "@mozilla.org/push/Service;1",
   "nsIPushService"
 );
 
 const PREF_PDFJS_ENABLED_CACHE_STATE = "pdfjs.enabledCache.state";
@@ -746,16 +752,17 @@ function BrowserGlue() {
 
   this._init();
 }
 
 BrowserGlue.prototype = {
   _saveSession: false,
   _migrationImportsDefaultBookmarks: false,
   _placesBrowserInitComplete: false,
+  _isNewProfile: undefined,
 
   _setPrefToSaveSession: function BG__setPrefToSaveSession(aForce) {
     if (!this._saveSession && !aForce) {
       return;
     }
 
     if (!PrivateBrowsingUtils.permanentPrivateBrowsing) {
       Services.prefs.setBoolPref(
@@ -2672,16 +2679,27 @@ BrowserGlue.prototype = {
 
             if (backupAge > BOOKMARKS_BACKUP_MAX_INTERVAL_DAYS) {
               this._bookmarksBackupIdleTime /= 2;
             }
           }
         }
         this._idleService.addIdleObserver(this, this._bookmarksBackupIdleTime);
       }
+
+      if (this._isNewProfile) {
+        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);
+        }
+      }
     })()
       .catch(ex => {
         Cu.reportError(ex);
       })
       .then(() => {
         // NB: deliberately after the catch so that we always do this, even if
         // we threw halfway through initializing in the Task above.
         this._placesBrowserInitComplete = true;
@@ -2808,21 +2826,19 @@ BrowserGlue.prototype = {
         toolbarFolder.containerOpen = false;
         return toolbarChildCount;
       };
 
       if (
         toolbarIsCustomized ||
         getToolbarFolderCount() > NUM_TOOLBAR_BOOKMARKS_TO_UNHIDE
       ) {
-        xulStore.setValue(
-          BROWSER_DOCURL,
-          "PersonalToolbar",
-          "collapsed",
-          "false"
+        CustomizableUI.setToolbarVisibility(
+          CustomizableUI.AREA_BOOKMARKS,
+          true
         );
       }
     }
   },
 
   _migrateXULStoreForDocument(fromURL, toURL) {
     Array.from(Services.xulStore.getIDsEnumerator(fromURL)).forEach(id => {
       Array.from(Services.xulStore.getAttributeEnumerator(fromURL, id)).forEach(
@@ -2839,29 +2855,21 @@ BrowserGlue.prototype = {
     // Use an increasing number to keep track of the current migration state.
     // Completely unrelated to the current Firefox release number.
     const UI_VERSION = 88;
     const BROWSER_DOCURL = AppConstants.BROWSER_CHROME_URL;
 
     let currentUIVersion;
     if (Services.prefs.prefHasUserValue("browser.migration.version")) {
       currentUIVersion = Services.prefs.getIntPref("browser.migration.version");
+      this._isNewProfile = false;
     } 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;
+      this._isNewProfile = true;
     }
 
     if (currentUIVersion >= UI_VERSION) {
       return;
     }
 
     let xulStore = Services.xulStore;
 
--- a/browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js
+++ b/browser/components/tests/browser/browser_default_bookmark_toolbar_visibility.js
@@ -3,16 +3,36 @@
 
 /**
  * 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() {
+  // The Bookmarks Toolbar visibility state should be set only after
+  // Places has notified that it's done initializing.
+  const browserGlue = Cc["@mozilla.org/browser/browserglue;1"].getService(
+    Ci.nsIObserver
+  );
+
+  let placesInitCompleteObserved = TestUtils.topicObserved(
+    "places-browser-init-complete"
+  );
+
+  // If places-browser-init-complete has already notified, this will cause it
+  // to notify again. Otherwise, we wait until the notify is done.
+  browserGlue.observe(
+    null,
+    "browser-glue-test",
+    "places-browser-init-complete"
+  );
+
+  await placesInitCompleteObserved;
+
   const BROWSER_DOCURL = AppConstants.BROWSER_CHROME_URL;
   let xulStore = Services.xulStore;
 
   is(
     xulStore.getValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed"),
     "",
     "Check that @collapsed isn't persisted"
   );