WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
175599
[Cache API] Implement Worker connection to the Cache storage engine
https://bugs.webkit.org/show_bug.cgi?id=175599
Summary
[Cache API] Implement Worker connection to the Cache storage engine
youenn fablet
Reported
2017-08-15 14:33:08 PDT
[Cache API] Implement Worker connection to the Cache storage engine
Attachments
Patch
(40.29 KB, patch)
2017-08-15 15:03 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(41.07 KB, patch)
2017-08-15 16:15 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(41.07 KB, patch)
2017-08-15 16:39 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(36.68 KB, patch)
2017-08-16 13:52 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(36.67 KB, patch)
2017-08-16 14:23 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(37.70 KB, patch)
2017-08-16 14:27 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2017-08-15 15:03:34 PDT
Created
attachment 318179
[details]
Patch
youenn fablet
Comment 2
2017-08-15 16:15:31 PDT
Created
attachment 318194
[details]
Patch
youenn fablet
Comment 3
2017-08-15 16:39:26 PDT
Created
attachment 318197
[details]
Patch
Chris Dumez
Comment 4
2017-08-16 10:57:17 PDT
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.
youenn fablet
Comment 5
2017-08-16 13:52:05 PDT
Created
attachment 318287
[details]
Patch
youenn fablet
Comment 6
2017-08-16 13:52:56 PDT
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
Chris Dumez
Comment 7
2017-08-16 14:00:39 PDT
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
youenn fablet
Comment 8
2017-08-16 14:23:13 PDT
Created
attachment 318288
[details]
Patch for landing
youenn fablet
Comment 9
2017-08-16 14:27:04 PDT
Created
attachment 318289
[details]
Patch for landing
WebKit Commit Bot
Comment 10
2017-08-16 15:09:38 PDT
Comment on
attachment 318289
[details]
Patch for landing Clearing flags on attachment: 318289 Committed
r220810
: <
http://trac.webkit.org/changeset/220810
>
WebKit Commit Bot
Comment 11
2017-08-16 15:09:40 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12
2017-08-16 15:11:44 PDT
<
rdar://problem/33927819
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug