Bug 1391280 - store last sidebar command irrespective of whether sidebar was open, r?bgrins,mixedpuppy draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 23 Aug 2017 14:06:26 +0100
changeset 664759 4a812d5d998b5147163729e0924eece4cf96d02b
parent 664736 dd6b788f149763c4014c27f2fe1a1d13228bda82
child 731533 63671bcb259ecf5e53125115d693fbf3d109622a
push id79793
push usergijskruitbosch@gmail.com
push dateThu, 14 Sep 2017 10:06:23 +0000
reviewersbgrins, mixedpuppy
bugs1391280
milestone57.0a1
Bug 1391280 - store last sidebar command irrespective of whether sidebar was open, r?bgrins,mixedpuppy MozReview-Commit-ID: HBfdW5vEZaD
browser/base/content/browser-sidebar.js
browser/components/customizableui/test/browser_sidebar_toggle.js
browser/components/sessionstore/SessionStore.jsm
--- a/browser/base/content/browser-sidebar.js
+++ b/browser/base/content/browser-sidebar.js
@@ -59,28 +59,34 @@ var SidebarUI = {
     this._switcherArrow = document.getElementById("sidebar-switcher-arrow");
 
     this._switcherTarget.addEventListener("command", () => {
       this.toggleSwitcherPanel();
     });
   },
 
   uninit() {
-    let enumerator = Services.wm.getEnumerator(null);
+    // If this is the last browser window, persist various values that should be
+    // remembered for after a restart / reopening a browser window.
+    let enumerator = Services.wm.getEnumerator("navigator:browser");
     enumerator.getNext();
     if (!enumerator.hasMoreElements()) {
       document.persist("sidebar-box", "sidebarcommand");
 
       let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
-
       if (this._box.hasAttribute("positionend")) {
         document.persist("sidebar-box", "positionend");
       } else {
         xulStore.removeValue(document.documentURI, "sidebar-box", "positionend");
       }
+      if (this._box.hasAttribute("checked")) {
+        document.persist("sidebar-box", "checked");
+      } else {
+        xulStore.removeValue(document.documentURI, "sidebar-box", "checked");
+      }
 
       document.persist("sidebar-box", "width");
       document.persist("sidebar-title", "value");
     }
   },
 
   /**
    * Opens the switcher panel if it's closed, or closes it if it's open.
@@ -175,23 +181,29 @@ var SidebarUI = {
     // If the opener had a sidebar, open the same sidebar in our window.
     // The opener can be the hidden window too, if we're coming from the state
     // where no windows are open, and the hidden window has no sidebar box.
     let sourceUI = sourceWindow.SidebarUI;
     if (!sourceUI || !sourceUI._box) {
       // no source UI or no _box means we also can't adopt the state.
       return false;
     }
+
+    // Set sidebar command even if hidden, so that we keep the same sidebar
+    // even if it's currently closed.
+    let commandID = sourceUI._box.getAttribute("sidebarcommand");
+    if (commandID) {
+      this._box.setAttribute("sidebarcommand", commandID);
+    }
+
     if (sourceUI._box.hidden) {
       // just hidden means we have adopted the hidden state.
       return true;
     }
 
-    let commandID = sourceUI._box.getAttribute("sidebarcommand");
-
     // dynamically generated sidebars will fail this check, but we still
     // consider it adopted.
     if (!document.getElementById(commandID)) {
       return true;
     }
 
     this._box.setAttribute("width", sourceUI._box.boxObject.width);
     this.showInitially(commandID);
@@ -218,28 +230,32 @@ var SidebarUI = {
       }
       // Try to adopt the sidebar state from the source window
       if (this.adoptFromWindow(sourceWindow)) {
         return;
       }
     }
 
     // If we're not adopting settings from a parent window, set them now.
-    let commandID = this._box.getAttribute("sidebarcommand");
-    if (!commandID) {
+    let wasOpen = this._box.getAttribute("checked");
+    if (!wasOpen) {
       return;
     }
 
-    if (document.getElementById(commandID)) {
+    let commandID = this._box.getAttribute("sidebarcommand");
+    if (commandID && document.getElementById(commandID)) {
       this.showInitially(commandID);
     } else {
+      this._box.removeAttribute("checked");
       // Remove the |sidebarcommand| attribute, because the element it
       // refers to no longer exists, so we should assume this sidebar
       // panel has been uninstalled. (249883)
-      this._box.removeAttribute("sidebarcommand");
+      // We use setAttribute rather than removeAttribute so it persists
+      // correctly.
+      this._box.setAttribute("sidebarcommand", "");
       // On a startup in which the startup cache was invalidated (e.g. app update)
       // extensions will not be started prior to delayedLoad, thus the
       // sidebarcommand element will not exist yet.  Store the commandID so
       // extensions may reopen if necessary.  A startup cache invalidation
       // can be forced (for testing) by deleting compatibility.ini from the
       // profile.
       this.lastOpenedId = commandID;
     }
@@ -269,20 +285,19 @@ var SidebarUI = {
    * True if the sidebar is currently open.
    */
   get isOpen() {
     return !this._box.hidden;
   },
 
   /**
    * The ID of the current sidebar (ie, the ID of the broadcaster being used).
-   * This can be set even if the sidebar is hidden.
    */
   get currentID() {
-    return this._box.getAttribute("sidebarcommand");
+    return this.isOpen ? this._box.getAttribute("sidebarcommand") : "";
   },
 
   get title() {
     return this._title.value;
   },
 
   set title(value) {
     this._title.value = value;
@@ -303,18 +318,22 @@ var SidebarUI = {
    *
    * @param  {string}  commandID     ID of the xul:broadcaster element to use.
    * @param  {DOMNode} [triggerNode] Node, usually a button, that triggered the
    *                                 visibility toggling of the sidebar.
    * @return {Promise}
    */
   toggle(commandID = this.lastOpenedId, triggerNode) {
     // First priority for a default value is this.lastOpenedId which is set during show()
-    // and not reset in hide(), unlike currentID. If show() hasn't been called or the command
-    // doesn't exist anymore, then fallback to a default sidebar.
+    // and not reset in hide(), unlike currentID. If show() hasn't been called and we don't
+    // have a persisted command either, or the command doesn't exist anymore, then
+    // fallback to a default sidebar.
+    if (!commandID) {
+      commandID = this._box.getAttribute("sidebarcommand");
+    }
     if (!commandID || !this.getBroadcasterById(commandID)) {
       commandID = this.DEFAULT_SIDEBAR_ID;
     }
 
     if (this.isOpen && commandID == this.currentID) {
       this.hide(triggerNode);
       return Promise.resolve();
     }
@@ -451,17 +470,16 @@ var SidebarUI = {
     // so that we can free memory by unloading the page. We need to explicitly
     // create a new content viewer because the old one doesn't get destroyed
     // until about:blank has loaded (which does not happen as long as the
     // element is hidden).
     this.browser.setAttribute("src", "about:blank");
     this.browser.docShell.createAboutBlankContentViewer(null);
 
     sidebarBroadcaster.removeAttribute("checked");
-    this._box.setAttribute("sidebarcommand", "");
     this._box.removeAttribute("checked");
     this._box.hidden = this._splitter.hidden = true;
 
     let selBrowser = gBrowser.selectedBrowser;
     selBrowser.focus();
     selBrowser.messageManager.sendAsyncMessage("Sidebar:VisibilityChange",
       {commandID, isOpen: false}
     );
--- a/browser/components/customizableui/test/browser_sidebar_toggle.js
+++ b/browser/components/customizableui/test/browser_sidebar_toggle.js
@@ -8,36 +8,44 @@ registerCleanupFunction(async function()
   await resetCustomization();
 
   // Ensure sidebar is hidden after each test:
   if (!document.getElementById("sidebar-box").hidden) {
     SidebarUI.hide();
   }
 });
 
-var showSidebar = async function() {
-  let button = document.getElementById("sidebar-button");
-  let sidebarFocusedPromise = BrowserTestUtils.waitForEvent(document, "SidebarFocused");
-  EventUtils.synthesizeMouseAtCenter(button, {});
+var showSidebar = async function(win = window) {
+  let button = win.document.getElementById("sidebar-button");
+  let sidebarFocusedPromise = BrowserTestUtils.waitForEvent(win.document, "SidebarFocused");
+  EventUtils.synthesizeMouseAtCenter(button, {}, win);
   await sidebarFocusedPromise;
-  ok(SidebarUI.isOpen, "Sidebar is opened");
+  ok(win.SidebarUI.isOpen, "Sidebar is opened");
   ok(button.hasAttribute("checked"), "Toolbar button is checked");
 };
 
-var hideSidebar = async function() {
-  let button = document.getElementById("sidebar-button");
-  EventUtils.synthesizeMouseAtCenter(button, {});
-  ok(!SidebarUI.isOpen, "Sidebar is closed");
+var hideSidebar = async function(win = window) {
+  let button = win.document.getElementById("sidebar-button");
+  EventUtils.synthesizeMouseAtCenter(button, {}, win);
+  ok(!win.SidebarUI.isOpen, "Sidebar is closed");
   ok(!button.hasAttribute("checked"), "Toolbar button isn't checked");
 };
 
 // Check the sidebar widget shows the default items
 add_task(async function() {
   CustomizableUI.addWidgetToArea("sidebar-button", "nav-bar");
 
   await showSidebar();
   is(SidebarUI.currentID, "viewBookmarksSidebar", "Default sidebar selected");
   await SidebarUI.show("viewHistorySidebar");
 
   await hideSidebar();
   await showSidebar();
   is(SidebarUI.currentID, "viewHistorySidebar", "Selected sidebar remembered");
+
+  await hideSidebar();
+  let otherWin = await BrowserTestUtils.openNewBrowserWindow({opener: window});
+  await showSidebar(otherWin);
+  is(otherWin.SidebarUI.currentID, "viewHistorySidebar", "Selected sidebar remembered across windows");
+  await hideSidebar(otherWin);
+
+  await BrowserTestUtils.closeWindow(otherWin);
 });
--- a/browser/components/sessionstore/SessionStore.jsm
+++ b/browser/components/sessionstore/SessionStore.jsm
@@ -3070,18 +3070,19 @@ var SessionStoreInternal = {
     var hidden = WINDOW_HIDEABLE_FEATURES.filter(function(aItem) {
       return aWindow[aItem] && !aWindow[aItem].visible;
     });
     if (hidden.length != 0)
       winData.hidden = hidden.join(",");
     else if (winData.hidden)
       delete winData.hidden;
 
-    var sidebar = aWindow.document.getElementById("sidebar-box").getAttribute("sidebarcommand");
-    if (sidebar)
+    let sidebarBox = aWindow.document.getElementById("sidebar-box");
+    let sidebar = sidebarBox.getAttribute("sidebarcommand");
+    if (sidebar && sidebarBox.getAttribute("checked") == "true")
       winData.sidebar = sidebar;
     else if (winData.sidebar)
       delete winData.sidebar;
   },
 
   /**
    * gather session data as object
    * @param aUpdateAll
@@ -4084,18 +4085,19 @@ var SessionStoreInternal = {
         case "minimized":
           aWindow.minimize();
           break;
         case "normal":
           aWindow.restore();
           break;
         }
       }
-      var sidebar = aWindow.document.getElementById("sidebar-box");
-      if (sidebar.getAttribute("sidebarcommand") != aSidebar) {
+      let sidebarBox = aWindow.document.getElementById("sidebar-box");
+      if (aSidebar && (sidebarBox.getAttribute("sidebarcommand") != aSidebar ||
+                       !sidebarBox.getAttribute("checked"))) {
         aWindow.SidebarUI.showInitially(aSidebar);
       }
       // since resizing/moving a window brings it to the foreground,
       // we might want to re-focus the last focused window
       if (this.windowToFocus) {
         this.windowToFocus.focus();
       }
     } finally {