Bug 1258987 - Stop generating useless xul:command and xul:broadcaster. r=jryans
authorAlexandre Poirot <poirot.alex@gmail.com>
Tue, 05 Apr 2016 07:16:48 -0700
changeset 291788 4b00370192a495bf15b4110a2dd6c8fd8084e0c9
parent 291787 1300b2a51f1d65579b4875f33f0075538e1391b6
child 291789 0912147b690ebe731de6c976e30cfd821c99ed6c
push id74679
push userkwierso@gmail.com
push dateTue, 05 Apr 2016 23:39:26 +0000
treeherdermozilla-inbound@b70ae970d45d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjryans
bugs1258987
milestone48.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 1258987 - Stop generating useless xul:command and xul:broadcaster. r=jryans
devtools/client/framework/browser-menus.js
devtools/client/framework/test/browser_toolbox_dynamic_registration.js
--- a/devtools/client/framework/browser-menus.js
+++ b/devtools/client/framework/browser-menus.js
@@ -11,156 +11,101 @@
  * - devtools/client/menus for top level entires
  * - devtools/client/definitions for tool-specifics entries
  */
 
 const Services = require("Services");
 const MenuStrings = Services.strings.createBundle("chrome://devtools/locale/menus.properties");
 
 loader.lazyRequireGetter(this, "gDevTools", "devtools/client/framework/devtools", true);
+loader.lazyRequireGetter(this, "gDevToolsBrowser", "devtools/client/framework/devtools-browser", true);
 
 // Keep list of inserted DOM Elements in order to remove them on unload
 // Maps browser xul document => list of DOM Elements
 const FragmentsCache = new Map();
 
 function l10n(key) {
   return MenuStrings.GetStringFromName(key);
 }
 
 /**
  * Create a xul:key element
  *
  * @param {XULDocument} doc
  *        The document to which keys are to be added.
- * @param {String} l10nKey
- *        Prefix of the properties entry to look for key shortcut in
- *        localization file. We will look for {property}.key and
- *        {property}.keytext for non-character shortcuts like F12.
- * @param {String} command
- *        Id of the xul:command to map to.
- * @param {Object} key definition dictionnary
- *        Definition with following attributes:
- *        - {String} id
- *          xul:key's id, automatically prefixed with "key_",
- *        - {String} modifiers
- *          Space separater list of modifier names,
- *        - {Boolean} keytext
- *          If true, consider the shortcut as a characther one,
- *          otherwise a non-character one like F12.
+ * @param {String} id
+ *        key's id, automatically prefixed with "key_".
+ * @param {String} shortcut
+ *        The key shortcut value.
+ * @param {String} keytext
+ *        If `shortcut` refers to a function key, refers to the localized
+ *        string to describe a non-character shortcut.
+ * @param {String} modifiers
+ *        Space separated list of modifier names.
+ * @param {Function} oncommand
+ *        The function to call when the shortcut is pressed.
  *
  * @return XULKeyElement
  */
-function createKey(doc, l10nKey, command, key) {
+function createKey({ doc, id, shortcut, keytext, modifiers, oncommand }) {
   let k = doc.createElement("key");
-  k.id = "key_" + key.id;
-  let shortcut = l10n(l10nKey + ".key");
+  k.id = "key_" + id;
+
   if (shortcut.startsWith("VK_")) {
     k.setAttribute("keycode", shortcut);
-    k.setAttribute("keytext", l10n(l10nKey + ".keytext"));
+    if (keytext) {
+      k.setAttribute("keytext", keytext);
+    }
   } else {
     k.setAttribute("key", shortcut);
   }
-  if (command) {
-    k.setAttribute("command", command);
+
+  if (modifiers) {
+    k.setAttribute("modifiers", modifiers);
   }
-  if (key.modifiers) {
-    k.setAttribute("modifiers", key.modifiers);
-  }
+
+  // Bug 371900: command event is fired only if "oncommand" attribute is set.
+  k.setAttribute("oncommand", ";");
+  k.addEventListener("command", oncommand);
+
   return k;
 }
 
 /**
  * Create a xul:menuitem element
  *
  * @param {XULDocument} doc
  *        The document to which keys are to be added.
  * @param {String} id
  *        Element id.
  * @param {String} label
  *        Menu label.
- * @param {String} broadcasterId (optional)
- *        Id of the xul:broadcaster to map to.
  * @param {String} accesskey (optional)
  *        Access key of the menuitem, used as shortcut while opening the menu.
- * @param {Boolean} isCheckbox
+ * @param {Boolean} isCheckbox (optional)
  *        If true, the menuitem will act as a checkbox and have an optional
  *        tick on its left.
  *
  * @return XULMenuItemElement
  */
-function createMenuItem({ doc, id, label, broadcasterId, accesskey, isCheckbox }) {
+function createMenuItem({ doc, id, label, accesskey, isCheckbox }) {
   let menuitem = doc.createElement("menuitem");
   menuitem.id = id;
-  if (label) {
-    menuitem.setAttribute("label", label);
-  }
-  if (broadcasterId) {
-    menuitem.setAttribute("observes", broadcasterId);
-  }
+  menuitem.setAttribute("label", label);
   if (accesskey) {
     menuitem.setAttribute("accesskey", accesskey);
   }
   if (isCheckbox) {
     menuitem.setAttribute("type", "checkbox");
     menuitem.setAttribute("autocheck", "false");
   }
   return menuitem;
 }
 
 /**
- * Create a xul:broadcaster element
- *
- * @param {XULDocument} doc
- *        The document to which keys are to be added.
- * @param {String} id
- *        Element id.
- * @param {String} label
- *        Broadcaster label.
- * @param {Boolean} isCheckbox
- *        If true, the broadcaster is a checkbox one.
- *
- * @return XULMenuItemElement
- */
-function createBroadcaster({ doc, id, label, isCheckbox }) {
-  let broadcaster = doc.createElement("broadcaster");
-  broadcaster.id = id;
-  broadcaster.setAttribute("label", label);
-  if (isCheckbox) {
-    broadcaster.setAttribute("type", "checkbox");
-    broadcaster.setAttribute("autocheck", "false");
-  }
-  return broadcaster;
-}
-
-/**
- * Create a xul:command element
- *
- * @param {XULDocument} doc
- *        The document to which keys are to be added.
- * @param {String} id
- *        Element id.
- * @param {String} oncommand
- *        JS String to run when the command is fired.
- * @param {Boolean} disabled
- *        If true, the command is disabled and hidden.
- *
- * @return XULCommandElement
- */
-function createCommand({ doc, id, oncommand, disabled }) {
-  let command = doc.createElement("command");
-  command.id = id;
-  command.setAttribute("oncommand", oncommand);
-  if (disabled) {
-    command.setAttribute("disabled", "true");
-    command.setAttribute("hidden", "true");
-  }
-  return command;
-}
-
-/**
  * Add a <key> to <keyset id="devtoolsKeyset">.
  * Appending a <key> element is not always enough. The <keyset> needs
  * to be detached and reattached to make sure the <key> is taken into
  * account (see bug 832984).
  *
  * @param {XULDocument} doc
  *        The document to which keys are to be added
  * @param {XULElement} or {DocumentFragment} keys
@@ -183,175 +128,139 @@ function attachKeybindingsToBrowser(doc,
  *
  * @param {Object} toolDefinition
  *        Tool definition of the tool to add a menu entry.
  * @param {XULDocument} doc
  *        The document to which the tool menu item is to be added.
  */
 function createToolMenuElements(toolDefinition, doc) {
   let id = toolDefinition.id;
+  let menuId = "menuitem_" + id;
 
   // Prevent multiple entries for the same tool.
-  if (doc.getElementById("Tools:" + id)) {
+  if (doc.getElementById(menuId)) {
     return;
   }
 
-  let cmd = createCommand({
-    doc,
-    id: "Tools:" + id,
-    oncommand: 'gDevToolsBrowser.selectToolCommand(gBrowser, "' + id + '");',
-  });
+  let oncommand = function (id, event) {
+    let window = event.target.ownerDocument.defaultView;
+    gDevToolsBrowser.selectToolCommand(window.gBrowser, id);
+  }.bind(null, id);
 
   let key = null;
   if (toolDefinition.key) {
-    key = doc.createElement("key");
-    key.id = "key_" + id;
-
-    if (toolDefinition.key.startsWith("VK_")) {
-      key.setAttribute("keycode", toolDefinition.key);
-    } else {
-      key.setAttribute("key", toolDefinition.key);
-    }
-
-    key.setAttribute("command", cmd.id);
-    key.setAttribute("modifiers", toolDefinition.modifiers);
-  }
-
-  let bc = createBroadcaster({
-    doc,
-    id: "devtoolsMenuBroadcaster_" + id,
-    label: toolDefinition.menuLabel || toolDefinition.label
-  });
-  bc.setAttribute("command", cmd.id);
-
-  if (key) {
-    bc.setAttribute("key", "key_" + id);
+    key = createKey({
+      doc,
+      id,
+      shortcut: toolDefinition.key,
+      modifiers: toolDefinition.modifiers,
+      oncommand: oncommand
+    });
   }
 
   let menuitem = createMenuItem({
     doc,
     id: "menuitem_" + id,
-    broadcasterId: "devtoolsMenuBroadcaster_" + id,
+    label: toolDefinition.menuLabel || toolDefinition.label,
     accesskey: toolDefinition.accesskey
   });
+  if (key) {
+    // Refer to the key in order to display the key shortcut at menu ends
+    menuitem.setAttribute("key", key.id);
+  }
+  menuitem.addEventListener("command", oncommand);
 
   return {
-    cmd: cmd,
-    key: key,
-    bc: bc,
-    menuitem: menuitem
+    key,
+    menuitem
   };
 }
 
 /**
- * Create xul menuitem, command, broadcaster and key elements for a given tool.
+ * Create xul menuitem, key elements for a given tool.
  * And then insert them into browser DOM.
  *
  * @param {XULDocument} doc
  *        The document to which the tool is to be registered.
  * @param {Object} toolDefinition
  *        Tool definition of the tool to register.
  * @param {Object} prevDef
  *        The tool definition after which the tool menu item is to be added.
  */
 function insertToolMenuElements(doc, toolDefinition, prevDef) {
-  let elements = createToolMenuElements(toolDefinition, doc);
-
-  doc.getElementById("mainCommandSet").appendChild(elements.cmd);
+  let { key, menuitem } = createToolMenuElements(toolDefinition, doc);
 
-  if (elements.key) {
-    attachKeybindingsToBrowser(doc, elements.key);
+  if (key) {
+    attachKeybindingsToBrowser(doc, key);
   }
 
-  doc.getElementById("mainBroadcasterSet").appendChild(elements.bc);
-
   let ref;
   if (prevDef) {
     let menuitem = doc.getElementById("menuitem_" + prevDef.id);
     ref = menuitem && menuitem.nextSibling ? menuitem.nextSibling : null;
   } else {
     ref = doc.getElementById("menu_devtools_separator");
   }
 
   if (ref) {
-    ref.parentNode.insertBefore(elements.menuitem, ref);
+    ref.parentNode.insertBefore(menuitem, ref);
   }
 }
 exports.insertToolMenuElements = insertToolMenuElements;
 
 /**
  * Remove a tool's menuitem from a window
  *
  * @param {string} toolId
  *        Id of the tool to add a menu entry for
  * @param {XULDocument} doc
  *        The document to which the tool menu item is to be removed from
  */
 function removeToolFromMenu(toolId, doc) {
-  let command = doc.getElementById("Tools:" + toolId);
-  if (command) {
-    command.parentNode.removeChild(command);
-  }
-
   let key = doc.getElementById("key_" + toolId);
   if (key) {
-    key.parentNode.removeChild(key);
-  }
-
-  let bc = doc.getElementById("devtoolsMenuBroadcaster_" + toolId);
-  if (bc) {
-    bc.parentNode.removeChild(bc);
+    key.remove();
   }
 
   let menuitem = doc.getElementById("menuitem_" + toolId);
   if (menuitem) {
-    menuitem.parentNode.removeChild(menuitem);
+    menuitem.remove();
   }
 }
 exports.removeToolFromMenu = removeToolFromMenu;
 
 /**
  * Add all tools to the developer tools menu of a window.
  *
  * @param {XULDocument} doc
  *        The document to which the tool items are to be added.
  */
 function addAllToolsToMenu(doc) {
-  let fragCommands = doc.createDocumentFragment();
   let fragKeys = doc.createDocumentFragment();
-  let fragBroadcasters = doc.createDocumentFragment();
   let fragMenuItems = doc.createDocumentFragment();
 
   for (let toolDefinition of gDevTools.getToolDefinitionArray()) {
     if (!toolDefinition.inMenu) {
       continue;
     }
 
     let elements = createToolMenuElements(toolDefinition, doc);
 
     if (!elements) {
       continue;
     }
 
-    fragCommands.appendChild(elements.cmd);
     if (elements.key) {
       fragKeys.appendChild(elements.key);
     }
-    fragBroadcasters.appendChild(elements.bc);
     fragMenuItems.appendChild(elements.menuitem);
   }
 
-  let mcs = doc.getElementById("mainCommandSet");
-  mcs.appendChild(fragCommands);
-
   attachKeybindingsToBrowser(doc, fragKeys);
 
-  let mbs = doc.getElementById("mainBroadcasterSet");
-  mbs.appendChild(fragBroadcasters);
-
   let mps = doc.getElementById("menu_devtools_separator");
   if (mps) {
     mps.parentNode.insertBefore(fragMenuItems, mps);
   }
 }
 
 /**
  * Add global menus and shortcuts that are not panel specific.
@@ -380,31 +289,41 @@ function addTopLevelItems(doc) {
         accesskey: l10n(l10nKey + ".accesskey"),
         isCheckbox: item.checkbox
       });
       menuitem.addEventListener("command", item.oncommand);
       menuItems.appendChild(menuitem);
 
       if (item.key && l10nKey) {
         // Create a <key>
-        let key = createKey(doc, l10nKey, null, item.key);
-        // Bug 371900: command event is fired only if "oncommand" attribute is set.
-        key.setAttribute("oncommand", ";");
-        key.addEventListener("command", item.oncommand);
+        let shortcut = l10n(l10nKey + ".key");
+        let key = createKey({
+          doc,
+          id: item.key.id,
+          shortcut: shortcut,
+          keytext: shortcut.startsWith("VK_") ? l10n(l10nKey + ".keytext") : null,
+          modifiers: item.key.modifiers,
+          oncommand: item.oncommand
+        });
         // Refer to the key in order to display the key shortcut at menu ends
         menuitem.setAttribute("key", key.id);
         keys.appendChild(key);
       }
       if (item.additionalKeys) {
         // Create additional <key>
         for (let key of item.additionalKeys) {
-          let node = createKey(doc, key.l10nKey, null, key);
-          // Bug 371900: command event is fired only if "oncommand" attribute is set.
-          node.setAttribute("oncommand", ";");
-          node.addEventListener("command", item.oncommand);
+          let shortcut = l10n(key.l10nKey + ".key");
+          let node = createKey({
+            doc,
+            id: key.id,
+            shortcut: shortcut,
+            keytext: shortcut.startsWith("VK_") ? l10n(key.l10nKey + ".keytext") : null,
+            modifiers: key.modifiers,
+            oncommand: item.oncommand
+          });
           keys.appendChild(node);
         }
       }
     }
   }
 
   // Cache all nodes before insertion to be able to remove them on unload
   let nodes = [];
--- a/devtools/client/framework/test/browser_toolbox_dynamic_registration.js
+++ b/devtools/client/framework/test/browser_toolbox_dynamic_registration.js
@@ -23,17 +23,18 @@ function testRegister(aToolbox)
   toolbox = aToolbox
   gDevTools.once("tool-registered", toolRegistered);
 
   gDevTools.registerTool({
     id: "test-tool",
     label: "Test Tool",
     inMenu: true,
     isTargetSupported: () => true,
-    build: function() {}
+    build: function() {},
+    key: "t"
   });
 }
 
 function toolRegistered(event, toolId)
 {
   is(toolId, "test-tool", "tool-registered event handler sent tool id");
 
   ok(gDevTools.getToolDefinitionMap().has(toolId), "tool added to map");
@@ -42,18 +43,18 @@ function toolRegistered(event, toolId)
   let doc = toolbox.frame.contentDocument;
   let tab = doc.getElementById("toolbox-tab-" + toolId);
   ok(tab, "new tool's tab exists in toolbox UI");
 
   let panel = doc.getElementById("toolbox-panel-" + toolId);
   ok(panel, "new tool's panel exists in toolbox UI");
 
   for (let win of getAllBrowserWindows()) {
-    let command = win.document.getElementById("Tools:" + toolId);
-    ok(command, "command for new tool added to every browser window");
+    let key = win.document.getElementById("key_" + toolId);
+    ok(key, "key for new tool added to every browser window");
     let menuitem = win.document.getElementById("menuitem_" + toolId);
     ok(menuitem, "menu item of new tool added to every browser window");
   }
 
   // then unregister it
   testUnregister();
 }
 
@@ -84,18 +85,18 @@ function toolUnregistered(event, toolDef
   let doc = toolbox.frame.contentDocument;
   let tab = doc.getElementById("toolbox-tab-" + toolId);
   ok(!tab, "tool's tab was removed from the toolbox UI");
 
   let panel = doc.getElementById("toolbox-panel-" + toolId);
   ok(!panel, "tool's panel was removed from toolbox UI");
 
   for (let win of getAllBrowserWindows()) {
-    let command = win.document.getElementById("Tools:" + toolId);
-    ok(!command, "command removed from every browser window");
+    let key = win.document.getElementById("key_" + toolId);
+    ok(!key , "key removed from every browser window");
     let menuitem = win.document.getElementById("menuitem_" + toolId);
     ok(!menuitem, "menu item removed from every browser window");
   }
 
   cleanup();
 }
 
 function cleanup()