1422289 - ContentLinkHandler should guess icon type from the extension when type is not defined

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P2)

Description

[Marco Bonardo [:mak]]

See bug 1421495, if the page doesn't define a type, we don't pick a preferred icon and end up just using the last defined favicon.
We can guess type from the extension, when possible.

Comment 1

[Ed Lee :Mardak]

Attachment: Bug 1422289 - ContentLinkHandler should guess icon type from the extension when type is not defined.

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

Comment 2

[Ed Lee :Mardak]

Attachment: screenshot of pontoon.mozilla top site with fix

The website still has the double <link rel=icon> with no type from bug 1421495, and this fix works for pontoon.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8933794 [details]
Bug 1422289 - ContentLinkHandler should guess icon type from the extension when type is not defined.

https://reviewboard.mozilla.org/r/204728/#review210624

::: browser/modules/ContentLinkHandler.jsm:141
(Diff revision 1)
> +    case "image/vnd.microsoft.icon":
> +    case TYPE_ICO:
> +      return TYPE_ICO;
> +    case TYPE_SVG:
> +      return TYPE_SVG;
> +  }

nit: I think this would be a bit more compact:
if (icon.type === "image/vnd.microsoft.icon") {
  return TYPE_ICO;
}
if (!icon.type) {
  let extension...
}
return icon.type || "";

::: browser/modules/ContentLinkHandler.jsm:144
(Diff revision 1)
> +    case TYPE_SVG:
> +      return TYPE_SVG;
> +  }
> +
> +  // Use the file extension to guess at a type we're interested in
> +  let extension = icon.iconUri.filePath.split(".").slice(-1)[0];

.pop()?

Comment 4

[Ed Lee :Mardak]

Comment on attachment 8933794 [details]
Bug 1422289 - ContentLinkHandler should guess icon type from the extension when type is not defined.

https://reviewboard.mozilla.org/r/204728/#review210624

> nit: I think this would be a bit more compact:
> if (icon.type === "image/vnd.microsoft.icon") {
>   return TYPE_ICO;
> }
> if (!icon.type) {
>   let extension...
> }
> return icon.type || "";

Combined the type check to the same line as fallback while keeping the extension check as only if there's not a type. There is a slight behavior change of the original patch would check the extension even if there was a type, e.g., `<link type="invalid" href="….ico" />` which might not be desired anyway… ? Although there's also a related issue of someone doing `<link type="image/svg+xml" href="….png" />` which would confuse our selection logic…

Also added a test to touch the vnd.microsoft.icon code path.

> .pop()?

Oh. Duh :p

Comment 5

[Ed Lee :Mardak]

Comment on attachment 8933794 [details]
Bug 1422289 - ContentLinkHandler should guess icon type from the extension when type is not defined.

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

Comment 6

[Ed Lee :Mardak]

Comment on attachment 8933794 [details]
Bug 1422289 - ContentLinkHandler should guess icon type from the extension when type is not defined.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87305b25064e
ContentLinkHandler should guess icon type from the extension when type is not defined. r=mak

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/87305b25064e