Closed Bug 1403829 Opened 3 years ago Closed 3 years ago

YouTube icon doesn't look good in Firefox

Categories

(Toolkit :: Places, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: Dolske, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

Attached image 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.
(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.
Priority: -- → P1
Whiteboard: [fxsearch]
Blocks: 1363620
Blocks: 1409969
No longer blocks: 1409969
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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.
Attachment #8924973 - Flags: review?(edilee) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/fef50221fd78
Prefer fetching properly sized favicons from pages. r=Mardak
https://hg.mozilla.org/mozilla-central/rev/fef50221fd78
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.