Szabolcs Horvát Reporter
	Description • 11 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20130109 Firefox/21.0
Build ID: 20130109030942

Steps to reproduce:

When running in HiDPI mode on OS X, load any of the following sites in both Firefox and Safari:

http://www.cnn.com/
http://www.arstechnica.com/
http://www.google.com/
http://www.archdaily.com/

(and many others)


Actual results:

Note the favicons displayed:  in Safari all these sites have high resolution favicons, displayed at retina resolution.  In Firefox they are displayed at standard resolution.


Expected results:

Firefox should display these favicons at retina resolution, just like Safari does.

	Matthias Versen [:Matti]
	Comment 1 • 11 years ago

depending on the fix this could be fixed by bug 795495

	Greg Edwards
	Comment 2 • 11 years ago

The affected platform should be All, not just Mac OSX, and it should block Bug 820679.

	Szabolcs Horvát Reporter
	Comment 3 • 11 years ago

I just noticed that there are certain websites whose favicon *is* shows at retina resolution in Firefox, but most of them aren't.  E.g. https://bitbucket.org/  Does it depend on how the favicon is specified?

	Marco Bonardo [:mak]
	Comment 4 • 11 years ago

most of the icons are rescaled to 16x16 internally, so they can't be at 2xDPI.

	Jonathan Kew [:jfkthame] Assignee
	Comment 5 • 11 years ago

It looks like we don't handle cases where favicon.ico contains multiple versions of the icon at different sizes - we just use the 16x16 version, even when there is a 32x32 version present. We ought to be able to fix that.

The bitbucket site only has a small image in its favicon.ico, but the HTML includes:
  <link rel="icon" type="image/png" href="https://d3oaxc4q5k2d6q.cloudfront.net/m/781fb77c3b12/img/favicon.png">
which points to a 32x32 version of its favicon; apparently we do recognize that.

	Yann Brelière
	Comment 6 • 11 years ago

There is the same problem on Android.

	Frank Yan (:fryn)
	Comment 7 • 11 years ago

Tiny patch that makes Firefox use 32x32 favicons for the tabs when available, but doesn't work for bookmarks & history.

Also, the concatenation of "#-moz-resolution=32,32" needs to be altered to handle existing '#' characters in the URI.

	Greg Edwards
	Comment 8 • 11 years ago

Instead of sniffing out min-resolution: 2dppx, you should scale the desired icon size by window.devicePixelRatio so that other pixel ratios work.

	Jonathan Kew [:jfkthame] Assignee
	Comment 9 • 11 years ago

Modified your patch to handle other scaling factors, as per Greg's comment, and leave it up to the icon decoder to pick the best matching size (I'm going to post a followup to bug 419588 to prefer downscaling larger icons rather than upscaling smaller ones). Going to try this on windows to see how the results look at 125% and 150% resolution scales...

	Jonathan Kew [:jfkthame] Assignee
	Comment 10 • 11 years ago

This seems to work well on Windows, provided we adjust the icon decoder to favor giving us a -larger- icon to be downscaled rather than a smaller one that needs to be upscaled to the requested size; see bug 854441.

	Frank Yan (:fryn)
	Comment 11 • 11 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> This seems to work well on Windows, provided we adjust the icon decoder to
> favor giving us a -larger- icon to be downscaled rather than a smaller one
> that needs to be upscaled to the requested size; see bug 854441.

Would changing Math.round to Math.ceil work just as well?

	Jonathan Kew [:jfkthame] Assignee
	Comment 12 • 11 years ago

We could, but it wouldn't make much difference. The main issue is that if the (rounded) size we want is (say) 20px, but the file only contains 16px and 32px, we want to choose the 32px version - i.e. the issue there isn't one of integer rounding direction.

(And we don't want to apply round() or ceil() to the devicePixelRatio itself, before multiplying, as there's no guarantee that the available sizes are restricted to integer multiples of 16px.)

	Frank Yan (:fryn)
	Comment 13 • 11 years ago

Comment on attachment 729001 [details] [diff] [review]
use larger favicons in hidpi mode if available

Review of attachment 729001 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> (And we don't want to apply round() or ceil() to the devicePixelRatio
> itself, before multiplying, as there's no guarantee that the available sizes
> are restricted to integer multiples of 16px.)

Applying ceil to devicePixelRatio is what I meant, but you're right.
Thanks for working on this. :)

	Jonathan Kew [:jfkthame] Assignee
	Comment 14 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb015c10d58f

This should fix the favicons in tab titles on Retina Macs, and on Windows once bug 854441 lands.

We'll still need to do something for other places where favicons are used (bookmarks, history - see comment 7). I suggest we let those be handled by followup bugs, for easier tracking of what's fixed and what isn't.

	Daniel Holbert [:dholbert]
	Comment 15 • 11 years ago

Backed out for causing browser-chrome test bustage like:
https://tbpl.mozilla.org/php/getParsedLog.php?id=21078596&tree=Mozilla-Inbound

It's possible that this didn't cause all the bustage in that log, but it's definitely responsible for the first one:
{
WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug477014.js |  - Got data:image/png;base64,iV...ggg==#-moz-resolution=16,16&-moz-resolution=16,16, expected data:image/png;base64,iV..ggg==
}

I stripped out most of the giant data URI there. The relevant differnece between "Got" and "expected" is the -moz-resolution=16,16 suffix that this bug added.

Not sure if that's a bug in the test or in the code, but either way, it needs fixing before this can re-land.

(Also, it seems a bit odd that the data URI has two copies of the -moz-resolution thing -- "#-moz-resolution=16,16&-moz-resolution=16,16".  That doesn't seem to be what's breaking the test, but it looks like a bug that'd be worth fixing)

	Daniel Holbert [:dholbert]
	Comment 16 • 11 years ago

Sorry -- forgot to include the URI of the backout cset. It's:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/02547d0a4402

	Jonathan Kew [:jfkthame] Assignee
	Comment 17 • 11 years ago

Argh - sorry, didn't think about tests that explicitly compare the icon as a data uri.

AFAICS there are two possible approaches to this: either we modify the affected tests so that they explicitly include the -moz-resolution media fragment in the expected result (adjusted for window.devicePixelRatio), or know to ignore it when comparing; or else we modify the getIcon method in tabbrowser.xml so that it returns the icon URL without this media fragment.

I think the latter makes more sense; we should consider the use of #-moz-resolution (here, at least) to be something that's purely an internal implementation detail of tabbrowser's icon handling. We don't include this in the URL that's passed to setIcon, so we shouldn't expect it to come back out of getIcon either.

I'll run this idea through tryserver to see if it makes things happier, and if so will post an updated patch for re-review.

	Jonathan Kew [:jfkthame] Assignee
	Comment 18 • 11 years ago

Updated the patch to avoid "leaking" the -moz-resolution media fragment out of tabbrowser through the getIcon API or to onLinkIconAvailable listeners. This prevents it messing up various testcases that have specific expectations about the icon URL. Also avoids the case where multiple -moz-resolution fragments get appended, which occurred when the icon was copied from one browser element to another and so passed through setIcon multiple times. This seems to pass tests locally; sent to tryserver for confirmation: https://tbpl.mozilla.org/?tree=Try&rev=da300d3dcdc7.

	Dão Gottwald [:dao]
	Comment 19 • 11 years ago

Comment on attachment 729528 [details] [diff] [review]
use higher-res favicons for tab titles in hidpi mode if available.

>+            if (iconUrl) {
>+              var size = Math.round(16 * window.devicePixelRatio);

Use let instead of var here.

>+                this.setIcon(aOurTab,
>+                             // strip -moz-resolution before passing the URL to setIcon,
>+                             // as it will re-add the appropriate fragment
>+                             otherBrowser.mIconURL.replace(/[#&]-moz-resolution=\d+,\d+$/, ""));

You should call remoteBrowser.getIcon(aOtherTab) here.

	Jonathan Kew [:jfkthame] Assignee
	Comment 20 • 11 years ago

On second thought, this looks like a better solution. Instead of adding the resolution media fragment to mIconURL, we can leave that property untouched, and just add the media fragment when actually setting the image attribute on the tab. This is simpler and cleaner. Also pushed this version to tryserver, to check for surprises (looking good in local testing)... https://tbpl.mozilla.org/?tree=Try&rev=86cf94d2af56.

	Dão Gottwald [:dao]
	Comment 21 • 11 years ago

Comment on attachment 729540 [details] [diff] [review]
use higher-res favicons for tab titles in hidpi mode if available.

>             if ((browser.mIconURL || "") != aTab.getAttribute("image")) {

This check doesn't work anymore since you're adding #-moz-resolution to the 'image' attribute but not to mIconURL. You could fix that by stripping off #-moz-resolution before doing the comparison.

	Jonathan Kew [:jfkthame] Assignee
	Comment 22 • 11 years ago

Good catch, thanks - so we'd end up redundantly re-setting the attribute. I think I'd prefer to fix it by including the resolution fragment in the URL that we compare to the current image attribute. That way, re-setting the icon after a change in screen resolution will work properly, whereas otherwise the "stale" icon could persist more stubbornly.

	Jonathan Kew [:jfkthame] Assignee
	Comment 24 • 11 years ago

Comment on attachment 729528 [details] [diff] [review]
use higher-res favicons for tab titles in hidpi mode if available.

Obsoleting the older patch that included -moz-resolution in mIconURL, as that leads to a messier solution overall.

	Jonathan Kew [:jfkthame] Assignee
	Comment 25 • 11 years ago

Tryserver job with latest version of the patch:
https://tbpl.mozilla.org/?tree=Try&rev=0bc07353ec1b

	Dão Gottwald [:dao]
	Comment 26 • 11 years ago

Comment on attachment 729566 [details] [diff] [review]
use higher-res favicons for tab titles in hidpi mode if available.

>+            let sizedIconUrl = (browser.mIconURL || "");

The parentheses can be dropped.

>+            if (sizedIconUrl != "") {

if (sizedIconUrl) {

>+              sizedIconUrl += (sizedIconUrl.indexOf("#") > -1 ? "&" : "#") +

sizedIconUrl += (sizedIconUrl.contains("#") ? "&" : "#") +

>+              if (sizedIconUrl != "") {
>+                aTab.setAttribute("image", sizedIconUrl);
>+              } else
>                 aTab.removeAttribute("image");

if (sizedIconUrl) {

Also, the curly brackets can be dropped (you're not using them after 'else' either).

	Jonathan Kew [:jfkthame] Assignee
	Comment 27 • 11 years ago

Thanks for the quick review. I'll wait for tryserver results to confirm mochitest-bc is green before cleaning up and pushing this.

(fryn, if you have any other comments - as you started the work in this bug - feel free to weigh in, otherwise I'll go ahead with dao's review.)

	Jonathan Kew [:jfkthame] Assignee
	Comment 28 • 11 years ago

Cleaned up as per comment 26, and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93cbcd7f34b3

I've filed bug 854956 about the favicons that appear in bookmarks and awesomebar results (i.e. things that come from Places).

	Frank Yan (:fryn)
	Comment 29 • 11 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #27)
> (fryn, if you have any other comments - as you started the work in this bug
> - feel free to weigh in, otherwise I'll go ahead with dao's review.)

No, this is fine. :)

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

This was backed out while investigating mochitest failures. Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8efcbaf42b

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/dd8efcbaf42b

	Greg Edwards
	Comment 34 • 9 years ago

This seems to have just regressed in 36.0.1.

	Matthew N. [:MattN]
	Comment 35 • 9 years ago

(In reply to Greg Edwards from comment #34)
> This seems to have just regressed in 36.0.1.

You're probably seeing the effect of bug 1121802. Make sure you favicon has a .ico extension to work around this.

	Greg Edwards
	Comment 36 • 9 years ago

The do have .ico as the extension, but I did find the real culprit:

<link rel="shortcut icon" href="~/favicon.ico?1" type="image/vnd.microsoft.icon" />

I placed a ?1 after the .ico to force invalidate the cache. (I often do this with CSS as well.) By removing the ?1, the high-res favicon works once again.