Bug 132928 – Add image to cache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Martin Hock]

Restore the call to add in MemoryCache::addImageToCache erroneously removed in r165117 and return the result.
<rdar://problem/16651547>

[Martin Hock]

Created attachment 231470 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 231470 [details]
patch

Is there any way to do a regression test for this fix?

[Martin Hock]

I create(In reply to comment #2)
> (From update of attachment 231470 [details])
> Is there any way to do a regression test for this fix?

I created https://bugs.webkit.org/show_bug.cgi?id=132962 to track adding some unit tests to MemoryCache.

[Martin Hock]

Created attachment 231535 [details]
API test

[Alexey Proskuryakov]

Comment on attachment 231535 [details]
API test

View in context: https://bugs.webkit.org/attachment.cgi?id=231535&action=review

> Tools/TestWebKitAPI/Tests/Cocoa/MemoryCacheAddImageToCache.mm:34
> +#if TARGET_OS_IPHONE

Would it be possible to create a separate directory for iOS tests, like we have for Mac ones?

> Tools/TestWebKitAPI/Tests/Cocoa/MemoryCacheAddImageToCache.mm:35
> +    CGColorSpaceRef rgb = CGColorSpaceCreateDeviceRGB();

This looks like it leaks.

> Tools/TestWebKitAPI/Tests/Cocoa/MemoryCacheAddImageToCache.mm:39
> +    NSURL* url = [NSURL URLWithString:@"about:blank"];

Misplaced star.

[Pratik Solanki]

Comment on attachment 231535 [details]
API test

View in context: https://bugs.webkit.org/attachment.cgi?id=231535&action=review

This is great! Thanks for adding this test.

> Tools/TestWebKitAPI/Tests/Cocoa/MemoryCacheAddImageToCache.mm:41
> +    EXPECT_TRUE([WebCache imageForURL:url] == image);

Let's test removeImageFrom cache as well. Something like this perhaps

[WebCache removeImageFromCacheForURL:url];
EXPECT_TRUE([WebCache imageForURL:url] == 0);

[Martin Hock]

(In reply to comment #6)
> (From update of attachment 231535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231535&action=review
> 
> This is great! Thanks for adding this test.
> 
> > Tools/TestWebKitAPI/Tests/Cocoa/MemoryCacheAddImageToCache.mm:41
> > +    EXPECT_TRUE([WebCache imageForURL:url] == image);
> 
> Let's test removeImageFrom cache as well. Something like this perhaps
> 
> [WebCache removeImageFromCacheForURL:url];
> EXPECT_TRUE([WebCache imageForURL:url] == 0);

Actually, it turns out that removeImageFromCache doesn't immediately remove the image from the cache since it's manually cached. See the following comment:

    // Removing the last client of a CachedImage turns the resource
    // into a dead resource which will eventually be evicted when
    // dead resources are pruned. That might be immediately since
    // removing the last client triggers a MemoryCache::prune, so the
    // resource may be deleted after this call.

But since the MemoryCache is not full, prune does nothing.

[Pratik Solanki]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 231535 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=231535&action=review
> > 
> > This is great! Thanks for adding this test.
> > 
> > > Tools/TestWebKitAPI/Tests/Cocoa/MemoryCacheAddImageToCache.mm:41
> > > +    EXPECT_TRUE([WebCache imageForURL:url] == image);
> > 
> > Let's test removeImageFrom cache as well. Something like this perhaps
> > 
> > [WebCache removeImageFromCacheForURL:url];
> > EXPECT_TRUE([WebCache imageForURL:url] == 0);
> 
> Actually, it turns out that removeImageFromCache doesn't immediately remove the image from the cache since it's manually cached. See the following comment:

Thanks for the explanation. I think your test is fine in that case.

[Martin Hock]

Created attachment 231561 [details]
address comments

[Martin Hock]

(In reply to comment #5)
> (From update of attachment 231535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231535&action=review
> 
> > Tools/TestWebKitAPI/Tests/Cocoa/MemoryCacheAddImageToCache.mm:34
> > +#if TARGET_OS_IPHONE
> 
> Would it be possible to create a separate directory for iOS tests, like we have for Mac ones?

I created an "ios" directory. There already was a similar directory at another level of the project, so I followed the pattern it established of ending the filename in IOS.

> > Tools/TestWebKitAPI/Tests/Cocoa/MemoryCacheAddImageToCache.mm:35
> > +    CGColorSpaceRef rgb = CGColorSpaceCreateDeviceRGB();
> 
> This looks like it leaks.

Fixed (along with the other leaks).

> > Tools/TestWebKitAPI/Tests/Cocoa/MemoryCacheAddImageToCache.mm:39
> > +    NSURL* url = [NSURL URLWithString:@"about:blank"];
> 
> Misplaced star.

Fixed.

[Martin Hock]

Committed r168970: <http://trac.webkit.org/changeset/168970>