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

I think this should do what you need. Let me know if it doesn't.

Comment on attachment 8727124 [details] MozReview Request: Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. r?Gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38367/diff/1-2/

Comment on attachment 8727124 [details] MozReview Request: Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. r?Gijs https://reviewboard.mozilla.org/r/38367/#review35289 I'm not entirely sure why I was flagged for review, and what this bug or patch is about, because there are 0 comments with content in the bug, besides "I think this should do what you need. Let me know if it doesn't.", which I think is aimed at the reporter rather than me. So I'm just stabbing in the dark in this review. It would have been helpful if there was at least some context included. As it is, I have too many questions / criticisms to give you r+ as-is. ::: browser/components/extensions/ext-browserAction.js:25 (Diff revision 2) > +global.browserActionOf = browserActionOf; Why is this necessary? I'm fairly sure that this is available on the backstagepass that Cu.import gives you anyway... For instance, running: Cu.import("resource:///modules/CustomizableWidgets.jsm") in the browser console gives me access to all the functions declared at toplevel, like setAttributes and a number of other ones... ::: browser/components/extensions/ext-browserAction.js:129 (Diff revision 2) > + let popup = ViewPopup.for(this.extension, window); I don't know the code here well enough as to know what is prevailing organization style, but I'm surprised a little here because it seems to imply: 1) you can only have one popup per add-on per window, even if you have multiple widgets that use a popup of some sort. It seems from the code you can only have one browseraction per add-on, which seems like an odd limitation but one I assume is present in Chrome/Opera/Edge as well, or something? Even so, I'm guessing browserActions aren't the only things that can use popups... 2) we keep track of these statically as part of the popups object, rather than having the add-on keep a list of all the popups it controls (which would make more sense to me, and would mean you wouldn't need a weakmap because you can just kill the list when you kill the add-on object). It's also not really clear how this relates to the browseraction instance having the "popup" property for this tab. Same kind of popup? Different kind of popup? It also seems like the docstring comment above this method does not mention the codepath where this is truthy, the if block is executed and we return early. ::: browser/components/extensions/ext-browserAction.js:147 (Diff revision 2) > + let event = new window.CustomEvent("command", {bubbles: true, cancelable: true}); > + widget.node.dispatchEvent(event); > + } else { > + this.emit("click"); Can you add a comment as to why these are distinct? That is, why isn't sending a new CustomEvent OK for non-popup items, or emitting a click for the popup items? It generally seems a bit weird that you'd have an API that behaves quite differently for a popup button and a normal one, in that the popup ones will expand the PanelUI (and potentially not close it? Hard to tell from the code here...), while the normal buttons will not do that. Should triggerAction even work for popup type actions? ::: browser/components/extensions/ext-utils.js:313 (Diff revision 2) > + * WeakMap[window -> WeakMap[Extension -> BasePopup]] What is Extension here and can you actually have weak references to them? AIUI it's still impossible to actually implement a weak ref in JS, so this seems suspect. Of course, the window is a real weak ref so if the window is closed, everything else goes away too, but that still means that if I disabled an add-on while the window remained open this might leak.

https://reviewboard.mozilla.org/r/38367/#review35289 Ah, sorry, I thought more of this conversation had happened in the bug. Matt is working on implementing the commands (keyboard shortcuts API). One of the things that API has to be able to do is trigger browser actions. Since triggering CUI popups has a lot of edge cases, I offered to implement a helper function for him to handle that part of it. I flagged you for review because I think you're the person best qualified to review it at this point. Matt is too new, and I don't know the CUI/PanelUI code quite well enough to be confident I'm not doing something too dodgy. > Why is this necessary? I'm fairly sure that this is available on the backstagepass that Cu.import gives you anyway... > > For instance, running: > > Cu.import("resource:///modules/CustomizableWidgets.jsm") > > in the browser console gives me access to all the functions declared at toplevel, like setAttributes and a number of other ones... These modules are implemented in a kind of strange way. They're all loaded into into the Extension.jsm scope, but into a sub-object, via the subscript loader. The `global` global is the actual Extension.jsm global, so properties assigned to it are available to all modules, and on the Extension.jsm global. Ordinary globals aren't. I know... it's definitely not ideal. I've been thinking about replacing it with something easier to follow for a while now, but there have been other priorities. > I don't know the code here well enough as to know what is prevailing organization style, but I'm surprised a little here because it seems to imply: > 1) you can only have one popup per add-on per window, even if you have multiple widgets that use a popup of some sort. It seems from the code you can only have one browseraction per add-on, which seems like an odd limitation but one I assume is present in Chrome/Opera/Edge as well, or something? Even so, I'm guessing browserActions aren't the only things that can use popups... > 2) we keep track of these statically as part of the popups object, rather than having the add-on keep a list of all the popups it controls (which would make more sense to me, and would mean you wouldn't need a weakmap because you can just kill the list when you kill the add-on object). > > It's also not really clear how this relates to the browseraction instance having the "popup" property for this tab. Same kind of popup? Different kind of popup? > > It also seems like the docstring comment above this method does not mention the codepath where this is truthy, the if block is executed and we return early. Well, the popups are auto-close, so in theory it should only be possible to have one open at a time. As for the limitations, yes, it comes from the Chrome API. They used to allow multiple browser actions and page actions per add-on, before they instituted a policy that all add-ons should do only one, simple thing, and changed the API to match (so now their add-ons can have only one browserAction, one pageAction, or one "app"). I think we could theoretically let add-ons have both a pageAction popup and a browserAction popup open in the same window, at the same time (since we actually allow add-ons to have one of each), but that's a corner case I don't want to deal with for now. I suspect it would cause problems I haven't thought of yet, so I decided to go with the simpler solution of only allowing one popup per add-on, per window. The popup property is the URL of the popup. This is basically a reimplementation of the logic that handles toolbar button clicks. If there's a popup attribute, a panel opens with that URL. Otherwise we dispatch a click event. I couldn't think of a clean way to implement this that shared the logic between those two code paths, so I just made sure it was tested well. I guess I forgot to update the docstring after I updated the patch to add the panel closing logic. When Matt tested this, he noticed that the original version was adding multiple browsers to the PanelUI view when he repeatedly pressed the trigger key, so I updated it to close the panel if we triggered it while it was already open. > Can you add a comment as to why these are distinct? That is, why isn't sending a new CustomEvent OK for non-popup items, or emitting a click for the popup items? > > It generally seems a bit weird that you'd have an API that behaves quite differently for a popup button and a normal one, in that the popup ones will expand the PanelUI (and potentially not close it? Hard to tell from the code here...), while the normal buttons will not do that. > > Should triggerAction even work for popup type actions? Yeah, I should have copied some of the comments from the other path. I'd have liked to just dispatch a click or command event to the toolbar button node, and have the normal click logic handle it, but that's not going to work well when it's in a hidden panel. When there's a popup defined, we need to open a panel for that popup (possibly opening the menu panel first), which dispatching a "command" event accomplishes. When there isn't, we need to dispatch a synthetic click event to the extension code. Just sending the "command" event would accomplish both for the normal case, but for a button in the menu, having the menu pop up when we only mean to dispatch a synthetic click event isn't desirable. > What is Extension here and can you actually have weak references to them? AIUI it's still impossible to actually implement a weak ref in JS, so this seems suspect. Of course, the window is a real weak ref so if the window is closed, everything else goes away too, but that still means that if I disabled an add-on while the window remained open this might leak. It's an instance of the Extension object defined in Extension.jsm. We use them as keys in most of our WeakMaps to handle extension-specific data (though we generally explicitly clear them when the extension shuts down). WeakMaps can handle all ordinary JS objects reliably. Actually, they can *only* really handle ordinary JS objects reliably. For wrapped natives, they can currently only handle WebIDL objects, which have to explicitly preserve their JS wrapper when they're added as keys. Alas, I recently spent several days working in that code to add support for MessageManagers as keys, so I know this subject much better than I'd like.

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

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

(I'll try to get to this on Monday.)

Comment on attachment 8769366 [details] Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. https://reviewboard.mozilla.org/r/63308/#review60308 r- because of the popup closure case (see below). If I'm right, it would be great to have a test for that case, too. ::: browser/components/extensions/ext-browserAction.js:132 (Diff revision 1) > + * > + * This has no effect if the browser action is disabled for, or not > + * present in, the given window. > + */ > + triggerAction: Task.async(function* (window) { > + let popup = ViewPopup.for(this.extension, window); Please can we change this so we don't use a reserved keyword like 'for'? It mis-highlights and breaks tools, even if by the letter of ES6 it might be OK to do this. `popupOf` or similar seems like it would do. ::: browser/components/extensions/ext-browserAction.js:133 (Diff revision 1) > + if (popup) { > + popup.closePopup(); > + return; > + } I believe this is wrong. You're closing a popup but you've not verified that the popup actually matches the widget whose action you're triggering. In particular, my reading of comment 5 seems to suggest that an add-on could have a page and a browser action, one with and one without a popup (with debate about whether both could have a popup). So there might be a popup for one while we're triggering the other, in which case this code does the wrong thing. Generally it seems too simplistic - if we need a map, why don't we map from widgets to popups? ::: browser/components/extensions/ext-browserAction.js:145 (Diff revision 1) > + > + if (!(widget && this.getProperty(tab, "enabled"))) { > + return; > + } > + > + if (this.getProperty(tab, "popup")) { This block should have a comment along the lines of the reply Kris gave me in comment 5 so future readers of this code understand why there are two paths here. Bonus points if you explain *why* "having the menu pop up when we only mean to dispatch a synthetic click event isn't desirable" ::: browser/components/extensions/ext-browserAction.js:146 (Diff revision 1) > + if (this.widget.areaType == CustomizableUI.TYPE_MENU_PANEL) { > + yield window.PanelUI.show(); > + } > + > + let event = new window.CustomEvent("command", {bubbles: true, cancelable: true}); > + widget.node.dispatchEvent(event); IME in some edgecases on Linux tests we've seen the panelUI popup close when it's being automatically opened, meaning that by the time we return from the yield the popup is no longer open. To be 100% sure it is open while you run the command event, you'd need to use an actual event listener for `popupshown`, as well as checking for `popuphidden` which would fire if `popupshowing` is fired but the popup then closes before it's fully shown. Up to you whether you want to care about this or not, I guess. ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:179 (Diff revision 1) > let panel = getBrowserActionPopup(extension); > ok(panel, "Expect panel to exist"); > yield promisePopupShown(panel); > > extension.sendMessage("close-popup"); > > yield promisePopupHidden(panel); > ok(true, "Panel is closed"); I realize you're simply keeping this code as-is, but it's not clear to me why the code is showing and then hiding the panel. Feels like it should just assert that it's hidden. ::: toolkit/components/extensions/ExtensionUtils.jsm:120 (Diff revision 1) > -// |get| if a key is not present. > -function DefaultWeakMap(defaultValue) { > - this.defaultValue = defaultValue; > - this.weakmap = new WeakMap(); > + * Similar to a WeakMap, but creates a new key with the given > + * constructor if one is not present. > + */ > +class DefaultWeakMap extends WeakMap { > + constructor(defaultConstructor) { > + super(); Seems like you should make the constructor take the iterable argument the WeakMap constructor takes, and forward it to the superclass constructor when calling it so DefaultWeakMap can also be instantiated with an iterable like WeakMap.

https://reviewboard.mozilla.org/r/63308/#review60308 > Please can we change this so we don't use a reserved keyword like 'for'? It mis-highlights and breaks tools, even if by the letter of ES6 it might be OK to do this. > > `popupOf` or similar seems like it would do. This is the convention in WebExtension code for the moment. I was on the fence about using the reserved word, but decided to go for it because things like `Array.of` have become common in ES6 APIs. > I realize you're simply keeping this code as-is, but it's not clear to me why the code is showing and then hiding the panel. Feels like it should just assert that it's hidden. `promisePopupHidden` doesn't close the popup, it just resolves when the popup has been successfully hidden. > Seems like you should make the constructor take the iterable argument the WeakMap constructor takes, and forward it to the superclass constructor when calling it so DefaultWeakMap can also be instantiated with an iterable like WeakMap. +1

Comment on attachment 8769367 [details] Bug 1246034: Part 2 - [webext] Add support for _execute_browser_action. https://reviewboard.mozilla.org/r/63310/#review60396 ::: browser/components/extensions/.eslintrc:6 (Diff revision 1) > { > "extends": "../../../toolkit/components/extensions/.eslintrc", > > "globals": { > "AllWindowEvents": true, > + "browserActionFor": true, Please correct indentation, and expand tabs. ::: browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js:5 (Diff revision 1) > +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* vim: set sts=2 sw=2 et tw=80: */ > +"use strict"; > + > +add_task(function* test_execute_page_action_without_popup() { This function name is inaccurate. ::: browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js:59 (Diff revision 1) > + extension.onMessage("send-keys", () => { > + EventUtils.synthesizeKey("j", {altKey: true, shiftKey: true}); > + }); > + > + yield extension.startup(); > + yield extension.awaitFinish("browser-action-popup"); This is missing some things: 1) We also need to test this when the browser action is in the hamburger menu. 2) We need to close the popup every time we open it. 3) We need to test this when we have a click listener rather than a popup.

https://reviewboard.mozilla.org/r/63308/#review60308 > `promisePopupHidden` doesn't close the popup, it just resolves when the popup has been successfully hidden. But presumably the `sendMessage("close-popup")` closes it, right? Why does the code call `promisePopupShown` and/or show it, then hide it? `expecting.expectClosed` sounds like it expects the popup is closed at the point where this code runs. Why doesn't it just verify the popup is closed, rather than showing and then hiding it?

https://reviewboard.mozilla.org/r/63308/#review60308 > But presumably the `sendMessage("close-popup")` closes it, right? Why does the code call `promisePopupShown` and/or show it, then hide it? `expecting.expectClosed` sounds like it expects the popup is closed at the point where this code runs. Why doesn't it just verify the popup is closed, rather than showing and then hiding it? `expectClosed` should probably renamed. It made more sense in a previous version of this code. This tests that the popup can close itself with `window.close()`, so once the popup has opened, we ask it to close itself, and then verify that it worked.

Comment on attachment 8769366 [details] Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63308/diff/1-2/

Comment on attachment 8769367 [details] Bug 1246034: Part 2 - [webext] Add support for _execute_browser_action. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63310/diff/1-2/

https://reviewboard.mozilla.org/r/63308/#review60308 > I believe this is wrong. You're closing a popup but you've not verified that the popup actually matches the widget whose action you're triggering. In particular, my reading of comment 5 seems to suggest that an add-on could have a page and a browser action, one with and one without a popup (with debate about whether both could have a popup). So there might be a popup for one while we're triggering the other, in which case this code does the wrong thing. Generally it seems too simplistic - if we need a map, why don't we map from widgets to popups? Since the BrowserAction code usings ViewPopups and PageAction uses PanelPopups, I thought I'd be able to simply update it so ViewPopup.for only accesses the ViewPopup instances. Since each extension can only have one BrowerAction, this should fix the issue. However, it doesn't seem to work nicely - it causes several of the tests in browser_ext_browserAction_popup.js to fail. I'll look into this some more in addition to adding support to map widgets to popoups. > This block should have a comment along the lines of the reply Kris gave me in comment 5 so future readers of this code understand why there are two paths here. Bonus points if you explain *why* "having the menu pop up when we only mean to dispatch a synthetic click event isn't desirable" The behavior is defined in the documentation for browserAction.onClicked here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/BrowserAction/onClicked I added a comment to the code and referenced this. > `expectClosed` should probably renamed. It made more sense in a previous version of this code. This tests that the popup can close itself with `window.close()`, so once the popup has opened, we ask it to close itself, and then verify that it worked. I renamed `expectedClosed` and the message to reflect what's being tested more clearly. > +1 Sgtm. I added support for this in the latest revision.

Comment on attachment 8769366 [details] Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63308/diff/2-3/

Comment on attachment 8769367 [details] Bug 1246034: Part 2 - [webext] Add support for _execute_browser_action. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63310/diff/2-3/

Comment on attachment 8769367 [details] Bug 1246034: Part 2 - [webext] Add support for _execute_browser_action. https://reviewboard.mozilla.org/r/63310/#review66930 Very close. Just a few nits at this point. ::: browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js:22 (Diff revision 3) > + "browser_style": true, > + }, > + }; > + > + if (options.withPopup) { > + extensionOptions.manifest["browser_action"]["default_popup"] = "popup.html"; `manifest.browser_action.default_popup` ::: browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js:27 (Diff revision 3) > + extensionOptions.manifest["browser_action"]["default_popup"] = "popup.html"; > + > + extensionOptions.files = { > + "popup.html": ` > + <!DOCTYPE html> > + <html><body> `<head><meta charset="utf-8"></head>` And the script node should be in the head, not the body. ::: browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js:48 (Diff revision 3) > + } > + }); > + > + browser.browserAction.onClicked.addListener(() => { > + if (withPopup) { > + browser.test.fail("The onClick listener should never fire if the browserAction has a popup."); This needs a `notifyFail` fall, so we don't get timeouts if it fails. ::: browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js:62 (Diff revision 3) > + } > + }); > + > + browser.test.sendMessage("send-keys"); > + }); > + browser.test.sendMessage("ready"); This isn't necessary anymore. ::: browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js:90 (Diff revision 3) > +add_task(function* test_execute_browser_action_with_popup() { > + yield testExecuteBrowserActionWithOptions(); > +}); > + > +add_task(function* test_execute_browser_action_without_popup() { > + yield testExecuteBrowserActionWithOptions(); > +}); I think you forgot to add `withPopup` options to these tests.

Comment on attachment 8769366 [details] Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. https://reviewboard.mozilla.org/r/63308/#review66922 ::: browser/components/extensions/ext-browserAction.js:13 (Diff revision 3) > XPCOMUtils.defineLazyGetter(this, "colorUtils", () => { > return require("devtools/shared/css-color").colorUtils; > }); > > + > +Cu.import("resource://gre/modules/Task.jsm"); nit, don't need to add a blank line above this and this can be moved down to be after the ExtensionUtils.jsm import to keep them somewhat grouped and ordered. ::: browser/components/extensions/ext-browserAction.js:109 (Diff revision 3) > > // If the widget has a popup URL defined, we open a popup, but do not > // dispatch a click event to the extension. > // If it has no popup URL defined, we dispatch a click event, but do not > // open a popup. > + // @see https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/BrowserAction/onClicked These URLs are not guaranteed to exist in a couple years while the code may live much longer. Instead of putting these URLs in the code, can you adjust the comment to explain why this is done? Also, the comment above this line basically describes what the code is doing, and doesn't add much value since it's pretty obvious from just reading the code. How about using this for the comment instead: // Popups are shown only if a popup URL is defined // and click events are dispatched otherwise. This // is done for compatibility with the Google Chrome // onClicked extension API. ::: browser/components/extensions/ext-browserAction.js:149 (Diff revision 3) > + } > + > + let widget = this.widget.forWindow(window); > + let tab = window.gBrowser.selectedTab; > + > + if (!(widget && this.getProperty(tab, "enabled"))) { I find this confusing. Can you instead propagate the negation through the condition? `if (!widget || !this.getProperty(tab, "enabled"))` ::: browser/components/extensions/ext-browserAction.js:153 (Diff revision 3) > + // The "click" event will not fire if the browser action has a popup. > + // @see https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/BrowserAction/onClicked As mentioned earlier, this comment may "break" in the future. ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:173 (Diff revision 3) > extension.onMessage("next-test", Task.async(function* (expecting = {}) { > if (!widget) { > widget = getBrowserActionWidget(extension); > CustomizableUI.addWidgetToArea(widget.id, area); > } > - if (expecting.expectClosed) { > + if (expecting.awaitClosed) { The usage of 'await' here is still confusing, as it is sometimes used to refer to promises. Can you switch this to 'shouldCloseAutomatically' or 'shouldCloseItself'?

Comment on attachment 8769366 [details] Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. https://reviewboard.mozilla.org/r/63308/#review66922 > These URLs are not guaranteed to exist in a couple years while the code may live much longer. Instead of putting these URLs in the code, can you adjust the comment to explain why this is done? > > Also, the comment above this line basically describes what the code is doing, and doesn't add much value since it's pretty obvious from just reading the code. > > How about using this for the comment instead: > > // Popups are shown only if a popup URL is defined > // and click events are dispatched otherwise. This > // is done for compatibility with the Google Chrome > // onClicked extension API. Sgtm, thanks.

Comment on attachment 8769367 [details] Bug 1246034: Part 2 - [webext] Add support for _execute_browser_action. https://reviewboard.mozilla.org/r/63310/#review68554 ::: browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js:29 (Diff revisions 3 - 4) > extensionOptions.files = { > "popup.html": ` > <!DOCTYPE html> > - <html><body> > + <html> > + <head> > - <script src="popup.js"></script> > + <script src="popup.js"></script> You forgot the <meta charset="utf-8">

Comment on attachment 8769366 [details] Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. https://reviewboard.mozilla.org/r/63308/#review60308 > Since the BrowserAction code usings ViewPopups and PageAction uses PanelPopups, I thought I'd be able to simply update it so ViewPopup.for only accesses the ViewPopup instances. Since each extension can only have one BrowerAction, this should fix the issue. However, it doesn't seem to work nicely - it causes several of the tests in browser_ext_browserAction_popup.js to fail. I'll look into this some more in addition to adding support to map widgets to popoups. I think this is a rare enough edge case to handle in a follow up bug, but I definitely think it's worth handling. I filed bug 1295276 to track this issue.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8cce25ece209 Part 1: [webext] Add a helper function to trigger a browserAction. r=jaws https://hg.mozilla.org/integration/mozilla-inbound/rev/2c396099a21d Part 2: [webext] Add support for _execute_browser_action. r=kmag

Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f24546c26c ESLint followup a=me CLOSED TREE

Backed out all three in https://hg.mozilla.org/integration/mozilla-inbound/rev/0a9bfe07afa4 for Windows e10s timeouts in browser_ext_commands_execute_browser_action.js, https://treeherder.mozilla.org/logviewer.html#?job_id=33887534&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=33895364&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=33892434&repo=mozilla-inbound etc.

Comment on attachment 8769366 [details] Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. https://reviewboard.mozilla.org/r/63308/#review82954 ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:193 (Diff revision 7) > if (!widget) { > widget = getBrowserActionWidget(extension); > CustomizableUI.addWidgetToArea(widget.id, area); > } > - if (expecting.expectClosed) { > + if (expecting.waitUntilClosed) { > + dump(`L0G HERE`); Please remove.

Comment on attachment 8769366 [details] Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. reviewboard :/

Good catch, thanks

I also fixed 2 eslint errors that I had in browser_ext_commands_execute_browser_action.js

Part 1 has two unresolved issues in MozReview preventing autoland from working.

Comment on attachment 8769366 [details] Bug 1246034: Part 1 - [webext] Add a helper function to trigger a browserAction. https://reviewboard.mozilla.org/r/63308/#review60308 > IME in some edgecases on Linux tests we've seen the panelUI popup close when it's being automatically opened, meaning that by the time we return from the yield the popup is no longer open. > > To be 100% sure it is open while you run the command event, you'd need to use an actual event listener for `popupshown`, as well as checking for `popuphidden` which would fire if `popupshowing` is fired but the popup then closes before it's fully shown. Up to you whether you want to care about this or not, I guess. If I do handle this I'd prefer to do it in a follow up bug - I'll see if I can reproduce it locally and if so I'll file one.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/02d2e07063e2 Part 1 - [webext] Add a helper function to trigger a browserAction. r=jaws https://hg.mozilla.org/integration/autoland/rev/5c2c1a65c458 Part 2 - [webext] Add support for _execute_browser_action. r=kmag

https://hg.mozilla.org/mozilla-central/rev/02d2e07063e2 https://hg.mozilla.org/mozilla-central/rev/5c2c1a65c458

In addition to documenting this event, the incompatibility section needs updating (currently it says Firefox does not support the event) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands#Chrome_incompatibilities_2

I removed that statement from the "Chrome incompatibilities" section but "_execute_browser_section" would need to be added to the compatibility section in order to indicate that Firefox only supports it starting with 52.0. Maybe somebody else wants to create a pull request for https://github.com/mdn/browser-compat-data/ where this data is stored.

I've added a bit on this at: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands#Syntax, and also https://developer.mozilla.org/en-US/Add-ons/WebExtensions/User_interface_components#Popups. Please let me know if this covers it. (In reply to Wladimir Palant from comment #46) > Maybe somebody else wants to create a pull request for > https://github.com/mdn/browser-compat-data/ where this data is stored. Thanks for adding that Wladimir. Unfortunately the JSON data is currently only used for the JS APIs, not for manifest keys,and the "commands" manifest key would be the best place to note this. I have it on my plate to make the manifest keys use the JSON compat data too, but I'm not there yet.

Looks good to me.