Restore the call to add in MemoryCache::addImageToCache erroneously removed in r165117 and return the result. <rdar://problem/16651547>
Created attachment 231470 [details] patch
Comment on attachment 231470 [details] patch Is there any way to do a regression test for this fix?
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.
Created attachment 231535 [details] API test
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 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);
(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.
(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.
Created attachment 231561 [details] address comments
(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.
Committed r168970: <http://trac.webkit.org/changeset/168970>