|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
55.21 KB, image/png
52 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
59 bytes, text/x-review-board-request
|Details | Review|
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20171204150510 Steps to reproduce: Hello, I keep the preference browser.chrome.favicons set to false in about:config and until version 58b8 no favicon was shown in the bookmarks toolbar This is what I found updating Firefox from 58b8 to 58b9: Actual results: - Updated to Firefox 58b9: since then, I see some favicons in the bookmark toolbar - Tried to empty browser cache = no effect - Tried to close Firefox and delete favicons.sqlite from profile folder = icons are re-created upon restart - Tried to delete favicons.sqlite and downgrade Firefox to version 58b8 = favicons are no more present and the preference is honored - Tried to upgrade Firefox again to 58b9 = favicons are shown again Expected results: Like in previous version, I expect that the preference browser.chrome.favicons is honored and no favicon is shown in the bookmarks toolbar. Thanks for your attention
Maybe bug 1421306 is storing some favicons. I can't find a good list of changesets between b8 and b9 yet and I can't think of icon patches we recently uplifted. Mardak, do you think that's a possiblity?
Component: Bookmarks & History → Activity Streams: Newtab
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID 20171130160223 I've tried to reproduce this issue on Windows 10 and 7 using 58.0b7, 58.0b8 and 58.0b9. And in all of these builds I can't make the favicons from the browser toolbar disappear using the "browser.chrome.favicons" pref. They always show up, so there might have been some extra options/prefs that were changed. @Underpass, could you try to reproduce the issue on 58.0b8 using a new profile and post your results?
bug 1421306 got uplifted to 58 and handles favicons.
(In reply to Ciprian Muresan [:cmuresan], Desktop Engineering QA from comment #2) > User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) > Gecko/20100101 Firefox/58.0 > Build ID 20171130160223 > > I've tried to reproduce this issue on Windows 10 and 7 using 58.0b7, 58.0b8 > and 58.0b9. And in all of these builds I can't make the favicons from the > browser toolbar disappear using the "browser.chrome.favicons" pref. They > always show up, so there might have been some extra options/prefs that were > changed. > > @Underpass, could you try to reproduce the issue on 58.0b8 using a new > profile and post your results? Hello, sorry then probably the preference is browser.chrome.site_icons. I managed to make them disappear setting the preference to false, closing Firefox and deleting the favicons.sqlite file.
Created attachment 8934978 [details] IMG_06122017_163236_0.png I've set browser.chrome.site_icons and browser.chrome.favicons to false, deleted the favicons.sqlite file from my profile, opened the browser, navigated to a few websites and my tabs no longer had favicons, but the bookmarks that I've made still had a "generic" favicon. I've then updated to 58.0b9 and I still don't have favicons in tabs and the generic ones persisted in the bookmarks toolbar. Were the generic favicons supposed to not be displayed as well?
(In reply to Ciprian Muresan [:cmuresan], Desktop Engineering QA from comment #5) > Were the generic favicons supposed to not be displayed as well? No, the generic favicon (the globe) should be present.
Bug 1421306 did introduce a new way that favicons get stored, and I see that ContentLinkHandler does skip adding an icon if `.site_icons` is `false`: https://searchfox.org/mozilla-central/source/browser/modules/ContentLinkHandler.jsm#336 Bug 1421306 did not have logic checking for that pref, so there could indeed be a bug assuming we rely on all potential sources to *not* add favicons based on the pref vs additionally having consumers check the pref to *not* show site icons. (Where the latter means once favicons get stored, they will be shown independent of the pref.)
Summary: Upgrading to 58b9 the preference browser.chrome.favicons is no more honored → Upgrading to 58b9 the preference browser.chrome.site_icons is no more honored
Underpass, does setting browser.newtabpage.activity-stream.tippyTop.service.endpoint to empty "" string and clearing favicons.sqlite avoid the favicons from showing up?
Oh, and when you delete `favicons.sqlite`, also delete `activity-stream.tippytop.json`
status-firefox58: --- → affected
status-firefox59: --- → affected
Hello, apparently, when you set browser.newtabpage.activity-stream.tippyTop.service.endpoint to empty and delete the files, the icons aren't created anymore.
mak, any thoughts on fixing this to uplift to 58? Necessary for this about:config pref? Approach of checking pref to not add vs checking pref to not show? Or maybe whether to keep maintaining the pref?
Summary: Upgrading to 58b9 the preference browser.chrome.site_icons is no more honored → Tippytop does not honor preference browser.chrome.site_icons
Created attachment 8935248 [details] [review] Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/3898
(In reply to Ed Lee :Mardak from comment #11) > mak, any thoughts on fixing this to uplift to 58? Necessary for this > about:config pref? Approach of checking pref to not add vs checking pref to > not show? Or maybe whether to keep maintaining the pref? I'm not sure I understand the question, but since it's a regression I think we should uplift it to 58 if possible. Note the full check is ShouldLoadFavicon, that checks both prefs and the scheme, it seems to make sense to me, maybe it should be shared more broadly, or even done directly by Places? https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/browser/base/content/tabbrowser.xml#1057 https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/browser/base/content/tabbrowser.xml#1093 The prefs situations looks as bad as it could be. I cannot understand the difference between browser.chrome.site_icons and browser.chrome.favicons, why do we have 2? Imo browser.chrome.favicons should be migrated to browser.chrome.site_icons and removed.
Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/f0e1304ce05e16a8a0592f029b7d9462d5a43222 fix(favicons): Check endpoint and site_icons prefs before fetching icons (#3898) Fix Bug 1423506 - Tippytop does not honor preference browser.chrome.site_icons
Status: UNCONFIRMED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Assignee: nobody → edilee
Iteration: --- → 1.25
Priority: -- → P1
[Tracking Requested - why for this release]: This is a regression in 58 from bug 1421306 for an about:config pref
tracking-firefox58: --- → ?
Comment on attachment 8935881 [details] Bug 1423506 - Tippytop does not honor preference browser.chrome.site_icons. https://reviewboard.mozilla.org/r/206744/#review212558 Looks good
Attachment #8935881 - Flags: review?(khudson) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/b41233cc2b97 Tippytop does not honor preference browser.chrome.site_icons. r=k88hudson
status-firefox59: affected → fixed
Target Milestone: --- → Firefox 59
Please request Beta approval on this when you get a chance.
status-firefox57: --- → unaffected
status-firefox-esr52: --- → unaffected
Can somebody verify the fix on Nightly? Most likely will need to set site_icons pref to false and also delete favicons.sqlite
(In reply to Ed Lee :Mardak from comment #21) > Can somebody verify the fix on Nightly? Most likely will need to set > site_icons pref to false and also delete favicons.sqlite Underpass, can you verify the fix?
Hello, I've updated Firefox to the last beta, reset the "Tippytop" preference, set the browser.chrome.site_icons preference to false and deleted the favicons.sqlite. Upon restarting the problems seems to be solved. Thanks a lot. (In reply to Marco Bonardo [::mak] (Away 16 Dec - 2 Jan) from comment #13) > I cannot understand the difference between browser.chrome.site_icons and > browser.chrome.favicons, why do we have 2? > Imo browser.chrome.favicons should be migrated to browser.chrome.site_icons > and removed. About this, I've found these two pages on the MozillaZine KB http://kb.mozillazine.org/Browser.chrome.favicons http://kb.mozillazine.org/Browser.chrome.site_icons
The fix hasn't actually made it to beta. Could you try restarting another time to see if it causes the icons to appear, i.e., run into this bug. (I believe your prefs and empty favicons are in the correct state, so no need to change / delete them, and just restart.)
Hello, I've restarted the browser and the icons don't appear (saved a couple of bookmarks to see them). Then I picked the favicons.sqlite file and opened it with the SQLite browser and didn't find any reference to these bookmarked pages.
From comment 23, if this is no longer a problem on latest beta, perhaps there's no more that needs to be done here for 58?
We don't have a worksforme status flag, so I'm calling Fx58 wontfix based on the recent comments in the bug (because we're not intending to uplift any fixes to Beta at this point).
status-firefox58: affected → wontfix
You need to log in before you can comment on or make changes to this bug.