[Webkit-unassigned] [Bug 136449] [GTK] UserMedia Permission Request API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=136449

--- Comment #9 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 241416
  --> https://bugs.webkit.org/attachment.cgi?id=241416
Patch

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

Looks good to me in general, it's good that we can reuse the existing permission-request api

> Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:79
>  }

Please, add a changelog entry for these changes too.

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:164
> +        GRefPtr<WebKitUserMediaPermissionRequest> userMediaPermissionRequest = adoptGRef(webkitUserMediaPermissionRequestCreate(&permissionRequest, &securityOrigin));

Do not pass pointers, pass the references instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:111
> + * Obtains the media parameters of the permission request. Some web
> + * pages will ask access to only the audio device, while others might
> + * ask access to the video device or both.
> + *
> + * Returns: the media parameters associated with the request.

You should document that the guint32 we are returning is a bitmask with the WebKitUserMediaTypes

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:115
> +guint32 webkit_user_media_permission_get_media_types(WebKitUserMediaPermissionRequest* request)

I still find this name a bit confusing, but I'm not familiar with the media APIs. Isn't this more about the type of request instead of the type of media? what about requested_media_types? does it sounds redundant?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:144
> +     * The media device type(s) requested, either audio, video or both.

You should mention here that flags are WebKitUserMediaTypes

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:145
> +     */

Since 2.8

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:157
> +    WebKitUserMediaPermissionRequest* usermediaPermissionRequest = WEBKIT_USER_MEDIA_PERMISSION_REQUEST(g_object_new(WEBKIT_TYPE_USER_MEDIA_PERMISSION_REQUEST, NULL));

nullptr instead of NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:165
> +    RefPtr<ImmutableDictionary> parameters = request->mediaParameters();
> +    API::Boolean* audio = reinterpret_cast<API::Boolean*>(parameters->get("audio"));
> +    API::Boolean* video = reinterpret_cast<API::Boolean*>(parameters->get("video"));

I wonder why the proxy returns an array of API::Object. I would expect it to use non-API types that are converted to API types when passed to the C API.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h:48
> + * Enum values used to specify which type of media device is requested.

If this is about media devices, what about WebKitMediaDeviceType? and webkit_media_permission_request_get_requested_device_types? Another option would be to not expose the enum, and provide methods to query this directly like get_requested_device_is_audio() and get_requested_device_is_video().

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h:49
> + */

Since: 2.8

> Tools/MiniBrowser/gtk/BrowserWindow.c:422
> +        g_object_get(WEBKIT_USER_MEDIA_PERMISSION_REQUEST(request), "media-types", &media_types, NULL);

I would use webkit_user_media_permission_get_media_types() instead of g_object_get(), but in case of using g_object_get do not cast, since it expects a gpointer.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestUIClient.cpp:695
> +}

The get_media_types API is untested here, maybe we could another test to check that the values returned by get_media_types correspond to the values given in the JS (without doing any allow/deny check, since that's already tested by this one)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141112/d3378bbc/attachment-0002.html>