Use higher-res favicons in tabs in HiDPI mode when available

RESOLVED FIXED in Firefox 22

Status

()

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: szhorvat, Assigned: jfkthame)

Tracking

(Depends on: 3 bugs, Blocks: 2 bugs)

Trunk
Firefox 22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Posted image screenshot.png
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.
depending on the fix this could be fixed by bug 795495
Blocks: 785330
Component: Untriaged → Theme

Updated

6 years ago
Blocks: 795495
Status: UNCONFIRMED → NEW
Depends on: 419588
Ever confirmed: true
Hardware: x86 → All
Version: 21 Branch → Trunk

Comment 2

6 years ago
The affected platform should be All, not just Mac OSX, and it should block Bug 820679.
(Reporter)

Comment 3

6 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?
most of the icons are rescaled to 16x16 internally, so they can't be at 2xDPI.
(Assignee)

Comment 5

6 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.

Updated

6 years ago
No longer blocks: 795495

Comment 6

6 years ago
There is the same problem on Android.

Updated

6 years ago
Summary: Favicons are not shown at retina resolution in HiDPI mode → Use 32x32 favicons in HiDPI mode when available

Comment 7

6 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.

Comment 8

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

Comment 9

6 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...
Attachment #729001 - Flags: review?(fyan)
(Assignee)

Updated

6 years ago
Assignee: nobody → jfkthame
(Assignee)

Comment 10

6 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.
(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?

Updated

6 years ago
Attachment #728672 - Attachment is obsolete: true
(Assignee)

Comment 12

6 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.)
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. :)
Attachment #729001 - Flags: review?(fyan) → review+
(Assignee)

Comment 14

6 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.
Summary: Use 32x32 favicons in HiDPI mode when available → Use 32x32 favicons in tab titles in HiDPI mode when available
Target Milestone: --- → Firefox 22
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)
Sorry -- forgot to include the URI of the backout cset. It's:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/02547d0a4402
(Assignee)

Comment 17

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

Comment 18

6 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.
Attachment #729528 - Flags: review?(fyan)
(Assignee)

Updated

6 years ago
Attachment #729001 - Attachment is obsolete: true
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.
(Assignee)

Comment 20

6 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.
Attachment #729540 - Flags: review?(fyan)
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.
Attachment #729540 - Flags: review?(fyan) → review-
(Assignee)

Comment 22

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

Updated

6 years ago
Attachment #729540 - Attachment is obsolete: true
(Assignee)

Comment 24

6 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.
Attachment #729528 - Attachment is obsolete: true
Attachment #729528 - Flags: review?(fyan)
(Assignee)

Comment 25

6 years ago
Tryserver job with latest version of the patch:
https://tbpl.mozilla.org/?tree=Try&rev=0bc07353ec1b
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).
Attachment #729566 - Flags: review?(fyan) → review+
(Assignee)

Comment 27

6 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.)
Component: Theme → Tabbed Browser
OS: Mac OS X → All
Summary: Use 32x32 favicons in tab titles in HiDPI mode when available → Use higher-res favicons in tabs in HiDPI mode when available
(Assignee)

Comment 28

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

Updated

6 years ago
Blocks: 820679
(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. :)
This was backed out while investigating mochitest failures. Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8efcbaf42b
https://hg.mozilla.org/mozilla-central/rev/dd8efcbaf42b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 856337

Updated

6 years ago
Depends on: 860578

Updated

6 years ago
Depends on: 866444

Updated

6 years ago
Duplicate of this bug: 860334
Duplicate of this bug: 880731

Updated

5 years ago
Depends on: 1088948

Comment 34

4 years ago
This seems to have just regressed in 36.0.1.
(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.

Comment 36

4 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.
You need to log in before you can comment on or make changes to this bug.