Bug 221199 – [GLib] Permission request API for MediaKeySystem access support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

.

[Philippe Normand]

Created attachment 418860 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Philippe Normand]

Created attachment 419567 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 419567 [details]
Patch

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

Please fix my comments before landing.

> Source/WebKit/UIProcess/API/glib/WebKitMediaKeySystemPermissionRequest.cpp:105
> +WebKitMediaKeySystemPermissionRequest* webkitMediaKeySystemPermissionRequestCreate(MediaKeySystemPermissionRequest* request)

This is receiving the newly created request, so it could be Ref<MediaKeySystemPermissionRequest>&& instead of a pointer

> Source/WebKit/UIProcess/API/glib/WebKitMediaKeySystemPermissionRequest.cpp:107
> +    WebKitMediaKeySystemPermissionRequest* permissionRequest = WEBKIT_MEDIA_KEY_SYSTEM_PERMISSION_REQUEST(g_object_new(WEBKIT_TYPE_MEDIA_KEY_SYSTEM_PERMISSION_REQUEST, NULL));

NULL -> nullptr

> Source/WebKit/UIProcess/API/glib/WebKitMediaKeySystemPermissionRequest.cpp:108
> +    permissionRequest->priv->request = request;

And here we would WTFMove(request)

> Source/WebKit/UIProcess/API/glib/WebKitMediaKeySystemPermissionRequest.cpp:116
> + *
> + * Returns: The name of the key system for which access permission is being requested.

Add a body description, the Returns: can be simpler to not duplicate

* Get the key system for which access permission is being requested.
*
* Returns: the key system name for @request

Or something similar.

> Source/WebKit/UIProcess/API/glib/WebKitMediaKeySystemPermissionRequest.cpp:123
> +    return request->priv->request->keySystem().utf8().data();

This is a temporary, we need to cache its value in a CString in the priv struct to be able to return a const char*

> Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:287
> +    void decidePolicyForMediaKeySystemPermissionRequest(WebPageProxy&, API::SecurityOrigin& topLevelDocumentOrigin, const WTF::String& keySystem, CompletionHandler<void(bool)>&& completionHandler) final

topLevelDocumentOrigin can be omitted as it's unused, no?

> Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:289
> +        auto permissionRequest = adoptGRef(webkitMediaKeySystemPermissionRequestCreate(MediaKeySystemPermissionRequest::create(keySystem, std::exchange(completionHandler, nullptr)).ptr()));

why exchange and not WTFMove()?

> Source/WebKit/UIProcess/API/gtk/WebKitMediaKeySystemPermissionRequest.h:62
> +WEBKIT_API const gchar*

gchar* -> gchar *

> Source/WebKit/UIProcess/API/wpe/WebKitMediaKeySystemPermissionRequest.h:62
> +WEBKIT_API const gchar*

Ditto.

> Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:67
> +#if ENABLE(ENCRYPTED_MEDIA)
> +    auto* settings = webkit_web_view_get_settings(m_webView);
> +    webkit_settings_set_enable_encrypted_media(settings, TRUE);
> +#endif

Let's enable this only for the test that needs it. It could just one line.

[Philippe Normand]

Comment on attachment 419567 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitMediaKeySystemPermissionRequest.cpp:108
>> +    permissionRequest->priv->request = request;
> 
> And here we would WTFMove(request)

error: reference to type 'Ref<WebKitMediaKeySystemPermissionRequest>' (aka 'Ref<_WebKitMediaKeySystemPermissionRequest>') could not bind to an rvalue of type 'typename remove_reference<_WebKitMediaKeySystemPermissionRequest *&>::type' (aka '_WebKitMediaKeySystemPermissionRequest *')
    return WTFMove(permissionRequest);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~
DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:532:24: note: expanded from macro 'WTFMove'
#define WTFMove(value) std::move<WTF::CheckMoveParameter>(value)
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

>> Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:67
>> +#endif
> 
> Let's enable this only for the test that needs it. It could just one line.

Then I need to write a WebViewTest sub-class, only to add that one line? Why is it an issue to do this here?

[Carlos Garcia Campos]

Comment on attachment 419567 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/glib/WebKitMediaKeySystemPermissionRequest.cpp:108
>>> +    permissionRequest->priv->request = request;
>> 
>> And here we would WTFMove(request)
> 
> error: reference to type 'Ref<WebKitMediaKeySystemPermissionRequest>' (aka 'Ref<_WebKitMediaKeySystemPermissionRequest>') could not bind to an rvalue of type 'typename remove_reference<_WebKitMediaKeySystemPermissionRequest *&>::type' (aka '_WebKitMediaKeySystemPermissionRequest *')
>     return WTFMove(permissionRequest);
>            ^~~~~~~~~~~~~~~~~~~~~~~~~~
> DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:532:24: note: expanded from macro 'WTFMove'
> #define WTFMove(value) std::move<WTF::CheckMoveParameter>(value)
>                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.

WTFMove(request) not WTFMove(permissionRequest) in the return

>>> Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:67
>>> +#endif
>> 
>> Let's enable this only for the test that needs it. It could just one line.
> 
> Then I need to write a WebViewTest sub-class, only to add that one line? Why is it an issue to do this here?

No, just do 

webkit_settings_set_enable_encrypted_media(webkit_web_view_get_settings(test->m_webView), TRUE);

before test->showInWindow(); in the test

[Philippe Normand]

Committed r272485: <https://trac.webkit.org/changeset/272485>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 221588

[Philippe Normand]

Committed r272574: <https://trac.webkit.org/changeset/272574>