Bug 132928

Summary: Add image to cache
Product: WebKit Reporter: Martin Hock <mhock>
Component: WebCore Misc.Assignee: Martin Hock <mhock>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, japhet, psolanki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
ap: review+
API test
none
address comments ap: review+

Description Martin Hock 2014-05-14 16:06:22 PDT
Restore the call to add in MemoryCache::addImageToCache erroneously removed in r165117 and return the result.
<rdar://problem/16651547>
Comment 1 Martin Hock 2014-05-14 16:10:24 PDT
Created attachment 231470 [details]
patch
Comment 2 Alexey Proskuryakov 2014-05-15 10:48:05 PDT
Comment on attachment 231470 [details]
patch

Is there any way to do a regression test for this fix?
Comment 3 Martin Hock 2014-05-15 11:02:47 PDT
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.
Comment 4 Martin Hock 2014-05-15 14:35:13 PDT
Created attachment 231535 [details]
API test
Comment 5 Alexey Proskuryakov 2014-05-15 15:07:10 PDT
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.
Comment 6 Pratik Solanki 2014-05-15 15:10:57 PDT
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);
Comment 7 Martin Hock 2014-05-15 19:34:25 PDT
(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.
Comment 8 Pratik Solanki 2014-05-16 00:12:16 PDT
(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.
Comment 9 Martin Hock 2014-05-16 00:30:05 PDT
Created attachment 231561 [details]
address comments
Comment 10 Martin Hock 2014-05-16 00:33:08 PDT
(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.
Comment 11 Martin Hock 2014-05-16 10:07:55 PDT
Committed r168970: <http://trac.webkit.org/changeset/168970>