The frontend should pass an appropriate #size= hint to fetch favicons, that can be built using PlacesUtils.urlWithSizeRef(window, href, size). This is not a trivial replacement cause some components, like history views, autocomplete and so on, like to pass out an icon. In some cases (all the cases where we always pass out a page-icon) we could probably remove the dependency on the backend. Searching for "page-icon:" is probably a first good hint for points that need to be updated. This old changeset removing -moz-resolution fragment can also be used for finding further references that may need updates: https://bug1118926.bmoattachments.org/attachment.cgi?id=8668178

This may improve crispness in some cases.

Interesting, there's a problem here (thus the test failures). When we add a new icon we clear from the image cache the existing page-icon, but the image cache also stores an eventual ref, and that won't be cleared. Clearing all the possible refs is obviously unfeasible. We could clear just the one for the ui (16px) but that won't cover the issue completely. Or maybe we should just fix bug 1337404, but that won't be nice for UI performance where we show hundreds or thousands of icons. Maybe we should do both long term?

Timothy, would it be crazy and rejected a patch that ignores ref in generating the imageCacheKey for the page-icon protocol?

Even if actually, those could be different images, so it would be wrong for the image cache to return a 16px icon for a 32 px ref... I guess we may need to clear the image cache from the UI code where we know the size ref.

[Tracking Requested - why for this release]: Seems to be needed to prevent a UI regression in favicons shown in tabstrip, bug 1401777

(In reply to Daniel Holbert [:dholbert] from comment #6) > [Tracking Requested - why for this release]: > Seems to be needed to prevent a UI regression in favicons shown in tabstrip, > bug 1401777 This is not actually necessary, bug 1401777 can be fixed apart, this bug will only affect bookmarks and similar Places views, and we're ok with that for 57.

we should try to fix this for 58.

> this bug will only affect bookmarks and similar Places views, and we're ok with that for 57. It also affects the awesomebar.

the awesomebar is indeed a Places view. Well, most of the UI showing icons go through the same code, but the tabs. What is fixed is the tabs, all the rest is potentially affected.

Yesterday I was thinking to invert the approach here, rather than adding ref everywhere in the ui and complicate the image cache invalidation, I'd rather add an API to set the page uri protocol default size an let the few consumers needing larger icons to manage that apart. This should make sense because the most consumers need an icon in the "default" 16*dpi resolution, only a few consumers need very large icons, and those can manage their own needs. Currently the only consumer of big icons is AS, that goes through the API, so it should not be a problem.

Comment on attachment 8919514 [details] Bug 1347532 - Allow to set a default icon size for the page-icon protocol. https://reviewboard.mozilla.org/r/190346/#review196318 Looks good. r=Standard8

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/e38dd8c3f152 Allow to set a default icon size for the page-icon protocol. r=standard8