1345992 - Fix pageAction icon size on Android

Status: RESOLVED FIXED

(WebExtensions :: Android, enhancement)

Description

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

The pageAction icon currently rendered on Firefox for Android is too small for a user to be able to touch it.

Comment 1

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

It looks like the reason is that the `IconDetails.convertImageURLToDataURL` method [1](currently used only by the ext-pageAction.js Android implementation) doesn't currently set the size of the canvas element, that is used to resize the icon and convert it into a data URL.

[1]: http://searchfox.org/mozilla-central/rev/7cb75d87753de9103253e34bc85592e26378f506/toolkit/components/extensions/ExtensionUtils.jsm#289-319

Comment 2

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

Attachment: Bug 1345992 - Fix pageAction icon size on Android.

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

Comment 3

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

Hey Kris,
If I'm not wrong, the attached patch sets the size of the canvas so that the resized icon is in the center of the resulting image, that is then turned into the data URL.

I tried it on the android emulator and the size and alignment of the rendered icon seems the right one with the attached change applied.

Let me know what do you think about it.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8845620 [details]
Bug 1345992 - Fix pageAction icon size on Android.

https://reviewboard.mozilla.org/r/118748/#review121294

::: toolkit/components/extensions/ExtensionUtils.jsm:295
(Diff revision 1)
>          let dSize = size * browserWindow.devicePixelRatio;
>  
>          // Scales the image while maintaing width to height ratio.
>          // If the width and height differ, the image is centered using the
>          // smaller of the two dimensions.
>          let dWidth, dHeight, dx, dy;

These variable names are terrible... `dx` and `dy` are deltas, per the normal naming convention, but `dWidth`, `dHeight`, and `dSize` are not. While you're here, please fix them.

Thanks.

::: toolkit/components/extensions/ExtensionUtils.jsm:313
(Diff revision 1)
> +        canvas.width = dWidth + dx * 2;
> +        canvas.height = dHeight + dy * 2;

The final image should always be a square, the size of the `size` param (compensated for device pixel ratio), so please just use that rather than recalculating it.

Comment 5

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

Comment on attachment 8845620 [details]
Bug 1345992 - Fix pageAction icon size on Android.

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

Comment 6

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

Comment on attachment 8845620 [details]
Bug 1345992 - Fix pageAction icon size on Android.

https://reviewboard.mozilla.org/r/118748/#review121294

> These variable names are terrible... `dx` and `dy` are deltas, per the normal naming convention, but `dWidth`, `dHeight`, and `dSize` are not. While you're here, please fix them.
> 
> Thanks.

To be fair, I think that these identifiers have been named based to our MDN docs related to the drawImage method: https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/drawImage#Parameters

I'm not strongly against renaming them, but I think it worth to mention that the names are coming from the related MDN docs before proceeding to rename them to something else.

> The final image should always be a square, the size of the `size` param (compensated for device pixel ratio), so please just use that rather than recalculating it.

oh, yep, I think that you are defnitely right, after re-reading again, both the canvas width and height should just be dSize (fixed in the last updated patch).

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8845620 [details]
Bug 1345992 - Fix pageAction icon size on Android.

https://reviewboard.mozilla.org/r/118748/#review123690

Comment 8

[Carsten Book [:Tomcat]]

seems have one open issue in mozreview that need to be fixed first. Could you take a look, thanks!

Comment 9

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

Sorry, you are right, I forgot to clear the last issue on mozreview.

I've updated mozreview accordingly to what have been discussed in the comments right now.

Thanks!

Comment 10

[Ian Moody [:Kwan] (UTC+0)]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/065eb6072909
Fix pageAction icon size on Android. r=kmag

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/065eb6072909