ContentLinkHandler only picks perfectly sized icons

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Depends on 1 bug, Blocks 1 bug)

55 Branch
Firefox 61
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed, firefox61 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

Assignee

Description

a year ago
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 hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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)…
Attachment #8963066 - Flags: review?(edilee) → review+
Assignee

Comment 3

a year ago
(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

a year ago
Oh duh. Blocks. Some reason I totally skipped over that. :p
Comment hidden (mozreview-request)

Comment 6

a year ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/266a3c9eae61
ContentLinkHandler only picks perfectly sized icons. r=Mardak

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/266a3c9eae61
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee

Comment 8

a year ago
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
Attachment #8963066 - Flags: approval-mozilla-beta?
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.
Attachment #8963066 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

9 months ago
Depends on: 1481497
You need to log in before you can comment on or make changes to this bug.