Bug 178236 – Implement listing origins for which CacheStorage is storing data

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Implement listing origins for which CacheStorage is storing data

[youenn fablet]

Created attachment 323632 [details]
Patch

[Build Bot]

Attachment 323632 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.h:84:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 323632 [details]
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.

[youenn fablet]

Created attachment 323703 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323703 [details]
Patch

Clearing flags on attachment: 323703

Committed r223299: <https://trac.webkit.org/changeset/223299>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34984542>

[Matt Lewis]

The test changed in this revision http/tests/cache-storage/cache-clearing-origin.https.html has become extremely flaky on macOS. 

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcache-storage%2Fcache-clearing-origin.https.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r223296%20(3517)/results.html
https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/3517

Diff:
--- /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/http/tests/cache-storage/cache-clearing-origin.https-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/http/tests/cache-storage/cache-clearing-origin.https-actual.txt
@@ -1,4 +1,4 @@
 
 PASS Cleaning existing caches 
-PASS Clearing disk cache of a given origin 
+FAIL Clearing disk cache of a given origin promise_test: Unhandled rejection with value: object "TypeError: Failed reading data from the file system"