Bug 178316 – Implement Cache API support for WPE/GTK

• Status: REOPENED
• Component: WebKit Misc.

[youenn fablet]

In the meantime, related tests should be skipped.

[youenn fablet]

Created attachment 323828 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323828 [details]
Patch

Rejecting attachment 323828 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 323828, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4858971

[youenn fablet]

Created attachment 323833 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323833 [details]
Patch

Clearing flags on attachment: 323833

Committed r223325: <https://trac.webkit.org/changeset/223325>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34995899>

[Michael Catanzaro]

Reopening

[Frédéric Wang (:fredw)]

The remaining failure is http/wpt/cache-storage/cache-quota.any.html

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #8)
> The remaining failure is http/wpt/cache-storage/cache-quota.any.html

@Youenn: Can you please write down details regarding what we discussed during the hackfest about how this is supposed to be fixed (reducing max cache size for test runner)?

[youenn fablet]

(In reply to Frédéric Wang (:fredw) from comment #9)
> (In reply to Frédéric Wang (:fredw) from comment #8)
> > The remaining failure is http/wpt/cache-storage/cache-quota.any.html
> 
> @Youenn: Can you please write down details regarding what we discussed
> during the hackfest about how this is supposed to be fixed (reducing max
> cache size for test runner)?

Quota in WebKitTestRunner is set to 400 * 1024 in TestControllerCocoa.mm.
GTK WTR should be set up the same.

[Frédéric Wang (:fredw)]

*** Bug 190912 has been marked as a duplicate of this bug. ***

[darshan]

Created attachment 353300 [details]
fixed cache quota test for wpe

[Frédéric Wang (:fredw)]

Comment on attachment 353300 [details]
fixed cache quota test for wpe

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

> LayoutTests/ChangeLog:3
> +        Implement Cache API support for WPE/GTK

Can you please unskip the test on WPE too?

> LayoutTests/platform/gtk/TestExpectations:1143
> +webkit.org/b/178316 http/wpt/cache-storage/cache-quota.any.html [ Pass ]

Please update your tree. In the latest revision, only http/wpt/cache-storage/cache-quota.any.html is marked as failing and you need to remove this expectation.

[Frédéric Wang (:fredw)]

Comment on attachment 353300 [details]
fixed cache quota test for wpe

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

> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:117
> +    WKWebsiteDataStoreSetCacheStoragePerOriginQuota(websiteDataStore, 400 * 1024);

We want the same on WPE too. I'm willing to see what the result of EWS is. if that works, we can try to move everything to the main TestController class as Michael suggested on another bug.

[darshan]

Created attachment 353616 [details]
fixed cache quota test for gkt

[darshan]

Created attachment 353717 [details]
fixed cache quota test for gkt/wpe

Implemented the cache quota limit for wpt/gtk

[darshan]

Created attachment 353720 [details]
fixed cache quota test with all the platform having same storage

[Frédéric Wang (:fredw)]

Comment on attachment 353720 [details]
fixed cache quota test with all the platform having same storage

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

> LayoutTests/ChangeLog:10
> +

The three ChangeLogs should be updated and completed with explanations in the final patch.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:116
> +WK_EXPORT void WKWebsiteDataStoreSetCacheStoragePerOriginQuota(WKWebsiteDataStoreRef dataStoreRef, uint64_t quota);

It seems this new line was on purpose (to separate ServiceWorkerRegistrationDirectory) so you should not remove it

> Tools/WebKitTestRunner/TestController.cpp:479
> +    initializeCacheStoragePerOriginQuota();

I think you can just call WKWebsiteDataStoreSetCacheStoragePerOriginQuota here and remove Cocoa-specific stuff.

> Tools/WebKitTestRunner/TestController.cpp:527
> +{

I don't think you need this function if it's only called in TestController::generatePageConfiguration()

[Frédéric Wang (:fredw)]

Comment on attachment 353717 [details]
fixed cache quota test for gkt/wpe

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

OK, this patch looks good to me. Great that the tests are passing on WPE/GTK now! I'm marked r+ but I see you already started to do the refactoring suggested by Michael, so let's just finish that refactoring in this bug then. It is probably not be hard enough to justify a follow-up bug.

> LayoutTests/ChangeLog:10
> +

As I said for the other patch, you should give details of the changes in the ChangeLog.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:-116
> -

As I said for the other patch, you probably don't want to remove that new line.

[Michael Catanzaro]

Comment on attachment 353720 [details]
fixed cache quota test with all the platform having same storage

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

> Tools/WebKitTestRunner/TestController.cpp:529
> +    WKWebsiteDataStoreSetCacheStoragePerOriginQuota(websiteDataStore, 400 * 1024);

Yes, the Cocoa call really needs to be removed from TestControllerCocoa.cpp, because it's redundant with this call. Doesn't make sense to do the same thing twice. And then remove the Cocoa SPI, since you'd be removing its only use. And then this will be good IMO!

[darshan]

Created attachment 353771 [details]
fixed cache quota test for gkt/wpe

[darshan]

Created attachment 353775 [details]
fixed cache quota test with all the platform having same storage

[Frédéric Wang (:fredw)]

Comment on attachment 353771 [details]
fixed cache quota test for gkt/wpe

Let's focus on the platform independent way, now that you already started it, okay?

[Frédéric Wang (:fredw)]

Comment on attachment 353775 [details]
fixed cache quota test with all the platform having same storage

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

> Source/WebKit/ChangeLog:8
> +        Added a new API function WKWebsiteDataStoreSetCacheStoragePerOriginQuota which set the 

sets

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:116
> +WK_EXPORT void WKWebsiteDataStoreSetCacheStoragePerOriginQuota(WKWebsiteDataStoreRef dataStoreRef, uint64_t quota);

nit: you should probably still add a new line to separate WKWebsiteDataStoreSetCacheStoragePerOriginQuota from the ServiceWorker API functions.

> Tools/ChangeLog:8
> +        Called WKWebsiteDataStoreSetCacheStoragePerOriginQuota funtion to set the cache limit to 400 * 1200

s/funtion/function/

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:-72
> -    [poolWebsiteDataStore _setCacheStoragePerOriginQuota: 400 * 1024];

As Michael mentioned in comment 20, you can now probably remove the _setCacheStoragePerOriginQuota function and all the related logic.

[darshan]

Created attachment 353778 [details]
fixed cache quota test with all the platform having same storage

[Frédéric Wang (:fredw)]

Comment on attachment 353778 [details]
fixed cache quota test with all the platform having same storage

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:-271
> -

I believe it should be removed from Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h too.

[darshan]

Created attachment 353779 [details]
fixed cache quota test with all the platform having same storage

[EWS Watchlist]

Comment on attachment 353779 [details]
fixed cache quota test with all the platform having same storage

Attachment 353779 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9847208

New failing tests:
imported/w3c/web-platform-tests/webrtc/RTCPeerConnection-setRemoteDescription-tracks.https.html

[EWS Watchlist]

Created attachment 353782 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Frédéric Wang (:fredw)]

Comment on attachment 353779 [details]
fixed cache quota test with all the platform having same storage

Thanks, this looks good to me now! The test failure seemed unrelated.

@Youenn: Can you please review this? (as this patch removes the _setCacheStoragePerOriginQuota function from WKWebsiteDataStorePrivate.h)

[Michael Catanzaro]

Comment on attachment 353779 [details]
fixed cache quota test with all the platform having same storage

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

I agree this version is much better. LGTM now.

> Tools/WebKitTestRunner/TestController.cpp:481
> +    WKWebsiteDataStoreSetCacheStoragePerOriginQuota(websiteDataStore, 400 * 1024);
>      platformInitializeContext();

Leave a blank line there, because this code is not closely-related to the call to platformInitializeContext().

[darshan]

Created attachment 353797 [details]
fixed cache quota test with all the platform having same storage

[darshan]

> Leave a blank line there, because this code is not closely-related to the
> call to platformInitializeContext().

Donee

[Frédéric Wang (:fredw)]

@dkadu: You should not close this bug before the fix actually lands.

[youenn fablet]

Comment on attachment 353797 [details]
fixed cache quota test with all the platform having same storage

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:-51
> -@property (nonatomic, setter=_setCacheStoragePerOriginQuota:) NSUInteger _cacheStoragePerOriginQuota WK_API_AVAILABLE(macosx(10.13.4), ios(11.3));

I do not think we should remove the ObjC API.
When adding a C API, we want to have a corresponding ObjC API as well.
Other parts of the patch looks ok.

[Frédéric Wang (:fredw)]

(In reply to youenn fablet from comment #35)
> Comment on attachment 353797 [details]
> fixed cache quota test with all the platform having same storage
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353797&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:-51
> > -@property (nonatomic, setter=_setCacheStoragePerOriginQuota:) NSUInteger _cacheStoragePerOriginQuota WK_API_AVAILABLE(macosx(10.13.4), ios(11.3));
> 
> I do not think we should remove the ObjC API.
> When adding a C API, we want to have a corresponding ObjC API as well.
> Other parts of the patch looks ok.

OK, thanks!

@Darshan: Can you please do this final change? I think the patch is ready to land after that!

[Michael Catanzaro]

(In reply to youenn fablet from comment #35)
> I do not think we should remove the ObjC API.
> When adding a C API, we want to have a corresponding ObjC API as well.
> Other parts of the patch looks ok.

Removing the ObjC API is what I've done in the past. What's the point of having an internal API that is no longer used anywhere?

[Michael Catanzaro]

Youenn, what do you think?

[youenn fablet]

(In reply to Michael Catanzaro from comment #37)
> (In reply to youenn fablet from comment #35)
> > I do not think we should remove the ObjC API.
> > When adding a C API, we want to have a corresponding ObjC API as well.
> > Other parts of the patch looks ok.
> 
> Removing the ObjC API is what I've done in the past. What's the point of
> having an internal API that is no longer used anywhere?

These APIs can be used not only by WebKitTestRunner but by other apps as well for which ObjC is the preferred approach. There is of course a tension between keeping parity between the two and having APIs that do not get tested. An API test for the ObjC API might be good (not in this patch though).

[youenn fablet]

Comment on attachment 353797 [details]
fixed cache quota test with all the platform having same storage

Please do not remove the ObjC API.

[Michael Catanzaro]

(In reply to youenn fablet from comment #39)
> These APIs can be used not only by WebKitTestRunner but by other apps as
> well for which ObjC is the preferred approach.

But it's in the Private.h header, so it shouldn't be accessible outside WebKit?

[Michael Catanzaro]

I mean, it's in WKWebsiteDataStorePrivate.h. Surely it should move to a public header if it's intended to be public?

[youenn fablet]

(In reply to Michael Catanzaro from comment #42)
> I mean, it's in WKWebsiteDataStorePrivate.h. Surely it should move to a
> public header if it's intended to be public?

Indeed, it might move to public at some point.

[darshan]

Created attachment 354355 [details]
fixed cache quota test with all the platform having same storage-- with c api not removed

[Wenson Hsieh]

(In reply to Michael Catanzaro from comment #41)
> (In reply to youenn fablet from comment #39)
> > These APIs can be used not only by WebKitTestRunner but by other apps as
> > well for which ObjC is the preferred approach.
> 
> But it's in the Private.h header, so it shouldn't be accessible outside
> WebKit?

FYI — WebKit headers that are marked private still appear in the Apple internal SDK, and as such, are generally accessible to Apple-internal clients such as Mail and Safari.

[WebKit Commit Bot]

Comment on attachment 354355 [details]
fixed cache quota test with all the platform having same storage-- with c api not removed

Clearing flags on attachment: 354355

Committed r238125: <https://trac.webkit.org/changeset/238125>