Bug 178236 - Implement listing origins for which CacheStorage is storing data
Summary: Implement listing origins for which CacheStorage is storing data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-12 15:08 PDT by youenn fablet
Modified: 2017-10-13 16:47 PDT (History)
8 users (show)

See Also:


Attachments
Patch (24.51 KB, patch)
2017-10-12 21:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (24.56 KB, patch)
2017-10-13 10:56 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-10-12 15:08:23 PDT
Implement listing origins for which CacheStorage is storing data
Comment 1 youenn fablet 2017-10-12 21:49:12 PDT
Created attachment 323632 [details]
Patch
Comment 2 Build Bot 2017-10-12 21:50:12 PDT
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.
Comment 3 Chris Dumez 2017-10-13 09:47:57 PDT
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.
Comment 4 youenn fablet 2017-10-13 10:56:43 PDT
Created attachment 323703 [details]
Patch
Comment 5 WebKit Commit Bot 2017-10-13 13:31:37 PDT
Comment on attachment 323703 [details]
Patch

Clearing flags on attachment: 323703

Committed r223299: <https://trac.webkit.org/changeset/223299>
Comment 6 WebKit Commit Bot 2017-10-13 13:31:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2017-10-13 13:32:23 PDT
<rdar://problem/34984542>
Comment 8 Matt Lewis 2017-10-13 16:47:36 PDT
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"