Bug 1303384 - Part 2: Move some extension shortcut utils to ShortcutUtils r=aswan
authorMark Striemer <mstriemer@mozilla.com>
Fri, 11 Jan 2019 22:32:39 +0000
changeset 510836 d1eacc92d6d6dc2996018d1e7fb583e956bbcd41
parent 510835 f4697feb30df5c2f0e5711a5a2f34353996b42a3
child 510837 456b9b7963eb9c0815f8fa250de5424317c00001
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaswan
bugs1303384
milestone66.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 1303384 - Part 2: Move some extension shortcut utils to ShortcutUtils r=aswan Differential Revision: https://phabricator.services.mozilla.com/D4506
browser/components/extensions/test/browser/browser_ext_commands_update.js
toolkit/components/extensions/ExtensionShortcuts.jsm
toolkit/components/extensions/ExtensionUtils.jsm
toolkit/components/extensions/Schemas.jsm
toolkit/modules/ShortcutUtils.jsm
--- a/browser/components/extensions/test/browser/browser_ext_commands_update.js
+++ b/browser/components/extensions/test/browser/browser_ext_commands_update.js
@@ -163,37 +163,37 @@ add_task(async function test_update_defi
     is(numpadKey.getAttribute("modifiers"), modifiers, "The modifiers are correct");
 
     let originalNumericKey = keyset.children[1];
     is(originalNumericKey.getAttribute("keycode"), `VK_${key}`, "The original key is correct.");
     is(originalNumericKey.getAttribute("modifiers"), modifiers, "The modifiers are correct");
   }
 
   // Check that the <key> is set for the original shortcut.
-  checkKey(extension.id, "I", "accel shift");
+  checkKey(extension.id, "I", "accel,shift");
 
   await extension.awaitMessage("ready");
   extension.sendMessage("run");
   await extension.awaitFinish("commands");
 
   // Check that the <keycode> has been updated.
-  checkNumericKey(extension.id, "9", "alt shift");
+  checkNumericKey(extension.id, "9", "alt,shift");
 
   // Check that the updated command is stored in ExtensionSettingsStore.
   let storedCommands = ExtensionSettingsStore.getAllForExtension(
     extension.id, "commands");
   is(storedCommands.length, 1, "There is only one stored command");
   let command = ExtensionSettingsStore.getSetting("commands", "foo", extension.id).value;
   is(command.description, "The new command", "The description is stored");
   is(command.shortcut, "Alt+Shift+9", "The shortcut is stored");
 
   // Check that the key is updated immediately.
   extension.sendMessage("update", {name: "foo", shortcut: "Ctrl+Shift+M"});
   await extension.awaitMessage("updateDone");
-  checkKey(extension.id, "M", "accel shift");
+  checkKey(extension.id, "M", "accel,shift");
 
   // Ensure all successive updates are stored.
   // Force the command to only have a description saved.
   await ExtensionSettingsStore.addSetting(
     extension.id, "commands", "foo", {description: "description only"});
   // This command now only has a description set in storage, also update the shortcut.
   extension.sendMessage("update", {name: "foo", shortcut: "Alt+Shift+9"});
   await extension.awaitMessage("updateDone");
@@ -201,17 +201,17 @@ add_task(async function test_update_defi
     "commands", "foo", extension.id);
   is(storedCommand.value.shortcut, "Alt+Shift+9", "The shortcut is saved correctly");
   is(storedCommand.value.description, "description only", "The description is saved correctly");
 
   // Calling browser.commands.reset("foo") should reset to manifest version.
   extension.sendMessage("reset", "foo");
   await extension.awaitMessage("resetDone");
 
-  checkKey(extension.id, "I", "accel shift");
+  checkKey(extension.id, "I", "accel,shift");
 
   // Check that enable/disable removes the keyset and reloads the saved command.
   let addon = await AddonManager.getAddonByID(extension.id);
   await disableAddon(addon);
   let keyset = extensionKeyset(extension.id);
   is(keyset, null, "The extension keyset is removed when disabled");
   // Add some commands to storage, only "foo" should get loaded.
   await ExtensionSettingsStore.addSetting(
@@ -219,17 +219,17 @@ add_task(async function test_update_defi
   await ExtensionSettingsStore.addSetting(
     extension.id, "commands", "unknown", {shortcut: "Ctrl+Shift+P"});
   storedCommands = ExtensionSettingsStore.getAllForExtension(extension.id, "commands");
   is(storedCommands.length, 2, "There are now 2 commands stored");
   await enableAddon(addon);
   // Wait for the keyset to appear (it's async on enable).
   await TestUtils.waitForCondition(() => extensionKeyset(extension.id));
   // The keyset is back with the value from ExtensionSettingsStore.
-  checkNumericKey(extension.id, "9", "alt shift");
+  checkNumericKey(extension.id, "9", "alt,shift");
 
   // Check that an update to a shortcut in the manifest is mapped correctly.
   updatedExtension = ExtensionTestUtils.loadExtension({
     useAddonManager: "permanent",
     manifest: {
       version: "1.0",
       applications: {gecko: {id: "commands@mochi.test"}},
       commands: {
@@ -241,17 +241,17 @@ add_task(async function test_update_defi
         },
       },
     },
   });
   await updatedExtension.startup();
 
   await TestUtils.waitForCondition(() => extensionKeyset(extension.id));
   // Shortcut is unchanged since it was previously updated.
-  checkNumericKey(extension.id, "9", "alt shift");
+  checkNumericKey(extension.id, "9", "alt,shift");
 });
 
 add_task(async function updateSidebarCommand() {
   let extension = ExtensionTestUtils.loadExtension({
     useAddonManager: "temporary",
     manifest: {
       commands: {
         _execute_sidebar_action: {
--- a/toolkit/components/extensions/ExtensionShortcuts.jsm
+++ b/toolkit/components/extensions/ExtensionShortcuts.jsm
@@ -4,16 +4,17 @@
 "use strict";
 
 /* exported ExtensionShortcuts */
 const EXPORTED_SYMBOLS = ["ExtensionShortcuts"];
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.import("resource://gre/modules/ExtensionCommon.jsm");
 ChromeUtils.import("resource://gre/modules/ExtensionUtils.jsm");
+ChromeUtils.import("resource://gre/modules/ShortcutUtils.jsm");
 
 ChromeUtils.defineModuleGetter(this, "ExtensionParent",
                                "resource://gre/modules/ExtensionParent.jsm");
 ChromeUtils.defineModuleGetter(this, "ExtensionSettingsStore",
                                "resource://gre/modules/ExtensionSettingsStore.jsm");
 
 XPCOMUtils.defineLazyGetter(this, "windowTracker", () => {
   return ExtensionParent.apiManager.global.windowTracker;
@@ -23,20 +24,17 @@ XPCOMUtils.defineLazyGetter(this, "brows
 });
 XPCOMUtils.defineLazyGetter(this, "pageActionFor", () => {
   return ExtensionParent.apiManager.global.pageActionFor;
 });
 XPCOMUtils.defineLazyGetter(this, "sidebarActionFor", () => {
   return ExtensionParent.apiManager.global.sidebarActionFor;
 });
 
-const {
-  chromeModifierKeyMap,
-  ExtensionError,
-} = ExtensionUtils;
+const {ExtensionError} = ExtensionUtils;
 const {makeWidgetId} = ExtensionCommon;
 
 const EXECUTE_PAGE_ACTION = "_execute_page_action";
 const EXECUTE_BROWSER_ACTION = "_execute_browser_action";
 const EXECUTE_SIDEBAR_ACTION = "_execute_sidebar_action";
 
 function normalizeShortcut(shortcut) {
   return shortcut ? shortcut.replace(/\s+/g, "") : null;
@@ -333,67 +331,26 @@ class ExtensionShortcuts {
     let keyElement = doc.createXULElement("key");
 
     let parts = shortcut.split("+");
 
     // The key is always the last element.
     let chromeKey = parts.pop();
 
     // The modifiers are the remaining elements.
-    keyElement.setAttribute("modifiers", this.getModifiersAttribute(parts));
+    keyElement.setAttribute("modifiers", ShortcutUtils.getModifiersAttribute(parts));
 
     // A keyElement with key "NumpadX" is created above and isn't from the
     // manifest. The id will be set on the keyElement with key "X" only.
     if (name == EXECUTE_SIDEBAR_ACTION && !chromeKey.startsWith("Numpad")) {
       let id = `ext-key-id-${this.id}-sidebar-action`;
       keyElement.setAttribute("id", id);
     }
 
-    if (/^[A-Z]$/.test(chromeKey)) {
-      // We use the key attribute for all single digits and characters.
-      keyElement.setAttribute("key", chromeKey);
-    } else {
-      keyElement.setAttribute("keycode", this.getKeycodeAttribute(chromeKey));
+    let [attribute, value] = ShortcutUtils.getKeyAttribute(chromeKey);
+    keyElement.setAttribute(attribute, value);
+    if (attribute == "keycode") {
       keyElement.setAttribute("event", "keydown");
     }
 
     return keyElement;
   }
-
-  /**
-   * Determines the corresponding XUL keycode from the given chrome key.
-   *
-   * For example:
-   *
-   *    input     |  output
-   *    ---------------------------------------
-   *    "PageUP"  |  "VK_PAGE_UP"
-   *    "Delete"  |  "VK_DELETE"
-   *
-   * @param {string} chromeKey The chrome key (e.g. "PageUp", "Space", ...)
-   * @returns {string} The constructed value for the Key's 'keycode' attribute.
-   */
-  getKeycodeAttribute(chromeKey) {
-    if (/^[0-9]/.test(chromeKey)) {
-      return `VK_${chromeKey}`;
-    }
-    return `VK${chromeKey.replace(/([A-Z])/g, "_$&").toUpperCase()}`;
-  }
-
-  /**
-   * Determines the corresponding XUL modifiers from the chrome modifiers.
-   *
-   * For example:
-   *
-   *    input             |   output
-   *    ---------------------------------------
-   *    ["Ctrl", "Shift"] |   "accel shift"
-   *    ["MacCtrl"]       |   "control"
-   *
-   * @param {Array} chromeModifiers The array of chrome modifiers.
-   * @returns {string} The constructed value for the Key's 'modifiers' attribute.
-   */
-  getModifiersAttribute(chromeModifiers) {
-    return Array.from(chromeModifiers, modifier => {
-      return chromeModifierKeyMap[modifier];
-    }).join(" ");
-  }
 }
--- a/toolkit/components/extensions/ExtensionUtils.jsm
+++ b/toolkit/components/extensions/ExtensionUtils.jsm
@@ -254,27 +254,17 @@ function getMessageManager(target) {
   }
   return target;
 }
 
 function flushJarCache(jarPath) {
   Services.obs.notifyObservers(null, "flush-cache-entry", jarPath);
 }
 
-const chromeModifierKeyMap = {
-  "Alt": "alt",
-  "Command": "accel",
-  "Ctrl": "accel",
-  "MacCtrl": "control",
-  "Shift": "shift",
-};
-
-
 var ExtensionUtils = {
-  chromeModifierKeyMap,
   flushJarCache,
   getInnerWindowID,
   getMessageManager,
   getUniqueId,
   filterStack,
   getWinUtils,
   promiseDocumentIdle,
   promiseDocumentLoaded,
--- a/toolkit/components/extensions/Schemas.jsm
+++ b/toolkit/components/extensions/Schemas.jsm
@@ -10,25 +10,26 @@ const global = this;
 ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyGlobalGetters(this, ["URL"]);
 
 ChromeUtils.import("resource://gre/modules/ExtensionUtils.jsm");
 var {
-  chromeModifierKeyMap,
   DefaultMap,
   DefaultWeakMap,
 } = ExtensionUtils;
 
 ChromeUtils.defineModuleGetter(this, "ExtensionParent",
                                "resource://gre/modules/ExtensionParent.jsm");
 ChromeUtils.defineModuleGetter(this, "NetUtil",
                                "resource://gre/modules/NetUtil.jsm");
+ChromeUtils.defineModuleGetter(this, "ShortcutUtils",
+                               "resource://gre/modules/ShortcutUtils.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "contentPolicyService",
                                    "@mozilla.org/addons/content-policy;1",
                                    "nsIAddonContentPolicy");
 
 XPCOMUtils.defineLazyGetter(this, "StartupCache", () => ExtensionParent.StartupCache);
 
 var EXPORTED_SYMBOLS = ["SchemaRoot", "Schemas"];
 
@@ -988,65 +989,25 @@ const FORMATS = {
     // constructor do the dirty work of validating.
     if (isNaN(new Date(string))) {
       throw new Error(`Invalid date string ${string}`);
     }
     return string;
   },
 
   manifestShortcutKey(string, context) {
-    // A valid shortcut key for a webextension manifest
-    const MEDIA_KEYS = /^(MediaNextTrack|MediaPlayPause|MediaPrevTrack|MediaStop)$/;
-    const BASIC_KEYS = /^([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)$/;
-    const FUNCTION_KEYS = /^(F[1-9]|F1[0-2])$/;
+    if (ShortcutUtils.validate(string) == ShortcutUtils.IS_VALID) {
+      return string;
+    }
     let errorMessage = (`Value "${string}" must consist of `
                         + `either a combination of one or two modifiers, including `
                         + `a mandatory primary modifier and a key, separated by '+', `
                         + `or a media key. For details see: `
                         + `https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands#Key_combinations`);
-    if (MEDIA_KEYS.test(string.trim())) {
-      return string;
-    }
-
-    let modifiers = string.split("+").map(s => s.trim());
-    let key = modifiers.pop();
-
-    if (!BASIC_KEYS.test(key) && !FUNCTION_KEYS.test(key)) {
-      throw new Error(errorMessage);
-    }
-
-    let chromeModifiers = modifiers.map(m => chromeModifierKeyMap[m]);
-    // If the modifier wasn't found it will be undefined.
-    if (chromeModifiers.some(modifier => !modifier)) {
-      throw new Error(errorMessage);
-    }
-
-    switch (modifiers.length) {
-      case 0:
-        // A lack of modifiers is only allowed with function keys.
-        if (!FUNCTION_KEYS.test(key)) {
-          throw new Error(errorMessage);
-        }
-        break;
-      case 1:
-        // Shift is only allowed on its own with function keys.
-        if (chromeModifiers[0] == "shift" && !FUNCTION_KEYS.test(key)) {
-          throw new Error(errorMessage);
-        }
-        break;
-      case 2:
-        if (chromeModifiers[0] == chromeModifiers[1]) {
-          throw new Error(errorMessage);
-        }
-        break;
-      default:
-        throw new Error(errorMessage);
-    }
-
-    return string;
+    throw new Error(errorMessage);
   },
 };
 
 // Schema files contain namespaces, and each namespace contains types,
 // properties, functions, and events. An Entry is a base class for
 // types, properties, functions, and events.
 class Entry {
   constructor(schema = {}) {
--- a/toolkit/modules/ShortcutUtils.jsm
+++ b/toolkit/modules/ShortcutUtils.jsm
@@ -15,29 +15,45 @@ XPCOMUtils.defineLazyGetter(this, "Platf
 });
 
 XPCOMUtils.defineLazyGetter(this, "Keys", function() {
   return Services.strings.createBundle(
     "chrome://global/locale/keys.properties");
 });
 
 var ShortcutUtils = {
+  IS_VALID: "valid",
+  INVALID_KEY: "invalid_key",
+  INVALID_MODIFIER: "invalid_modifier",
+  INVALID_COMBINATION: "invalid_combination",
+  DUPLICATE_MODIFIER: "duplicate_modifier",
+  MODIFIER_REQUIRED: "modifier_required",
+
   /**
     * Prettifies the modifier keys for an element.
     *
     * @param Node aElemKey
     *        The key element to get the modifiers from.
     * @param boolean aNoCloverLeaf
     *        Pass true to use a descriptive string instead of the cloverleaf symbol. (OS X only)
     * @return string
     *         A prettified and properly separated modifier keys string.
     */
   prettifyShortcut(aElemKey, aNoCloverLeaf) {
+    let elemString = this.getModifierString(
+      aElemKey.getAttribute("modifiers"),
+      aNoCloverLeaf);
+    let key = this.getKeyString(
+      aElemKey.getAttribute("keycode"),
+      aElemKey.getAttribute("key"));
+    return elemString + key;
+  },
+
+  getModifierString(elemMod, aNoCloverLeaf) {
     let elemString = "";
-    let elemMod = aElemKey.getAttribute("modifiers");
     let haveCloverLeaf = false;
 
     if (elemMod.match("accel")) {
       if (Services.appinfo.OS == "Darwin") {
         // XXX bug 779642 Use "Cmd-" literal vs. cloverleaf meta-key until
         // Orion adds variable height lines.
         if (aNoCloverLeaf) {
           elemString += "Cmd-";
@@ -79,35 +95,183 @@ var ShortcutUtils = {
         PlatformKeys.GetStringFromName("MODIFIER_SEPARATOR");
     }
 
     if (haveCloverLeaf) {
       elemString += PlatformKeys.GetStringFromName("VK_META") +
         PlatformKeys.GetStringFromName("MODIFIER_SEPARATOR");
     }
 
+    return elemString;
+  },
+
+  getKeyString(keyCode, keyAttribute) {
     let key;
-    let keyCode = aElemKey.getAttribute("keycode");
     if (keyCode) {
       keyCode = keyCode.toUpperCase();
       try {
         let bundle = keyCode == "VK_RETURN" ? PlatformKeys : Keys;
         // Some keys might not exist in the locale file, which will throw.
         key = bundle.GetStringFromName(keyCode);
       } catch (ex) {
         Cu.reportError("Error finding " + keyCode + ": " + ex);
         key = keyCode.replace(/^VK_/, "");
       }
     } else {
-      key = aElemKey.getAttribute("key");
-      key = key.toUpperCase();
+      key = keyAttribute.toUpperCase();
+    }
+
+    return key;
+  },
+
+  getKeyAttribute(chromeKey) {
+    if (/^[A-Z]$/.test(chromeKey)) {
+      // We use the key attribute for single characters.
+      return ["key", chromeKey];
     }
-    return elemString + key;
+    return ["keycode", this.getKeycodeAttribute(chromeKey)];
+  },
+
+  /**
+   * Determines the corresponding XUL keycode from the given chrome key.
+   *
+   * For example:
+   *
+   *    input     |  output
+   *    ---------------------------------------
+   *    "PageUp"  |  "VK_PAGE_UP"
+   *    "Delete"  |  "VK_DELETE"
+   *
+   * @param {string} chromeKey The chrome key (e.g. "PageUp", "Space", ...)
+   * @returns {string} The constructed value for the Key's 'keycode' attribute.
+   */
+  getKeycodeAttribute(chromeKey) {
+    if (/^[0-9]/.test(chromeKey)) {
+      return `VK_${chromeKey}`;
+    }
+    return `VK${chromeKey.replace(/([A-Z])/g, "_$&").toUpperCase()}`;
   },
 
   findShortcut(aElemCommand) {
     let document = aElemCommand.ownerDocument;
     return document.querySelector("key[command=\"" + aElemCommand.getAttribute("id") + "\"]");
   },
+
+  chromeModifierKeyMap: {
+    "Alt": "alt",
+    "Command": "accel",
+    "Ctrl": "accel",
+    "MacCtrl": "control",
+    "Shift": "shift",
+  },
+
+  /**
+   * Determines the corresponding XUL modifiers from the chrome modifiers.
+   *
+   * For example:
+   *
+   *    input             |   output
+   *    ---------------------------------------
+   *    ["Ctrl", "Shift"] |   "accel,shift"
+   *    ["MacCtrl"]       |   "control"
+   *
+   * @param {Array} chromeModifiers The array of chrome modifiers.
+   * @returns {string} The constructed value for the Key's 'modifiers' attribute.
+   */
+  getModifiersAttribute(chromeModifiers) {
+    return Array.from(chromeModifiers, modifier => {
+      return ShortcutUtils.chromeModifierKeyMap[modifier];
+    }).sort().join(",");
+  },
+
+  /**
+   * Validate if a shortcut string is valid and return an error code if it
+   * isn't valid.
+   *
+   * For example:
+   *
+   *    input            |   output
+   *    ---------------------------------------
+   *    "Ctrl+Shift+A"   |   IS_VALID
+   *    "Shift+F"        |   MODIFIER_REQUIRED
+   *    "Command+>"      |   INVALID_KEY
+   *
+   * @param {string} string The shortcut string.
+   * @returns {string} The code for the validation result.
+   */
+  validate(string) {
+    // A valid shortcut key for a webextension manifest
+    const MEDIA_KEYS = /^(MediaNextTrack|MediaPlayPause|MediaPrevTrack|MediaStop)$/;
+    const BASIC_KEYS = /^([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)$/;
+    const FUNCTION_KEYS = /^(F[1-9]|F1[0-2])$/;
+
+    if (MEDIA_KEYS.test(string.trim())) {
+      return this.IS_VALID;
+    }
+
+    let modifiers = string.split("+").map(s => s.trim());
+    let key = modifiers.pop();
+
+    let chromeModifiers = modifiers.map(m => ShortcutUtils.chromeModifierKeyMap[m]);
+    // If the modifier wasn't found it will be undefined.
+    if (chromeModifiers.some(modifier => !modifier)) {
+      return this.INVALID_MODIFIER;
+    }
+
+    switch (modifiers.length) {
+      case 0:
+        // A lack of modifiers is only allowed with function keys.
+        if (!FUNCTION_KEYS.test(key)) {
+          return this.MODIFIER_REQUIRED;
+        }
+        break;
+      case 1:
+        // Shift is only allowed on its own with function keys.
+        if (chromeModifiers[0] == "shift" && !FUNCTION_KEYS.test(key)) {
+          return this.MODIFIER_REQUIRED;
+        }
+        break;
+      case 2:
+        if (chromeModifiers[0] == chromeModifiers[1]) {
+          return this.DUPLICATE_MODIFIER;
+        }
+        break;
+      default:
+        return this.INVALID_COMBINATION;
+    }
+
+    if (!BASIC_KEYS.test(key) && !FUNCTION_KEYS.test(key)) {
+      return this.INVALID_KEY;
+    }
+
+    return this.IS_VALID;
+  },
+
+  /**
+   * Attempt to find a key for a given shortcut string, such as
+   * "Ctrl+Shift+A" and determine if it is a system shortcut.
+   *
+   * @param {Object} win The window to look for key elements in.
+   * @param {string} value The shortcut string.
+   * @returns {boolean} Whether a system shortcut was found or not.
+   */
+  isSystem(win, value) {
+    let modifiers = value.split("+");
+    let chromeKey = modifiers.pop();
+    let modifiersString = this.getModifiersAttribute(modifiers);
+    let keycode = this.getKeycodeAttribute(chromeKey);
+
+    let baseSelector = "key";
+    if (modifiers.length > 0) {
+      baseSelector += `[modifiers="${modifiersString}"]`;
+    }
+
+    let keyEl = win.document.querySelector([
+      `${baseSelector}[key="${chromeKey}"]`,
+      `${baseSelector}[key="${chromeKey.toLowerCase()}"]`,
+      `${baseSelector}[keycode="${keycode}"]`,
+    ].join(","));
+    return keyEl && !keyEl.closest("keyset").id.startsWith("ext-keyset-id");
+  },
 };
 
 Object.freeze(ShortcutUtils);