Bug 1421811 - Part 1: Support commands.update() to modify a command r?mixedpuppy draft
authorMark Striemer <mstriemer@mozilla.com>
Wed, 31 Jan 2018 15:48:32 -0600
changeset 752333 8dbeec1f5b7b4b3493afb1fde17bbd0c6a8947ee
parent 750876 3503512d062abe0318d2b0f0edf7551772b232eb
child 752334 b189a895fa3365b5a5a389686ef9f2661ccb7eed
push id98231
push userbmo:mstriemer@mozilla.com
push dateWed, 07 Feb 2018 23:16:08 +0000
reviewersmixedpuppy
bugs1421811
milestone60.0a1
Bug 1421811 - Part 1: Support commands.update() to modify a command r?mixedpuppy MozReview-Commit-ID: 5A6ZmvNT294
browser/components/extensions/ext-browser.json
browser/components/extensions/ext-commands.js
browser/components/extensions/schemas/commands.json
browser/components/extensions/test/browser/browser-common.ini
browser/components/extensions/test/browser/browser_ext_commands_update.js
--- a/browser/components/extensions/ext-browser.json
+++ b/browser/components/extensions/ext-browser.json
@@ -30,16 +30,17 @@
     "events": ["update", "uninstall"],
     "schema": "chrome://browser/content/schemas/chrome_settings_overrides.json",
     "manifest": ["chrome_settings_overrides"]
   },
   "commands": {
     "url": "chrome://browser/content/ext-commands.js",
     "schema": "chrome://browser/content/schemas/commands.json",
     "scopes": ["addon_parent"],
+    "events": ["uninstall"],
     "manifest": ["commands"],
     "paths": [
       ["commands"]
     ]
   },
   "devtools": {
     "url": "chrome://browser/content/ext-devtools.js",
     "schema": "chrome://browser/content/schemas/devtools.json",
--- a/browser/components/extensions/ext-commands.js
+++ b/browser/components/extensions/ext-commands.js
@@ -3,51 +3,97 @@
 "use strict";
 
 // The ext-* files are imported into the same scopes.
 /* import-globals-from ext-browserAction.js */
 /* import-globals-from ext-browser.js */
 
 ChromeUtils.defineModuleGetter(this, "ExtensionParent",
                                "resource://gre/modules/ExtensionParent.jsm");
+ChromeUtils.defineModuleGetter(this, "ExtensionSettingsStore",
+                               "resource://gre/modules/ExtensionSettingsStore.jsm");
 
 var XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
 
+function normalizeShortcut(shortcut) {
+  return shortcut ? shortcut.replace(/\s+/g, "") : null;
+}
+
 this.commands = class extends ExtensionAPI {
-  onManifestEntry(entryName) {
+  static async onUninstall(extensionId) {
+    // Cleanup the updated commands. In some cases the extension is installed
+    // and uninstalled so quickly that `this.commands` hasn't loaded yet. To
+    // handle that we need to make sure ExtensionSettingsStore is initialized
+    // before we clean it up.
+    await ExtensionSettingsStore.initialize();
+    ExtensionSettingsStore
+      .getAllForExtension(extensionId, "commands")
+      .forEach(key => {
+        ExtensionSettingsStore.removeSetting(extensionId, "commands", key);
+      });
+  }
+
+  async onManifestEntry(entryName) {
     let {extension} = this;
 
     this.id = makeWidgetId(extension.id);
     this.windowOpenListener = null;
 
     // Map[{String} commandName -> {Object} commandProperties]
-    this.commands = this.loadCommandsFromManifest(this.extension.manifest);
+    this.manifestCommands = this.loadCommandsFromManifest(extension.manifest);
+
+    this.commands = new Promise(async (resolve) => {
+      // Deep copy the manifest commands to commands so we can keep the original
+      // manifest commands and update commands as needed.
+      let commands = new Map();
+      this.manifestCommands.forEach((command, name) => {
+        commands.set(name, {...command});
+      });
+
+      // Update the manifest commands with the persisted updates from
+      // browser.commands.update().
+      let savedCommands = await this.loadCommandsFromStorage(extension.id);
+      savedCommands.forEach((update, name) => {
+        let command = commands.get(name);
+        if (command) {
+          // We will only update commands, not add them.
+          Object.assign(command, update);
+        }
+      });
+
+      resolve(commands);
+    });
 
     // WeakMap[Window -> <xul:keyset>]
     this.keysetsMap = new WeakMap();
 
-    this.register();
+    await this.register();
   }
 
   onShutdown(reason) {
     this.unregister();
   }
 
+  registerKeys(commands) {
+    for (let window of windowTracker.browserWindows()) {
+      this.registerKeysToDocument(window, commands);
+    }
+  }
+
   /**
    * Registers the commands to all open windows and to any which
    * are later created.
    */
-  register() {
-    for (let window of windowTracker.browserWindows()) {
-      this.registerKeysToDocument(window);
-    }
+  async register() {
+    let commands = await this.commands;
+    this.registerKeys(commands);
 
     this.windowOpenListener = (window) => {
       if (!this.keysetsMap.has(window)) {
-        this.registerKeysToDocument(window);
+        this.registerKeysToDocument(window, commands);
       }
     };
 
     windowTracker.addOpenListener(this.windowOpenListener);
   }
 
   /**
    * Unregisters the commands from all open windows and stops commands
@@ -72,35 +118,48 @@ this.commands = class extends ExtensionA
   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 {PlatformInfo} = ExtensionParent;
     let os = PlatformInfo.os == "win" ? "windows" : PlatformInfo.os;
     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;
+      let shortcut = normalizeShortcut(suggested_key[os] || suggested_key.default);
       commands.set(name, {
         description: command.description,
         shortcut,
       });
     }
     return commands;
   }
 
+  async loadCommandsFromStorage(extensionId) {
+    await ExtensionSettingsStore.initialize();
+    let names = ExtensionSettingsStore.getAllForExtension(extensionId, "commands");
+    return names.reduce((map, name) => {
+      let command = ExtensionSettingsStore.getSetting(
+        "commands", name, extensionId).value;
+      return map.set(name, command);
+    }, new Map());
+  }
+
   /**
    * Registers the commands to a document.
    * @param {ChromeWindow} window The XUL window to insert the Keyset.
+   * @param {Map} commands The commands to be set.
    */
-  registerKeysToDocument(window) {
+  registerKeysToDocument(window, commands) {
     let doc = window.document;
     let keyset = doc.createElementNS(XUL_NS, "keyset");
     keyset.id = `ext-keyset-id-${this.id}`;
-    this.commands.forEach((command, name) => {
+    if (this.keysetsMap.has(window)) {
+      this.keysetsMap.get(window).remove();
+    }
+    commands.forEach((command, name) => {
       if (command.shortcut) {
         let keyElement = this.buildKey(doc, name, command.shortcut);
         keyset.appendChild(keyElement);
       }
     });
     doc.documentElement.appendChild(keyset);
     this.keysetsMap.set(window, keyset);
   }
@@ -230,25 +289,56 @@ this.commands = class extends ExtensionA
     return Array.from(chromeModifiers, modifier => {
       return modifiersMap[modifier];
     }).join(" ");
   }
 
   getAPI(context) {
     return {
       commands: {
-        getAll: () => {
-          let commands = this.commands;
-          return Promise.resolve(Array.from(commands, ([name, command]) => {
+        getAll: async () => {
+          let commands = await this.commands;
+          return Array.from(commands, ([name, command]) => {
             return ({
               name,
               description: command.description,
               shortcut: command.shortcut,
             });
-          }));
+          });
+        },
+        update: async ({name, description, shortcut}) => {
+          let {extension} = this;
+          let commands = await this.commands;
+          let command = commands.get(name);
+
+          if (!command) {
+            throw new ExtensionError(`Unknown command "${name}"`);
+          }
+
+          // Only store the updates so manifest changes can take precedence
+          // later.
+          let previousUpdates = await ExtensionSettingsStore.getSetting(
+            "commands", name, extension.id);
+          let commandUpdates = (previousUpdates && previousUpdates.value) || {};
+
+          if (description && description != command.description) {
+            commandUpdates.description = description;
+            command.description = description;
+          }
+
+          if (shortcut && shortcut != command.shortcut) {
+            shortcut = normalizeShortcut(shortcut);
+            commandUpdates.shortcut = shortcut;
+            command.shortcut = shortcut;
+          }
+
+          await ExtensionSettingsStore.addSetting(
+            extension.id, "commands", name, commandUpdates);
+
+          this.registerKeys(commands);
         },
         onCommand: new EventManager(context, "commands.onCommand", fire => {
           let listener = (eventName, commandName) => {
             fire.async(commandName);
           };
           this.on("command", listener);
           return () => {
             this.off("command", listener);
--- a/browser/components/extensions/schemas/commands.json
+++ b/browser/components/extensions/schemas/commands.json
@@ -120,16 +120,44 @@
             "name": "command",
             "type": "string"
           }
         ]
       }
     ],
     "functions": [
       {
+        "name": "update",
+        "type": "function",
+        "async": true,
+        "description": "Update the details of an already defined command.",
+        "parameters": [
+          {
+            "type": "object",
+            "name": "detail",
+            "description": "The new description for the command.",
+            "properties": {
+              "name": {
+                "type": "string",
+                "description": "The name of the command."
+                },
+              "description": {
+                "type": "string",
+                "optional": true,
+                "description": "The new description for the command."
+              },
+              "shortcut": {
+                "$ref": "manifest.KeyName",
+                "optional": true
+              }
+            }
+          }
+        ]
+      },
+      {
         "name": "getAll",
         "type": "function",
         "async": "callback",
         "description": "Returns all the registered extension commands for this extension and their shortcut (if active).",
         "parameters": [
           {
             "type": "function",
             "name": "callback",
--- a/browser/components/extensions/test/browser/browser-common.ini
+++ b/browser/components/extensions/test/browser/browser-common.ini
@@ -61,16 +61,17 @@ skip-if = (os == 'win' && !debug) # bug 
 [browser_ext_browsingData_pluginData.js]
 [browser_ext_browsingData_serviceWorkers.js]
 [browser_ext_chrome_settings_overrides_home.js]
 [browser_ext_commands_execute_browser_action.js]
 [browser_ext_commands_execute_page_action.js]
 [browser_ext_commands_execute_sidebar_action.js]
 [browser_ext_commands_getAll.js]
 [browser_ext_commands_onCommand.js]
+[browser_ext_commands_update.js]
 [browser_ext_contentscript_connect.js]
 [browser_ext_contextMenus.js]
 [browser_ext_contextMenus_checkboxes.js]
 [browser_ext_contextMenus_commands.js]
 [browser_ext_contextMenus_icons.js]
 [browser_ext_contextMenus_onclick.js]
 [browser_ext_contextMenus_radioGroups.js]
 [browser_ext_contextMenus_uninstall.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/extensions/test/browser/browser_ext_commands_update.js
@@ -0,0 +1,229 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+"use strict";
+
+ChromeUtils.defineModuleGetter(this, "ExtensionSettingsStore",
+                               "resource://gre/modules/ExtensionSettingsStore.jsm");
+ChromeUtils.defineModuleGetter(this, "AddonManager",
+                               "resource://gre/modules/AddonManager.jsm");
+
+function enableAddon(addon) {
+  return new Promise(resolve => {
+    AddonManager.addAddonListener({
+      onEnabled(enabledAddon) {
+        if (enabledAddon.id == addon.id) {
+          resolve();
+          AddonManager.removeAddonListener(this);
+        }
+      },
+    });
+    addon.userDisabled = false;
+  });
+}
+
+function disableAddon(addon) {
+  return new Promise(resolve => {
+    AddonManager.addAddonListener({
+      onDisabled(disabledAddon) {
+        if (disabledAddon.id == addon.id) {
+          resolve();
+          AddonManager.removeAddonListener(this);
+        }
+      },
+    });
+    addon.userDisabled = true;
+  });
+}
+
+add_task(async function test_update_defined_command() {
+  let extension;
+  let updatedExtension;
+
+  registerCleanupFunction(async () => {
+    await extension.unload();
+
+    // updatedExtension might not have started up if we didn't make it that far.
+    if (updatedExtension) {
+      await updatedExtension.unload();
+    }
+
+    // Check that ESS is cleaned up on uninstall.
+    let storedCommands = ExtensionSettingsStore.getAllForExtension(
+      extension.id, "commands");
+    is(storedCommands.length, 0, "There are no stored commands after unload");
+  });
+
+  extension = ExtensionTestUtils.loadExtension({
+    useAddonManager: "permanent",
+    manifest: {
+      version: "1.0",
+      applications: {gecko: {id: "commands@mochi.test"}},
+      commands: {
+        foo: {
+          suggested_key: {
+            default: "Ctrl+Shift+I",
+          },
+          description: "The foo command",
+        },
+      },
+    },
+    background() {
+      browser.test.onMessage.addListener(async (msg, data) => {
+        if (msg == "update") {
+          await browser.commands.update(data);
+          return browser.test.sendMessage("updateDone");
+        } else if (msg != "run") {
+          return;
+        }
+        // Test initial manifest command.
+        let commands = await browser.commands.getAll();
+        browser.test.assertEq(1, commands.length, "There is 1 command");
+        let command = commands[0];
+        browser.test.assertEq("foo", command.name, "The name is right");
+        browser.test.assertEq("The foo command", command.description, "The description is right");
+        browser.test.assertEq("Ctrl+Shift+I", command.shortcut, "The shortcut is right");
+
+        // Update the shortcut.
+        await browser.commands.update({name: "foo", shortcut: "Ctrl+Shift+L"});
+
+        // Test the updated shortcut.
+        commands = await browser.commands.getAll();
+        browser.test.assertEq(1, commands.length, "There is still 1 command");
+        command = commands[0];
+        browser.test.assertEq("foo", command.name, "The name is unchanged");
+        browser.test.assertEq("The foo command", command.description, "The description is unchanged");
+        browser.test.assertEq("Ctrl+Shift+L", command.shortcut, "The shortcut is updated");
+
+        // Update the description.
+        await browser.commands.update({name: "foo", description: "The only command"});
+
+        // Test the updated shortcut.
+        commands = await browser.commands.getAll();
+        browser.test.assertEq(1, commands.length, "There is still 1 command");
+        command = commands[0];
+        browser.test.assertEq("foo", command.name, "The name is unchanged");
+        browser.test.assertEq("The only command", command.description, "The description is updated");
+        browser.test.assertEq("Ctrl+Shift+L", command.shortcut, "The shortcut is unchanged");
+
+        // Update the description and shortcut.
+        await browser.commands.update({
+          name: "foo",
+          description: "The new command",
+          shortcut: "   Alt+  Shift +P",
+        });
+
+        // Test the updated shortcut.
+        commands = await browser.commands.getAll();
+        browser.test.assertEq(1, commands.length, "There is still 1 command");
+        command = commands[0];
+        browser.test.assertEq("foo", command.name, "The name is unchanged");
+        browser.test.assertEq("The new command", command.description, "The description is updated");
+        browser.test.assertEq("Alt+Shift+P", command.shortcut, "The shortcut is updated");
+
+        // Test a bad shortcut update.
+        browser.test.assertThrows(
+          () => browser.commands.update({name: "foo", shortcut: "Ctl+Shift+L"}),
+          /Type error for parameter detail/,
+          "It rejects for a bad shortcut");
+
+        // Try to update a command that doesn't exist.
+        await browser.test.assertRejects(
+          browser.commands.update({name: "bar", shortcut: "Ctrl+Shift+L"}),
+          'Unknown command "bar"',
+          "It rejects for an unknown command");
+
+        browser.test.notifyPass("commands");
+      });
+      browser.test.sendMessage("ready");
+    },
+  });
+
+  await extension.startup();
+
+  function extensionKeyset(extensionId) {
+    return document.getElementById(makeWidgetId(`ext-keyset-id-${extensionId}`));
+  }
+
+  function checkKey(extensionId, shortcutKey, modifiers) {
+    let keyset = extensionKeyset(extensionId);
+    is(keyset.children.length, 1, "There is 1 key in the keyset");
+    let key = keyset.children[0];
+    is(key.getAttribute("key"), shortcutKey, "The key is correct");
+    is(key.getAttribute("modifiers"), modifiers, "The modifiers are correct");
+  }
+
+  // Check that the <key> is set for the original shortcut.
+  checkKey(extension.id, "I", "accel shift");
+
+  await extension.awaitMessage("ready");
+  extension.sendMessage("run");
+  await extension.awaitFinish("commands");
+
+  // Check that the <key> has been updated.
+  checkKey(extension.id, "P", "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+P", "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");
+
+  // 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+P"});
+  await extension.awaitMessage("updateDone");
+  let storedCommand = await ExtensionSettingsStore.getSetting(
+    "commands", "foo", extension.id);
+  is(storedCommand.value.shortcut, "Alt+Shift+P", "The shortcut is saved correctly");
+  is(storedCommand.value.description, "description only", "The description is saved correctly");
+
+  // 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(
+    extension.id, "commands", "foo", {shortcut: "Alt+Shift+P"});
+  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.
+  checkKey(extension.id, "P", "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: {
+        foo: {
+          suggested_key: {
+            default: "Ctrl+Shift+L",
+          },
+          description: "The foo command",
+        },
+      },
+    },
+  });
+  await updatedExtension.startup();
+
+  await TestUtils.waitForCondition(() => extensionKeyset(extension.id));
+  // Shortcut is unchanged since it was previously updated.
+  checkKey(extension.id, "P", "alt shift");
+});