(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)?
(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.
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.
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.
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()
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.
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!
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?
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.
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
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)
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().
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.
(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!
(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?
(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).
(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?
(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.
(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.
2017-10-14 16:08 PDT, youenn fablet
2017-10-14 16:29 PDT, youenn fablet
2018-10-29 10:58 PDT, darshan
2018-11-01 10:42 PDT, darshan
2018-11-02 11:14 PDT, darshan
fred.wang: commit-queue-
2018-11-02 11:53 PDT, darshan
fred.wang: commit-queue-
2018-11-03 07:20 PDT, darshan
2018-11-03 09:38 PDT, darshan
2018-11-03 10:57 PDT, darshan
2018-11-03 11:49 PDT, darshan
2018-11-03 13:51 PDT, EWS Watchlist
2018-11-03 23:23 PDT, darshan
2018-11-09 10:01 PST, darshan