1246029 - [commands] Implement chrome.commands.onCommand

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Comment 1

[Matthew Wein [:mattw]]

Attachment: WIP: - Implement chrome.commands.onCommand

Comment 2

[Matthew Wein [:mattw]]

Comment on attachment 8722248 [details] [diff] [review]
WIP:  - Implement chrome.commands.onCommand

Review of attachment 8722248 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-commands.js
@@ +22,5 @@
> +    this.commands.set(name, {
> +      description: command.description,
> +      shortcut: command.suggested_key[PlatformInfo.os] || command.suggested_key.default,
> +    });
> +  }

Lines 19-26 should be replaced with `this.commands = this.parseFromManifest(parseFromManifest)`.  I'm also considering renaming that method to `loadCommandsFromManifest`.

@@ +69,5 @@
> +    for (let keyElement in this.keyset.childNodes) {
> +      if (keyElement.hasEventListener("command")) {
> +        keyElement.removeEventListener("command");
> +      }
> +    }

I plan to add unit tests for this in my next patch.

@@ +95,5 @@
> +    return keyElement;
> +  },
> +
> +  // TODO(mattw): Complete the implementation of parseShortcut, which should take a shortcut
> +  // from the manifest and determine the corresponding attributes needed for a XUL key element.

This method should eventually use the utility added by Bug 1246032 (https://bugzilla.mozilla.org/show_bug.cgi?id=1246032).  I have this bug depending on Bug 1246032.

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8722248 [details] [diff] [review]
WIP:  - Implement chrome.commands.onCommand

Review of attachment 8722248 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-commands.js
@@ +36,5 @@
> +  });
> +
> +  // Unregister keys for any closed window.
> +  WindowListManager.addCloseListener((event) => {
> +    this.unRegisterKeys(event.target.ownerDocument);

This shouldn't be necessary.

@@ +56,5 @@
> +    return commands;
> +  },
> +
> +  registerKeys(doc) {
> +    this.keyset = doc.createElementNS(XUL_NS, "keyset");

This should have an ID, which needs to be unique to the extension.

@@ +64,5 @@
> +    });
> +    doc.documentElement.appendChild(this.keyset);
> +  },
> +
> +  unRegisterKeys(doc) {

*unregisterKeys

@@ +65,5 @@
> +    doc.documentElement.appendChild(this.keyset);
> +  },
> +
> +  unRegisterKeys(doc) {
> +    for (let keyElement in this.keyset.childNodes) {

You want `of`, not `in`.

@@ +66,5 @@
> +  },
> +
> +  unRegisterKeys(doc) {
> +    for (let keyElement in this.keyset.childNodes) {
> +      if (keyElement.hasEventListener("command")) {

There's no such thing as "hasEventListener"

@@ +67,5 @@
> +
> +  unRegisterKeys(doc) {
> +    for (let keyElement in this.keyset.childNodes) {
> +      if (keyElement.hasEventListener("command")) {
> +        keyElement.removeEventListener("command");

This requires an argument.

@@ +68,5 @@
> +  unRegisterKeys(doc) {
> +    for (let keyElement in this.keyset.childNodes) {
> +      if (keyElement.hasEventListener("command")) {
> +        keyElement.removeEventListener("command");
> +      }

We need to actually remove the keyset element.

@@ +85,5 @@
> +    }
> +
> +    // We need to have the attribute "oncommand" for the "command" listener to fire.
> +    // @see https://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLWindowKeyHandler.cpp#492
> +    keyElement.setAttribute("oncommand", "javascript:void(0);");

"oncommand" is not expected to be a URL. This is technically valid JavaScript, but it would be better to just leave the attribute empty.

@@ +123,5 @@
> +  commandsMap.set(extension, new CommandList(manifest.commands));
> +});
> +
> +extensions.on("shutdown", (type, extension) => {
> +  commandsMap.delete(extension);

We need to remove the generated keyset elements at this point.

::: browser/components/extensions/test/browser/browser_ext_commands.js
@@ +42,5 @@
> +  EventUtils.synthesizeKey("o", {accelKey: true, shiftKey: true});
> +  message = yield extension.awaitMessage("oncommand");
> +  is("toggle-feature-B", message, "Expected onCommand listener to fire with correct message");
> +
> +  yield extension.unload();

Need to check that elements have been removed here.

Comment 6

[Matthew Wein [:mattw]]

Attachment: Implement chrome.commands.onCommand

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8724343 [details] [diff] [review]
Implement chrome.commands.onCommand

Review of attachment 8724343 [details] [diff] [review]:
-----------------------------------------------------------------

This is very nice. Thanks.

::: browser/components/extensions/ext-commands.js
@@ +16,5 @@
>  var commandsMap = new WeakMap();
>  
> +function CommandList(commandsObj, extensionID) {
> +  this.commands = this.loadCommandsFromManifest(commandsObj);
> +  this.keysetID = `ext-keyset-id-${extensionID}`;

We should probably use `makeWidgetId` here: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-utils.js#127

@@ +47,5 @@
> +  unregister() {
> +    for (let window of WindowListManager.browserWindows()) {
> +      let keyset = window.document.getElementById(this.keysetID);
> +      if (keyset) {
> +        keyset.parentNode.removeChild(keyset);

keyset.remove()

@@ +90,5 @@
> +
> +  /**
> +   * Builds a XUL Key element.
> +   *
> +   * @param {Element} doc The XUL document.

Documents don't inherit from Element. This should be {Document}

@@ +193,4 @@
>  });
>  
>  extensions.on("shutdown", (type, extension) => {
> +  let commandsList = commandsMap.get(extension);

We need to check that `commandsList` is not null here.

::: browser/components/extensions/test/browser/browser_ext_commands.js
@@ -1,1 @@
> -/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

Please make sure to always use `hg mv` when you're renaming files, rather than removing and re-adding them.

Comment 8

[Matthew Wein [:mattw]]

> This is very nice. Thanks.
Yw, thanks!

> > +  this.keysetID = `ext-keyset-id-${extensionID}`;
> 
> We should probably use `makeWidgetId` here:
> https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/
> ext-utils.js#127
I meant to do that but forgot to, thanks. Done.

> > +        keyset.parentNode.removeChild(keyset);
> 
> keyset.remove()
That's much better, thanks.

> > +   * @param {Element} doc The XUL document.
> 
> Documents don't inherit from Element. This should be {Document}
Done.

> > +  let commandsList = commandsMap.get(extension);
> 
> We need to check that `commandsList` is not null here.
Done.

> Please make sure to always use `hg mv` when you're renaming files, rather
> than removing and re-adding them.
Will do, sorry about that.

Comment 9

[Matthew Wein [:mattw]]

Attachment: WIP: - Implement chrome.commands.onCommand

Comment 10

[Matthew Wein [:mattw]]

Attachment: Implement chrome.commands.onCommand

Comment 11

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b246b7703810

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49ab89b42dbf

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_ext_commands_onCommand.js permafail across all platforms.
https://treeherder.mozilla.org/logviewer.html#?job_id=22595070&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/ade444def972

Comment 14

[Kris Maglione [:kmag]]

Heh. Looks like your key events are triggering some built-in key commands.

They failed in your try run, too: https://treeherder.mozilla.org/logviewer.html#?job_id=17316654&repo=try

Comment 15

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91eed68cbc80

Comment 16

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=359992458cfc

Comment 17

[Matthew Wein [:mattw]]

Attachment: Implement chrome.commands.onCommand

Comment 18

[Matthew Wein [:mattw]]

The tests should pass now that I've switched to unused commands: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d59f53a9e132&selectedJob=17393886

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/89e36be196d9

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/89e36be196d9