[Webkit-unassigned] [Bug 94373] [gtk] adding methods and signals for usermedia communication between webkit and browser

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Danilo Cesar Lemes de Paula <danilo.cesar at collabora.co.uk>  2012-09-24 13:30:16 PST ---
(From update of attachment 159705)
View in context: https://bugs.webkit.org/attachment.cgi?id=159705&action=review

>> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:83
>> +    list->priv = priv;
> 
> Use placement new syntax, see webkit_web_view_init()

ok

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:31
>> +#include <glib.h>
> 
> This is already included by the header.

removed

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:32
>> +#include <glib/gi18n-lib.h>
> 
> No translatable strings here either.

removed

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:52
>> +G_DEFINE_TYPE(WebKitWebUserMediaRequest, webkit_web_user_media_request, G_TYPE_OBJECT);
> 
> Trailing ; is not needed

fixed;

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:64
>> +    request->priv = priv;
> 
> Use the placement new syntax here too, so that the destructor is called and the RefPtr is destroyed.

fixed, but would be nice a second check since I never mixed gobjects and new operator.

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:111
>> +const gchar* webkit_web_user_media_request_get_origin(WebKitWebUserMediaRequest* request)
> 
> get_security_origin? This is confusing, because there's a WebKitSecurityOrigin calls in the API, shouldn't we return that instead?

I didn't know that SecurityOrigin was part of the official API, makes sense to change that, thanks for noticing.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2749
>> +#if ENABLE(MEDIA_STREAM)
> 
> I don't think having public API that depends on compilation options is a good idea. The API should always be there, but if media stream support is not compiled, the API should do nothing or fallback to a default behaviour.

Agree

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2751
>> +    /*
> 
> /* -> /**

ok

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2752
>> +     * WebKitWebView::user-media-requested
> 
> Trailing : missing here.

ok

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2759
>> +     * information and the available options to be selected.
> 
> Then the user is expected to call either webkit_web_view_accept_user_media_request() or webkit_web_view_reject_user_media_request()?

ok.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2765
>> +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
> 
> Why G_SIGNAL_ACTION?

not really necessary, removed.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2774
>> +    /*
> 
> /* -> /**

fixed

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2775
>> +     * WebKitWebView::user-media-request-cancelled
> 
> Trailing : missing here.

fixed

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2779
>> +     * This signal will be emited when the user canceled its userMedia request.
> 
> Why do we need to notify the user that request has cancelled by him/her?

If a website calls stop() on the localmediastream before the user selects a device, the notification should disappear.
Since the notification box is controlled by the browser, the browser should be notified. It will be explained on the example I'm pushing with this commit.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2785
>> +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
> 
> Why G_SIGNAL_ACTION?

removed.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:2792
>> +#endif /* ENABLE(MEDIA_STREAM) */
> 
> Where are the signals emitted?

The signals are emited by the userMediaClientGtk, I can include it on this commit, but I was expecting to push the API first, and then the internal elements.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5398
>> +void webkit_web_view_accept_user_media_request(WebKitWebView *webView, WebKitWebUserMediaRequest *webRequest, WebKitWebUserMediaList *audioMediaList, WebKitWebUserMediaList *videoMediaList)
> 
> All * are incorrectly placed.

ops, I thought webkit-check-style would warn me about it. fixed.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5410
>> +    for (size_t i = audioSources.size() - 1; i != (size_t) -1; --i)
> 
> Why do you need to iterate from size -1 to (size_t) - 1? Can't you just iterate from i = 0 to audioSources.size() - 1? or from size() - 1 to 0?

We're iterating over a list and removing indexes from another. iterating from size to 0 would be complicated since the index can change.
But in fact, we want to go from size() - 1 to 0, so Ok, fixed.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5414
>> +    for (size_t i = videoSources.size() - 1; i != (size_t) -1; --i)
> 
> Ditto.

fixed

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5420
>> +    // client->userMediaRequestSucceeded(core(webRequest), audioSources, videoSources);
> 
> What happen if the user doesn't handle the signal? would the client always expect that either userMediaRequestSucceeded or userMediaRequestFailed are called? In that case we should have a default implementation in cade the signal is not handled.

Right now, if the application doesn't implement the methods or handle the signal, nothing happens. 

Maybe the implementation (UserMediaClientGTK) should check for the enable_media_stream flag on Settings, and, if it's false, call reject automatically.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5441
>> +    // client->userMediaRequestFailed(core(webRequest));
> 
> If WebCore implementation is not yet ready, I think it's better to wait until it's done to add the public API instead of adding API that does nothing.

it is ready. I just don't want to push a huge commit. But for sure would be nice some discussion about that part.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.