Bug 183784 – Non-Cocoa ports use default directory for ServiceWorker data during testing

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

Non-Cocoa ports use default directory for ServiceWorker data during testing

[Zan Dobersek]

Cocoa ports specify this directory via WKWebsiteDataStore in initializeWebViewConfiguration() in TestControllerCocoa.mm. GLib API has WKWebsiteDataStore functionality, but that's not accessible through WK2 C API (like WKWebsiteDataStore is retrieved by casting the WKContextGetWebsiteDataStore() return value).

To follow the way other directories are specified, new WKContextConfiguration API is required.

[Zan Dobersek]

Created attachment 336131 [details]
WIP

[youenn fablet]

We probably want to move all these paths to WebsiteDataStore instead of moving service worker directory path to the context configuration.

[Zan Dobersek]

Created attachment 336847 [details]
Patch

[Michael Catanzaro]

EWS is red :(

It's probably better to change Cocoa to use the new cross-platform API as well; that will help keep the TestController code more manageable.

[Zan Dobersek]

I won't be changing usage of the Cocoa-specific API. Most likely it's used there for a good reason.

[Zan Dobersek]

Created attachment 337000 [details]
Patch

[youenn fablet]

Comment on attachment 337000 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:2400
> +        WKWebsiteDataStoreSetServiceWorkerRegistrationDirectory(dataStore, toWK(temporaryFolder + separator + "ServiceWorkerRegistration").get());

Cocoa uses "ServiceWorkers" as a folder name.
Not sure this is worth being consistent.

[Michael Catanzaro]

(In reply to Zan Dobersek from comment #6)
> I won't be changing usage of the Cocoa-specific API. Most likely it's used
> there for a good reason.

It's not: it only exists there because the C API that you add here did not yet exist. We should be reducing the amount of code in TestControllerCocoa.mm.

Specifically:

 (1) The code you add in this patch really belongs in TestController::generateContextConfiguration rather than TestController::platformAdjustContext, so that it gets used on all platforms.

 (2) The following code should be removed from initializeWebViewConfiguration in TestControllerCocoa.mm:

        String serviceWorkerRegistrationDirectory = String(libraryPath) + '/' + "ServiceWorkers";
        [poolWebsiteDataStore _setServiceWorkerRegistrationDirectory: serviceWorkerRegistrationDirectory];

I checked to see if _setServiceWorkerRegistrationDirectory can itself be removed, but it has to stay because it's needed by an API test.

[Zan Dobersek]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Zan Dobersek from comment #6)
> > I won't be changing usage of the Cocoa-specific API. Most likely it's used
> > there for a good reason.
> 
> It's not: it only exists there because the C API that you add here did not
> yet exist. We should be reducing the amount of code in
> TestControllerCocoa.mm.
> 
> Specifically:
> 
>  (1) The code you add in this patch really belongs in
> TestController::generateContextConfiguration rather than
> TestController::platformAdjustContext, so that it gets used on all platforms.
> 
>  (2) The following code should be removed from
> initializeWebViewConfiguration in TestControllerCocoa.mm:
> 
>         String serviceWorkerRegistrationDirectory = String(libraryPath) +
> '/' + "ServiceWorkers";
>         [poolWebsiteDataStore _setServiceWorkerRegistrationDirectory:
> serviceWorkerRegistrationDirectory];
> 
> I checked to see if _setServiceWorkerRegistrationDirectory can itself be
> removed, but it has to stay because it's needed by an API test.

Again, Cocoa ports are using Obj-C WKWebsiteDataStore API that registers the service worker directory as an explicit property. I don't think changing that would work flawlessly, but in contrast I'm pretty sure I'm not the one who should be making that change in the first place.

Adding the C API but leaving Cocoa code paths unchanged is mirroring what's been done for resource load statistics tests in bug #168171.

[youenn fablet]

There is indeed a trade-off between: 
- Sharing multi-platform code between different ports
- Making testing code use the public API exposed on a given plaftorm as a way to make the public API implementation more tested/robust

[Michael Catanzaro]

(In reply to youenn fablet from comment #11)
> There is indeed a trade-off between: 
> - Sharing multi-platform code between different ports
> - Making testing code use the public API exposed on a given plaftorm as a
> way to make the public API implementation more tested/robust

My thought process here:

 * I would almost always favor the later, because testing the public API is extremely important.
 * But in this case, we're talking about a private API used only for tests.
 * Even still, this one must be kept because it's needed by Cocoa API tests.
 * But that doesn't mean we should continue using it in TestController.
 * TestController is not really intended to test API itself, but to provide API for use by layout tests. It's surely not a substitute for good platform API tests.
 * Use of platform-specific APIs in the TestController has caused problems in the past.
 * So we should minimize the use of platform-specific APIs in the TestController.

(In reply to Zan Dobersek from comment #10)
> Adding the C API but leaving Cocoa code paths unchanged is mirroring what's
> been done for resource load statistics tests in bug #168171.

In retrospect, that was a mistake because it resulted in the tests breaking for us whenever Jon needed to change anything in resource load statistics. We would likely have needed to give up on running those tests as it was too much effort to maintain. That was solved by r228304, and is no longer a problem.

The service worker registration directory is a much simpler and much less important case, fortunately, since it's just one API call and not really a big deal. If there were many service worker functions here, then it would have been a big deal and I would have given r-.

(In reply to Zan Dobersek from comment #10)
> Again, Cocoa ports are using Obj-C WKWebsiteDataStore API that registers the
> service worker directory as an explicit property. I don't think changing
> that would work flawlessly,

The Cocoa APIs and C APIs just wrap the underlying WebsiteDataStore functions, so I think they should be entirely equivalent. See:

// WKWebsiteDataStore.mm

- (void)_setServiceWorkerRegistrationDirectory:(NSString *)directory
{
    _websiteDataStore->websiteDataStore().setServiceWorkerRegistrationDirectory(directory);
}

versus your new:

void WKWebsiteDataStoreSetServiceWorkerRegistrationDirectory(WKWebsiteDataStoreRef dataStoreRef, WKStringRef serviceWorkerRegistrationDirectory)
{
    WebKit::toImpl(dataStoreRef)->websiteDataStore().setServiceWorkerRegistrationDirectory(WebKit::toImpl(serviceWorkerRegistrationDirectory)->string());
}

But if I'm wrong and they are not actually equivalent, then this would be a good point. I think the only way it would not be equivalent is if Objective C has property change notifications or something along those lines, and I don't think it does (right, Youenn?), so I think they are equivalent.

> but in contrast I'm pretty sure I'm not the one
> who should be making that change in the first place.

On the contrary, I would say that since you're proposing to add a cross-platform API that's equivalent to the existing Cocoa API, and making use of it here in the TestController, you're creating a new redundancy with the Cocoa-specific implementation, and now is the ideal time to address that so as to minimize the need for platform-specific code. If it was difficult, I would not be so picky, but this should be very simple, right? EWS will let you know if there are any problems.

Anyway, I did not give r-, so you can commit if you disagree. It will only mean a small bit more work for whoever winds up trying to clean up TestController in the future.

[Zan Dobersek]

Comment on attachment 337000 [details]
Patch

Clearing flags on attachment: 337000

Committed r230389: <https://trac.webkit.org/changeset/230389>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39272508>