Closed Bug 1422289 Opened 2 years ago Closed 2 years ago

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

Categories

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

55 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 59
Iteration:
1.25
Tracking Status
firefox59 --- fixed

People

(Reporter: mak, Assigned: Mardak)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

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.
Assignee: nobody → edilee
Iteration: --- → 1.25
Component: General → Activity Streams: Newtab
The website still has the double <link rel=icon> with no type from bug 1421495, and this fix works for pontoon.
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()?
Attachment #8933794 - Flags: review?(mak77) → review+
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
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/87305b25064e
ContentLinkHandler should guess icon type from the extension when type is not defined. r=mak
https://hg.mozilla.org/mozilla-central/rev/87305b25064e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.