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

[bugzilla-daemon at webkit.org]

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

--- Comment #50 from Anders Carlsson <andersca at apple.com> ---
Comment on attachment 241347
  --> https://bugs.webkit.org/attachment.cgi?id=241347
cmake build fix

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

> Source/WebKit2/config.h:100
> +#ifndef ENABLE_NETWORK_CACHE
> +#if PLATFORM(MAC) && ENABLE(NETWORK_PROCESS)
> +#define ENABLE_NETWORK_CACHE 1
> +#endif
> +#endif

Will this enable the cache by default? I don't think we want to do that right now.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:145
> +    RefPtr<NetworkResourceLoader> loader = this;

I think there should be a newline before here.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:146
> +    NetworkCache::shared().retrieve(originalRequest(), [loader](std::unique_ptr<NetworkCache::Entry> entry) {

Is there just a single network cache? It seems like you would want it to be per session?

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:332
> +            double now = currentTime();
> +            double responseEndOfValidity = now + WebCore::computeFreshnessLifetimeForHTTPFamily(m_response, now) - WebCore::computeCurrentAge(m_response, now);

I think this should use std::chrono.

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

I think this should be named didRetrieveCacheEntry or something along those lines.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:542
> +            send(Messages::WebResourceLoader::DidReceiveResource(entry->memoryMapHandle, currentTime()));

This should also use std::chrono instead of currentTime().

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:51
> +NetworkCache& NetworkCache::shared()
> +{
> +    static NeverDestroyed<NetworkCache> instance;
> +    return instance;
> +}

Like I said earlier, I don't think we want a singleton network cache - We want one per session or NetworkContext or something.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:57
> +bool NetworkCache::initialize(const String& applicationCachePath)

This shouldn't be called applicationCachePath because it can be confused with the app cache.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:97
> +    auto timeStamp = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::duration<double>(currentTime()));

This can just use std::chrono::system_clock::current_time() instead of currentTime().

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:103
> +    return NetworkCacheStorage::Entry { timeStamp, header, body };

Very nice with the uniform initialization here!

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:137
> +    fprintf(stderr, "decodeStorageEntry\n");

Lol wut.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:164
> +    auto timeStamp = std::chrono::duration_cast<std::chrono::duration<double>>(storageEntry.timeStamp);
> +    double age = WebCore::computeCurrentAge(cachedResponse, timeStamp.count());
> +    double lifetime = WebCore::computeFreshnessLifetimeForHTTPFamily(cachedResponse, timeStamp.count());

Would be nice if this all used std::chrono.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:172
> +            return nullptr;

Should the entry be removed from the cache in this case?

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

Why does this have to cast to void*?

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

Why is this called memoryMap? What is it a map of?

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:212
> +    if (partition.isEmpty())
> +        partition = ASCIILiteral("No partition");

Can't we let an empty partition mean no partition? Seems stupid to write out "No partition" to disk (if that's what we're doing).

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:216
> +void NetworkCache::retrieve(const WebCore::ResourceRequest& originalRequest, std::function<void (std::unique_ptr<Entry>)> completion)

completion should be completionHandler.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:223
> +        completion(nullptr);

It's dangerous that this can call the completionHandler from inside the retrieve call. You should either document this, or call it later on the main thread.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:227
> +    double start = currentTime();

std::chrono.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:237
> +    // FIXME: With C++14 this could use unique_ptr and initilized lambda capture

Typo, “initilized”.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:238
> +    auto capture = std::make_shared<Capture>(Capture { originalRequest, completion });

Can use WTF::move to move the original request.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:259
> +    if (originalRequest.httpMethod() != "GET") {

I think you want equalIgnoringCase here?

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

It would be nice if each of these cases had a comment stating the name of each status code.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.h:68
> +    HashType m_hash;

I think you want to store some type of hash version here as well so we can bump the version if we decide to change the WTF hash (which is likely to happen in the future).

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:62
> +        Data(OSObjectPtr<dispatch_data_t>);

Should be explicit.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:81
> +        std::chrono::milliseconds timeStamp;

std::chrono - very nice!

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:100
> +    struct Retrieve {

RetrieveOperation?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:46
> +static void traverseDirectory(const String& path, uint8_t type, std::function<void (const String&)> function) {

I think this can be a const std::function, or maybe even make this a function template (function having a template parameter type).

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:70
> +    traverseDirectory(cachePath, DT_DIR, [&cachePath, &function](const String& subdirName) {
> +        String partitionPath = WebCore::pathByAppendingComponent(cachePath, subdirName);
> +        traverseDirectory(partitionPath, DT_REG, [&function, &partitionPath](const String& fileName) {
> +            if (fileName.length() != 8)
> +                return;
> +            function(fileName, partitionPath);
> +        });
> +    });

What happens if there are symlinks inside the directory that links to the parent here?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:181
> +    dispatch_io_set_low_water(channel.get(), SIZE_MAX);

Should use std::numeric_limits here instead of SIZE_MAX.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:258
> +    if (metaData.headerChecksum != hashData(headerData.get())) {

Should version the hash here too.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:314
> +    Vector<uint8_t> filler(dataOffset - headerSize, 0);

This can be Vector<uint8_t, 4096> if you want to ;)

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:329
> +    unlink(path.data());

File I/O on the main thread!

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:364
> +void NetworkCacheStorage::dispatchPendingRetrieves()

retrieveOperations.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:368
> +    const unsigned maximumActiveRetrieveCount = 5;

retrieveOperationCount?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:403
> +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0), [this, key, entry, cachePathCapture, completion]() {

It'd be somewhat cleaner to create a queue for the network cache storage instead of using the global queue everywhere.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:433
> +    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), [directoryPathCapture] {

What happens if you try to update the cache while it's being cleared?

> Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:149
> +    if (NetworkCache::shared().isEnabled()) {
> +        NetworkCache::shared().setMaximumSize(urlCacheDiskCapacity);
> +        return;
> +    }

Like I've stated before, we need the ability to support multiple caches in the network process - shouldn't be too hard to do.

> Source/WebKit2/Platform/IPC/ArgumentDecoder.h:44
> +    size_t currentOffset() const { return m_bufferPos - m_buffer; }

I think this can be removed now.

-- 
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/20141118/cd7d24a3/attachment-0002.html>