Closed Bug 1494016 Opened 6 years ago Closed 3 months ago

When picking a favicon, pick the one with the shortest size difference

Categories

(Toolkit :: Places, enhancement, P3)

Product:
Toolkit
Component:
Places
Type:
enhancement
Points:
5

Tracking

()

Status:
RESOLVED FIXED
Milestone:
133 Branch
Tracking Flags:
Tracking Status
firefox133 --- fixed

People

(Reporter: mak, Assigned: yazan)

References

(Blocks 4 open bugs, Regressed 2 open bugs)

Details

(Keywords: papercut, Whiteboard: [snt-scrubbed][places-papercut][favicons-2024])

User Story

Fix Bug 1772264 first, we must be able to distinguish rich favicons from others in the database.

When serving "small-ish" icons (<= 64px), always filter our rich icons. This can be first part/revision here.

Then among the remaining icons, when there's no perfect match, consider both the smaller and the larger ones. If the larger one is >= 200% the requested size, and the smaller is >= 50% the requested size, prefer the smaller ((size_requested - size_small) >= (size_big - size_requested) / 4)
This second part is more complex and can be moved to a follow-up bug. 

See comment 7 for some code pointers.

Attachments

(1 file, 1 obsolete file)

Bug 1494016 - Allow smaller icons to be returned based on a size difference comparison with larger icon picked, and account for SVG icons during selection. r?mak
48 bytes, text/x-phabricator-request
Details | Review
Suppose that a website provides a 16px icon and a 200px icon. When the favicons API gets a request for a 32px icon, it will currently return the 200px icon, because the 16px one would appear blocky. Though, with a so large difference it's possible the 200px icon is not suited to be scaled down to 32px, it may become unrecognizable. The 16px icon may be a better pick, even if blocky, because the 200px icon should be scaled down too much. We can improve the pick here by checking the scaling factor, and picking the one that better fits a given threshold.
Priority: -- → P3
Whiteboard: [fxsearch]
Blocks: 1494772

and while we check the scaling factor, if it's just 2, we could still prefer the local favicon to the root one.

Blocks: 1575809

Do you think this would be a good first bug? If so could you tag it as such? Thanks!

Flags: needinfo?(mak77)

It involves Cpp and complex SQL, plus some calculation, it could be mentored maybe, but it's not a good first one.

Flags: needinfo?(mak77)

I just wanted to comment that this issue has been affecting my work.

I made a helpful example page with steps to reproduce the issue and verify it easily:

https://firefox-favicon-ico-size-bug-retina.netlify.app/

(Code on GitHub: https://github.com/wavebeem/firefox-favicon-ico-size-bug-retina)

yeah, there's a couple things we should do to improve favicons matching:

  1. annotate in the db if an icon is a rich one (apple-touch). When serving a favicon at common small sizes (16-64) prefer the non-rich ones
  2. when there's no perfect match, pick shortest size distance instead of the next larger one
Keywords: papercut
Severity: normal → S3
Whiteboard: [fxsearch] → [snt-scrubbed][places-papercut]
Assignee: nobody → scunnane

For the first part, identifying a rich icon and storing the info, you can look here
https://searchfox.org/mozilla-central/rev/6f77213807eb5359c8afe458ac5628f973e92a25/browser/actors/LinkHandlerChild.jsm#112,115-118,130
then it goes through https://searchfox.org/mozilla-central/source/browser/modules/FaviconLoader.jsm that loads it in content
finally it is stored in the parent process https://searchfox.org/mozilla-central/rev/6f77213807eb5359c8afe458ac5628f973e92a25/browser/actors/LinkHandlerParent.jsm#141-154

The storing is done through 2 APIs:
https://searchfox.org/mozilla-central/rev/6f77213807eb5359c8afe458ac5628f973e92a25/browser/components/places/PlacesUIUtils.sys.mjs#226-231,234-241
the first one sets the icon in a memory cache, while the second one copies it from the memory cache into the database (why these are separate is an historical fact, we would like to change it in the future into a single API)

The storing process is async in cpp, an object is created (actually pulled from the memory cached) containing the icon info and passed to a runnable in the async thread
https://searchfox.org/mozilla-central/rev/6f77213807eb5359c8afe458ac5628f973e92a25/toolkit/components/places/nsFaviconService.cpp#378,437-441
The icon is finally stored
https://searchfox.org/mozilla-central/rev/6f77213807eb5359c8afe458ac5628f973e92a25/toolkit/components/places/FaviconHelpers.cpp#164,208-210,1050,1068

You'll have to add a new column to the icons table, there's an example in Bug 1776606. And then pass-through the rich information, and store it in this new rich boolean column (that in practice is an integer column because in Sqlite bool is stored as a 0-sized int)

For the second part, the selection of which icon to return is done here:
https://searchfox.org/mozilla-central/rev/6f77213807eb5359c8afe458ac5628f973e92a25/toolkit/components/places/FaviconHelpers.cpp#388-408
It may be part SQL and part code, no obligation.

I'm happy to help with it, also on Zoom if you wish, or answer questions. This bug may be an interesting learning process.

See Also: → 1801614
Depends on: 1772264
Whiteboard: [snt-scrubbed][places-papercut] → [snt-scrubbed][places-papercut][favicons-2024]
Points: --- → 5
User Story: (updated)
Assignee: scunnane → nobody
Blocks: 1359487
Assignee: nobody → yalmacki
Blocks: 1419039
Attachment #9424443 - Attachment description: Bug 1494016 - Modified FetchIconPerSpec to allow smaller icons to be returned based on a size difference comparison with larger icon picked. r?mak → Bug 1494016 - Allow smaller icons to be returned based on a size difference comparison with larger icon picked, and account for SVG icons during selection. r?mak
Attachment #9424443 - Attachment is obsolete: true
Pushed by yalmacki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ec6364b47f31 Allow smaller icons to be returned based on a size difference comparison with larger icon picked, and account for SVG icons during selection. r=mak,places-reviewers

https://hg.mozilla.org/mozilla-central/rev/ec6364b47f31

Status: NEW → RESOLVED
Closed: 3 months ago
status-firefox133: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 133 Branch
See Also: 1801614 → 1337397
Regressions: 1933158
Regressions: 1933921
Regressions: 1936587
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: