Closed
Bug 1352408
Opened 8 years ago
Closed 8 years ago
Need a way to bypass image cache for certain loads or scheme
Categories
(Core :: Graphics: ImageLib, enhancement)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mak, Assigned: tnikkel)
References
Details
Attachments
(1 file)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
For new favicons I have a page-icon:page_url protocol that returns the best icon for a given url.
I'm using these urls in the ui to show favicons for the various nodes (bookmarks toolbar, menu library and so on).
The problem is that when a page is reloaded, the icon may change (maybe from the default one to the proper icon from the page) and then I need to change the icon in the UI.
I can't find a way to do that, the image cache keeps noticing the image url is the same and doesn't even start a new load.
I wonder if there's a way to bypass the cache for a specific schema, load, or clear the cache for a url when I know it's invalidated.
Fwiw, this sounds a lot like https://bugzilla.mozilla.org/show_bug.cgi?id=1202085#c42, but at least I'm on a custom protocol that gives me some sandboxing from global changes to the image cache.
Ideally I'd still like to use the image cache (so if a view has ten identical favicons we don't load all of them), but I really need a way to force a cache refresh when an icon changes.
I wonder if there's any way the cache could read some caching information from the image channel, for example if the channel has INHIBIT_CACHING or LOAD_BYPASS_CACHE.
Any ideas are very welcome since this is a show stopper for me, but I'd prefer avoiding fancy things like appending random query strings to my url.
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 1•8 years ago
|
||
How are you loading the images? Calling imgLoader::LoadImage?
Flags: needinfo?(mak77)
Reporter | ||
Comment 2•8 years ago
|
||
No, I'm just setting the "image" attribute into xul elements (menuitems, menus, also trees), that is likely changing list-style-image or src.
Off-hand I can't predict on which elements this may be used in the future, for example Activity Stream may use icons in their html UI.
My protocol just fetches payloads from the db and streams them out through a pipe with the proper content type.
Flags: needinfo?(mak77)
Reporter | ||
Comment 3•8 years ago
|
||
The only alternatives I see atm are:
1. completely blacklist this scheme from the cache. This is bad from a perf point of view.
2. have a way to tell to the cache that a certain url is now invalid
3. have a way to tell to the cache to refresh all urls with my scheme
Hope you may have better ideas.
In case you can fetch debug Try builds from this run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2fd5b4af803426d9d116d34f50e3f08ea6aa1e
Reporter | ||
Comment 4•8 years ago
|
||
The protocol implementation is in PageIconProtocolHandler.js, it is slightly modified by bug 977177 but the basics are not changing.
Reporter | ||
Comment 5•8 years ago
|
||
ehr, in comment 3, when I say refresh I actually mean "mark as invalid".
Assignee | ||
Comment 6•8 years ago
|
||
You don't need elements to have their image updated without setting their src again do you?
Assignee | ||
Comment 7•8 years ago
|
||
By that I mean that for any elements whose visual display you want to update you mutate the element so that it at least asks the image cache for an image (even if the image cache is returning the old image with the existing code)?
Reporter | ||
Comment 8•8 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #6)
> You don't need elements to have their image updated without setting their
> src again do you?
nope, I know when the image changes, I have a notification. I don't expect the image to magically change by itself.
Assignee | ||
Comment 9•8 years ago
|
||
Ehsan, what was the reason RemoveEntry was removed in bug 1202085? Can we just provide an updated version of RemoveEntry with the addition of a document argument?
Flags: needinfo?(ehsan)
Reporter | ||
Comment 10•8 years ago
|
||
I was able to confirm that the problem is with the image cache, removing the specific entry from it when I know the image is about to be invalidated solves it.
Reporter | ||
Comment 11•8 years ago
|
||
Sorry for nagging, but this a priority for us, we would like to have enough testing time in Nightly before the next merge and all this work is planned for 55.
Comment 12•8 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #9)
> Ehsan, what was the reason RemoveEntry was removed in bug 1202085? Can we
> just provide an updated version of RemoveEntry with the addition of a
> document argument?
Sorry for my delay in responding here.
The reason was this part of the patch set <https://hg.mozilla.org/integration/mozilla-inbound/rev/afc27ae3cc45> where we added the pointer to the controlled document to the hash key for image cache entries, so the previous API needed to be changed. IIRC there wasn't any consumers in the codebase so after some IRC discussion (around bug 1202085 comment 18) we agreed that removing the API is fine. I think there was some rough agreement that if we needed to bring it back again we just needed to add a document argument as you suggest here, so doing that now should be OK.
Flags: needinfo?(ehsan)
Assignee | ||
Comment 13•8 years ago
|
||
I'm not sure if uuids need to be updated at all anymore?
I made the function noscript because the snippet you showed me was C++. Do you need this to be scriptable? If not I'd prefer to have noscript because it's easy to make it scriptable in the future, harder to remove a scriptable interface.
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel) → needinfo?(mak77)
Attachment #8855955 -
Flags: review?(ehsan)
Reporter | ||
Comment 15•8 years ago
|
||
(and no, uuids don't need to be updated anymore)
Comment 16•8 years ago
|
||
Comment on attachment 8855955 [details] [diff] [review]
restore removeentry
Review of attachment 8855955 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/imgICache.idl
@@ +18,5 @@
> * @author Stuart Parmenter <pavlov@netscape.com>
> * @version 0.1
> * @see imagelib2
> */
> +[scriptable, builtinclass, uuid(9bfedca8-b20f-4427-aa65-1e9648753e6f)]
This isn't needed as Marco said but doesn't hurt either!
::: image/imgLoader.cpp
@@ +1374,5 @@
> }
>
> NS_IMETHODIMP
> +imgLoader::RemoveEntry(nsIURI* aURI,
> + nsIDOMDocument* aDOMDoc)
Nit: indentation.
Attachment #8855955 -
Flags: review?(ehsan) → review+
Comment 17•8 years ago
|
||
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52e2627b287
Restore imgICache::RemoveEntry. r=ehsan
Reporter | ||
Comment 18•8 years ago
|
||
Thank you for the help.
Comment 19•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•