Closed Bug 1409969 Opened 4 years ago Closed 4 years ago
bookmarked favicon would not be updated
59 bytes, text/x-review-board-request
11.86 KB, image/png
16.21 KB, image/png
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.
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
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 firstname.lastname@example.org: 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
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.