Bug 193585 – Add API for setting MediaCacheDirectory in WK Context Config.

• Component: WebKit API
• Status: NEW

[Gurdal Oruklu]

Just like local storage or application cache directories, it should be possible to set the media cache directory in WKContextConfiguration

[Gurdal Oruklu]

Created attachment 359528 [details]
Patch

[Carlos Garcia Campos]

I don't think we are going to add new C API unless it's required by WTR.

[Michael Catanzaro]

Comment on attachment 359528 [details]
Patch

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

That's true, we don't want to add new C API unless it's needed by the test runner, because the C API is neither public nor stable. But this patch *does* use it in the test runner. So I actually think this patch is fine. It needs to be approved by an owner, though.

> Source/WebKit/ChangeLog:3
> +        Add API for setting MediaCacheDirectory in WK Context Config.

I would name this something like: "TestController should set media cache directory properly". Adding the new C API is uninteresting, because the C API is not public and applications cannot use it. It's just an implementation detail of WebKit. (I know Comcast has patched WebKit to expose the C API, but that's a bad idea.) But fixing the TestController to set the media cache directory is interesting.

> Source/WebKit/ChangeLog:10
> +
> +

Only one blank line here.

> Source/WebKit/UIProcess/API/C/WKContextConfigurationRef.cpp:131
> +
> +

Only one blank line here too.

> Tools/ChangeLog:10
> +
> +

Juts one blank line here too.

> Tools/WebKitTestRunner/TestController.cpp:462
> +        WKContextConfigurationSetMediaCacheDirectory(configuration.get(), toWK(temporaryFolder + separator + "MediaCache").get());

Where is MediaCache being saved currently?

Is this the only storage directory that TestController is missing here?

[Xabier Rodríguez Calvar]

Comment on attachment 359528 [details]
Patch

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

>> Tools/WebKitTestRunner/TestController.cpp:462
>> +        WKContextConfigurationSetMediaCacheDirectory(configuration.get(), toWK(temporaryFolder + separator + "MediaCache").get());
> 
> Where is MediaCache being saved currently?
> 
> Is this the only storage directory that TestController is missing here?

That's managed, IIRC, in our media player private.

[Gurdal Oruklu]

Created attachment 359732 [details]
Patch

[Gurdal Oruklu]

Updated the patch (removed extra lines and changed the description of the patch)

[Michael Catanzaro]

(In reply to Xabier Rodríguez Calvar from comment #4)
> That's managed, IIRC, in our media player private.

I don't understand, there's no code in the MediaPlayerPrivateGStreamer files related to MediaCache. Do you see a problem with this patch?

[Alex Christensen]

Comment on attachment 359732 [details]
Patch

This belongs in WKWebsiteDataStoreRef.h

[Michael Catanzaro]

Can you please explain why? Currently only service worker registration directory is exposed there; seems the other directories are all exposed in WKContextConfiguration.

[Xabier Rodríguez Calvar]

(In reply to Michael Catanzaro from comment #7)
> I don't understand, there's no code in the MediaPlayerPrivateGStreamer files
> related to MediaCache. Do you see a problem with this patch?

No, what I mean is that there is code there to select where GStreamer is placing the cached files but so far we're not honoring anything coming from the API.

[Xabier Rodríguez Calvar]

(In reply to Michael Catanzaro from comment #7)
> I don't understand, there's no code in the MediaPlayerPrivateGStreamer files
> related to MediaCache. Do you see a problem with this patch?

And no, I don't see any other problem with this patch other than GStreamer was and would be ignoring it.

[youenn fablet]

(In reply to Michael Catanzaro from comment #9)
> Can you please explain why? Currently only service worker registration
> directory is exposed there; seems the other directories are all exposed in
> WKContextConfiguration.

We are trying to move such APIs to WKWebsiteDataStoreRef.
The principle is that a context might have pages that relate to different website data store through their session ids.