Matthew Wein [:mattw] Assignee
	Description • 8 years ago

This bug is meant to track the implementation and testing of default_icon in the chrome.pageAction manifest on android.

	Matthew Wein [:mattw] Assignee
	Comment 1 • 8 years ago

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

	Matthew Wein [:mattw] Assignee
	Comment 2 • 8 years ago

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/

	Kris Maglione [:kmag]
	Comment 3 • 8 years ago

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.

	Matthew Wein [:mattw] Assignee
	Comment 4 • 8 years ago

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?

	Matthew Wein [:mattw] Assignee
	Comment 5 • 8 years ago

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/

	Kris Maglione [:kmag]
	Comment 6 • 8 years ago

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.

	Matthew Wein [:mattw] Assignee
	Comment 7 • 8 years ago

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?

	Kris Maglione [:kmag]
	Comment 8 • 8 years ago

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`

	Matthew Wein [:mattw] Assignee
	Comment 9 • 8 years ago

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

> Multiply 18 by `window.devicePixelRatio`

Ah, thanks

	Matthew Wein [:mattw] Assignee
	Comment 10 • 8 years ago

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/

	Kris Maglione [:kmag]
	Comment 11 • 8 years ago

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.

	Matthew Wein [:mattw] Assignee
	Comment 12 • 8 years ago

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/

	Matthew Wein [:mattw] Assignee
	Comment 13 • 8 years ago

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/

	Kris Maglione [:kmag]
	Comment 14 • 8 years ago

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: ...})`

	Matthew Wein [:mattw] Assignee
	Comment 15 • 8 years ago

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/

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago

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$

	Matthew Wein [:mattw] Assignee
	Comment 17 • 8 years ago

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/

	Pulsebot
	Comment 18 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/5e6356e7a99e
Add support for default_icon in chrome.pageAction. r=kmag

	Bob Silverberg [:bsilverberg]
	Comment 19 • 8 years ago

It looks like there are eslint issues with this, Matt.

	Carsten Book [:Tomcat]
	Comment 20 • 8 years ago

yes :) sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9974463&repo=fx-team

	Pulsebot
	Comment 21 • 8 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/071e9554e3e7
Backed out changeset 5e6356e7a99e for eslint test failures

	Matthew Wein [:mattw] Assignee
	Comment 22 • 8 years ago

Sorry about that! I ran eslint for all of `mobile/` but I needed to also run it on `browser/`.

	Matthew Wein [:mattw] Assignee
	Comment 23 • 8 years ago

And also `toolkit/` for ExtensionUtils.jsm

	Matthew Wein [:mattw] Assignee
	Comment 24 • 8 years ago

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/

	Matthew Wein [:mattw] Assignee
	Comment 25 • 8 years ago

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.

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago

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

	Matthew Wein [:mattw] Assignee
	Comment 27 • 8 years ago

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/

	Matthew Wein [:mattw] Assignee
	Comment 28 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ee5541d1c7

	Pulsebot
	Comment 29 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/1008f5b88e6b
Add support for default_icon in chrome.pageAction. r=kmag

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 30 • 8 years ago

Backed out for failing browser_ext_browserAction_context.js:

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=b3a0f1a9ede7f50e2da02d7efd8741da9a5c5937
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=10076488&repo=fx-team

	Matthew Wein [:mattw] Assignee
	Comment 31 • 8 years ago

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/

	Matthew Wein [:mattw] Assignee
	Comment 32 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7c5519d7eaa

	Matthew Wein [:mattw] Assignee
	Comment 33 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6480e12294df

	Matthew Wein [:mattw] Assignee
	Comment 34 • 8 years ago

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/

	Matthew Wein [:mattw] Assignee
	Comment 35 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab574a23e13

	Matthew Wein [:mattw] Assignee
	Comment 36 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91f2e092944a

	Matthew Wein [:mattw] Assignee
	Comment 37 • 8 years ago

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/

	Matthew Wein [:mattw] Assignee
	Comment 38 • 8 years ago

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/

	Matthew Wein [:mattw] Assignee
	Comment 39 • 8 years ago

This should be ready to check in now

	Pulsebot
	Comment 40 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/32bb090d7e62
Add support for default_icon in chrome.pageAction r=kmag

	Carsten Book [:Tomcat]
	Comment 42 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/32bb090d7e62

	Will Bamberg [:wbamberg]
	Comment 43 • 8 years ago

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...

	Matthew Wein [:mattw] Assignee
	Comment 44 • 8 years ago

Thanks, it looks good to me.