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

RESOLVED FIXED in Firefox 8

Status

Fennec Graveyard
General
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 8
Dependency tree / graph
Bug Flags:
in-testsuite ?

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → fabrice
(Assignee)

Updated

7 years ago
Blocks: 583750
(Assignee)

Updated

7 years ago
Depends on: 441770
(Assignee)

Comment 1

7 years ago
Created attachment 480601 [details] [diff] [review]
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.
Attachment #480601 - Flags: review?(mark.finkle)
(Assignee)

Comment 2

7 years ago
Created attachment 481788 [details] [diff] [review]
patch v2

New patch that doesn't depend on the platform patch, but directly uses the DOM attribute.
Attachment #480601 - Attachment is obsolete: true
Attachment #481788 - Flags: review?(mark.finkle)
Attachment #480601 - Flags: review?(mark.finkle)
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
Attachment #481788 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 4

7 years ago
Created attachment 482621 [details] [diff] [review]
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.
Attachment #481788 - Attachment is obsolete: true
Attachment #482621 - Flags: review?(mark.finkle)
Attachment #482621 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 5

7 years ago
Created attachment 488246 [details] [diff] [review]
updated patch on current m-b
Attachment #482621 - Attachment is obsolete: true
Attachment #488246 - Flags: review?(mark.finkle)
Attachment #488246 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 538641 [details] [diff] [review]
updated patch
Attachment #488246 - Attachment is obsolete: true
Attachment #538641 - Flags: review+
(Assignee)

Comment 7

6 years ago
pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9f227fa1d0db
Merged:
http://hg.mozilla.org/mozilla-central/rev/9f227fa1d0db
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
You need to log in before you can comment on or make changes to this bug.