Bug 136449 - [GTK] UserMedia Permission Request API
Summary: [GTK] UserMedia Permission Request API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 123158
Blocks:
  Show dependency treegraph
 
Reported: 2014-09-02 02:53 PDT by Philippe Normand
Modified: 2014-12-08 10:16 PST (History)
18 users (show)

See Also:


Attachments
Patch (24.29 KB, patch)
2014-09-02 03:13 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (29.43 KB, patch)
2014-09-02 08:17 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (27.76 KB, patch)
2014-09-02 09:21 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (27.72 KB, patch)
2014-11-12 00:44 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (32.44 KB, patch)
2014-11-13 03:43 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (34.44 KB, patch)
2014-11-18 07:36 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (34.67 KB, patch)
2014-11-18 07:39 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (34.68 KB, patch)
2014-11-18 07:42 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (35.09 KB, patch)
2014-11-20 02:08 PST, Philippe Normand
cgarcia: review+
cgarcia: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2014-09-02 02:53:55 PDT
.
Comment 1 Philippe Normand 2014-09-02 03:13:19 PDT
Created attachment 237483 [details]
Patch
Comment 2 WebKit Commit Bot 2014-09-02 03:13:56 PDT
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
Comment 3 Carlos Garcia Campos 2014-09-02 03:22:51 PDT
Already added some comments to the bug #123158
Comment 4 Philippe Normand 2014-09-02 08:14:47 PDT
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 :)
Comment 5 Philippe Normand 2014-09-02 08:17:01 PDT
Created attachment 237487 [details]
Patch
Comment 6 Carlos Garcia Campos 2014-09-02 08:33:44 PDT
(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.
Comment 7 Philippe Normand 2014-09-02 09:21:49 PDT
Created attachment 237493 [details]
Patch
Comment 8 Philippe Normand 2014-11-12 00:44:31 PST
Created attachment 241416 [details]
Patch
Comment 9 Carlos Garcia Campos 2014-11-12 05:37:24 PST
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)
Comment 10 Philippe Normand 2014-11-13 03:43:28 PST
Created attachment 241480 [details]
Patch
Comment 11 Philippe Normand 2014-11-13 03:47:04 PST
(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?
Comment 12 Gustavo Noronha (kov) 2014-11-18 04:53:35 PST
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.
Comment 13 Carlos Garcia Campos 2014-11-18 05:33:33 PST
(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.
Comment 14 Philippe Normand 2014-11-18 07:36:47 PST
Created attachment 241789 [details]
Patch
Comment 15 Philippe Normand 2014-11-18 07:39:36 PST
Created attachment 241790 [details]
Patch
Comment 16 Philippe Normand 2014-11-18 07:42:40 PST
Created attachment 241791 [details]
Patch
Comment 17 Carlos Garcia Campos 2014-11-19 05:04:37 PST
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.
Comment 18 Philippe Normand 2014-11-20 02:08:35 PST
Created attachment 241934 [details]
Patch
Comment 19 Carlos Garcia Campos 2014-11-20 23:04:00 PST
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
Comment 20 Philippe Normand 2014-11-21 08:50:03 PST
Adding Benjamin in CC
Comment 21 Philippe Normand 2014-11-21 08:51:13 PST
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?
Comment 22 Benjamin Poulain 2014-12-05 12:37:47 PST
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()?
Comment 23 Philippe Normand 2014-12-08 10:15:49 PST
Thanks Benjamin, I went with the requires{Audio,Video} version and explained the FIXME.
Comment 24 Philippe Normand 2014-12-08 10:16:22 PST
Committed r176952: <http://trac.webkit.org/changeset/176952>