[Webkit-unassigned] [Bug 30322] WebKit level persistent caching

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30322

--- Comment #29 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 240740
  --> https://bugs.webkit.org/attachment.cgi?id=240740
yet yet another

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

> Source/WebKit2/NetworkProcess/NetworkCache.cpp:39
> +#include <dispatch/dispatch.h>

Is dispatch used in this file?

> Source/WebKit2/NetworkProcess/NetworkCache.cpp:172
> +    RefPtr<SharedMemory> sharedMemory = storageEntry.body.size() ? SharedMemory::createFromVMBuffer((void*)storageEntry.body.data(), storageEntry.body.size()) : nullptr;

hmm, SharedMemory::createFromVMBuffer is mac only, I wonder why it's not defined inside #ifdefs though.

> Source/WebKit2/NetworkProcess/NetworkCache.cpp:173
> +    RefPtr<ShareableResource> memoryMap = sharedMemory ? ShareableResource::create(sharedMemory.release(), 0, storageEntry.body.size()) : nullptr;

And ShareableResource is mac only too, and defined only when ENABLE_SHAREABLE_RESOURCE is defined.

> Source/WebKit2/NetworkProcess/NetworkCache.cpp:178
> +    if (memoryMap && memoryMap->createHandle(entry->memoryMapHandle))
> +        entry->buffer = entry->memoryMapHandle.tryWrapInSharedBuffer();
> +    else
> +        entry->buffer = WebCore::SharedBuffer::create(storageEntry.body.data(), storageEntry.body.size());

So, I guess we can move all this inside a #if ENABLE(SHAREABLE_RESOURCE) block, falling back to SharedBuffer when not defined

> Source/WebKit2/NetworkProcess/NetworkCache.cpp:269
> +    case 200:
> +    case 203:
> +    case 300:
> +    case 301:
> +    case 302:
> +    case 307:
> +    case 410:

There's HTTPStatusCodes.h in platform/network, that it seems to be used only by EFL port, but maybe we could add the missing status codes to the enum, and use the enum values instead. I think it would make this code easier to read.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:241
> +#if ENABLE(NETWORK_CACHE)
> +static bool isConditionalRequest(const WebCore::ResourceRequest& request)
> +{
> +    if (!request.httpHeaderField(WebCore::HTTPHeaderName::IfNoneMatch).isEmpty())
> +        return true;
> +    if (!request.httpHeaderField(WebCore::HTTPHeaderName::IfModifiedSince).isEmpty())
> +        return true;
> +    return false;
> +}
> +#endif

Could we use ResourceRequest::isConditional()? or de we really want to check only these two headers? Could we use httpHeaderFields().contains() instead?

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:533
> +void NetworkResourceLoader::didFinishCacheRetrieve
> +    (std::unique_ptr<NetworkCache::Entry> entry)

This should be a single line

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141101/a8660246/attachment-0002.html>