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 347688 4b00370192a495bf15b4110a2dd6c8fd8084e0c9
parent 347687 1300b2a51f1d65579b4875f33f0075538e1391b6
child 347689 0912147b690ebe731de6c976e30cfd821c99ed6c
child 347796 d88fc62c8f90d3d6fc1f054fada1f7669b0f9d9a
child 348082 58b00412ca1965bc3bcea32dd885cd58c7b245b0
child 348645 a581b6dd40af1d4e71345a89cfcd63b47c2be22f
push id14647
push userbmo:ahunt@mozilla.com
push dateTue, 05 Apr 2016 18:15:38 +0000
reviewersjryans
bugs1258987
milestone48.0a1
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()