Bug 136449 – [GTK] UserMedia Permission Request API

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

[Philippe Normand]

.

[Philippe Normand]

Created attachment 237483 [details]
Patch

[WebKit Commit Bot]

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

[Carlos Garcia Campos]

Already added some comments to the bug #123158

[Philippe Normand]

Thanks for the review! I'm going to upload a new patch.

(In reply to comment #53)
> > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:124
> > +const gchar* webkit_user_media_permission_get_origin(WebKitUserMediaPermissionRequest* request)
> 
> Is a string enough to represent a security origin?
> 

I don't know. That's all I needed in the MiniBrowser but it's a weak argument :)
Also I wonder if that property should be stored in the parent class instead. I can also remove this for now and we'd add it later.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:189
> > +    if (audio->value())
> > +        mediaParameters |= WEBKIT_USER_MEDIA_PARAMETERS_AUDIO;
> > +    if (video->value())
> > +        mediaParameters |= WEBKIT_USER_MEDIA_PARAMETERS_VIDEO;
> 
> So parameters is the media type?
> 

yes

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h:55
> > +} WebKitUserMediaParameters;
> 
> I find it a bit confusing, using Parameters for the media type, or will this be expanded eventually with more "parameters"?
> 

I don't think there will be new parameters... Another option would simply be 2 boolean properties, I don't really mind changing this code but it'd be nice to decide so I don't lose more time with GObject :)

[Philippe Normand]

Created attachment 237487 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #4)
> Thanks for the review! I'm going to upload a new patch.
> 
> (In reply to comment #53)
> > > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:124
> > > +const gchar* webkit_user_media_permission_get_origin(WebKitUserMediaPermissionRequest* request)
> > 
> > Is a string enough to represent a security origin?
> > 
> 
> I don't know. That's all I needed in the MiniBrowser but it's a weak argument :)
> Also I wonder if that property should be stored in the parent class instead. I can also remove this for now and we'd add it later.

Yes, please.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:189
> > > +    if (audio->value())
> > > +        mediaParameters |= WEBKIT_USER_MEDIA_PARAMETERS_AUDIO;
> > > +    if (video->value())
> > > +        mediaParameters |= WEBKIT_USER_MEDIA_PARAMETERS_VIDEO;
> > 
> > So parameters is the media type?
> > 
> 
> yes
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h:55
> > > +} WebKitUserMediaParameters;
> > 
> > I find it a bit confusing, using Parameters for the media type, or will this be expanded eventually with more "parameters"?
> > 
> 
> I don't think there will be new parameters... Another option would simply be 2 boolean properties, I don't really mind changing this code but it'd be nice to decide so I don't lose more time with GObject :)

I'm fine with the flags, what I find confusing is the name "parameters" to refer to the media type this request is about.

[Philippe Normand]

Created attachment 237493 [details]
Patch

[Philippe Normand]

Created attachment 241416 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 241416 [details]
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)

[Philippe Normand]

Created attachment 241480 [details]
Patch

[Philippe Normand]

(In reply to comment #9)
> 
> > 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?
> 

I think it's a bit redundant yeah.

> 
> > 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().
> 

I don't think the enum would grow so it'd be also ok to simply use methods for this. What do Gustavo and Martin think about this?

[Gustavo Noronha (kov)]

So, I think methods or an enum won't really make a difference in terms of being future proof: we'll have to add either new enum entries or new methods if the number of media types ends up growing, anyway. We should probably consider what is more convenient for bindings, and I think methods are the answer in this case.

[Carlos Garcia Campos]

(In reply to comment #12)
> So, I think methods or an enum won't really make a difference in terms of
> being future proof: we'll have to add either new enum entries or new methods
> if the number of media types ends up growing, anyway. We should probably
> consider what is more convenient for bindings, and I think methods are the
> answer in this case.

What about the names? it seems to me that it should be WebKitMediaDeviceType instead of media type.

[Philippe Normand]

Created attachment 241789 [details]
Patch

[Philippe Normand]

Created attachment 241790 [details]
Patch

[Philippe Normand]

Created attachment 241791 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 241791 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:106
> + * Returns: TRUE if access to an audio device was requested.

%TRUE

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:110
> +gboolean webkit_user_media_permission_is_audio(WebKitUserMediaPermissionRequest* request)

I have the same doubts about the names, this sounds to me like if the permission itself was audio, maybe is_audio_device? or is_for_audio_device?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:120
> + * Returns: TRUE if access to a video device was requested.

%TRUE

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:155
> +     * Wether the media device requested has a microphone or not.

We are not requesting a device, but permission to access a device, no?

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

Extra line here.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h:65
> +WEBKIT_API gboolean
> +webkit_user_media_permission_is_audio(WebKitUserMediaPermissionRequest *request);
> +
> +WEBKIT_API gboolean
> +webkit_user_media_permission_is_video(WebKitUserMediaPermissionRequest *request);

Nit: the parameters should be lined up with the previous method webkit_user_media_permission_request_get_type

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestUIClient.cpp:203
> +            g_assert_cmpint(webkit_user_media_permission_is_audio(userMediaRequest), ==, test->m_expectedAudioMedia);
> +            g_assert_cmpint(webkit_user_media_permission_is_video(userMediaRequest), ==, test->m_expectedVideoMedia);

Those are not actually int, use g_assert(a == b);

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestUIClient.cpp:302
> +    gboolean m_verifyMediaTypes;
> +    gboolean m_expectedAudioMedia;
> +    gboolean m_expectedVideoMedia;

Use bool instead of gboolean.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestUIClient.cpp:702
> +    test->loadHtml(userMediaRequestHTML, 0);

nullptr instead of 0

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestUIClient.cpp:707
> +    test->loadHtml(userMediaRequestHTML, 0);

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestUIClient.cpp:739
> +    test->loadHtml(userMediaRequestHTML, 0);

Ditto.

[Philippe Normand]

Created attachment 241934 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 241934 [details]
Patch

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

The GTK+ API looks good to me, please ask a WebKit2 owner to review the cross-platform changes before landing. Thanks!

> Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:76
> +    // FIXME: fill source vectors

Nit: finish the comment with period. A bug link for these FIXMEs would be great too.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:23
> +#include "WebKitEnumTypes.h"

I don't think you need this

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:27
> +#include <wtf/text/CString.h>

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequestPrivate.h:25
> +WebKitUserMediaPermissionRequest* webkitUserMediaPermissionRequestCreate(WebKit::UserMediaPermissionRequestProxy&, WebKit::WebSecurityOrigin&);

I wonder why you don't need to include UserMediaPermissionRequestProxy.h or forward declare the class, same for WebSecurityOrigin. I guess there's something in WebKitPrivate.h

[Philippe Normand]

Adding Benjamin in CC

[Philippe Normand]

Comment on attachment 241934 [details]
Patch

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

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:42
> -    PassRefPtr<ImmutableDictionary> mediaParameters() const { return m_mediaParameters; }
> +    bool audio() const { return m_audio; }
> +    bool video() const { return m_video; }

What do you think about this API change, Benjamin?

[Benjamin Poulain]

Comment on attachment 241934 [details]
Patch

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

Approved for WebKit2.

Please poke me on IRC when you need me to rubber-stamp something. I easily miss emails.

> Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:65
> +    // FIXME: Actually do constraints validation.

IMHO, you should extend this FIXME to fully explain what is missing.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:42
>> +    bool video() const { return m_video; }
> 
> What do you think about this API change, Benjamin?

That simplification make sense for the permission request but the name is a bit too generic.

Maybe audioConstraint()? needsAudio()? requestAudio()? requiresAudio()?

[Philippe Normand]

Thanks Benjamin, I went with the requires{Audio,Video} version and explained the FIXME.

[Philippe Normand]

Committed r176952: <http://trac.webkit.org/changeset/176952>