[webkit-reviews] review granted: [Bug 54501] WebKit2: Need a way to manage the WebCore Cache : [Attachment 82585] [PATCH] Fix + Qt (Take 4)

[bugzilla-daemon at webkit.org]

Jon Honeycutt <jhoneycutt at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 54501: WebKit2: Need a way to manage the WebCore Cache
https://bugs.webkit.org/show_bug.cgi?id=54501

Attachment 82585: [PATCH] Fix + Qt (Take 4)
https://bugs.webkit.org/attachment.cgi?id=82585&action=review

------- Additional Comments from Jon Honeycutt <jhoneycutt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=82585&action=review

r=me. Please investigate whether it's possible to send SecurityOrigins without
converting to string and back.

> Source/WebCore/loader/cache/MemoryCache.cpp:477
> +void MemoryCache::removeResourcesWithOrigin(RefPtr<SecurityOrigin> origin)

I think this should take a raw pointer.

> Source/WebCore/loader/cache/MemoryCache.cpp:495
> +void MemoryCache::originsWithCache(HashSet<String>& origins)

"getOriginsWithCache" would better indicate the purpose.

> Source/WebKit2/UIProcess/WebCacheManagerProxy.cpp:86
> +    Vector<RefPtr<APIObject> > securityOrigins(originIdentifiersCount);
> +
> +    for (size_t i = 0; i < originIdentifiersCount; ++i) {
> +	   RefPtr<APIObject> origin =
WebSecurityOrigin::createFromString(originIdentifiers[i]);
> +	   if (!origin)
> +	       continue;
> +	   securityOrigins[i] = origin;
> +    }

If WebSecurityOrigin::createFromString() can return null, it might be better to
use reserveCapacity and uncheckedAppend to prevent null entries in the vector.

> Source/WebKit2/UIProcess/API/C/WKCacheManager.cpp:60
> +#ifdef __BLOCKS__
> +static void callGetCacheOriginsBlockBlockAndDispose(WKArrayRef resultValue,
WKErrorRef errorRef, void* context)
> +{
> +    WKCacheManagerGetCacheOriginsBlock block =
(WKCacheManagerGetCacheOriginsBlock)context;
> +    block(resultValue, errorRef);
> +    Block_release(block);
> +}
> +
> +void WKCacheManagerGetCacheOrigins_b(WKCacheManagerRef cacheManagerRef,
WKCacheManagerGetCacheOriginsBlock block)
> +{
> +    WKCacheManagerGetCacheOrigins(cacheManagerRef, Block_copy(block),
callGetCacheOriginsBlockBlockAndDispose);
> +}
> +#endif

You might remove this and add it later as needed.

> Source/WebKit2/UIProcess/API/C/WKCacheManager.h:42
> +#ifdef __BLOCKS__
> +typedef void (^WKCacheManagerGetCacheOriginsBlock)(WKArrayRef, WKErrorRef);
> +WK_EXPORT void WKCacheManagerGetCacheOrigins_b(WKCacheManagerRef
databaseManager, WKCacheManagerGetCacheOriginsBlock block);
> +#endif

Ditto.

> Source/WebKit2/WebProcess/WebProcess.cpp:637
> +    // If the memory cache is currently enabled, toggle it on and off to
evict its resources.
> +    if (!memoryCache()->disabled()) {
> +	   memoryCache()->setDisabled(true);
> +	   memoryCache()->setDisabled(false);
> +    }

It'd be nicer if MemoryCache had an "evictResources()" function that did this.

> Source/WebKit2/WebProcess/MemoryCache/WebCacheManager.cpp:73
> +    size_t numOrigins = origins.size();
> +
> +    Vector<String> identifiers(numOrigins);
> +    HashSet<String>::iterator end = origins.end();
> +
> +    unsigned i = 0;
> +    for (HashSet<String>::iterator it = origins.begin(); it != end; ++it)
> +	   identifiers[i++] = *it;

You can use WTF::copyToVector() to do this.