1278685 - Add a test to prove that removing all items from a context menu does not break context menus for other extensions

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Bob Silverberg [:bsilverberg]]

This is a test for the fix described in bug 1274877. It should load two extensions both of which have context menus, then remove all items from the context menu of one extension and ensure that the context menu from the other extension still displays items as expected.

Comment 1

[Tomislav Jovanovic :zombie]

Attachment: bug 1278685 - test contextMenus.removeAll with two extensions,

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

Comment 2

[Tomislav Jovanovic :zombie]

Loading two test extensions at the same time still doesn't work nicely withing browser/ mochitests, so I used makeAndInstallXPI() from another test file, generalized slightly and moved into head.js.

Also included some code cleanup, like DRY constants and avoiding console warnings spam. I know bundling unrelated changes is not ideal, but this is all test code, so I guess it's fine.

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8795100 [details]
bug 1278685 - test contextMenus.removeAll with two extensions,

https://reviewboard.mozilla.org/r/81268/#review81450

Sorry for the slow reply, this fell through the cracks.
makeAndInstallXPI() is a clunky old thing that I don't think we should be encouraging, I'd rather see this test just use ExtensionTestUtils.loadExtension().
I realize there's some work to do to make that support multiple extensions being loaded simultaneously but lets just do that first.
I can work on those changes or I'd be happy to help you if you're interesting in doing them.

Comment 4

[Tomislav Jovanovic :zombie]

(In reply to Andrew Swan [:aswan] from comment #3)
> I can work on those changes or I'd be happy to help you if you're
> interesting in doing them.

Kris wanted bug 1278687, un-assigning until that is fixed.

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8795100 [details]
bug 1278685 - test contextMenus.removeAll with two extensions,

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

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8795100 [details]
bug 1278685 - test contextMenus.removeAll with two extensions,

https://reviewboard.mozilla.org/r/81268/#review82696

Thanks, this is good to go after addressing the one comment below.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:273
(Diff revision 2)
> +  const tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE);
> +  const manifest = {permissions: ["contextMenus"]};
> +
> +  const first = ExtensionTestUtils.loadExtension({manifest, background() {
> +    browser.contextMenus.create({title: "alpha", contexts: ["all"]});
> +    browser.test.notifyPass();

notifyPass() is meant to indicate that an individual test is finished.  Can you replace it with browser.test.sendMessage("ready") and replace awaitFinish() below with awaitMessage("ready")

You may not need it at all, the promise from startup() won't resolve until the background page has run, I think leaving it out altogether would be equivalent to what you have now, but isn't there a potential race here since contextMenus.create() is async?

Comment 7

[Tomislav Jovanovic :zombie]

Comment on attachment 8795100 [details]
bug 1278685 - test contextMenus.removeAll with two extensions,

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

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8795100 [details]
bug 1278685 - test contextMenus.removeAll with two extensions,

https://reviewboard.mozilla.org/r/81268/#review82696

> notifyPass() is meant to indicate that an individual test is finished.  Can you replace it with browser.test.sendMessage("ready") and replace awaitFinish() below with awaitMessage("ready")
> 
> You may not need it at all, the promise from startup() won't resolve until the background page has run, I think leaving it out altogether would be equivalent to what you have now, but isn't there a potential race here since contextMenus.create() is async?

I was going by the code from the tests above: 
http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus.js#23

Anyway, in that case I don't think there is race potential here. Messages are guaranteed to be processed in order, and there are at least a few between the create() message, and when ext-contextMenus.js code gets called to populate the XUL menu:
 - openContextMenu() does a round-trip of messages through the content process,
 - startup() resolving only after background is done probably means a message (or sure will, once it moves to content process).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a86e65bfc8b8
test contextMenus.removeAll with two extensions, r=aswan

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a86e65bfc8b8