Closed
Bug 1423506
Opened 7 years ago
Closed 7 years ago
Tippytop does not honor preference browser.chrome.site_icons
Categories
(Firefox :: New Tab Page, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | unaffected |
firefox58 | + | wontfix |
firefox59 | --- | fixed |
People
(Reporter: underpass_bugzilla, Assigned: Mardak)
References
Details
(Keywords: regression)
Attachments
(3 files)
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
Comment 1•7 years ago
|
||
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
Flags: needinfo?(edilee)
Comment 2•7 years ago
|
||
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?
Flags: needinfo?(underpass_bugzilla)
(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.
Flags: needinfo?(underpass_bugzilla)
Comment 5•7 years ago
|
||
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?
Flags: needinfo?(underpass_bugzilla)
(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.
Flags: needinfo?(underpass_bugzilla)
Assignee | ||
Comment 7•7 years ago
|
||
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.)
Flags: needinfo?(edilee)
Keywords: regression
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
Assignee | ||
Comment 8•7 years ago
|
||
Underpass, does setting browser.newtabpage.activity-stream.tippyTop.service.endpoint to empty "" string and clearing favicons.sqlite avoid the favicons from showing up?
Flags: needinfo?(underpass_bugzilla)
Assignee | ||
Comment 9•7 years ago
|
||
Oh, and when you delete `favicons.sqlite`, also delete `activity-stream.tippytop.json`
Assignee | ||
Updated•7 years ago
|
Reporter | ||
Comment 10•7 years ago
|
||
Hello, apparently, when you set browser.newtabpage.activity-stream.tippyTop.service.endpoint to empty and delete the files, the icons aren't created anymore.
Flags: needinfo?(underpass_bugzilla)
Assignee | ||
Comment 11•7 years ago
|
||
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?
Flags: needinfo?(mak77)
Assignee | ||
Updated•7 years ago
|
Summary: Upgrading to 58b9 the preference browser.chrome.site_icons is no more honored → Tippytop does not honor preference browser.chrome.site_icons
Comment 12•7 years ago
|
||
Comment 13•7 years ago
|
||
(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.
Flags: needinfo?(mak77)
Comment 14•7 years ago
|
||
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
Updated•7 years ago
|
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → edilee
Iteration: --- → 1.25
Priority: -- → P1
Assignee | ||
Comment 15•7 years ago
|
||
[Tracking Requested - why for this release]: This is a regression in 58 from bug 1421306 for an about:config pref
tracking-firefox58:
--- → ?
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
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+
Comment 18•7 years ago
|
||
Pushed by edilee@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b41233cc2b97 Tippytop does not honor preference browser.chrome.site_icons. r=k88hudson
Assignee | ||
Updated•7 years ago
|
Target Milestone: --- → Firefox 59
Assignee | ||
Comment 21•7 years ago
|
||
Can somebody verify the fix on Nightly? Most likely will need to set site_icons pref to false and also delete favicons.sqlite
Comment 22•7 years ago
|
||
(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?
Flags: needinfo?(underpass_bugzilla)
Updated•7 years ago
|
Reporter | ||
Comment 23•7 years ago
|
||
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
Flags: needinfo?(underpass_bugzilla)
Assignee | ||
Comment 24•7 years ago
|
||
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.)
Flags: needinfo?(underpass_bugzilla)
Reporter | ||
Comment 25•7 years ago
|
||
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.
Flags: needinfo?(underpass_bugzilla)
Assignee | ||
Comment 26•7 years ago
|
||
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?
Flags: needinfo?(edilee)
Comment 27•7 years ago
|
||
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).
Updated•5 years ago
|
Component: Activity Streams: Newtab → New Tab Page
You need to log in
before you can comment on or make changes to this bug.
Description
•