Closed
Bug 1246035
Opened 8 years ago
Closed 8 years ago
[commands] Add support for _execute_page_action
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
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.
Updated•8 years ago
|
Whiteboard: [commands]triaged
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mwein
Summary: Add support for _execute_page_action → [commands] Add support for _execute_page_action
Assignee | ||
Updated•8 years ago
|
Iteration: --- → 47.2 - Feb 22
Assignee | ||
Updated•8 years ago
|
Iteration: 47.2 - Feb 22 → ---
Assignee | ||
Updated•8 years ago
|
Iteration: --- → 48.1 - Mar 21
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38375/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38375/
Attachment #8727127 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6827b171d436
Assignee | ||
Updated•8 years ago
|
Attachment #8727127 -
Attachment is obsolete: true
Attachment #8727127 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38379/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38379/
Attachment #8727137 -
Flags: review?(kmaglione+bmo)
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8727137 -
Flags: review?(kmaglione+bmo)
Comment 7•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8727137 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 8•8 years ago
|
||
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/
Assignee | ||
Comment 9•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•8 years ago
|
||
(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•8 years ago
|
||
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
Assignee | ||
Comment 14•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mwein)
Keywords: checkin-needed
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/70960616621c
Keywords: checkin-needed
Comment 16•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8096042&repo=fx-team
Flags: needinfo?(mwein)
Comment 17•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/3d37c7e5b8dd
Assignee | ||
Updated•8 years ago
|
Iteration: 48.1 - Mar 21 → ---
Assignee | ||
Updated•8 years ago
|
Iteration: --- → 48.2 - Apr 4
Assignee | ||
Comment 18•8 years ago
|
||
> 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)
Comment 19•8 years ago
|
||
(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
Assignee | ||
Comment 20•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a3dd2b4d9f7
Assignee | ||
Comment 21•8 years ago
|
||
Ah okay, thanks!
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 22•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 23•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ddf8219182fb
Keywords: checkin-needed
Comment 25•8 years ago
|
||
Follow-up push to add missing quote character to .eslintrc: https://hg.mozilla.org/integration/fx-team/rev/4a92742ac20d
Comment 26•8 years ago
|
||
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)
Assignee | ||
Comment 27•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c69c155d48
Assignee | ||
Comment 28•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=669a22c721d1
Assignee | ||
Comment 29•8 years ago
|
||
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 30•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3357cb502d0c
Keywords: checkin-needed
Comment 31•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3f54e19a24326db7c75d49ee000d53db6b628c83 Bug 1246035: Follow-up: Fix ESLint bustage. r=bustage
Assignee | ||
Comment 32•8 years ago
|
||
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
Assignee | ||
Comment 33•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f216696714d8
Assignee | ||
Comment 34•8 years ago
|
||
^^ e10s fix
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 35•8 years ago
|
||
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
Assignee | ||
Comment 36•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42841/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42841/
Attachment #8735546 -
Flags: review?(kmaglione+bmo)
Updated•8 years ago
|
Attachment #8735546 -
Flags: review?(kmaglione+bmo) → review+
Comment 37•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a66e1fb0320678f5d2acb2b8712a4133104d4b45 Bug 1246035: Add support for _execute_page_action r=kmag
Comment 39•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3357cb502d0c https://hg.mozilla.org/mozilla-central/rev/3f54e19a2432 https://hg.mozilla.org/mozilla-central/rev/a66e1fb03206
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•