[webkit-reviews] review granted: [Bug 178236] Implement listing origins for which CacheStorage is storing data : [Attachment 323632] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 13 09:47:57 PDT 2017
Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 178236: Implement listing origins for which CacheStorage is storing data
https://bugs.webkit.org/show_bug.cgi?id=178236
Attachment 323632: Patch
https://bugs.webkit.org/attachment.cgi?id=323632&action=review
--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 323632
--> https://bugs.webkit.org/attachment.cgi?id=323632
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=323632&action=review
r=me with comments.
> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:349
> + static Ref<ReadOriginsTaskCounter>
create(WTF::CompletionHandler<void(Vector<WebsiteData::Entry>)>&& callback) {
return adoptRef(*new ReadOriginsTaskCounter(WTFMove(callback))); }
Please indent code.
> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:350
> + ~ReadOriginsTaskCounter()
Please add lank line before this.
> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:366
> + WTF::CompletionHandler<void(Vector<WebsiteData::Entry>)> m_callback;
Should be Vector<WebsiteData::Entry>&& not Vector<WebsiteData::Entry> for
clarity.
> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:373
> + Vector<WebsiteData::Entry> entries;
Should use reserveInitialCapacity() / uncheckedAppend().
> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:98
> + void fetchEntries(bool /* shouldComputeSize */,
WTF::CompletionHandler<void(Vector<WebsiteData::Entry>)>&&);
We should really not use boolean parameter and opt for enum classes instead:
enum class ShouldComputeSize { No, Yes };
> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:57
> + completionHandler(std::nullopt);
Calling completionHandler on the wrong thread!
> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:62
> + if (channel->fileDescriptor() < 0) {
I don't think we need our own error handling here, IOChannel does this for us
and passes an error code to the read() callback below.
> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:70
> + ASSERT(RunLoop::isMain());
Not checking for error? Data may be empty and error may be set.
> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:86
> + WTF::Persistence::Encoder encoder;
Maybe we should assert that we are not on the main thread?
> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95
> +std::optional<WebCore::SecurityOriginData> Caches::readOrigin(const Data&
data)
Maybe we should assert that we are not on the main thread?
> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:97
> + // FIXME: We should be able to use modern decoders for Persistent.
Persistent -> persistence ?
> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.h:69
> + WebCore::SecurityOriginData origin() const { return m_origin; }
const WebCore::SecurityOriginData& ?
> Tools/WebKitTestRunner/TestController.cpp:2357
> + for (size_t cptr = 0; cptr < WKArrayGetSize(origins) &&
!context->result; ++cptr) {
Bad name: cptr
I would suggest caching WKArrayGetSize(origins) before the loop.
More information about the webkit-reviews
mailing list