Closed Bug 1246035 Opened 4 years ago Closed 4 years ago

[commands] Add support for _execute_page_action

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.2 - Apr 4
Tracking Status
firefox48 --- fixed

People

(Reporter: mattw, Assigned: mattw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [commands]triaged)

Attachments

(2 files, 1 obsolete file)

No description provided.
Whiteboard: [commands]triaged
Assignee: nobody → mwein
Summary: Add support for _execute_page_action → [commands] Add support for _execute_page_action
Iteration: --- → 47.2 - Feb 22
Iteration: 47.2 - Feb 22 → ---
Iteration: --- → 48.1 - Mar 21
Attachment #8727127 - Attachment is obsolete: true
Attachment #8727127 - Flags: review?(kmaglione+bmo)
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.
Attachment #8727137 - Flags: review?(kmaglione+bmo)
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/
Attachment #8727137 - Flags: review?(kmaglione+bmo)
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.
Attachment #8727137 - Flags: review?(kmaglione+bmo)
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.
Attachment #8727137 - Flags: review?(kmaglione+bmo)
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/
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.
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 on attachment 8727137 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

https://reviewboard.mozilla.org/r/38379/#review36599
Attachment #8727137 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
(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.
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$
Flags: needinfo?(mwein)
Keywords: checkin-needed
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/
Flags: needinfo?(mwein)
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8096042&repo=fx-team
Flags: needinfo?(mwein)
Iteration: 48.1 - Mar 21 → ---
Iteration: --- → 48.2 - Apr 4
 > 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);`.
Flags: needinfo?(mwein)
(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
Ah okay, thanks!
Attachment #8727137 - Attachment description: MozReview Request: Bug 1246035 - Add support for _execute_page_action r?kmag → MozReview Request: Bug 1246035 - Add support for _execute_page_action r=kmag
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/
Keywords: checkin-needed
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
Flags: needinfo?(mwein)
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.
Flags: needinfo?(mwein)
Keywords: checkin-needed
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/
Attachment #8727137 - Attachment description: MozReview Request: Bug 1246035 - Add support for _execute_page_action r=kmag → MozReview Request: Bug 1246035 - Add support for _execute_page_action r=kmag try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell -n
^^ e10s fix
Keywords: checkin-needed
Keywords: checkin-needed
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/
Attachment #8727137 - Attachment description: MozReview Request: Bug 1246035 - Add support for _execute_page_action r=kmag try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell -n → MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag
Attachment #8735546 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8735546 [details]
MozReview Request: Bug 1246035: Add support for _execute_page_action r=kmag

https://reviewboard.mozilla.org/r/42841/#review39293
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.