Bug 1337457 - Handle missing commands[*].suggested_key r=kmag
authorRob Wu <rob@robwu.nl>
Tue, 07 Feb 2017 20:06:47 +0100
changeset 345583 b9e7efc94543531e2a7c754c60f2444a108772f4
parent 345582 1878102f1ceaa128465d17d858da3d66d21d1fc2
child 345584 3b9e06e3d9b812b219fbdfa91e185a6fb998a057
push id38229
push userrob@robwu.nl
push dateThu, 02 Mar 2017 17:57:57 +0000
treeherderautoland@b9e7efc94543 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1337457
milestone54.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 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();