1409969 - bookmarked favicon would not be updated

Status: RESOLVED FIXED

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

Description

[Alice0775 White]

Reproducible : always:

Steps To Reproduce:
1. open MDN from bookmark

Actual Results:
bookmarked favicon would not be updated

Expected Results:
Bookmarked favicon should be updated to new favicon

Comment 1

[Marco Bonardo [:mak]]

Please provide the exact url contained in the bookmark.

Comment 2

[Alice0775 White]

sure,
URL: https://developer.mozilla.org/en-US/Add-ons/WebExtensions

Comment 3

[Marco Bonardo [:mak]]

Ah, I misread the problem.
You mean the icon is present, but it's not the new one?
I see the Black&White icon in the bookmarks, the orange icon in the tab. The B&W icon is likely the rich icon.
If so, the patch in bug 1347532 fixes this for me.

Comment 4

[Alice0775 White]

(In reply to Marco Bonardo [::mak] from comment #3)
> Ah, I misread the problem.
> You mean the icon is present, but it's not the new one?

yes.

> I see the Black&White icon in the bookmarks, the orange icon in the tab. The
> B&W icon is likely the rich icon.
> If so, the patch in bug 1347532 fixes this for me.

Okay, thanks.

Comment 5

[Marco Bonardo [:mak]]

Looks like this works fine for a new profile, but not for an old profile. Could be the 7 days expiration rule, that'll need a few more days to confirm. Otherwise, we still have bug 1403829 to look into better icon picks.

Comment 6

[Marco Bonardo [:mak]]

Interesting, in the db I have a 32px icon and a 144px icon, based on my dpi (125%) I should get the 32px icon. There may be a bug somewhere, I'll investigate.

Comment 7

[Marco Bonardo [:mak]]

ah, there is also a root domain icon 32x32 that is white.

Comment 8

[Marco Bonardo [:mak]]

Found the bug, if there are multiple 32px icons, and we require a 20px icon, we pick the last 32px icon, while we should instead pick the first. By picking the last, we end up picking the root icon.
http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/toolkit/components/places/FaviconHelpers.cpp#441

I'll fix this in the next days.

Comment 9

[Marco Bonardo [:mak]]

Attachment: Bug 1409969 - The root domain icon should not be preferred if 2 icons for the same size exist.

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

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8920807 [details]
Bug 1409969 - The root domain icon should not be preferred if 2 icons for the same size exist.

https://reviewboard.mozilla.org/r/191792/#review197192

I can't reproduce this locally, but the test looks to cover it. So r=Standard8

Comment 11

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #10)
> I can't reproduce this locally, but the test looks to cover it. So
> r=Standard8

It depends on DPI, if you're on Mac, dpi is 2x, so we query a 32px icon. on my Windows box I use 125%, so we query for a 20px icon, the basic problem here happens when we request a non-perfect size and we round up with the immediate bigger size we support.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb1f48b6e2d6
The root domain icon should not be preferred if 2 icons for the same size exist. r=standard8

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 14

[atlanto]

Excuse me.
I'm using Windows10 and Linux with 96dpi. Maybe required favicon size is 16px.
At least on twitter.com and developer.mozilla.org, the root favicon is 16px.
As a result, the root favicon is selected on those sites.(after the root favicon was fetched)
(58.0a1, Build ID 20171023220222)

Data in favicons.sqlite

sqlite> select * from moz_icons;
1|https://abs.twimg.com/icons/apple-touch-icon-192x192.png|115765101|192|0||1509410980094|?PNG
2|https://abs.twimg.com/favicons/favicon.ico|2081382807|32|0||1509410980118|?PNG
3|https://cdn.mdn.mozilla.net/static/img/favicon144.0d89fc050967.png|463105214|144|0||1509410986487|?PNG
4|https://cdn.mdn.mozilla.net/static/img/favicon32-local.329e8131018f.png|2471753525|32|0||1509410986510|?PNG
5|https://developer.mozilla.org/favicon.ico|1346301517|48|1||1509410988913|?PNG
6|https://developer.mozilla.org/favicon.ico|1346301517|32|1||1509410988913|?PNG
7|https://developer.mozilla.org/favicon.ico|1346301517|16|1||1509410988913|?PNG
8|https://twitter.com/favicon.ico|3854518688|16|1||1508806197089|?PNG

sqlite> select * from moz_icons_to_pages;
1|1
1|2
2|3
2|4

sqlite> select * from moz_pages_w_icons;
1|https://twitter.com/FirefoxNightly|47356985947419
2|https://developer.mozilla.org/en-US/|47357077650470

The root favicon should not be selected when the page has its own favicon link.

Comment 15

[atlanto]

Attachment: Good case - until the root favicon is fetched

Comment 16

[atlanto]

Attachment: Bad case - after the root favicon was fetched

Comment 17

[Marco Bonardo [:mak]]

(In reply to atlanto from comment #14)
> The root favicon should not be selected when the page has its own favicon
> link.

This has nothing to do with this bug fix.
This bug fix is about the case when the db contains 2 icons of the same size, your database contains only 1 16px icon, and that's what is picked, regardless of being the root icon or not. In bug 1403829 I'll tweak the selection algorithm to try to pick a better icon depending on the screen dpi. The decision to favor the resolution over the root-iness is just a code choice.