Bug 1337457 - Handle missing commands[*].suggested_key draft
authorRob Wu <rob@robwu.nl>
Tue, 07 Feb 2017 20:06:47 +0100
changeset 492010 7324a3cf848a04b71134afbd0289f475e0272b22
parent 492009 d29f84406483c721a13cf9a52936ecced0c5c98a
child 547607 29d6dfc176fca370f9f05d05708ca78598e0d9c5
push id47482
push userbmo:rob@robwu.nl
push dateThu, 02 Mar 2017 16:08:46 +0000
bugs1337457
milestone54.0a1
Bug 1337457 - Handle missing commands[*].suggested_key Defaulting to "" instead of null because Chrome too defaults to "". MozReview-Commit-ID: 7pJYCzVR4f6
browser/components/extensions/ext-commands.js
browser/components/extensions/test/browser/browser_ext_commands_getAll.js
--- a/browser/components/extensions/ext-commands.js
+++ b/browser/components/extensions/ext-commands.js
@@ -69,40 +69,41 @@ CommandList.prototype = {
    * @param {Object} manifest The manifest JSON object.
    * @returns {Map<string, object>}
    */
   loadCommandsFromManifest(manifest) {
     let commands = new Map();
     // For Windows, chrome.runtime expects 'win' while chrome.commands
     // expects 'windows'.  We can special case this for now.
     let os = PlatformInfo.os == "win" ? "windows" : PlatformInfo.os;
-    for (let name of Object.keys(manifest.commands)) {
-      let command = manifest.commands[name];
-      let shortcut = command.suggested_key[os] || command.suggested_key.default;
-      if (shortcut) {
-        commands.set(name, {
-          description: command.description,
-          shortcut: shortcut.replace(/\s+/g, ""),
-        });
-      }
+    for (let [name, command] of Object.entries(manifest.commands)) {
+      let suggested_key = command.suggested_key || {};
+      let shortcut = suggested_key[os] || suggested_key.default;
+      shortcut = shortcut ? shortcut.replace(/\s+/g, "") : null,
+      commands.set(name, {
+        description: command.description,
+        shortcut,
+      });
     }
     return commands;
   },
 
   /**
    * Registers the commands to a document.
    * @param {ChromeWindow} window The XUL window to insert the Keyset.
    */
   registerKeysToDocument(window) {
     let doc = window.document;
     let keyset = doc.createElementNS(XUL_NS, "keyset");
     keyset.id = `ext-keyset-id-${this.id}`;
     this.commands.forEach((command, name) => {
-      let keyElement = this.buildKey(doc, name, command.shortcut);
-      keyset.appendChild(keyElement);
+      if (command.shortcut) {
+        let keyElement = this.buildKey(doc, name, command.shortcut);
+        keyset.appendChild(keyElement);
+      }
     });
     doc.documentElement.appendChild(keyset);
     this.keysetsMap.set(window, keyset);
   },
 
   /**
    * Builds a XUL Key element and attaches an onCommand listener which
    * emits a command event with the provided name when fired.
--- a/browser/components/extensions/test/browser/browser_ext_commands_getAll.js
+++ b/browser/components/extensions/test/browser/browser_ext_commands_getAll.js
@@ -21,24 +21,29 @@ add_task(function* () {
         "with-platform-info": {
           "suggested_key": {
             "mac": "Ctrl+Shift+M",
             "linux": "Ctrl+Shift+L",
             "windows": "Ctrl+Shift+W",
             "android": "Ctrl+Shift+A",
           },
         },
+        "without-suggested-key": {
+          "description": "has no suggested_key",
+        },
+        "without-suggested-key-nor-description": {
+        },
       },
     },
 
     background: function() {
       browser.test.onMessage.addListener((message, additionalScope) => {
         browser.commands.getAll((commands) => {
           let errorMessage = "getAll should return an array of commands";
-          browser.test.assertEq(commands.length, 3, errorMessage);
+          browser.test.assertEq(commands.length, 5, errorMessage);
 
           let command = commands.find(c => c.name == "with-desciption");
 
           errorMessage = "The description should match what is provided in the manifest";
           browser.test.assertEq("should have a description", command.description, errorMessage);
 
           errorMessage = "The shortcut should match the default shortcut provided in the manifest";
           browser.test.assertEq("Ctrl+Shift+Y", command.shortcut, errorMessage);
@@ -59,16 +64,28 @@ add_task(function* () {
           };
 
           command = commands.find(c => c.name == "with-platform-info");
           let platformKey = platformKeys[additionalScope.platform];
           let shortcut = `Ctrl+Shift+${platformKey}`;
           errorMessage = `The shortcut should match the one provided in the manifest for OS='${additionalScope.platform}'`;
           browser.test.assertEq(shortcut, command.shortcut, errorMessage);
 
+          command = commands.find(c => c.name == "without-suggested-key");
+
+          browser.test.assertEq("has no suggested_key", command.description, "The description should match what is provided in the manifest");
+
+          browser.test.assertEq(null, command.shortcut, "The shortcut should be empty if not provided");
+
+          command = commands.find(c => c.name == "without-suggested-key-nor-description");
+
+          browser.test.assertEq(null, command.description, "The description should be empty when it is not provided");
+
+          browser.test.assertEq(null, command.shortcut, "The shortcut should be empty if not provided");
+
           browser.test.notifyPass("commands");
         });
       });
       browser.test.sendMessage("ready");
     },
   });
 
   yield extension.startup();