1403829 - YouTube icon doesn't look good in Firefox

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Justin Dolske [:Dolske]]

Attachment: Screenshot

The YouTube favicon is not looking good in Firefox. It was already kind of wonky in Firefox 55, and is worse now in Nightly. (With the fix for bug 1401777, but I think it's been looking that way for a while.)

Screenshot attached shows Release 55.0.3, current Nightly, and Chrome Canary.

Markup seems to be:

<link rel="shortcut icon" href="/yts/img/favicon-vfl8qSV2F.ico" type="image/x-icon" >
<link rel="icon" href="/yts/img/favicon_32-vflOogEID.png" sizes="32x32" >
<link rel="icon" href="/yts/img/favicon_48-vflVjB_Qk.png" sizes="48x48" >
<link rel="icon" href="/yts/img/favicon_96-vflW9Ec0w.png" sizes="96x96" >
<link rel="icon" href="/yts/img/favicon_144-vfliLAfaB.png" sizes="144x144" >

In Nightly, devtools shows that we're using favicon-vfl8qSV2F.ico, which contains just a 16x16 icon. Looks "correct" for what it is, but isn't the best choice of available icons for us to use.

In 55, devtools shows that we're using avicon_144-vfliLAfaB.png. That's the 144x144 icon, and it looks poor because it's downscaled with -moz-crisp-edges.

Hard to tell what Chrome is using, but since it looks properly scaled it's probably the specified 32x32 icon.


What should Nightly be using? Ideally the 32x32 (since I'm on a Retina display), but it's a little surprising we're not trying to use the 144x144 like 55 did (even though that would still look bad) -- not sure if that's a bug or known limitation.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Justin Dolske [:Dolske] from comment #0)
> In Nightly, devtools shows that we're using favicon-vfl8qSV2F.ico, which
> contains just a 16x16 icon. Looks "correct" for what it is, but isn't the
> best choice of available icons for us to use.

It's very uncommon that they provide an ico with just a 16x16 resolution ;(

> What should Nightly be using? Ideally the 32x32 (since I'm on a Retina
> display), but it's a little surprising we're not trying to use the 144x144
> like 55 did (even though that would still look bad) -- not sure if that's a
> bug or known limitation.

the 144x144 is considered a Rich icon and thus we don't use it for the tabs, since it would be heavily downscaled.
55 just stores the last favicon defined in the page, nothing more than that. It works by luck.

We are not done yet with picking icons in ContentLinkHandler, Nan did a first pass, but we need to check the sizes attribute yet and always store a 32px icon if it's available in any form.

We'll try to fix the main favicon problems in 58.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Bug 1403829 - Prefer fetching properly sized favicons from pages.

Review commit: https://reviewboard.mozilla.org/r/196230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196230/

Comment 3

[Ed Lee :Mardak]

Comment on attachment 8924973 [details]
Bug 1403829 - Prefer fetching properly sized favicons from pages.

https://reviewboard.mozilla.org/r/196230/#review201714

::: browser/modules/ContentLinkHandler.jsm:152
(Diff revision 1)
>    // Rich icons are either apple-touch or fluid icons, or the ones of the
>    // dimension 96x96 or greater
>    let largestRichIcon;
>  
> +  function isICO(icon) {
> +    return icon.type == "image/x-icon" || icon.type == "image/vnd.microsoft.icon";

nit: this could be declared outside of `faviconTimeoutCallback`

::: browser/modules/ContentLinkHandler.jsm:162
(Diff revision 1)
> -      icon.type === "image/vnd.microsoft.icon") {
> -      preferredIcon = icon;
> +        preferredIcon = icon;
> -      continue;
> +      } else if (icon.width == preferredWidth && preferredIcon.type != "image/svg+xml") {
> +        preferredIcon = icon;
> +      } else if (isICO(icon) && (preferredIcon.type == null || isICO(preferredIcon))) {
> +        preferredIcon = icon;

Something about these conditions made it hard to follow, but after staring at it for 30 minutes, I can't think of anything particularly better. Maybe have the comment for `preferredIcon` closer to this logic?

I think it's just the asymmetry of each of the conditions:
- always pick svg
- prefer width but don't replace svg
- use ico only if currently ico or no preferred yet (implicitly meaning don't replace a non-svg/non-ico which must have been preferred because it was the desired width)

But something also that it could be very easy to accidentally overwrite an actually preferred icon in the future, so good thing with all the tests.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8924973 [details]
Bug 1403829 - Prefer fetching properly sized favicons from pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196230/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fef50221fd78
Prefer fetching properly sized favicons from pages. r=Mardak

Comment 6

[Cristina Coroiu [:ccoroiu]]

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