Closed Bug 1409969 Opened 4 years ago Closed 4 years ago

bookmarked favicon would not be updated

Categories

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

x86_64
Windows 10
defect

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: alice0775, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files)

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
Please provide the exact url contained in the bookmark.
Flags: needinfo?(alice0775)
sure,
URL: https://developer.mozilla.org/en-US/Add-ons/WebExtensions
Flags: needinfo?(alice0775)
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.
Depends on: 1347532
(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.
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.
Depends on: 1403829
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.
ah, there is also a root domain icon 32x32 that is white.
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [fxsearch]
No longer depends on: 1403829
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
Attachment #8920807 - Flags: review?(standard8) → review+
(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.
Pushed by mak77@bonardo.net:
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
https://hg.mozilla.org/mozilla-central/rev/bb1f48b6e2d6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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.
(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.
You need to log in before you can comment on or make changes to this bug.