Cache API should make sure to resolve caches.open promises in the same order as called.
Created attachment 332429 [details] Patch
Created attachment 332436 [details] Patch
<rdar://problem/36930363>
Created attachment 332470 [details] Patch
Comment on attachment 332470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332470&action=review > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:554 > + m_pendingWritingCachesToDiskCallbacks.append([this, weakThis = m_weakFactory.createWeakPtr(*this), updateCounter, callback = WTFMove(callback)] () mutable { Why do we need this weakThis? m_pendingWritingCachesToDiskCallbacks is a data member of |this| and therefore would be cleared when |this| is destroyed. Same comment for the other lambdas. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.h:94 > + WeakPtr<Caches> createWeakPtr() { return m_weakFactory.createWeakPtr(*this); } If we need to use a weakPtr somewhere for some reason, I think you should rely on this getter more instead of using m_weakFactory.createWeakPtr(*this) in your lambda captures. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.h:110 > + Deque<CompletionHandler<void()>> m_pendingWritingCachesToDiskCallbacks; Assuming the Caches object can be deleted, how can this be CompletionHandlers? When the Caches object gets destroyed, what makes sure the completion handlers have been called?
Created attachment 332577 [details] Patch
GTK build is red.
(In reply to Chris Dumez from comment #7) > GTK build is red. It seems GTK would like this->clear() instead of clear()...
Created attachment 332580 [details] Patch
Comment on attachment 332580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332580&action=review > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:211 > + ASSERT(!error); Why this assertion? If I have 2 clear requests in m_pendingWritingCachesToDiskCallbacks, then presumably, the second one will be resolved with an error once the first request is processed, right?
(In reply to Chris Dumez from comment #10) > Comment on attachment 332580 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332580&action=review > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:211 > > + ASSERT(!error); > > Why this assertion? If I have 2 clear requests in > m_pendingWritingCachesToDiskCallbacks, then presumably, the second one will > be resolved with an error once the first request is processed, right? In fact, even if a previous write disk operation failed or if there is a clear request, we should anyway try clearing things, especially since we have no ways currently to expose a potential failure in clearing stuff. I'll remove the ASSERT and the if statement.
Created attachment 332594 [details] Patch for landing
The commit-queue encountered the following flaky tests while processing attachment 332594 [details]: imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-worker.html bug 182270 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 332594 [details]: js/dom/JSON-stringify.html bug 144784 (author: fpizlo@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 332594 [details] Patch for landing Clearing flags on attachment: 332594 Committed r227768: <https://trac.webkit.org/changeset/227768>
All reviewed patches have been landed. Closing bug.