1246035 - [commands] Add support for _execute_page_action

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Comment 1

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Bug 1246035 - Add support for _execute_page_action r?kmag

Review commit: https://reviewboard.mozilla.org/r/38375/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38375/

Comment 2

[Matthew Wein [:mattw]]

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

Comment 3

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

Review commit: https://reviewboard.mozilla.org/r/38379/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38379/

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

https://reviewboard.mozilla.org/r/38379/#review35127

This is a good start. It just needs some code moved around, and a bit more testing.

::: browser/components/extensions/ext-commands.js:118
(Diff revision 1)
> +        }

This is good, but for the sake of encapsulation, it should really live in the `PageAction` class.

Can you add a helper method to that class, and call it from here, instead?

::: browser/components/extensions/test/browser/browser_ext_commands_onCommand.js:102
(Diff revision 1)
> +        browser.test.sendMessage("page-action-clicked");

We need to test this with pageActions that have a popup, too. And, ideally, we should check that it doesn't trigger either clicks or popups when the pageAction is hidden.

Also, it would probably be best to move this test to its own file.

Comment 5

[Matthew Wein [:mattw]]

Comment on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38379/diff/1-2/

Comment 6

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/38379/#review35127

> We need to test this with pageActions that have a popup, too. And, ideally, we should check that it doesn't trigger either clicks or popups when the pageAction is hidden.
> 
> Also, it would probably be best to move this test to its own file.

Yeah, that makes sense.  I moved this test to a new file and added tests for these cases.

The tests got fairly complicated when I added tests to make sure it doesn't trigger either clicks or popups when the pageAction is hidden.  Let me know if you think I could restructure it in any way.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

https://reviewboard.mozilla.org/r/38379/#review36381

::: browser/components/extensions/ext-commands.js:40
(Diff revisions 1 - 2)
>     * Registers the commands to all open windows and to any which
>     * are later created.
>     */
>    register() {
>      for (let window of WindowListManager.browserWindows()) {
> -      this.registerKeysToDocument(window.document);
> +      if (!this.keysetsMap.has(window)) {

Why is this check necessary?

::: browser/components/extensions/ext-commands.js:127
(Diff revisions 1 - 2)
>      // We remove all references to the key elements when the extension is shutdown,
>      // therefore the listeners for these elements will be garbage collected.
>      keyElement.addEventListener("command", (event) => {
>        if (name == "_execute_page_action") {
> -        let elem = doc.getElementById(`${this.id}-page-action`);
> -        if (elem && !elem.hidden) {
> +        let win = event.target.ownerDocument.defaultView;
> +        global.pageActionOf(this.extension).triggerAction(win);

No need for `global.`

::: browser/components/extensions/test/browser/browser_ext_commands_onCommand.js:46
(Diff revisions 1 - 2)
>    yield BrowserTestUtils.browserLoaded(win2.gBrowser.selectedBrowser);
>  
>    // Confirm the keysets have been added to both windows.
>    let keysetID = `ext-keyset-id-${makeWidgetId(extension.id)}`;
>    let keyset = win1.document.getElementById(keysetID);
> -  is(keyset.childNodes.length, 2, "Expected keyset to exist and have 2 children");
> +  is(keyset != null, true, "Expected keyset to exist");

`ok(keyset != null, ...)`

::: browser/components/extensions/test/browser/browser_ext_commands_onCommand.js:50
(Diff revisions 1 - 2)
>    let keyset = win1.document.getElementById(keysetID);
> -  is(keyset.childNodes.length, 2, "Expected keyset to exist and have 2 children");
> +  is(keyset != null, true, "Expected keyset to exist");
> +  is(keyset.childNodes.length, 2, "Expected keyset to have 2 children");
>  
>    keyset = win2.document.getElementById(keysetID);
> -  is(keyset.childNodes.length, 2, "Expected keyset to exist and have 2 children");
> +  is(keyset != null, true, "Expected keyset to exist");

`ok(keyset != null, ...)`

::: browser/components/extensions/ext-pageAction.js:138
(Diff revision 2)
> +   * Triggers this page action for the given window, with the same effects as
> +   * if it were clicked by a user.
> +   *
> +   * This has no effect if the page action is hidden for the selected tab.
> +   */
> +  triggerAction: Task.async(function* (window) {

`Task.async` isn't necessary here.

::: browser/components/extensions/ext-pageAction.js:207
(Diff revision 2)
>      pageAction: {
>        onClicked: new EventManager(context, "pageAction.onClicked", fire => {
>          let listener = (evt, tab) => {
>            fire(TabManager.convert(extension, tab));
>          };
> -        let pageAction = PageAction.for(extension);
> +        let pageAction = pageActionOf(extension);

Please keep `PageAction.for` here. If we're going to change one of these for consistency, I'd rather change `BrowserAction`.

::: browser/components/extensions/test/browser/browser_ext_commands_execute_page_action.js:26
(Diff revision 2)
> +    },
> +
> +    background: function() {
> +      let isShown = false;
> +
> +      browser.commands.onCommand.addListener((message) => {

This isn't really a message. Maybe 'command' or 'commandName`.

::: browser/components/extensions/test/browser/browser_ext_commands_execute_page_action.js:31
(Diff revision 2)
> +      browser.commands.onCommand.addListener((message) => {
> +        if (message == "_execute_page_action") {
> +          browser.test.fail(`The onCommand listener should never fire for ${message}.`);
> +        }
> +
> +        if (message == "send-keys-command") {

`else if`

::: browser/components/extensions/test/browser/browser_ext_commands_execute_page_action.js:135
(Diff revision 2)
> +  extension.onMessage("send-keys", () => {
> +    EventUtils.synthesizeKey("j", {altKey: true, shiftKey: true});
> +    EventUtils.synthesizeKey("3", {altKey: true, shiftKey: true});
> +  });
> +
> +  yield extension.awaitFinish("page-action");

Please use a different name for `awaitFinish` in each task.

Comment 8

[Matthew Wein [:mattw]]

Comment on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38379/diff/2-3/

Comment 9

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/38379/#review36381

> Why is this check necessary?

Removed.

> No need for `global.`

Interesting.  Updated.

> `ok(keyset != null, ...)`

Updated.

> `ok(keyset != null, ...)`

Updated.

> `Task.async` isn't necessary here.

Okay, I removed this.  Is it only needed if the method does asynchronous tasks?

> Please keep `PageAction.for` here. If we're going to change one of these for consistency, I'd rather change `BrowserAction`.

Sgtm.  Done.

> This isn't really a message. Maybe 'command' or 'commandName`.

Done.

> `else if`

Done.

> Please use a different name for `awaitFinish` in each task.

Done.

Comment 10

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/38379/#review36381

> Okay, I removed this.  Is it only needed if the method does asynchronous tasks?

For functions that need to wait for a promise to resolve at some point, yes.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

https://reviewboard.mozilla.org/r/38379/#review36599

Comment 12

[Matthew Wein [:mattw]]

(In reply to Kris Maglione [:kmag] from comment #10)
> https://reviewboard.mozilla.org/r/38379/#review36381
> 
> > Okay, I removed this.  Is it only needed if the method does asynchronous tasks?
> 
> For functions that need to wait for a promise to resolve at some point, yes.

Okay, thanks.

Comment 13

[Carsten Book [:Tomcat]]

has problems to apply:

apply changeset? [ynmpcq?]: y
applying 7b87ea9bc9a1
patching file browser/components/extensions/test/browser/browser.ini
Hunk #1 FAILED at 17
1 out of 1 hunks FAILED -- saving rejects to file browser/components/extensions/test/browser/browser.ini.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Tomcats-MacBook-Pro-2:fx-team Tomcat$

Comment 14

[Matthew Wein [:mattw]]

Comment on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38379/diff/3-4/

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/70960616621c

Comment 16

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8096042&repo=fx-team

Comment 17

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/3d37c7e5b8dd

Comment 18

[Matthew Wein [:mattw]]

 > No need for `global.`
Are you sure I don't need global here? This failed on try because `pageActionFor` was undefined.

I have it defined in ext-pageAction.js as `global.pageActionFor = PageAction.for;` and I'm using it in ext-commands.js like so: `pageActionFor(this.extension).triggerAction(win);`.

Comment 19

[Kris Maglione [:kmag]]

(In reply to Matthew Wein [:mattw] from comment #18)
>  > No need for `global.`
> Are you sure I don't need global here? This failed on try because
> `pageActionFor` was undefined.
> 
> I have it defined in ext-pageAction.js as `global.pageActionFor =
> PageAction.for;` and I'm using it in ext-commands.js like so:
> `pageActionFor(this.extension).triggerAction(win);`.

Yes, I'm sure. `global` is the global scope that's common to all of the extension scripts.

It's only failing ESLint, which is only because "pageActionFor" needs to be added to the list of globals in our .eslintrc

Comment 20

[Matthew Wein [:mattw]]

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

Comment 21

[Matthew Wein [:mattw]]

Ah okay, thanks!

Comment 22

[Matthew Wein [:mattw]]

Comment on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38379/diff/4-5/

Comment 23

[Pulsebot]

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

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4a92742ac20d

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Follow-up push to add missing quote character to .eslintrc: https://hg.mozilla.org/integration/fx-team/rev/4a92742ac20d

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for OS X 10.6 opt M-e10s(bc2) failure in a modified test.

Backout: https://hg.mozilla.org/integration/fx-team/rev/d4ccb3062261

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=28a35458f825
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8250022&repo=fx-team

14:38:25     INFO -  61 INFO Console message: [JavaScript Error: "TypeError: can't access dead object" {file: "resource://gre/modules/ExtensionUtils.jsm" line: 214}]
14:38:25     INFO -  62 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_commands_execute_page_action.js | Test timed out -
14:38:25     INFO -  Not taking screenshot here: see the one that was previously logged
14:38:25     INFO -  63 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_commands_execute_page_action.js | message queue is empty - Got 1, expected 0
14:38:25     INFO -  Stack trace:
14:38:25     INFO -      chrome://mochikit/content/browser-test.js:test_is:967
14:38:25     INFO -      chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:24
14:38:25     INFO -      chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest<:466
14:38:25     INFO -      timeoutFn@chrome://mochikit/content/browser-test.js:873:9
14:38:25     INFO -      setTimeout handler*Tester_execTest@chrome://mochikit/content/browser-test.js:835:9
14:38:25     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:711:7
14:38:25     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:740:59
14:38:25     INFO -  Not taking screenshot here: see the one that was previously logged
14:38:25     INFO -  64 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_commands_execute_page_action.js | Extension left running at test shutdown -
14:38:25     INFO -  Stack trace:
14:38:25     INFO -      chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:88
14:38:25     INFO -      chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest<:466
14:38:25     INFO -      timeoutFn@chrome://mochikit/content/browser-test.js:873:9
14:38:25     INFO -      setTimeout handler*Tester_execTest@chrome://mochikit/content/browser-test.js:835:9
14:38:25     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:711:7
14:38:25     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:740:59

Comment 27

[Matthew Wein [:mattw]]

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

Comment 28

[Matthew Wein [:mattw]]

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

Comment 29

[Matthew Wein [:mattw]]

Okay, it looks like it was an e10s issue, and moving the `extension.startup()` in the tests to after the `extension.onMessage` listener appears to fix it.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3357cb502d0c

Comment 31

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/3f54e19a24326db7c75d49ee000d53db6b628c83
Bug 1246035: Follow-up: Fix ESLint bustage. r=bustage

Comment 32

[Matthew Wein [:mattw]]

Comment on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38379/diff/5-6/

Comment 33

[Matthew Wein [:mattw]]

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

Comment 34

[Matthew Wein [:mattw]]

^^ e10s fix

Comment 35

[Matthew Wein [:mattw]]

Comment on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38379/diff/6-7/

Comment 36

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

Review commit: https://reviewboard.mozilla.org/r/42841/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42841/

Comment 37

[Kris Maglione [:kmag]]

Comment on attachment 8735546 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

https://reviewboard.mozilla.org/r/42841/#review39293

Comment 38

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/a66e1fb0320678f5d2acb2b8712a4133104d4b45
Bug 1246035: Add support for _execute_page_action r=kmag

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3357cb502d0c
https://hg.mozilla.org/mozilla-central/rev/3f54e19a2432
https://hg.mozilla.org/mozilla-central/rev/a66e1fb03206