Closed Bug 1155429 Opened 6 years ago Closed 6 years ago

Add an explicit ImageCacheKey type for the ImageLib cache

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Right now, the ImageLib cache is keyed on URIs. That doesn't give us much flexibility - if we want to ignore certain parts of the URI, for example, or use an alternate notion of equality for some URIs (like in bug 1155332), that's quite tricky to do with this design.

Let's create an explicit key type, ImageCacheKey.

For this bug we'll retain the same behavior we have now; this is pure refactoring.
Here's the patch.

I've taken pains to ensure that we never create an ImageCacheKey (and hence
never hash a URI) unless we actually need the hash, since that's not free.
That's why I converted a few functions to just take |aForChrome| arguments,
since that's really all the information they needed.
Attachment #8593650 - Flags: review?(amarchesini)
Sorry, bad copy/paste on my part. When I mentioned bug 1155332 in comment 0, I meant bug 1154974.
Blocks: 1154974
No longer blocks: 1155332
Comment on attachment 8593650 [details] [diff] [review]
Add an explicit ImageCacheKey type for the ImageLib cache

Review of attachment 8593650 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgLoader.cpp
@@ +1098,5 @@
> +// ImageCacheKey
> +///////////////////////////////////////////////////////////////////////////////
> +
> +ImageCacheKey::ImageCacheKey(nsIURI* aURI)
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +1100,5 @@
> +
> +ImageCacheKey::ImageCacheKey(nsIURI* aURI)
> +{
> +  bool isChrome;
> +  mIsChrome = NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)) && isChrome;

same comment about resource://

@@ +1107,5 @@
> +  mHash = ComputeHash(mSpec);
> +}
> +
> +ImageCacheKey::ImageCacheKey(ImageURL* aURI)
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +1109,5 @@
> +
> +ImageCacheKey::ImageCacheKey(ImageURL* aURI)
> +{
> +  bool isChrome;
> +  mIsChrome = NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)) && isChrome;

here too.

::: image/src/imgLoader.h
@@ +219,5 @@
> + *
> + * We key the cache on the initial URI (before any redirects), with some
> + * canonicalization applied. See ComputeHash() for the details.
> + */
> +class ImageCacheKey final

It seems to be MOZ_STACK_CLASS only, right?

@@ +240,5 @@
> +
> +private:
> +  static uint32_t ComputeHash(const nsAutoCString& aSpec);
> +
> +  nsAutoCString mSpec;

nsAutoCStrring should be used just for stack-based string. Should it be nsCString instead... or add MOZ_STACK_CLASS.

::: image/src/imgRequest.cpp
@@ +457,5 @@
> +bool
> +imgRequest::IsChrome() const
> +{
> +  bool isChrome = false;
> +  if (NS_FAILED(mURI->SchemeIs("chrome", &isChrome))) {

NS_WARN_IF(NS_FAILED( ...

@@ +459,5 @@
> +{
> +  bool isChrome = false;
> +  if (NS_FAILED(mURI->SchemeIs("chrome", &isChrome))) {
> +    return false;
> +  }

often we use resource:// in addons, jetpack and other stuff.
Attachment #8593650 - Flags: review?(amarchesini) → review+
ah... the chrome vs resource seems to be a preexisting issue. Maybe can be a follow up if you think it makes sense.
Thanks for the review! I'll make most of those changes; a couple of responses below.

(In reply to Andrea Marchesini (:baku) from comment #6)
> @@ +1100,5 @@
> > +
> > +ImageCacheKey::ImageCacheKey(nsIURI* aURI)
> > +{
> > +  bool isChrome;
> > +  mIsChrome = NS_SUCCEEDED(aURI->SchemeIs("chrome", &isChrome)) && isChrome;
> 
> same comment about resource://

As you mentioned in comment 7, I'm just maintaining how things work right now: only chrome:// stuff goes in the chrome cache. I can see a good case that resource:// should go in there as well, though. We can make that change in a followup.

> > +ImageCacheKey::ImageCacheKey(ImageURL* aURI)
> > +{
> 
> MOZ_ASSERT(NS_IsMainThread());

An ImageURL is safe to use off-main-thread - that's actually the reason ImageURL exists. Agreed about the nsIURI constructor, though.

> It seems to be MOZ_STACK_CLASS only, right?

Nah, we store these in the hash table.
Blocks: 1159084
I filed bug 1159084 about the resource:// URI issue.

Also, I finally figured out the bug that was causing those test failures on try: nsAutoCString cannot be memmove'd, and therefore cannot be stored in many of our data structures. Sigh. Lesson learned.

I'll upload a revised version of the patch shortly that addresses the review comments, and then I think this might be ready to go.
OK, here's the final version of the patch.
Attachment #8593650 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/03f05d08f718
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1160703
You need to log in before you can comment on or make changes to this bug.