Bug 136449

Created attachment 237483 [details] Patch

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

Already added some comments to the bug #123158

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 :)

Created attachment 237487 [details] Patch

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

Created attachment 237493 [details] Patch

Created attachment 241416 [details] Patch

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)

Created attachment 241480 [details] Patch

(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?

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.

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

Created attachment 241789 [details] Patch

Created attachment 241790 [details] Patch

Created attachment 241791 [details] Patch

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.

Created attachment 241934 [details] Patch

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

Adding Benjamin in CC

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

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

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