Last Comment Bug 596650 - Use the "sizes" attribute to select the best web app icon
: Use the "sizes" attribute to select the best web app icon
Status: RESOLVED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
Depends on: 441770
Blocks: 583750
  Show dependency treegraph
 
Reported: 2010-09-15 10:30 PDT by [:fabrice] Fabrice Desré
Modified: 2011-08-21 16:51 PDT (History)
3 users (show)
mounir: in‑testsuite?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (3.08 KB, patch)
2010-10-04 05:34 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
patch v2 (3.18 KB, patch)
2010-10-08 02:31 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Review
adressing nits (3.19 KB, patch)
2010-10-12 12:10 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Review
updated patch on current m-b (3.24 KB, patch)
2010-11-04 11:24 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Review
updated patch (3.51 KB, patch)
2011-06-10 17:06 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Review

Description [:fabrice] Fabrice Desré 2010-09-15 10:30:39 PDT
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é 2010-10-04 05:34:26 PDT
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.
Comment 2 [:fabrice] Fabrice Desré 2010-10-08 02:31:40 PDT
Created attachment 481788 [details] [diff] [review]
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?) 2010-10-12 08:13:33 PDT
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é 2010-10-12 12:10:04 PDT
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.
Comment 5 [:fabrice] Fabrice Desré 2010-11-04 11:24:39 PDT
Created attachment 488246 [details] [diff] [review]
updated patch on current m-b
Comment 6 [:fabrice] Fabrice Desré 2011-06-10 17:06:10 PDT
Created attachment 538641 [details] [diff] [review]
updated patch
Comment 7 [:fabrice] Fabrice Desré 2011-07-11 02:04:48 PDT
pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9f227fa1d0db
Comment 8 Mounir Lamouri (:mounir) 2011-07-11 07:29:21 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/9f227fa1d0db

Note You need to log in before you can comment on or make changes to this bug.