1449523 - ContentLinkHandler only picks perfectly sized icons

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Marco Bonardo [:mak]]

When the page only defines sized pngs, we currently pick one only if it's perfectly sized.
For example, at 250%, we'd need a 40px icon to show a 16px one, most pages won't provide a 40px icon, but they could provide a 96px icon.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1449523 - ContentLinkHandler only picks perfectly sized icons.

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

Comment 2

[Ed Lee :Mardak]

Comment on attachment 8963066 [details]
Bug 1449523 - ContentLinkHandler only picks perfectly sized icons.

https://reviewboard.mozilla.org/r/231926/#review237690

Curious, did you just notice it on some webpage or someone mentioned?

::: browser/modules/ContentLinkHandler.jsm:185
(Diff revision 1)
>        } else if (guessType(icon) == TYPE_ICO && (!preferredIcon || guessType(preferredIcon) == TYPE_ICO)) {
>          preferredIcon = icon;
>        }
> +
> +      if (icon.width >= preferredWidth &&
> +          (!bestSizedIcon || bestSizedIcon.width > icon.width)) {

Moving the comment closer and correcting it to refer to "the larger yet closest to preferredWidth" (where currently "first one larger" seems to imply any icon larger.

On that note of "first," do we actually want `bestSizedIcon.width >= icon.width` where the = will allow for whatever reason multiple icons of the same width to prefer the latest one (to be consistent with the logic of the previous block)…

Comment 3

[Marco Bonardo [:mak]]

(In reply to Ed Lee :Mardak from comment #2)
> Curious, did you just notice it on some webpage or someone mentioned?

recent reports in  bug 1262982.

Comment 4

[Ed Lee :Mardak]

Oh duh. Blocks. Some reason I totally skipped over that. :p

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8963066 [details]
Bug 1449523 - ContentLinkHandler only picks perfectly sized icons.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/266a3c9eae61
ContentLinkHandler only picks perfectly sized icons. r=Mardak

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/266a3c9eae61

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8963066 [details]
Bug 1449523 - ContentLinkHandler only picks perfectly sized icons.

Approval Request Comment
[Feature/Bug causing the regression]: Hi dpi icons feature (landed in 55)
[User impact if declined]: Some popular pages like Reddit show a very blurry icon on hi-dpi screens
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a simple fix with test coverage
[String changes made/needed]: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8963066 [details]
Bug 1449523 - ContentLinkHandler only picks perfectly sized icons.

Fixes ugly favicons on HiDPI screens under easily-hit circumstances and includes new tests for bonus points. Approved for 60.0b9.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0adb888b0fa4