It appears that the favicons the browser downloads from a website are 16x16 pixels and then the browser scales them up to a size optimized for the device resolution. This leads to fuzzy favicons in the URL bar and awesome screen, see screen shot.

fabrice, anything we can do?

We used to retrieve hi-res favicons when they are available but it looks like we don't anymore. (I still had the hi res one for http://www.bbc.co.uk/ before browsing the site again) I'm wondering if http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1279 should not be: if (rel.indexOf("icon") != -1) {

(In reply to Fabrice Desré [:fabrice] from comment #2) > We used to retrieve hi-res favicons when they are available but it looks > like we don't anymore. > (I still had the hi res one for http://www.bbc.co.uk/ before browsing the > site again) > > I'm wondering if > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp. > java#1279 should not be: > > if (rel.indexOf("icon") != -1) { Bug 696433 changed the data sent to Java. We now split the rel="" data in browser.js into an array, wrap each item in "[ ]" and join the items into a single string again. That way we know "[icon]" is matching just "icon" and not "rubicon". If we need more checks in the Java to look for other data, we can add that.

Simple patch that also uses the apple specific hi res favicons if they are available. The issue with this patch is that if a rel="icon" appears after the rel="apple-touch-icon" it will override it. In the xul fennec we kept track of the favicon size and only updated when getting a better one. I'm not familiar with the java code here but it looks that this would be implemented in the Tab class. Adding a parameter to updateFaviconURL(url) that specify the size of the icon should be enough. We would to updateFaviconURL(url, 16) for rel=icon and updateFaviconURL(url, 64) for res="apple-touch-icon"

Comment on attachment 586202 [details] [diff] [review] wip The plan sounds pretty good to me: * Look for and send the "size" data from JS to Java * Change the method to Tab.updateFaviconUrl(url, size) * Keep track of the "best" size in the Tab class

Can the priority of this bug be re-evaluated, please? In my eyes, this is a major flaw, that should be fixed in the initial NativeUI release, especially since there already is a solid proposal, how to do that. Thank you for your consideration!

apple-touch-icons are intended to be used for adding bookmarks to the homescreen (http://en.wikipedia.org/wiki/Favicon#Device_support). There's a couple of problems using them for favicons: 1) We'd be replacing the expected standard favicon (either the one given in link rel or favicon.ico). No other browser--including the mobile stock browser, Dolphin HD, Firefox on desktop, or Google Chrome--uses the apple-touch-icon as a favicon. 2) More importantly, many sites simply don't provide apple-touch-icons--so if the goal is to remove blurry favicons, this won't be a very effective approach. The fundamental problem is that many sites serve a 16x16px favicon, and we're blowing them up to be approximately 62x62px (on Galaxy Nexus), resulting in either a pixelated image (no anti-aliasing) or a blurry one (with anti-aliasing). A more robust alternative might be to just shrink the favicons in the AwesomeScreen (we could make them 32x32 without anti-aliasing, like we did in XUL Fennec). Chrome for Android actually avoids this problem by removing favicons completely. Thoughts?

To give an idea of how common apple-touch-icons are, consider the attached screenshot (https://bugzilla.mozilla.org/attachment.cgi?id=585843). nytimes.com, ca.msn.com, and google.com do *not* provide apple-touch-icons; CNN is the only one that does (incidentally, CNN also includes a 48x48 version in its favicon.ico file, which is why it looks better than the others).

CC'ing madhava in case he has any suggestions.

Perhaps use a better upsampling algorithm for this use case? e.g. http://research.microsoft.com/en-us/um/people/kopf/pixelart/paper/pixel.pdf

Soft blocking nom? Would be great to have favicons looking as good as possible.

This patch just does what XUL fennec did, but I also added apple-touch-icon-precomposed, since that got me more images: http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser-ui.js#1023 I'm not sure how I feel about using apple-touch-icons. Just testing a few sites, I noticed that they often give their apple-touch-icons a background color (Twitter and Wikipedia both do this), and it makes the favicon look bad, even though it's higher resolution. I agree with Brian that developers specify these icons with certain expectations of how they'll be used. I think that if we want to use them for the "Add to Home Screen" functionality, that would suit their purpose, but using them in the awesomescreen seems wrong. We could make this patch just about adding logic for the sizes attribute, but that's not going to give us much of a win until developers start actually using it (almost no websites do). More than anything, I think we just really need evangelism about websites serving up larger icons.

We do need to be careful with the types of images we use. Apple touch icons come in different resolutions (low and high) for different devices (iPhone and iPad). The "precomposed" variety allow the designer to add their own effects to the image, otherwise the OS will add the rounded corners and glare. See: http://developer.apple.com/library/ios/#DOCUMENTATION/AppleApplications/Reference/SafariWebContent/ConfiguringWebApplications/ConfiguringWebApplications.html http://developer.apple.com/library/ios/#DOCUMENTATION/UserExperience/Conceptual/MobileHIG/IconsImages/IconsImages.html#//apple_ref/doc/uid/TP40006556-CH14-SW11 Note that even the apple icons allow for a "sizes" attribute. I don't think we want to use the "precomposed" variety at all. Those icons are designed for iOS desktops, not internal browser favicons or Android home screen shortcuts. We know the folowing: * Not many sites provide high res favicons * Those that do lean heavily on the iOS spec and seem to use "precomposed" a lot. * The iOS touch icons tend to look different than the normal favicon and might not look "right" as internal browser favicons. Maybe we should just start by supporting the "sizes" variant and see what that gives us. I'd rather play it safer. We could also add non-precomposed apple-touch-icons to see how good or bad those are in the wild. Madhava, Ian - Thoughts?

Comment on attachment 614961 [details] [diff] [review] WIP >+ void handleLinkAdded(final int tabId, String rel, final String href, int size) { >+ if (rel.indexOf("[icon]") != -1 || rel.indexOf("[apple-touch-icon]") != -1 || >+ rel.indexOf("[apple-touch-icon-precomposed]") != -1) { Let's drop support for "precomposed" >+ if (rel.indexOf("apple") != -1) >+ size = 57; Keep in mind that "apple-touch-icon" can have a size attribute too. You might want to check and see if 'size' is not set first >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >--- a/mobile/android/chrome/content/browser.js >+++ b/mobile/android/chrome/content/browser.js >@@ -1832,23 +1832,37 @@ Tab.prototype = { > list = target.rel.toLowerCase().split(/\s+/); > let hash = {}; > list.forEach(function(value) { hash[value] = true; }); > list = []; > for (let rel in hash) > list.push("[" + rel + "]"); > } > >+ // We use the sizes attribute if available >+ // see http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon Not sure if it matters but we could also add the apple docs here too: http://developer.apple.com/library/ios/#DOCUMENTATION/AppleApplications/Reference/SafariWebContent/ConfiguringWebApplications/ConfiguringWebApplications.html hmm, that URL is huge - let's not :) >+ let size = 16; But maybe we should default to 57 here for "apple-touch-icon" > let json = { > type: "DOMLinkAdded", > tabID: this.id, > href: resolveGeckoURI(target.href), > charset: target.ownerDocument.characterSet, > title: target.title, >- rel: list.join(" ") >+ rel: list.join(" "), >+ size: size > }; > > // rel=icon can also have a sizes attribute > if (target.hasAttribute("sizes")) > json.sizes = target.getAttribute("sizes"); Do we want to keep this "sizes" JSON ?

Interesting. I didn't realize apple was adding the rounding and highlighting on the OS side for web clip icons, I thought they only did that for native apps. Sigh. It would be great to see a build where we allow whatever highest quality icon we can get, whether it's large favicons, or web clip apps (both precomposed and un-precomposed), and see what it looks like. My sense is we will probably want to ditch the icons that are meant to get the apple corner and lighting effect treatment, because they won't look as good when they are just plain squares. Can we try a build like this, and possibly even get a small list of pages where we know what kind of favicion / web clip icon we're getting with them?

I updated this patch to get rid of the apple icons. I tried testing a build with just apple-touch-icon (not apple-touch-icon-precomposed) and I found that at least one site (cnn.com) incorrectly uses a precomposed icon, and I found that other sites (e.g. wikipedia.com) use icons with colored backgrounds that don't really look good as favicons. More than anything else, I noticed that not many sites even serve us apple-touch-icons, so I don't think it's worth it to try to support it. I think the real solution is that we just need to evangelize specifying larger icons using the standardized <link rel=icon> method.

This patch just implements the sizes spec. I decided to just pass one size integer to Java for now, since that's all we're using in the UI. If eventually we want to keep track of multiple icons in Java, we can change this to pass an array of sizes. Since no real websites use this sizes attribute, I made some test pages: http://people.mozilla.com/~mleibovic/test/icon_sizes/ I also filed bug 751712 for the desktop part of this.

Comment on attachment 620891 [details] [diff] [review] patch >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java >- tab.setContentType(contentType); >- tab.updateFavicon(null); >- tab.updateFaviconURL(null); >+ tab.clearFavicon(); Did you mean to remove the | tab.setContentType(contentType); | ? clearFavicon() does not change the content type. >+ void handleLinkAdded(final int tabId, String rel, final String href, int size) { >+ // If tab is not loading and the favicon is updated, we >+ // want to load the image straight away. If tab is still >+ // loading, we only load the favicon once the page's content >+ // is fully loaded (see handleContentLoaded()). Off-topic, but I think we might want to stop waiting. I think if we updated the favicon here it would avoid the brief flash of "no favicon" image we see when the page loads. Can you file a new bug to investigate? >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java >+ public void updateFaviconURL(String faviconUrl, int size) { >+ // If we already have an "any" sized icon, don't update the icon. >+ if (mFaviconSize == -1) >+ return; An "any" sized icon must explicitly use "any" right? A simple, regular <link rel="shortcut icon".../> is not considered an "any" ? r+, but figure out the setContentType issue.

(In reply to Mark Finkle (:mfinkle) from comment #21) > >- tab.setContentType(contentType); > >- tab.updateFavicon(null); > >- tab.updateFaviconURL(null); > >+ tab.clearFavicon(); > > Did you mean to remove the | tab.setContentType(contentType); | ? > clearFavicon() does not change the content type. Good catch, that was a mistake. > >+ void handleLinkAdded(final int tabId, String rel, final String href, int size) { > > >+ // If tab is not loading and the favicon is updated, we > >+ // want to load the image straight away. If tab is still > >+ // loading, we only load the favicon once the page's content > >+ // is fully loaded (see handleContentLoaded()). > > Off-topic, but I think we might want to stop waiting. I think if we updated > the favicon here it would avoid the brief flash of "no favicon" image we see > when the page loads. Can you file a new bug to investigate? Filed bug 759501. I agree that's something worth looking into. > >diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java > >+ public void updateFaviconURL(String faviconUrl, int size) { > >+ // If we already have an "any" sized icon, don't update the icon. > >+ if (mFaviconSize == -1) > >+ return; > > An "any" sized icon must explicitly use "any" right? A simple, regular <link > rel="shortcut icon".../> is not considered an "any" ? I assume so, since the spec talks about how "any" should basically just be used for scalable images that really will look right at any size. I would think a website that's using the sizes attribute would use it on all of the rel="icon" links, not just some of them, so I don't think this would be a problem (worst case, we just end up using the icon with the sizes attribute).

This should be documented so site owners know they can use it and to help increase adoption, right?

(In reply to pretzer from comment #25) > This should be documented so site owners know they can use it and to help > increase adoption, right? Yes, good call. Right now it's mobile-only until bug 751712 gets fixed. It's also worth noting that it won't handle multiple images in one .ico/.icns file because we don't have an image decoder to handle that right now.

Margaret, is this worth nomming for .N+? Or scary enough to ride the trains?

(In reply to Johnathan Nightingale [:johnath] from comment #27) > Margaret, is this worth nomming for .N+? Or scary enough to ride the trains? I don't think we found any problems other than bug 760218, so it seems pretty safe. We'd just want to make sure to uplift that bug as well.

(In reply to Margaret Leibovic [:margaret] from comment #28) > (In reply to Johnathan Nightingale [:johnath] from comment #27) > > Margaret, is this worth nomming for .N+? Or scary enough to ride the trains? > > I don't think we found any problems other than bug 760218, so it seems > pretty safe. We'd just want to make sure to uplift that bug as well. Oooh, that's lovely news! Can you approval-beta nom a rollup patch?

[Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: no support for HTML5 sizes attribute in <link rel=icon> Testing completed (on m-c, etc.): been on m-c since 5/30 Risk to taking this patch (and alternatives if risky): could potentially break <link rel=icon> if web authors are putting weird things in the sizes attribute String or UUID changes made by this patch: n/a

Comment on attachment 634151 [details] [diff] [review] Rollup patch (includes patch from bug 760218) And after all that, we discussed it in triage and decided that 15 is probably soon enough for this, so no need for beta landing. Sorry for the makework, Maggie.