Summary: | Add Cache API support of records persistency | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, buildbot, cdumez, cgarcia, commit-queue, koivisto, sam, thisiskatewinslet, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=177380 | ||||||||||||||||||||||||
Bug Depends on: | 176818 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
youenn fablet
2017-09-13 09:14:37 PDT
Created attachment 320646 [details]
Patch
Created attachment 320701 [details]
Patch
Attachment 320701 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:238: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:424: More than one command on the same line [whitespace/newline] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320706 [details]
Patch
Attachment 320706 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:237: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:426: More than one command on the same line [whitespace/newline] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320709 [details]
Patch
Attachment 320709 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:244: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:433: More than one command on the same line [whitespace/newline] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320710 [details]
Patch
Attachment 320710 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:244: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:434: More than one command on the same line [whitespace/newline] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320710&action=review > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:61 > + if (keyURL.hasQuery()) > + keyURL.setQuery({ }); Seems like there should be a removeQuery(), just like there is a removeFragmentIdentifier(). > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:105 > + auto key = Key { ASCIILiteral("record"), String { m_uniqueName }, String(), createCanonicalUUIDString(), m_caches.salt() }; I would do Key key { ... }; rather than the auto key = Key { ... }; If m_uniqueName is already a String, why the need to copy-construct? > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:107 > + RecordInformation recordInformation = { WTFMove(key), insertionTime, record.identifier, 0 , record.request.url(), false, { } }; I don't think the = should be necessary. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:140 > +struct TraversalResult { > + Ref<Caches> caches; > + uint64_t cacheIdentifier; > + CompletionCallback callback; > + HashMap<String, Vector<RecordInformation>> records; > + Vector<Key> failedRecords; > +}; Probably want to wrap this in an anonymous namespace to static linkage. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:153 > + TraversalResult traversalResult = { makeRef(m_caches), m_identifier, WTFMove(callback), { }, { } }; Should not need =. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:180 > + RecordInformation recordInformation = { storageRecord->key, insertionTime, 0, 0, record.request.url(), false, { } }; Ditto. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:220 > + using ReadRecordsCallback = WTF::Function<void(Vector<Record>&&, Vector<uint64_t>&&)>; > + static RefPtr<ReadRecordTaskCounter> create(ReadRecordsCallback&& callback) { return adoptRef(new ReadRecordTaskCounter(WTFMove(callback))); } Why RefPtr<ReadRecordTaskCounter>? Can this return a Ref<>? > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:244 > + explicit ReadRecordTaskCounter(ReadRecordsCallback&& callback) : m_callback(WTFMove(callback)) { } We usually write this as: explicit ReadRecordTaskCounter(ReadRecordsCallback&& callback) : m_callback(WTFMove(callback)) { } > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:317 > + static RefPtr<AsynchronousPutTaskCounter> create() { return adoptRef(new AsynchronousPutTaskCounter()); } Ref? Also, you don't need the () in new AsynchronousPutTaskCounter() Comment on attachment 320710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320710&action=review >> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:61 >> + keyURL.setQuery({ }); > > Seems like there should be a removeQuery(), just like there is a removeFragmentIdentifier(). Since we're removing the fragment right after it, I think this needs a removeQueryAndFragmentIdentifier. I've seen this pattern before, too. Created attachment 320773 [details]
Patch
(In reply to Alex Christensen from comment #11) > Comment on attachment 320710 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320710&action=review > > >> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:61 > >> + keyURL.setQuery({ }); > > > > Seems like there should be a removeQuery(), just like there is a removeFragmentIdentifier(). > > Since we're removing the fragment right after it, I think this needs a > removeQueryAndFragmentIdentifier. I've seen this pattern before, too. Right, there are at least two places in CacheStorage API where we would benefit from that pattern. This is on my TODO list. Attachment 320773 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:243: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:433: More than one command on the same line [whitespace/newline] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Thanks for the review. (In reply to Sam Weinig from comment #10) > Comment on attachment 320710 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320710&action=review > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:61 > > + if (keyURL.hasQuery()) > > + keyURL.setQuery({ }); > > Seems like there should be a removeQuery(), just like there is a > removeFragmentIdentifier(). > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:105 > > + auto key = Key { ASCIILiteral("record"), String { m_uniqueName }, String(), createCanonicalUUIDString(), m_caches.salt() }; > > I would do Key key { ... }; rather than the auto key = Key { ... }; > > If m_uniqueName is already a String, why the need to copy-construct? Fixed. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:107 > > + RecordInformation recordInformation = { WTFMove(key), insertionTime, record.identifier, 0 , record.request.url(), false, { } }; > > I don't think the = should be necessary. Fixed. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:140 > > +struct TraversalResult { > > + Ref<Caches> caches; > > + uint64_t cacheIdentifier; > > + CompletionCallback callback; > > + HashMap<String, Vector<RecordInformation>> records; > > + Vector<Key> failedRecords; > > +}; > > Probably want to wrap this in an anonymous namespace to static linkage. I am not sure what you are proposing, can you elaborate? > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:153 > > + TraversalResult traversalResult = { makeRef(m_caches), m_identifier, WTFMove(callback), { }, { } }; > > Should not need =. Fixed. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:180 > > + RecordInformation recordInformation = { storageRecord->key, insertionTime, 0, 0, record.request.url(), false, { } }; > > Ditto. Fixed. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:220 > > + using ReadRecordsCallback = WTF::Function<void(Vector<Record>&&, Vector<uint64_t>&&)>; > > + static RefPtr<ReadRecordTaskCounter> create(ReadRecordsCallback&& callback) { return adoptRef(new ReadRecordTaskCounter(WTFMove(callback))); } > > Why RefPtr<ReadRecordTaskCounter>? Can this return a Ref<>? It makes capturing the counter in the lambda quicker to write. [taskCounter] vs [taskCounter = taskeCounter.copyRef()]. Maybe being explicit about the copyRef is a good thing though? > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:244 > > + explicit ReadRecordTaskCounter(ReadRecordsCallback&& callback) : m_callback(WTFMove(callback)) { } > > We usually write this as: > > explicit ReadRecordTaskCounter(ReadRecordsCallback&& callback) > : m_callback(WTFMove(callback)) > { > } Would it make sense to change the code style here? I forgot to change this in the patch, will do it asap. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:317 > > + static RefPtr<AsynchronousPutTaskCounter> create() { return adoptRef(new AsynchronousPutTaskCounter()); } > > Ref? Also, you don't need the () in new AsynchronousPutTaskCounter() Created attachment 320774 [details]
Patch
Filed bug 176911 for the URL helper method. Attachment 320774 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:436: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to youenn fablet from comment #15) > Thanks for the review. > > > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:140 > > > +struct TraversalResult { > > > + Ref<Caches> caches; > > > + uint64_t cacheIdentifier; > > > + CompletionCallback callback; > > > + HashMap<String, Vector<RecordInformation>> records; > > > + Vector<Key> failedRecords; > > > +}; > > > > Probably want to wrap this in an anonymous namespace to static linkage. > > I am not sure what you are proposing, can you elaborate? > When you have a function in a cpp file that you want to constrain to that translation unit, you mark that by saying static void functionName()... For a struct or class, you can do the same thing by wrapping it in an anonymous namespace. In this case: namespace { struct TraversalResult { Ref<Caches> caches; uint64_t cacheIdentifier; CompletionCallback callback; HashMap<String, Vector<RecordInformation>> records; Vector<Key> failedRecords; }; } > > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:220 > > > + using ReadRecordsCallback = WTF::Function<void(Vector<Record>&&, Vector<uint64_t>&&)>; > > > + static RefPtr<ReadRecordTaskCounter> create(ReadRecordsCallback&& callback) { return adoptRef(new ReadRecordTaskCounter(WTFMove(callback))); } > > > > Why RefPtr<ReadRecordTaskCounter>? Can this return a Ref<>? > > It makes capturing the counter in the lambda quicker to write. > [taskCounter] vs [taskCounter = taskeCounter.copyRef()]. > > Maybe being explicit about the copyRef is a good thing though? I think so. It's yet another think in my list of things that makes me wonder if Ref<> should be copyable. > When you have a function in a cpp file that you want to constrain to that > translation unit, you mark that by saying static void functionName()... For > a struct or class, you can do the same thing by wrapping it in an anonymous > namespace. In this case: We could do that for all classes declared in the cpp file. I guess the point is that TraversalResult name is generic enough that it could collide. > > Maybe being explicit about the copyRef is a good thing though? Will move to this then. > I think so. It's yet another think in my list of things that makes me wonder > if Ref<> should be copyable. That is a vast discussion indeed. Created attachment 320793 [details]
Patch
Attachment 320793 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:438: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320793&action=review r=me other than these comments. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:157 > + RunLoop::main().dispatch([traversalResult = WTFMove(traversalResult)]() mutable { An isolatedCopy would protect this from future changes accidentally introducing multiply-reffed strings going across threads here. The key is questionable already. I think until we have thread safe refcounted strings we need to isolatedCopy things when going across threads at places like this. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:218 > +class ReadRecordTaskCounter : public ThreadSafeRefCounted<ReadRecordTaskCounter> { This doesn't need to be ThreadSafeRefCounted. RefCounted is fine because all operations of this class are on the same thread. Add assertions. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:319 > +class AsynchronousPutTaskCounter : public RefCounted<AsynchronousPutTaskCounter> { This is a strange abstraction. I think it would be better to reorganize the code that uses it. Created attachment 320852 [details]
Patch for landing
Attachment 320852 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:473: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 320863 [details]
Patch for landing
Attachment 320863 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:474: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320863 [details] Patch for landing Clearing flags on attachment: 320863 Committed r222073: <http://trac.webkit.org/changeset/222073> All reviewed patches have been landed. Closing bug. |