Closed Bug 1270742 Opened 6 years ago Closed 5 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

Unspecified
Android
defect
Not set
normal

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Iteration:
49.2 - May 23
Tracking Status
firefox50 --- fixed

People

(Reporter: mattw, Assigned: mattw)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [pageAction]triaged)

Attachments

(1 file)

This bug is meant to track the implementation and testing of default_icon in the chrome.pageAction manifest on android.
Iteration: 49.1 - May 9 → 49.2 - May 23
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/1-2/
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

https://reviewboard.mozilla.org/r/54074/#review51076

This is a good start, but it needs a bit more work to handle corner cases better.

::: mobile/android/components/extensions/ext-pageAction.js:55
(Diff revision 2)
> +      IconDetails.convertImageURLToDataURI(imageURL, contentWindow).then(dataURI => {
> +        this.options.icon = dataURI;
> +        this.id = PageActions.add(this.options);
> +        resolve();
> +      })
> +    });

This would usually be phrased as:

    let imageURL = IconDetails.getURL(this.icons, contentWindow, this.extension);
    return IconDetails.convertImageURLToDataURI(imageURL, contentWindow).then(dataURI => {
      this.options.icon = dataURI;
      this.id = PageActions.add(this.options);
    });

We should try to handle it correctly when people do things like this, though:

    pageAction.show(tabId);
    pageAction.hide(tabId);

The easies thing would probably be to just keep a counter that we bump after
every show/hide call, and only take any action in the callback if the counter
still has the same value by the time it gets called.

::: mobile/android/components/extensions/ext-pageAction.js:102
(Diff revision 2)
>        show(tabId) {
> -        pageActionMap.get(extension).show(tabId);
> +        return new Promise(resolve => {
> +          pageActionMap.get(extension).show(tabId, context.contentWindow).then(() => {
> +            resolve();
> +          });
> +        })

return pageActionMap.get(extension)
                        .show(tabId, context.contentWindow)
                        .then(() => {});

::: mobile/android/components/extensions/schemas/page_action.json:60
(Diff revision 2)
>      "functions": [
>        {
>          "name": "show",
>          "type": "function",
>          "description": "Shows the page action. The page action is shown whenever the tab is selected.",
> +        "async": "callback",

Let's just make this `"async": true`, and leave out the callback argument, since this isn't supported on Chrome. The same goes for `hide()`.

Let me know if you need help adding Schema.jsm support for this.

Also, we need to make the same changes to the browser/ schema.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:55
(Diff revision 2)
>      manifest: {
>        "name": "PageAction Extension",
>        "page_action": {
>          "default_title": "Page Action",
> +        "default_icon": {
> +          "19": "extension.png",

We use 18px and 36px icons, not 19 and 38.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:62
(Diff revision 2)
> +        },
> +      },
> -      },
> +    },
> +    files: {
> +      "extension.png": IMAGE_ARRAYBUFFER,
> +      "a.png": IMAGE_ARRAYBUFFER,

Is this used?

::: toolkit/components/extensions/ExtensionUtils.jsm:451
(Diff revision 2)
> +    const DEFAULT = "chrome://browser/content/extension.svg";
> +
> +    return AddonManager.getPreferredIconURL({icons: icons}, size, window) || DEFAULT;
> +  },
> +
> +  convertImageURLToDataURI(imageURL, contentWindow) {

Can we make this `context` rather than `contentWindow` for parity with `convertImageDataToPNG`?

::: toolkit/components/extensions/ExtensionUtils.jsm:457
(Diff revision 2)
> +    return new Promise(resolve => {
> +      var image = new contentWindow.Image();
> +      image.onload = function () {
> +        var canvas = contentWindow.document.createElement('canvas');
> +        canvas.width = this.width;
> +        canvas.height = this.height;

We should pass in the expected dimensions, and scale the image to match, so that this works correctly with PNGs.

::: toolkit/components/extensions/ExtensionUtils.jsm:460
(Diff revision 2)
> +        var canvas = contentWindow.document.createElement('canvas');
> +        canvas.width = this.width;
> +        canvas.height = this.height;
> +        canvas.getContext('2d').drawImage(this, 0, 0);
> +        resolve(canvas.toDataURL('image/png'));
> +      };

This also needs an `onerror` handler, which should probably reject the promise.
Attachment #8754595 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/54074/#review51076

> We use 18px and 36px icons, not 19 and 38.

Do you know why IconDetails.normalize defaults to 19? Should that be switched to 18?

> We should pass in the expected dimensions, and scale the image to match, so that this works correctly with PNGs.

I think to handle dimensions other than 18x18 I'd need access to the `bestSize` variable calculated in `AddonManager.getPreferredIconURL`. Do you know of a different way I could get the expected dimensions?
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/2-3/
Attachment #8754595 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/54074/#review51076

> Do you know why IconDetails.normalize defaults to 19? Should that be switched to 18?

19px is the default size on Chrome, so that's the size that we use when creating the object. In practice, it doesn't really make a difference. We still choose the icon that's closest to the correct size.

> I think to handle dimensions other than 18x18 I'd need access to the `bestSize` variable calculated in `AddonManager.getPreferredIconURL`. Do you know of a different way I could get the expected dimensions?

We want to scale it to the final image size, regardless of the size that was specified in the object, so that's not really necessary.
https://reviewboard.mozilla.org/r/54074/#review51076

> We want to scale it to the final image size, regardless of the size that was specified in the object, so that's not really necessary.

But how do I know whether to size it to 18, or 36?
https://reviewboard.mozilla.org/r/54074/#review51076

> But how do I know whether to size it to 18, or 36?

Multiply 18 by `window.devicePixelRatio`
https://reviewboard.mozilla.org/r/54074/#review51076

> Multiply 18 by `window.devicePixelRatio`

Ah, thanks
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/3-4/
Summary: [PageAction] Add support for chrome.pageAction default_icon on Android → [PageAction] Add support for chrome.pageAction icons on Android
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

https://reviewboard.mozilla.org/r/54074/#review52624

::: toolkit/components/extensions/ExtensionUtils.jsm:457
(Diff revisions 3 - 4)
>      return new Promise((resolve, reject) => {
>        let win = context.contentWindow;
> -      var image = new win.Image();
> +      let image = new win.Image();
>        image.onload = function () {
> -        var canvas = win.document.createElement('canvas');
> -        canvas.width = size;
> +        let canvas = win.document.createElement('canvas');
> +        let scaledSize = size * win.devicePixelRatio

Hm. Unfortunately, this won't work, since background pages don't actually have the same device pixel ratio as browser windows. I think you're just going to have to grab the most recent browser window and take the pixel ratio from that.

::: toolkit/components/extensions/ExtensionUtils.jsm:460
(Diff revisions 3 - 4)
>        image.onload = function () {
> -        var canvas = win.document.createElement('canvas');
> -        canvas.width = size;
> -        canvas.height = size;
> +        let canvas = win.document.createElement('canvas');
> +        let scaledSize = size * win.devicePixelRatio
> +        canvas.width = scaledSize;
> +        canvas.height = scaledSize;
>          canvas.getContext('2d').drawImage(this, 0, 0);

You need to set the scaling on the on the context so that the final scaled size of the image matches the size of the canvas before you draw the image.

https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/scale

It would probably be good to make sure we're scaling the x and y by the same ratio, too, and center the image on the one axis if one ratio is bigger than the other.

::: toolkit/components/extensions/ExtensionUtils.jsm:461
(Diff revisions 3 - 4)
> -        var canvas = win.document.createElement('canvas');
> -        canvas.width = size;
> -        canvas.height = size;
> +        let canvas = win.document.createElement('canvas');
> +        let scaledSize = size * win.devicePixelRatio
> +        canvas.width = scaledSize;
> +        canvas.height = scaledSize;
>          canvas.getContext('2d').drawImage(this, 0, 0);
>          resolve(canvas.toDataURL('image/png'));

Double quotes for all strings, please.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:55
(Diff revision 4)
>      manifest: {
>        "name": "PageAction Extension",
>        "page_action": {
>          "default_title": "Page Action",
> +        "default_icon": {
> +          "18": "extension.png",       },

Add newline after comma.
Attachment #8754595 - Flags: review?(kmaglione+bmo)
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/4-5/
Attachment #8754595 - Attachment description: MozReview Request: Bug 1270742 - Add support for default_icon in chrome.pageAction r?kmag → Bug 1270742 - Add support for default_icon in chrome.pageAction
Attachment #8754595 - Flags: review?(kmaglione+bmo)
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/5-6/
Attachment #8754595 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

https://reviewboard.mozilla.org/r/54074/#review54602

::: mobile/android/components/extensions/ext-pageAction.js:61
(Diff revisions 4 - 6)
>        if (this.shouldShow) {
>          this.options.icon = dataURI;
>          this.id = PageActions.add(this.options);
>        }
>      }).catch(() => {
>        return Promise.reject("Failed to load PageAction icon");

This should be `Promise.reject({message: ...})`
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/6-7/
Keywords: checkin-needed
has problems to apply:

patching file mobile/android/components/extensions/ext-pageAction.js
Hunk #1 FAILED at 3
Hunk #2 FAILED at 77
2 out of 2 hunks FAILED -- saving rejects to file mobile/android/components/extensions/ext-pageAction.js.rej
patching file mobile/android/components/extensions/schemas/page_action.json
Hunk #1 FAILED at 13
1 out of 2 hunks FAILED -- saving rejects to file mobile/android/components/extensions/schemas/page_action.json.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
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/7-8/
Flags: needinfo?(mwein)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/5e6356e7a99e
Add support for default_icon in chrome.pageAction. r=kmag
Keywords: checkin-needed
It looks like there are eslint issues with this, Matt.
Flags: needinfo?(mwein)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/071e9554e3e7
Backed out changeset 5e6356e7a99e for eslint test failures
Sorry about that! I ran eslint for all of `mobile/` but I needed to also run it on `browser/`.
Flags: needinfo?(mwein)
And also `toolkit/` for ExtensionUtils.jsm
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/8-9/
Keywords: checkin-needed
Some additional notes for MDN documentation:
  - Since the Tabs API isn't supported yet, anything involving a tab ID should be ignored. For example, PageActions will be shown for the active tab regardless of the tab ID provided.
  - The API methods setTitle and getTitle aren't supported on Android since Android doesn't support tooltips.
  - Since there is no UI yet for displaying popups as overlays, popups will be opened in new tabs.
Hey Matt,

could you take a look at this, seems this has conflicts now:

applying f00cfc21b57d
patching file browser/components/extensions/ext-utils.js
Hunk #1 FAILED at 2
1 out of 1 hunks FAILED -- saving rejects to file browser/components/extensions/ext-utils.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(mwein)
Keywords: checkin-needed
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/9-10/
Flags: needinfo?(mwein)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/1008f5b88e6b
Add support for default_icon in chrome.pageAction. r=kmag
Keywords: checkin-needed
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/10-11/
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/11-12/
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/12-13/
Attachment #8754595 - Attachment description: Bug 1270742 - Add support for default_icon in chrome.pageAction → Bug 1270742 - Add support for default_icon in chrome.pageAction try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s
Comment on attachment 8754595 [details]
Bug 1270742 - Add support for default_icon in chrome.pageAction  try: -b o -p android-x86,android-api-15 -u mochitest-1,mochitest-e10s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54074/diff/13-14/
This should be ready to check in now
Flags: needinfo?(mwein)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/32bb090d7e62
Add support for default_icon in chrome.pageAction r=kmag
Keywords: checkin-needed
Duplicate of this bug: 1119890
https://hg.mozilla.org/mozilla-central/rev/32bb090d7e62
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
I've updated the data for Firefox for Android to note which pageAction APIs are supported in version 50.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction#Browser_compatibility

I've also noted that the APIs that are supported operate globally on all tabs and ignore the `tabId` parameter. For example:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/show#Browser_compatibility
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/hide#Browser_compatibility

Please let me know if I've missed anything...
Flags: needinfo?(mwein)
Keywords: dev-doc-needed
Thanks, it looks good to me.
Flags: needinfo?(mwein)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.