Bug 1482610 - Part 1 - Move the sidebar title and URL from the broadcasters to a JavaScript object. r=jaws,mixedpuppy
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Tue, 14 Aug 2018 15:18:03 +0100
changeset 432383 8ceb8813cc1d7beb81e044d411c0bac6babea9da
parent 432382 b3c038572979338ba2feae669ac7567f11c3264e
child 432384 be8aca082faf9aef30236657c9c486750e831a21
push id106720
push userpaolo.mozmail@amadzone.org
push dateMon, 20 Aug 2018 15:48:29 +0000
treeherdermozilla-inbound@be8aca082faf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws, mixedpuppy
bugs1482610
milestone63.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 1482610 - Part 1 - Move the sidebar title and URL from the broadcasters to a JavaScript object. r=jaws,mixedpuppy The immediate goal is only to remove the broadcasters, so we still require the labels to be set manually on the "toolbarbutton" and "menuitem" elements. Generating these elements programmatically from the new SidebarUI.sidebars object, both for built-in sidebars and extensions, can be a future improvement. The autoCheck attribute is also unnecessary since it is only intended for the menu items, and they are already properly updated after their command is invoked. Since the attribute was written with the wrong capitalization, it already had no effect. The persistence of the label of the sidebar selector is also unnecessary since it is already set on startup. Removing this does not seem to cause any additional flickering. Differential Revision: https://phabricator.services.mozilla.com/D3143
browser/base/content/browser-menubar.inc
browser/base/content/browser-sets.inc
browser/base/content/browser-sidebar.js
browser/base/content/browser.xul
browser/components/extensions/parent/ext-sidebarAction.js
--- a/browser/base/content/browser-menubar.inc
+++ b/browser/base/content/browser-menubar.inc
@@ -195,17 +195,18 @@
                   </menupopup>
                 </menu>
                 <menu id="viewSidebarMenuMenu"
                       label="&viewSidebarMenu.label;"
                       accesskey="&viewSidebarMenu.accesskey;">
                   <menupopup id="viewSidebarMenu">
                     <menuitem id="menu_bookmarksSidebar"
                               key="viewBookmarksSidebarKb"
-                              observes="viewBookmarksSidebar"/>
+                              observes="viewBookmarksSidebar"
+                              label="&bookmarksButton.label;"/>
                     <menuitem id="menu_historySidebar"
                               key="key_gotoHistory"
                               observes="viewHistorySidebar"
                               label="&historyButton.label;"/>
                     <menuitem id="menu_tabsSidebar"
                               class="sync-ui-item"
                               observes="viewTabsSidebar"
                               label="&syncedTabs.sidebar.label;"/>
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -117,24 +117,22 @@
              label="&zoomWindow.label;"
              oncommand="zoomWindow();" />
 #endif
   </commandset>
 
 #include ../../components/places/content/placesCommands.inc.xul
 
   <broadcasterset id="mainBroadcasterSet">
-    <broadcaster id="viewBookmarksSidebar" autoCheck="false" label="&bookmarksButton.label;"
+    <broadcaster id="viewBookmarksSidebar"
                  type="checkbox" group="sidebar"
-                 sidebarurl="chrome://browser/content/places/bookmarksSidebar.xul"
                  oncommand="SidebarUI.toggle('viewBookmarksSidebar');"/>
 
-    <broadcaster id="viewHistorySidebar" autoCheck="false" sidebartitle="&historyButton.label;"
+    <broadcaster id="viewHistorySidebar"
                  type="checkbox" group="sidebar"
-                 sidebarurl="chrome://browser/content/places/historySidebar.xul"
                  oncommand="SidebarUI.toggle('viewHistorySidebar');"/>
 
     <broadcaster id="isImage"/>
     <broadcaster id="canViewSource"/>
     <broadcaster id="isFrameImage"/>
 
     <!-- Sync broadcasters -->
     <!-- A broadcaster of a number of attributes suitable for "sync now" UI -
@@ -142,19 +140,18 @@
         attribute which changes from "sync now" to "syncing" etc. -->
     <broadcaster id="sync-status" onmouseover="gSync.refreshSyncButtonsTooltip();"/>
     <!-- broadcasters of the "hidden" attribute to reflect setup state for
          menus -->
     <broadcaster id="sync-setup-state" hidden="true"/>
     <broadcaster id="sync-unverified-state" hidden="true"/>
     <broadcaster id="sync-syncnow-state" hidden="true"/>
     <broadcaster id="sync-reauth-state" hidden="true"/>
-    <broadcaster id="viewTabsSidebar" autoCheck="false" sidebartitle="&syncedTabs.sidebar.label;"
+    <broadcaster id="viewTabsSidebar"
                  type="checkbox" group="sidebar"
-                 sidebarurl="chrome://browser/content/syncedtabs/sidebar.xhtml"
                  oncommand="SidebarUI.toggle('viewTabsSidebar');"/>
     <broadcaster id="workOfflineMenuitemState"/>
     <broadcaster id="devtoolsMenuBroadcaster_RecordExecution"
                  label="&devtoolsRecordExecution.label;"
                  command="Tools:RecordExecution"/>
     <broadcaster id="devtoolsMenuBroadcaster_SaveRecording"
                  label="&devtoolsSaveRecording.label;"
                  command="Tools:SaveRecording"/>
--- a/browser/base/content/browser-sidebar.js
+++ b/browser/base/content/browser-sidebar.js
@@ -1,30 +1,39 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /**
  * SidebarUI controls showing and hiding the browser sidebar.
- *
- * @note
- * Some of these methods take a commandID argument - we expect to find a
- * xul:broadcaster element with the specified ID.
- * The following attributes on that element may be used and/or modified:
- *  - id           (required) the string to match commandID. The convention
- *                 is to use this naming scheme: 'view<sidebar-name>Sidebar'.
- *  - sidebarurl   (required) specifies the URL to load in this sidebar.
- *  - sidebartitle or label (in that order) specify the title to
- *                 display on the sidebar.
- *  - checked      indicates whether the sidebar is currently displayed.
- *                 Note that this attribute is updated when
- *                 the sidebar's visibility is changed.
- *  - group        this attribute must be set to "sidebar".
  */
 var SidebarUI = {
+  get sidebars() {
+    if (this._sidebars) {
+      return this._sidebars;
+    }
+    return this._sidebars = new Map([
+      ["viewBookmarksSidebar", {
+        title: document.getElementById("sidebar-switcher-bookmarks")
+                       .getAttribute("label"),
+        url: "chrome://browser/content/places/bookmarksSidebar.xul",
+      }],
+      ["viewHistorySidebar", {
+        title: document.getElementById("sidebar-switcher-history")
+                       .getAttribute("label"),
+        url: "chrome://browser/content/places/historySidebar.xul",
+      }],
+      ["viewTabsSidebar", {
+        title: document.getElementById("sidebar-switcher-tabs")
+                       .getAttribute("label"),
+        url: "chrome://browser/content/syncedtabs/sidebar.xhtml",
+      }],
+    ]);
+  },
+
   // Avoid getting the browser element from init() to avoid triggering the
   // <browser> constructor during startup if the sidebar is hidden.
   get browser() {
     if (this._browser)
       return this._browser;
     return this._browser = document.getElementById("sidebar");
   },
   POSITION_START_PREF: "sidebar.position_start",
@@ -436,27 +445,18 @@ var SidebarUI = {
       this.setPosition();
 
       this.hideSwitcherPanel();
 
       this._box.setAttribute("checked", "true");
       this._box.setAttribute("sidebarcommand", sidebarBroadcaster.id);
       this.lastOpenedId = sidebarBroadcaster.id;
 
-      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 = title;
-      }
-
-      let url = sidebarBroadcaster.getAttribute("sidebarurl");
+      let {url, title} = this.sidebars.get(commandID);
+      this.title = title;
       this.browser.setAttribute("src", url); // kick off async load
 
       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 resolving.
           setTimeout(() => {
             resolve(sidebarBroadcaster);
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -316,16 +316,17 @@ xmlns="http://www.w3.org/1999/xhtml"
            class="cui-widget-panel"
            role="group"
            type="arrow"
            hidden="true"
            flip="slide"
            orient="vertical"
            position="bottomcenter topleft">
       <toolbarbutton id="sidebar-switcher-bookmarks"
+                     label="&bookmarksButton.label;"
                      class="subviewbutton subviewbutton-iconic"
                      key="viewBookmarksSidebarKb"
                      observes="viewBookmarksSidebar"
                      oncommand="SidebarUI.show('viewBookmarksSidebar');">
         <observes element="viewBookmarksSidebar" attribute="checked"/>
       </toolbarbutton>
       <toolbarbutton id="sidebar-switcher-history"
                      label="&historyButton.label;"
@@ -1215,17 +1216,17 @@ xmlns="http://www.w3.org/1999/xhtml"
 
   <deck id="content-deck" flex="1">
     <hbox flex="1" id="browser">
       <vbox id="browser-border-start" hidden="true" layer="true"/>
       <vbox id="sidebar-box" hidden="true" class="chromeclass-extrachrome">
         <sidebarheader id="sidebar-header" align="center">
           <toolbarbutton id="sidebar-switcher-target" flex="1" class="tabbable">
             <image id="sidebar-icon" consumeanchor="sidebar-switcher-target"/>
-            <label id="sidebar-title" persist="value" crop="end" flex="1" control="sidebar"/>
+            <label id="sidebar-title" crop="end" flex="1" control="sidebar"/>
             <image id="sidebar-switcher-arrow"/>
           </toolbarbutton>
           <image id="sidebar-throbber"/>
 # To ensure the button label's intrinsic width doesn't expand the sidebar
 # if the label is long, the button needs flex=1.
 # To ensure the button doesn't expand unnecessarily for short labels, the
 # spacer should significantly out-flex the button.
           <spacer flex="1000"/>
--- a/browser/components/extensions/parent/ext-sidebarAction.js
+++ b/browser/components/extensions/parent/ext-sidebarAction.js
@@ -117,16 +117,17 @@ this.sidebarAction = class extends Exten
         button.remove();
       }
       let broadcaster = document.getElementById(this.id);
       if (broadcaster) {
         broadcaster.remove();
       }
       let header = document.getElementById("sidebar-switcher-target");
       header.removeEventListener("SidebarShown", this.updateHeader);
+      SidebarUI.sidebars.delete(this.id);
     }
     windowTracker.removeOpenListener(this.windowOpenListener);
     windowTracker.removeCloseListener(this.windowCloseListener);
   }
 
   build() {
     this.tabContext.on("tab-select", // eslint-disable-line mozilla/balanced-listeners
                        (evt, tab) => { this.updateWindow(tab.ownerGlobal); });
@@ -140,25 +141,27 @@ this.sidebarAction = class extends Exten
         SidebarUI.show(this.id);
       }
     }
   }
 
   createMenuItem(window, details) {
     let {document, SidebarUI} = window;
 
+    SidebarUI.sidebars.set(this.id, {
+      title: details.title,
+      url: sidebarURL,
+    });
+
     // Use of the broadcaster allows browser-sidebar.js to properly manage the
     // checkmarks in the menus.
     let broadcaster = document.createXULElement("broadcaster");
     broadcaster.setAttribute("id", this.id);
-    broadcaster.setAttribute("autoCheck", "false");
     broadcaster.setAttribute("type", "checkbox");
     broadcaster.setAttribute("group", "sidebar");
-    broadcaster.setAttribute("label", details.title);
-    broadcaster.setAttribute("sidebarurl", sidebarURL);
     broadcaster.setAttribute("panel", details.panel);
     if (this.browserStyle) {
       broadcaster.setAttribute("browserStyle", "true");
     }
     broadcaster.setAttribute("extensionId", this.extension.id);
     let id = `ext-key-id-${this.id}`;
     broadcaster.setAttribute("key", id);
 
@@ -167,23 +170,25 @@ this.sidebarAction = class extends Exten
     broadcaster.setAttribute("oncommand", "SidebarUI.toggle(this.getAttribute('observes'))");
 
     let header = document.getElementById("sidebar-switcher-target");
     header.addEventListener("SidebarShown", this.updateHeader);
 
     // Insert a menuitem for View->Show Sidebars.
     let menuitem = document.createXULElement("menuitem");
     menuitem.setAttribute("id", this.menuId);
+    menuitem.setAttribute("label", details.title);
     menuitem.setAttribute("observes", this.id);
     menuitem.setAttribute("class", "menuitem-iconic webextension-menuitem");
     this.setMenuIcon(menuitem, details);
 
     // Insert a toolbarbutton for the sidebar dropdown selector.
     let toolbarbutton = document.createXULElement("toolbarbutton");
     toolbarbutton.setAttribute("id", this.buttonId);
+    toolbarbutton.setAttribute("label", details.title);
     toolbarbutton.setAttribute("observes", this.id);
     toolbarbutton.setAttribute("class", "subviewbutton subviewbutton-iconic webextension-menuitem");
     this.setMenuIcon(toolbarbutton, details);
 
     document.getElementById("mainBroadcasterSet").appendChild(broadcaster);
     document.getElementById("viewSidebarMenu").appendChild(menuitem);
     let separator = document.getElementById("sidebar-extensions-separator");
     separator.parentNode.insertBefore(toolbarbutton, separator);
@@ -214,29 +219,27 @@ this.sidebarAction = class extends Exten
   updateButton(window, tabData) {
     let {document, SidebarUI} = window;
     let title = tabData.title || this.extension.name;
     let menu = document.getElementById(this.menuId);
     if (!menu) {
       menu = this.createMenuItem(window, tabData);
     }
 
-    // Update the broadcaster first, it will update both menus.
     let broadcaster = document.getElementById(this.id);
-    broadcaster.setAttribute("tooltiptext", title);
-    broadcaster.setAttribute("label", title);
-
     let urlChanged = tabData.panel !== broadcaster.getAttribute("panel");
     if (urlChanged) {
       broadcaster.setAttribute("panel", tabData.panel);
     }
 
+    menu.setAttribute("label", title);
     this.setMenuIcon(menu, tabData);
 
     let button = document.getElementById(this.buttonId);
+    button.setAttribute("label", title);
     this.setMenuIcon(button, tabData);
 
     // Update the sidebar if this extension is the current sidebar.
     if (SidebarUI.currentID === this.id) {
       SidebarUI.title = title;
       let header = document.getElementById("sidebar-switcher-target");
       this.setMenuIcon(header, tabData);
       if (SidebarUI.isOpen && urlChanged) {