As can be seen from the flakiness dashboard, http/wpt/cache-storage/cache-put-keys.https.any.worker.html is still failing. From logging gathered on my machine, this is due to errors when writing files through NetworkStorageCache.
Created attachment 351695 [details] Patch
Comment on attachment 351695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351695&action=review > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:386 > +void Engine::writeFile(const String& folderPath, const String& filename, NetworkCache::Data&& data, WebCore::DOMCacheEngine::CompletionCallback&& callback) Shouldn't folderPath be derivable from filename? > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:1029 > + RunLoop::main().dispatch([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)]() mutable { > + completionHandler(); RunLoop::main().dispatch(WTFMove(completionHandler));
(In reply to Alex Christensen from comment #2) > Comment on attachment 351695 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351695&action=review > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:386 > > +void Engine::writeFile(const String& folderPath, const String& filename, NetworkCache::Data&& data, WebCore::DOMCacheEngine::CompletionCallback&& callback) > > Shouldn't folderPath be derivable from filename? Sure, I can change that to separate it. > > > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:1029 > > + RunLoop::main().dispatch([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)]() mutable { > > + completionHandler(); > > RunLoop::main().dispatch(WTFMove(completionHandler)); We cannot do that right now. We should probably change to use DestructionThread::Main and stop passing protectedThis back to the main thread. I'll do that in a separate patch though since it is touching more code in NetworkCacheStorage.cpp
Created attachment 351704 [details] Patch
Comment on attachment 351704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351704&action=review This is even worse. What I meant is we could only pass one string, like /path/to/file then inside the lambda do a substring operation on it. But I preferred your other approach to this. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:43 > +#define CACHESLIST_FILENAME "cacheslist"_s oh please no.
Created attachment 351831 [details] Patch
Comment on attachment 351831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351831&action=review > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:508 > + callback(Error::WriteDisk); DiskWrite would look better IMO.
Thanks for the review. > DiskWrite would look better IMO. And ReadDisk -> DiskRead I guess.
(In reply to youenn fablet from comment #8) > Thanks for the review. > > > DiskWrite would look better IMO. > > And ReadDisk -> DiskRead I guess. Yes.
Comment on attachment 351831 [details] Patch Clearing flags on attachment: 351831 Committed r237071: <https://trac.webkit.org/changeset/237071>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45230934>