Closed Bug 1267402 Opened 5 years ago Closed 5 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

Unspecified
Android
defect
Not set
normal

Tracking

(firefox49 fixed, firefox56 verified)

VERIFIED FIXED
mozilla49
Tracking Status
firefox49 --- fixed
firefox56 --- verified

People

(Reporter: mattw, Assigned: mattw)

References

Details

(Whiteboard: [pageAction]triaged)

Attachments

(3 files)

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`).
Whiteboard: triaged → [pageAction]triaged
Assignee: nobody → mwein
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 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.
Attachment #8752508 - Flags: review?(kmaglione+bmo) → review+
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/
Attachment #8752508 - Flags: review?(margaret.leibovic)
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 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.
Attachment #8752508 - Flags: review?(margaret.leibovic) → review+
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/
Thanks Margaret
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ccc23c5390ae
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Flags: needinfo?(cosmin.badescu)
Attached image 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.
Flags: needinfo?(cosmin.badescu)
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.