1267402 - [PageAction] Add support for chrome.pageAction.onClicked on Android

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Matthew Wein [:mattw]]

This bug is meant to track the implementation and testing of Chrome.pageAction.onClicked on android.

Firefox docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/PageAction/onClicked
Chrome docs: https://developer.chrome.com/extensions/pageAction#event-onClicked

Notes: There isn't an existing method in the PageActions.jsm module that we can wrap, but we should be able to add one by doing the following:
 - Modify the `clickCallback` property `PageActions._items` to store an object of callbacks instead of only one.
 - Add a method to PageActions.jsm for adding a callback. The extension will also need a way to look up the callback it adds, so we will need to add some sort of unique identifier in order to look them up.  One way to do this might be to associate the extension id to the callback.  So, we'd have `PageActions.addCallback(extensionId, pageActionId, callback)`, which would set `PageActions._items[id][extensionId]` to `callback` (the id is returned by `PageActions.add`).

Comment 1

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Bug 1267402 - Implement chrome.pageAction.onClicked on Android r?kmag

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

Comment 2

[Matthew Wein [:mattw]]

Comment on attachment 8752508 [details]
MozReview Request: Bug 1267402 - Implement chrome.pageAction.onClicked on Android r?kmag

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

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8752508 [details]
MozReview Request: Bug 1267402 - Implement chrome.pageAction.onClicked on Android r?kmag

https://reviewboard.mozilla.org/r/52628/#review49572

Looks good to me, but please ask a Fennec peer to review the PageAction.jsm changes.

::: mobile/android/components/extensions/ext-pageAction.js:71
(Diff revision 2)
>  /* eslint-enable mozilla/balanced-listeners */
>  
>  extensions.registerSchemaAPI("pageAction", null, (extension, context) => {
>    return {
>      pageAction: {
> +      onClicked: new EventManager(context, "pageAction.onClicked", fire => {

Please use `SingletonEventManager`

::: mobile/android/components/extensions/ext-pageAction.js:80
(Diff revision 2)
> +        pageActionMap.get(extension).on("click", listener);
> +        return () => {
> +          pageActionMap.get(extension).off("click", listener);
> +        };
> +      }).api(),
>        show(tabId) {

Please add blank line.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:28
(Diff revision 2)
>    browser.pageAction.show(tabId);
>    browser.test.sendMessage("page-action-shown");
>  
> +  browser.pageAction.onClicked.addListener(tab => {
> +    // TODO: Make sure we get the correct tab once the tabs API is supported.
> -  browser.test.notifyPass("page-action");
> +    browser.test.notifyPass("page-action");

Can we send a "pageAction-clicked" message, or something like that, instead?

::: mobile/android/modules/PageActions.jsm:76
(Diff revision 2)
>  
> +  synthesizeClick: function(id) {
> +    let item = this._items[id];
> +    if (item && item.clickCallback) {
> +      item.clickCallback();
> +    }

This should be reviewed by a Fennec peer.

Comment 4

[Matthew Wein [:mattw]]

Comment on attachment 8752508 [details]
MozReview Request: Bug 1267402 - Implement chrome.pageAction.onClicked on Android r?kmag

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

Comment 5

[Matthew Wein [:mattw]]

Margaret, could you review the changes I made to PageActions.jsm? I need a way to synthesize a click on the PageAction for the unit tests and this seems to be the easiest way to do it.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8752508 [details]
MozReview Request: Bug 1267402 - Implement chrome.pageAction.onClicked on Android r?kmag

https://reviewboard.mozilla.org/r/52628/#review49696

Looks fine to me.

Comment 7

[Matthew Wein [:mattw]]

Comment on attachment 8752508 [details]
MozReview Request: Bug 1267402 - Implement chrome.pageAction.onClicked on Android r?kmag

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

Comment 8

[Matthew Wein [:mattw]]

Thanks Margaret

Comment 9

[Pulsebot]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: bug-1267402-pageaction-onclicked-0.1.xpi

Hi Cosmin,
This should also be manually verified (similarly to how we are going to verify browserAction.onClicked for Bug 1331742).

The attached xpi can be used to verify this API using the following STR:

STR:
- install the attached extension xpi
- connect the Firefox for Android instance using the WebIDE (to be able to check that the expected console message has been logged when the browserAction has been clicked)
- create a new tab (because of Bug 1384964 which currently prevents pageAction.show to work as expected on a tab with tabId 0)
- load a regular webpage (e.g. mozilla.org, because pageAction.show currently show the pageAction icon only if a non-privileged page has been loaded in the tab, e.g. an about:blank page or a regular web page) 
- click the pageAction added by the extension
- Expected behavior: the message "bug-1267402-pageAction-onClicked - pageAction clicked" should have been logged in the console panel of the WebIDE window connected to the Firefox for Android instance.

Comment 12

[CosminB]

Attachment: pageAction-onClick.gif

Thanks Luca!

This issue is verified as fixed on Fennec 56.0a1 (2017-07-27), the message "bug-1267402-pageAction-onClicked - pageAction clicked" is displayed in the WebIDE console panel, as described by Luca.

This issue could not be verified on the previous builds, because I am blocked by the Bug 1367494.