Add support for cache names persistency
Created attachment 319106 [details] Patch
Attachment 319106 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125: More than one command on the same line [whitespace/newline] [4] Total errors found: 3 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 319107 [details] Patch
Attachment 319107 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125: More than one command on the same line [whitespace/newline] [4] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 319114 [details] Patch
Attachment 319114 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:123: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 319119 [details] Patch
Attachment 319119 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:123: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 319119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319119&action=review > Source/WebCore/Modules/cache/CacheStorageConnection.h:49 > + // Used only for testing purposes. > + virtual void setPersistency(const String& /* origin */, bool /* isPersistent */, DOMCache::CompletionCallback&& callback) { callback(DOMCache::Error::NotImplemented); } Maybe we should call this setPersistencyForTesting, and I think we should do something similar to setPrivateBrowsingEnabled instead and use SessionID to determine whether a cache should touch the disk or not.
Created attachment 319201 [details] Patch
Attachment 319201 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:122: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Alex Christensen from comment #9) > Comment on attachment 319119 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319119&action=review > > > Source/WebCore/Modules/cache/CacheStorageConnection.h:49 > > + // Used only for testing purposes. > > + virtual void setPersistency(const String& /* origin */, bool /* isPersistent */, DOMCache::CompletionCallback&& callback) { callback(DOMCache::Error::NotImplemented); } > > Maybe we should call this setPersistencyForTesting, and I think we should do > something similar to setPrivateBrowsingEnabled instead and use SessionID to > determine whether a cache should touch the disk or not. I was not sure whether we wanted to implement cache API in private browsing with in-memory-only caches. Let's do it then. This will probably end up having a much smaller quota for private browsing than for regular sessions. I removed setPersistencyForTesting and used setPrivateBrowsingEnabled in the test.
Comment on attachment 319201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319201&action=review > Source/WebCore/Modules/cache/DOMCache.cpp:44 > + return Exception { TypeError, ASCIILiteral("Failed reading data from disk") }; I'm not sure 'disk' is the best name here, as many machines don't have hard-disks anymore. I like 'FileSystem' as a modifier, but I am sure we could come up with others.
Comment on attachment 319201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319201&action=review > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:47 > + String sessionSubdirectory = ASCIILiteral("session") + String::number(sessionID.sessionID()); > + return WebCore::pathByAppendingComponent(NetworkProcess::singleton().cacheStorageDirectory(), sessionSubdirectory); This should be a String from the WebsiteDataStore.
Created attachment 319219 [details] Patch
Attachment 319219 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:117: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Sam Weinig from comment #13) > Comment on attachment 319201 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319201&action=review > > > Source/WebCore/Modules/cache/DOMCache.cpp:44 > > + return Exception { TypeError, ASCIILiteral("Failed reading data from disk") }; > > I'm not sure 'disk' is the best name here, as many machines don't have > hard-disks anymore. I like 'FileSystem' as a modifier, but I am sure we > could come up with others. Changed to "file system". (In reply to Alex Christensen from comment #14) > Comment on attachment 319201 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319201&action=review > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:47 > > + String sessionSubdirectory = ASCIILiteral("session") + String::number(sessionID.sessionID()); > > + return WebCore::pathByAppendingComponent(NetworkProcess::singleton().cacheStorageDirectory(), sessionSubdirectory); > > This should be a String from the WebsiteDataStore. I added a new parameter to WebsiteDataStoreParameters to pass the sub folder name. This enforces one main folder for cache storage and one subfolder for each cache storage engine.
Created attachment 319221 [details] Patch
Attachment 319221 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:118: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 319303 [details] Patch
Attachment 319303 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:105: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:129: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:111: More than one command on the same line [whitespace/newline] [4] Total errors found: 5 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 319303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319303&action=review > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:246 > + if (!shouldPersist()) { > + m_salt = makeSalt(); I don't think we should need a salt of it's not persistent. > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:361 > + callback(std::nullopt); Let's put an ASSERT(RunLoop::isMain()); here. > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:384 > + callback(data, error); Same here. It might be worth having a bunch of such assertions. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:163 > + Vector<String> names; > + names.reserveInitialCapacity(count); We should use the VectorCoders and put the reserveInitialCapacity optimization in them.
Created attachment 319409 [details] Patch for landing
Attachment 319409 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:105: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:128: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:111: More than one command on the same line [whitespace/newline] [4] Total errors found: 5 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Alex Christensen from comment #22) > Comment on attachment 319303 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319303&action=review > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:246 > > + if (!shouldPersist()) { > > + m_salt = makeSalt(); > > I don't think we should need a salt of it's not persistent. Will do as a follow-up > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:361 > > + callback(std::nullopt); > > Let's put an ASSERT(RunLoop::isMain()); here. OK > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:384 > > + callback(data, error); > > Same here. It might be worth having a bunch of such assertions. OK > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:163 > > + Vector<String> names; > > + names.reserveInitialCapacity(count); > > We should use the VectorCoders and put the reserveInitialCapacity > optimization in them. I sticked with the current decoder since the encoder side is not using the Vector encoder either as it would need to create a vector of string for that.
Comment on attachment 319409 [details] Patch for landing Clearing flags on attachment: 319409 Committed r221403: <http://trac.webkit.org/changeset/221403>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34174534>
The LayoutTest added with this change (http/tests/cache-storage/cache-persistency.https.html) is failing an assertion on El Capitan: ASSERTION FAILED: position != notFound /Volumes/Data/slave/elcapitan-debug/build/Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp(130) : void WebKit::CacheStorage::Caches::remove(uint64_t, CompletionCallback &&) https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221417%20(2811)/results.html
(In reply to Ryan Haddad from comment #29) > The LayoutTest added with this change > (http/tests/cache-storage/cache-persistency.https.html) is failing an > assertion on El Capitan: > > ASSERTION FAILED: position != notFound > /Volumes/Data/slave/elcapitan-debug/build/Source/WebKit/NetworkProcess/cache/ > CacheStorageEngineCaches.cpp(130) : void > WebKit::CacheStorage::Caches::remove(uint64_t, CompletionCallback &&) > > https://build.webkit.org/results/ > Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221417%20(2811)/results.html Will have a look at it. Thanks!
This change also caused several imported tests to fail intermittently and some to start timing out intermittently: Text diff: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fwindow%2Fcache-storage-keys.https.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fworker%2Fcache-storage-keys.https.html Timeout: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fcommon.https.html