Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;r=Gijs draft
authorBrian Grinstead <bgrinstead@mozilla.com>
Tue, 13 Jun 2017 08:14:10 -0700
changeset 593453 dc1f7224f62af95c50672e278613da7951db8571
parent 593303 a8f8e440d627d686fa8898483aa9c5da928a8fa4
child 593454 6a76148a3adaef616df2107f62d2a1138dd86aa0
push id63696
push userbgrinstead@mozilla.com
push dateTue, 13 Jun 2017 15:15:10 +0000
reviewersGijs
bugs1360282
milestone56.0a1
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;r=Gijs By running through the same path anytime the sidebar is shown it will be easier to implement the sidebar-button as a toggle, since we can rely on consistent state to determine which sidebar was opened last. MozReview-Commit-ID: 14HXtr8uNmb
browser/base/content/browser-sidebar.js
--- a/browser/base/content/browser-sidebar.js
+++ b/browser/base/content/browser-sidebar.js
@@ -59,17 +59,16 @@ var SidebarUI = {
   },
 
   uninit() {
     let enumerator = Services.wm.getEnumerator(null);
     enumerator.getNext();
     if (!enumerator.hasMoreElements()) {
       document.persist("sidebar-box", "sidebarcommand");
       document.persist("sidebar-box", "width");
-      document.persist("sidebar-box", "src");
       document.persist("sidebar-title", "value");
     }
   },
 
   /**
    * Opens the switcher panel if it's closed, or closes it if it's open.
    */
   toggleSwitcherPanel() {
@@ -164,37 +163,26 @@ var SidebarUI = {
       return false;
     }
     if (sourceUI._box.hidden) {
       // just hidden means we have adopted the hidden state.
       return true;
     }
 
     let commandID = sourceUI._box.getAttribute("sidebarcommand");
-    let commandElem = document.getElementById(commandID);
 
     // dynamically generated sidebars will fail this check, but we still
     // consider it adopted.
-    if (!commandElem) {
+    if (!document.getElementById(commandID)) {
       return true;
     }
 
-    this._title.setAttribute("value",
-                             sourceUI._title.getAttribute("value"));
     this._box.setAttribute("width", sourceUI._box.boxObject.width);
+    this._show(commandID);
 
-    this._box.setAttribute("sidebarcommand", commandID);
-    // Note: we're setting 'src' on this._box, which is a <vbox>, not on
-    // the <browser id="sidebar">. This lets us delay the actual load until
-    // delayedStartup().
-    this._box.setAttribute("src", sourceUI.browser.getAttribute("src"));
-
-    this._setVisibility(true);
-    commandElem.setAttribute("checked", "true");
-    this.browser.setAttribute("src", this._box.getAttribute("src"));
     return true;
   },
 
   windowPrivacyMatches(w1, w2) {
     return PrivateBrowsingUtils.isWindowPrivate(w1) === PrivateBrowsingUtils.isWindowPrivate(w2);
   },
 
   /**
@@ -217,21 +205,18 @@ var SidebarUI = {
     }
 
     // If we're not adopting settings from a parent window, set them now.
     let commandID = this._box.getAttribute("sidebarcommand");
     if (!commandID) {
       return;
     }
 
-    let command = document.getElementById(commandID);
-    if (command) {
-      this._setVisibility(true);
-      command.setAttribute("checked", "true");
-      this.browser.setAttribute("src", this._box.getAttribute("src"));
+    if (document.getElementById(commandID)) {
+      this._show(commandID);
     } else {
       // 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");
     }
   },
 
@@ -300,19 +285,33 @@ var SidebarUI = {
     }
     return this.show(commandID);
   },
 
   /**
    * Show the sidebar, using the parameters from the specified broadcaster.
    * @see SidebarUI note.
    *
+   * This wraps the internal method, including a ping to telemetry.
+   *
    * @param {string} commandID ID of the xul:broadcaster element to use.
    */
   show(commandID) {
+    return this._show(commandID).then(() => {
+      BrowserUITelemetry.countSidebarEvent(commandID, "show");
+    });
+  },
+
+  /**
+   * Implementation for show. Also used internally for sidebars that are shown
+   * when a window is opened and we don't want to ping telemetry.
+   *
+   * @param {string} commandID ID of the xul:broadcaster element to use.
+   */
+  _show(commandID) {
     return new Promise((resolve, reject) => {
       let sidebarBroadcaster = document.getElementById(commandID);
       if (!sidebarBroadcaster || sidebarBroadcaster.localName != "broadcaster") {
         reject(new Error("Invalid sidebar broadcaster specified: " + commandID));
         return;
       }
 
       if (this.isOpen && commandID != this.currentID) {
@@ -336,32 +335,29 @@ var SidebarUI = {
 
       this._setVisibility(true);
 
       this.hideSwitcherPanel();
 
       this._box.setAttribute("sidebarcommand", sidebarBroadcaster.id);
       this.lastOpenedId = sidebarBroadcaster.id;
 
-      let title = sidebarBroadcaster.getAttribute("sidebartitle");
-      if (!title) {
-        title = sidebarBroadcaster.getAttribute("label");
+      let title = sidebarBroadcaster.getAttribute("sidebartitle") ||
+                  sidebarBroadcaster.getAttribute("label");
+
+      // When loading a web page in the sidebar there is no title set on the
+      // broadcaster, as it is instead set by openWebPanel. Don't clear out
+      // the title in this case.
+      if (title) {
+        this._title.value = title;
       }
-      this._title.value = title;
 
       let url = sidebarBroadcaster.getAttribute("sidebarurl");
       this.browser.setAttribute("src", url); // kick off async load
 
-      // We set this attribute here in addition to setting it on the <browser>
-      // element itself, because the code in SidebarUI.uninit() persists this
-      // attribute, not the "src" of the <browser id="sidebar">. The reason it
-      // does that is that we want to delay sidebar load a bit when a browser
-      // window opens. See delayedStartup() and SidebarUI.startDelayedLoad().
-      this._box.setAttribute("src", url);
-
       if (this.browser.contentDocument.location.href != url) {
         this.browser.addEventListener("load", event => {
 
           // We're handling the 'load' event before it bubbles up to the usual
           // (non-capturing) event handlers. Let it bubble up before firing the
           // SidebarFocused event.
           setTimeout(() => this._fireFocusedEvent(), 0);
 
@@ -375,17 +371,16 @@ var SidebarUI = {
         this._fireFocusedEvent();
         resolve();
       }
 
       let selBrowser = gBrowser.selectedBrowser;
       selBrowser.messageManager.sendAsyncMessage("Sidebar:VisibilityChange",
         {commandID, isOpen: true}
       );
-      BrowserUITelemetry.countSidebarEvent(commandID, "show");
     });
   },
 
   /**
    * Hide the sidebar.
    */
   hide() {
     if (!this.isOpen) {