Bug 178350

Summary: Cache API implementation should be able to compute storage size for WebKit client applications.
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, cdumez, cgarcia, commit-queue, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description youenn fablet 2017-10-16 10:55:25 PDT
Cache API implementation should be able to compute storage size for WebKit client applications.
Comment 1 Radar WebKit Bug Importer 2017-10-16 14:08:51 PDT
<rdar://problem/35014434>
Comment 2 youenn fablet 2017-10-16 14:10:00 PDT
Created attachment 323939 [details]
Patch
Comment 3 Chris Dumez 2017-10-16 19:15:27 PDT
Comment on attachment 323939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323939&action=review

r=me with changes.

> Source/WebKit/ChangeLog:8
> +        When gathering data from DOM Cache, we compute the size as follow:

as follows:

> Source/WebKit/ChangeLog:9
> +        - If caches is not persistent, size is zero

caches (plural) vs is (singular)

> Source/WebKit/ChangeLog:10
> +        - If caches is persistent, we use the size computed by NetworkCache::Storage. 

caches (plural) vs is (singular)

> Source/WebCore/page/SecurityOriginData.h:70
> +    bool equals(const SecurityOriginData& data) const

There is already an operator==() on this class.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:402
> +                taskCounter->addOrigin(WTFMove(origin), result.value().get().storageSize());

Can .get(). be replace by -> ?

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:379
> +        for (const auto& dataRecord : dataRecords) {

We usually omit the const.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:380
> +            for (const auto& recordOrigin : dataRecord.origins) {

We usually omit the const.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:381
> +                if (WebKit::toImpl(origin)->string() == recordOrigin.securityOrigin()->toString()) {

Seems inefficient to Stringify just to compare. Assuming recordOrigin is a SecurityOriginData, we could do:
SecurityOriginData::fromSecurityOrigin(origin) == recordOrigin.

Also, we could cache SecurityOriginData::fromSecurityOrigin(origin) before the loops.
Comment 4 youenn fablet 2017-10-17 10:23:29 PDT
Created attachment 324025 [details]
Patch
Comment 5 WebKit Commit Bot 2017-10-17 11:21:41 PDT
Comment on attachment 324025 [details]
Patch

Clearing flags on attachment: 324025

Committed r223558: <https://trac.webkit.org/changeset/223558>
Comment 6 WebKit Commit Bot 2017-10-17 11:21:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Alex Christensen 2017-10-19 18:32:08 PDT
Youenn, please stop adding C SPI without corresponding ObjC SPI.  It takes my project a step in the wrong direction.
Chris, please r- any patches that add C SPI without adding corresponding ObjC SPI.
Comment 8 Chris Dumez 2017-10-19 18:40:29 PDT
(In reply to Alex Christensen from comment #7)
> Youenn, please stop adding C SPI without corresponding ObjC SPI.  It takes
> my project a step in the wrong direction.
> Chris, please r- any patches that add C SPI without adding corresponding
> ObjC SPI.

Ok.
Comment 9 youenn fablet 2017-10-19 18:46:06 PDT
(In reply to Alex Christensen from comment #7)
> Youenn, please stop adding C SPI without corresponding ObjC SPI.  It takes
> my project a step in the wrong direction.
> Chris, please r- any patches that add C SPI without adding corresponding
> ObjC SPI.

There is no plan to make use of this SPI outside WebKitTestRunner.
I do not plan to add a corresponding ObjC SPI.
Maybe we can move it elsewhere to ensure it never gets used outside WebKitTestRunner.