Summary: | [Cache API] Implement Worker connection to the Cache storage engine | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | beidson, buildbot, cdumez, commit-queue, dbates, japhet, sam, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
youenn fablet
2017-08-15 14:33:08 PDT
Created attachment 318179 [details]
Patch
Created attachment 318194 [details]
Patch
Created attachment 318197 [details]
Patch
Comment on attachment 318197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318197&action=review > Source/WebCore/Modules/cache/CacheQueryOptions.h:43 > + CacheQueryOptions options; return { ignoreSearch, ignoreMethod, ignoreVary, cacheName.isolatedCopy() }; It is about the same amount of lines and avoids some unnecessary refcounting churn for the String. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:137 > + iterator->value(cacheIdentifier, error); It is risky to call a callback while you're holding on to a HashMap iterator. The callback may do things that modify the m_openAndRemoveCachePendingRequests HashMap and would invalidate your iterator, causing a crash on the next line. Please fix. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:149 > + iterator->value(WTFMove(caches)); It is risky to call a callback while you're holding on to a HashMap iterator. The callback may do things that modify the m_retrieveCachesPendingRequests HashMap and would invalidate your iterator, causing a crash on the next line. Please fix. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:161 > + iterator->value(WTFMove(records)); It is risky to call a callback while you're holding on to a HashMap iterator. The callback may do things that modify the m_retrieveRecordsPendingRequests HashMap and would invalidate your iterator, causing a crash on the next line. Please fix. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:173 > + iterator->value(WTFMove(records), error); ditto > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:174 > + m_batchDeleteAndPutPendingRequests.remove(iterator); Method is called remove, member is called delete, please choose one and stick to it. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:185 > + iterator->value(WTFMove(records), error); ditto. > Source/WebCore/Modules/cache/CacheStorageConnection.h:107 > + uint64_t m_openAndRemoveCachePendingRequestsCounter { 0 }; Why do we need for counters? Can't we just have one to generate identifiers for all kinds of requests? > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:141 > + isolatedCaches.uncheckedAppend(CacheInfo { cache.identifier, cache.name.isolatedCopy() }); May be nice (i.e. more reusable) to have this as an isolatedCopy() method on the CacheInfo struct instead. > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:197 > + for (const auto& record : records) This logic is duplicated, may be nice to move it to a utility function. > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:206 > + for (auto& recordData : recordsData) This logic is duplicated, may be nice to move it to a utility function. > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.h:36 > +class WorkerCacheStorageConnection : public CacheStorageConnection { final? > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.h:56 > + RefPtr<CacheStorageConnection> m_mainThreadConnection; What makes sure this gets destroyed on the main thread? > Source/WebCore/loader/FetchOptions.h:66 > + FetchOptions options = *this; I don't like this pattern because if somebody adds a new data member that is not safely copyable cross-thread, this code will still build but crash. I'd rather it fails building when something adds a new data member and forgets to update this isolatedCopy() method. I.e. I'd rather we use { } initializer with all members explicitly initialized. Created attachment 318287 [details]
Patch
Thanks for the review. (In reply to youenn fablet from comment #5) > Created attachment 318287 [details] > Patch Review items should be fixed in this patch Comment on attachment 318287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318287&action=review r=me > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:83 > + uint64_t requestIdentifier = ++m_requestCounter; nit: m_requestCounter is a bit confusing as it is used as a way to generate identifiers, not *count* requests. Maybe m_lastRequestIdentifier ? > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:131 > + auto callback = m_openAndRemoveCachePendingRequests.take(requestIdentifier); Could be inside the if condition > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:138 > + auto callback = m_retrieveCachesPendingRequests.take(requestIdentifier); ditto > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:145 > + auto callback = m_retrieveRecordsPendingRequests.take(requestIdentifier); ditto > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:152 > + auto callback = m_batchDeleteAndPutPendingRequests.take(requestIdentifier); ditto > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:159 > + auto callback = m_batchDeleteAndPutPendingRequests.take(requestIdentifier); ditto Created attachment 318288 [details]
Patch for landing
Created attachment 318289 [details]
Patch for landing
Comment on attachment 318289 [details] Patch for landing Clearing flags on attachment: 318289 Committed r220810: <http://trac.webkit.org/changeset/220810> All reviewed patches have been landed. Closing bug. |