Bug 1337457 - Handle missing commands[*].suggested_key r=kmag
authorRob Wu <rob@robwu.nl>
Tue, 07 Feb 2017 20:06:47 +0100
changeset 374673 b9e7efc94543531e2a7c754c60f2444a108772f4
parent 374672 1878102f1ceaa128465d17d858da3d66d21d1fc2
child 374674 3b9e06e3d9b812b219fbdfa91e185a6fb998a057
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1337457
milestone54.0a1
Bug 1337457 - Handle missing commands[*].suggested_key r=kmag 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();