1917470 - Firefox started using apple-touch-icon for bookmarks?

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[eight04]

Attachment: out.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:132.0) Gecko/20100101 Firefox/132.0

Steps to reproduce:

Recently, I noticed that lots of bookmark icons changed after visiting the site. For example:

https://schedule.hololive.tv/
https://holodex.net/
https://www.gamer.com.tw/

Actual results:

See the attachment.

It seems that Firefox started using apple-touch-icon for bookmark icons.

Expected results:

I think the old favicon works better in this case. Also Firefox still uses favicon for tab icons.

Probably relate?
https://bugzilla.mozilla.org/show_bug.cgi?id=1772264

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::Bookmarks & History' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Marco Bonardo [:mak]]

(In reply to eight04 from comment #0)

It seems that Firefox started using apple-touch-icon for bookmark icons.

It always did, though the bug you linked to should have done exactly the opposite (prioritize non rich icons).

Looking at the database the rich flag is correctly set, it's likely we inadvertently inverted the condition.
This is luckily non destructive, a fix should be immediately effective.

Yazan, could you please check this?

Comment 3

[Marco Bonardo [:mak]]

The query seems correct, the problem is likely in the while loop after it, it stops once width < aPreferredWidth assuming width was decreasing across results, that is not true anymore.
I think for now we could change the first if to say lastWidth <= width, we can go fancier in the future in bug 1494016.
This also means the test likely needs improvement (rich icon should have larger width than icon)

Comment 4

[Yazan Al Macki [:yazan]]

Attachment: Bug 1917470 - Modify icon retrieval loop and test case to account for non width decreasing nature of new icon results. r?mak

Comment 5

[Yazan Al Macki [:yazan]]

Yes indeed, the problem came from the fact that the loop assumed it would find smaller better-fitting icons after processing the smallest non-rich icon (in the case that this icon is larger than the preferred size, and that the preferred size is lower than 64px in width). I submitted a patch to fix this.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80c9ebaa3dc0
Modify icon retrieval loop and test case to account for non width decreasing nature of new icon results. r=mak,places-reviewers

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/80c9ebaa3dc0

Comment 8

[Marco Bonardo [:mak]]

I can see that Nightly is now using the appropriate icon.