596650 - Use the "sizes" attribute to select the best web app icon

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon specifies that icons with different sizes can be added for a page. We should use the most appropriate size for the web app icon.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: patch

We consider that an icon without the "sizes" attribute has a size of 16px, and that an apple icon has a size of 57px.

The size of icon is min(w, h), since they could be non-square. We then rescale them to fit a 64x64 area, so maybe we should rather keep the closest size to 64px

Note: without the platform patch applied, the behavior is to just ignore the "sizes" attribute.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: patch v2

New patch that doesn't depend on the platform patch, but directly uses the DOM attribute.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 481788 [details] [diff] [review]
patch v2


>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js
>         };
>+        // rel=icon can also have a sizes attribute
>+        if (target.hasAttribute("sizes"))
>+          json.sizes = target.getAttribute("sizes");

nit: Blank line before comment

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
>+        else if ((rel.indexOf("apple-touch-icon") != -1) && (browser.appIcon.size < 57)) {

Is the apple-touch icon always 57px? I thought it could be bigger, but apple resized it

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>-    this.browser.appIcon = null;
>+    this.browser.appIcon = {href: null, size:-1};

nit: I like spaces around the inside of the {} block

Do we use the browser.appIcon.size to resize the icon? I guess the OS is responsible for that, right?

r+ with nits fixed

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: adressing nits

> Is the apple-touch icon always 57px? I thought it could be bigger, but apple
> resized it

According to http://developer.apple.com/library/ios/#documentation/userexperience/conceptual/mobilehig/IconsImages/IconsImages.html web sites must provide a 57x57 icon and a 114x114 one for high dpi screens. I choose worst case here.

> Do we use the browser.appIcon.size to resize the icon? I guess the OS is
> responsible for that, right?

yes, we let the OS do the work. We don't really check that the declared size is the actual size of the icon.

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: updated patch on current m-b

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Comment 7

[[:fabrice] Fabrice Desré]

pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9f227fa1d0db

Comment 8

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/9f227fa1d0db