Bug 54501 – WebKit2: Need a way to manage the WebCore Cache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brian Weinstein]

We need a way in WebKit2 to manager the WebCore cache. We need to expose methods to get a list of origins for where there are cached resources, evict the cached resources for a given origin, and evict all cached resources.

<rdar://problem/8764016>

[Brian Weinstein]

Created attachment 82530 [details]
WIP Patch - No ChangeLogs + Only Windows Build System

[Brian Weinstein]

Created attachment 82571 [details]
[PATCH] Fix

[Early Warning System Bot]

Attachment 82571 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7917161

[Brian Weinstein]

Created attachment 82576 [details]
[PATCH] Fix + Qt Build

[Early Warning System Bot]

Attachment 82576 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7920118

[Brian Weinstein]

Created attachment 82581 [details]
[PATCH] Fix + Qt (take 2)

[Brian Weinstein]

Created attachment 82582 [details]
[PATCH] Fix + Qt (Take 3)

[Early Warning System Bot]

Attachment 82582 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7918144

[Brian Weinstein]

Created attachment 82585 [details]
[PATCH] Fix + Qt (Take 4)

[Jon Honeycutt]

Comment on attachment 82585 [details]
[PATCH] Fix + Qt (Take 4)

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.

[Brian Weinstein]

Comment on attachment 82585 [details]
[PATCH] Fix + Qt (Take 4)

Doing some refactoring to make this API easier to handle under kinds of cache types. Clearing review flags for now.

[Brian Weinstein]

(In reply to comment #11)
> (From update of attachment 82585 [details])
> Doing some refactoring to make this API easier to handle under kinds of cache types. Clearing review flags for now.

After more discussion, keeping the API the way it is currently, and doing some renaming - will send out a new patch soon.

[Brian Weinstein]

Comment on attachment 82585 [details]
[PATCH] Fix + Qt (Take 4)

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

>> Source/WebCore/loader/cache/MemoryCache.cpp:477
>> +void MemoryCache::removeResourcesWithOrigin(RefPtr<SecurityOrigin> origin)
> 
> I think this should take a raw pointer.

Fixed.

>> Source/WebCore/loader/cache/MemoryCache.cpp:495
>> +void MemoryCache::originsWithCache(HashSet<String>& origins)
> 
> "getOriginsWithCache" would better indicate the purpose.

Fixed.

>> Source/WebKit2/UIProcess/WebCacheManagerProxy.cpp:86
>> +    }
> 
> If WebSecurityOrigin::createFromString() can return null, it might be better to use reserveCapacity and uncheckedAppend to prevent null entries in the vector.

Fixed.

>> Source/WebKit2/UIProcess/API/C/WKCacheManager.cpp:60
>> +#endif
> 
> You might remove this and add it later as needed.

Removed.

>> Source/WebKit2/UIProcess/API/C/WKCacheManager.h:42
>> +#endif
> 
> Ditto.

Removed as well.

>> Source/WebKit2/WebProcess/WebProcess.cpp:637
>> +    }
> 
> It'd be nicer if MemoryCache had an "evictResources()" function that did this.

Fixed.

>> Source/WebKit2/WebProcess/MemoryCache/WebCacheManager.cpp:73
>> +        identifiers[i++] = *it;
> 
> You can use WTF::copyToVector() to do this.

Fixed.

[Brian Weinstein]

Created attachment 82720 [details]
[PATCH] Fix + Jon's Comments + Renames

[WebKit Review Bot]

Attachment 82720 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/UIProcess/API/C/WKResourceCacheManager.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/WebResourceCacheManagerProxy.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/ResourceCache/WebResourceCacheManager.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Weinstein]

Created attachment 82762 [details]
[PATCH] Fix

[WebKit Review Bot]

Attachment 82762 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/ResourceCache/WebResourceCacheManager.h:58:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/SecurityOriginData.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/API/C/WKResourceCacheManager.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/ResourceCache/WebResourceCacheManager.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 82762 [details]
[PATCH] Fix

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

> Source/WebKit2/Shared/SecurityOriginData.h:43
> +    // FIXME: We should be sending more state across the wire than just the protocol,
> +    // host, and port.

Could you file a bug for this?

> Source/WebKit2/Shared/WebSecurityOrigin.h:55
> +    static PassRefPtr<WebSecurityOrigin> createFromString(const String& string)

Is this from an older version of the patch and is no longer needed?

> Source/WebKit2/UIProcess/API/C/WKAPICast.h:99
> -inline CacheModel toCacheModel(WKCacheModel wkCacheModel)
> +inline CacheModel toCacheModel(WKCacheModel WKCacheModel)
>  {
> -    switch (wkCacheModel) {
> +    switch (WKCacheModel) {

Surprised this compiled.  Please remove this change.

[Brian Weinstein]

(In reply to comment #18)
> (From update of attachment 82762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82762&action=review
> 
> > Source/WebKit2/Shared/SecurityOriginData.h:43
> > +    // FIXME: We should be sending more state across the wire than just the protocol,
> > +    // host, and port.
> 
> Could you file a bug for this?

Done - <rdar://problem/9018386>.

> 
> > Source/WebKit2/Shared/WebSecurityOrigin.h:55
> > +    static PassRefPtr<WebSecurityOrigin> createFromString(const String& string)
> 
> Is this from an older version of the patch and is no longer needed?

Yes, it is. Removed.

> 
> > Source/WebKit2/UIProcess/API/C/WKAPICast.h:99
> > -inline CacheModel toCacheModel(WKCacheModel wkCacheModel)
> > +inline CacheModel toCacheModel(WKCacheModel WKCacheModel)
> >  {
> > -    switch (wkCacheModel) {
> > +    switch (WKCacheModel) {
> 
> Surprised this compiled.  Please remove this change.

Reverted.

Thanks for the review!

[Brian Weinstein]

Landed in r78848.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78848 might have broken SnowLeopard Intel Release (Build)