[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