[webkit-reviews] review granted: [Bug 221199] [GLib] Permission request API for MediaKeySystem access support : [Attachment 419567] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 221199: [GLib] Permission request API for MediaKeySystem access support
https://bugs.webkit.org/show_bug.cgi?id=221199

Attachment 419567: Patch

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




--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 419567
  --> https://bugs.webkit.org/attachment.cgi?id=419567
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_S
YSTEM_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(MediaKeySystemPermissionR
equest::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.