Bug 94373 – [gtk] adding methods and signals for usermedia communication between webkit and browser

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Danilo de Paula]

patching will be added in a few minutes.

[Danilo de Paula]

Created attachment 159177 [details]
Patch

[Danilo de Paula]

Created attachment 159179 [details]
Patch

[Danilo de Paula]

I will add it's usage today to avoid dead code. So this patch doesn't need review yet.

[Danilo de Paula]

Created attachment 159705 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 159705 [details]
Patch

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

I have no idea about media stream API, so I'll just comment on general things. It would help to include unit tests with the patch. Setting r- because the patch seems incomplete.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:25
> +#include "MediaStreamSource.h"

This is already included by webkitwebusermedialistprivate.h

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:29
> +#include <glib.h>

This is already included in the header.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:30
> +#include <glib/gi18n-lib.h>

There aren't translatable strings in this file, no?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:31
> +#include <stdio.h>

What do you need this for?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:50
> +    gboolean* sourceSelections;

Use Vector<bool>

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:53
> +G_DEFINE_TYPE(WebKitWebUserMediaList, webkit_web_user_media_list, G_TYPE_OBJECT);

The trailing ; is not needed.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:58
> +static void webkit_web_user_media_list_dispose(GObject* object)
> +{
> +    G_OBJECT_CLASS(webkit_web_user_media_list_parent_class)->dispose(object);
> +}

Don't define dispose just to chain up.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:60
> +static void webkit_web_user_media_list_finalize(GObject* object)

We use the WebKit coding style for internal private methods, webkitWebViewFinalize().

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:64
> +    g_free(userMediaList->priv->sourceSelections);

Use the the placement new syntax to allocate the private structure and simply call the destructor here. See webkitwebview.cpp as an example.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:83
> +    WebKitWebUserMediaListPrivate* priv = G_TYPE_INSTANCE_GET_PRIVATE(list, WEBKIT_TYPE_WEB_USER_MEDIA_LIST, WebKitWebUserMediaListPrivate);
> +    list->priv = priv;

Use placement new syntax, see webkit_web_view_init()

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:122
> +    return g_strdup(sources[index]->name().utf8().data());

We could cache the name on demand using a CString to return a const gchar * instead.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:136
> +gboolean webkit_web_user_media_list_get_item_selected(WebKitWebUserMediaList* list, guint index)

is_item_selected?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:141
> +    WebCore::MediaStreamSourceVector& sources = core(list);
> +    g_return_val_if_fail(index < sources.size(), FALSE);

Could this be just g_return_val_if_fail(index < core(list).size(), FALSE); ?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:160
> +    WebCore::MediaStreamSourceVector& sources = core(list);
> +    g_return_if_fail(index < sources.size());

Ditto.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:31
> +#include <glib.h>

This is already included by the header.

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

No translatable strings here either.

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

Trailing ; is not needed

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:64
> +    WebKitWebUserMediaRequestPrivate* priv = G_TYPE_INSTANCE_GET_PRIVATE(request, WEBKIT_TYPE_WEB_USER_MEDIA_REQUEST, WebKitWebUserMediaRequestPrivate);
> +    request->priv = priv;

Use the placement new syntax here too, so that the destructor is called and the RefPtr is destroyed.

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

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:113
> +    g_return_val_if_fail(WEBKIT_IS_WEB_USER_MEDIA_REQUEST(request), NULL);

Use 0 instead of NULL.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:115
> +    return core(request)->scriptExecutionContext()->securityOrigin()->toString().utf8().data();

This looks like a temporary value, it should be cached to return a const gchar *, or duplicated and return a gchar *

> Source/WebKit/gtk/webkit/webkitwebview.cpp:126
> +#include "MediaStreamSource.h"
> +#include "UserMediaClientGtk.h"
> +#include "webkitwebusermedialist.h"
> +#include "webkitwebusermedialistprivate.h"

webkitwebusermedialistprivate.h already includes webkitwebusermedialist.h and MediaStreamSource.h

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

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

/* -> /**

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

Trailing : missing here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2759
> +     * When the application request userMedia, this signal will be emited with the request
> +     * 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()?

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

Why G_SIGNAL_ACTION?

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

/* -> /**

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

Trailing : missing here.

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

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

Why G_SIGNAL_ACTION?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2792
> +#endif /* ENABLE(MEDIA_STREAM) */

Where are the signals emitted?

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

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

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

Ditto.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5420
> +    // TODO: uncomment the following lines when the UserMediaClientGtk implementation is completed
> +    // UserMediaClientGtk* client = static_cast<UserMediaClientGtk*>(priv->userMediaClient.get());
> +    // 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.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5441
> +    // TODO: uncomment thw following lines when the UserMediaClientGtk implementation is completed
> +    // UserMediaClientGtk* client = static_cast<UserMediaClientGtk*>(priv->userMediaClient.get());
> +    // 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.

[Danilo de Paula]

Comment on attachment 159705 [details]
Patch

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.

[Danilo de Paula]

Created attachment 165854 [details]
Patch

[Danilo de Paula]

(In reply to comment #7)
> Created an attachment (id=165854) [details]
> Patch

Made a few changes on how the UserMediaGTK works, so it's not necessary to warn it about the request acceptance. The accept can be handled by the view now.

will push the commit again as soon as I get it build and check if everything is working as expected.

[Danilo de Paula]

Created attachment 166270 [details]
Patch

[WebKit Review Bot]

Attachment 166270 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:456:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:457:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:458:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:462:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2748:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2769:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 9 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

Created attachment 166279 [details]
Patch

[WebKit Review Bot]

Attachment 166279 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:456:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:457:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:458:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:462:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2748:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2769:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 9 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

Comment on attachment 166279 [details]
Patch

adding request for commit

[Danilo de Paula]

Created attachment 166722 [details]
Patch

[Danilo de Paula]

Comment on attachment 166722 [details]
Patch

Asking for review and commit-queue.

Tests on fast/mediastream should cover this.

[WebKit Review Bot]

Attachment 166722 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/GtkLauncher/main.c:241:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/GtkLauncher/main.c:242:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/GtkLauncher/main.c:243:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/GtkLauncher/main.c:244:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/GtkLauncher/main.c:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/GtkLauncher/main.c:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/GtkLauncher/main.c:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:456:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:457:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:458:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:462:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2748:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2769:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 16 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

(In reply to comment #16)
> Attachment 166722 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
> Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Tools/GtkLauncher/main.c:241:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Tools/GtkLauncher/main.c:242:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Tools/GtkLauncher/main.c:243:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Tools/GtkLauncher/main.c:244:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Tools/GtkLauncher/main.c:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Tools/GtkLauncher/main.c:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Tools/GtkLauncher/main.c:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebview.h:456:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebview.h:457:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebview.h:458:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebview.h:462:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebview.cpp:2748:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebview.cpp:2769:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Total errors found: 16 in 18 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

They're all false positive. Someone could please drop some comments?

[Carlos Garcia Campos]

Comment on attachment 166722 [details]
Patch

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

Still someone who knows about the media stream api should review this, and it's new api so we need two reviewers. I've just commented about general things only.

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:29
> -#include "UserMediaClientGtk.h"
>  
>  #if ENABLE(MEDIA_STREAM)
> -#include "MediaStreamSource.h"
> -#include "NotImplemented.h"
> -#include "UserMediaRequest.h"
> +
> +#include "UserMediaClientGtk.h"
> +
> +#include "webkitwebusermedialistprivate.h"
> +#include "webkitwebusermediarequestprivate.h"

That was correct, it should be config and then UserMediaClientGtk.h, followed by an empty line and then all other headers. UserMediaClientGtk.h already has #if ENABLE(MEDIA_STREAM) so it's not a problem if it's used before the #if here.

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:35
> +UserMediaClientGtk::UserMediaClientGtk(WebKitWebView *webView)

WebKitWebView *webView -> WebKitWebView* webView

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:49
> -void UserMediaClientGtk::requestUserMedia(WTF::PassRefPtr<UserMediaRequest> prpRequest, const MediaStreamSourceVector& audioSource, const MediaStreamSourceVector& videoSource)
> +void UserMediaClientGtk::requestUserMedia(WTF::PassRefPtr<UserMediaRequest> prpRequest, const WebCore::MediaStreamSourceVector& audioSources, const WebCore::MediaStreamSourceVector& videoSources)
>  {

This is not needed, there's using namespace WebCore; in this file

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:51
> +    RefPtr<UserMediaRequest> request = prpRequest;
> +    g_signal_emit_by_name(m_webView, "user-media-requested", kitNew(request.get()), kitNew(audioSources), kitNew(videoSources));

Who will be responsible for releasing the ref you are taking here?

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:59
> -} // namespace WebKit;
> +}

Why do you remove the comment?

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.h:41
> -    UserMediaClientGtk();
> -    virtual ~UserMediaClientGtk();
> +    UserMediaClientGtk(WebKitWebView*);
> +    ~UserMediaClientGtk();

Was the virtual wrong for the destructor?

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.h:52
> -} // namespace WebKit
> +}

why removing this?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:28
> +#include "config.h"
> +
> +#if ENABLE(MEDIA_STREAM)
> +
> +#include "webkitwebusermedialist.h"
> +
> +#include "webkitglobalsprivate.h"
> +#include "webkitwebusermedialistprivate.h"
> +
> +#include <wtf/text/CString.h>

This should be

#include "config.h"
#include "webkitwebusermedialist.h"

#if ENABLE(MEDIA_STREAM)
#include "webkitglobalsprivate.h"
#include "webkitwebusermedialistprivate.h"
#include <wtf/text/CString.h>

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:39
> + * Since: 1.10.0

1.10.0 was released already, this should be 2.0

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:59
> +static void webkitWebUserMediaListDispose(GObject* object)
> +{
> +    WebKitWebUserMediaList* userMediaList = WEBKIT_WEB_USER_MEDIA_LIST(object);
> +    WebKitWebUserMediaListPrivate* priv = userMediaList->priv;
> +
> +    priv->sourceSelections.clear();
> +
> +    G_OBJECT_CLASS(webkit_web_user_media_list_parent_class)->dispose(object);
> +}

I don't think you need this, the vector will be released by the struct destructor in Finalize()

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:69
> +    GObjectClass* gobject_class = G_OBJECT_CLASS(listClass);

gobject_class -> gobjectClass

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:95
> + * Since: 1.10.0

2.0

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:114
> + * Since: 1.10.0

Ditto.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:135
> + * Since: 1.10.0

Ditto.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:141
> +    g_return_val_if_fail(WEBKIT_IS_WEB_USER_MEDIA_LIST(list), FALSE);
> +
> +    g_return_val_if_fail(index < core(list).size(), FALSE);

Extra new line there

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:153
> + * Since: 1.10.0

2.0

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:159
> +    g_return_if_fail(WEBKIT_IS_WEB_USER_MEDIA_LIST(list));
> +
> +    g_return_if_fail(index < core(list).size());

Extra new line there

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:24
> +#include "config.h"
> +
> +#if ENABLE(MEDIA_STREAM)
> +
> +#include "webkitwebusermediarequest.h"

ame issue here with the headers

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:42
> + * Since: 1.10.0

2.0

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:84
> + * Return value: True if the request contains an audio request.

True -> %TRUE

True if the request contains an audio request or %FALSE otherwise.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:101
> + * Return value: True if the request contains a video request.

Ditto

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:133
> +    g_return_val_if_fail(WEBKIT_IS_WEB_USER_MEDIA_REQUEST(request), 0);

This is not a public API mehod, use ASSERT or nothing instead of g_return macros.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:140
> +    g_return_val_if_fail(request, 0);

Ditto.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.h:59
> +WEBKIT_API WebKitSecurityOrigin*

WebKitSecurityOrigin* -> WebKitSecurityOrigin *

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2742
> +     * Then the user is expected to call either #webkit_web_view_accept_user_media_request() or

Don't need to use # for function names

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2745
> +     * Since: 1.10.0

2.0

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5410
> +    core(webRequest)->succeed(audioSources, videoSources);

I wonder why this is a method of WebView if it doesn't use the web view. Looks too me like a method of WebKitWebUserMediaRequest, something like webkit_web_user_media_request_accept();

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5427
> +void webkit_web_view_reject_user_media_request(WebKitWebView *webView, WebKitWebUserMediaRequest *webRequest)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
> +    g_return_if_fail(WEBKIT_IS_WEB_USER_MEDIA_REQUEST(webRequest));
> +
> +    core(webRequest)->fail();

Same here, webView is unused

[Danilo de Paula]

Comment on attachment 166722 [details]
Patch

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

>> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:29
>> +#include "webkitwebusermediarequestprivate.h"
> 
> That was correct, it should be config and then UserMediaClientGtk.h, followed by an empty line and then all other headers. UserMediaClientGtk.h already has #if ENABLE(MEDIA_STREAM) so it's not a problem if it's used before the #if here.

fixed.

>> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:35
>> +UserMediaClientGtk::UserMediaClientGtk(WebKitWebView *webView)
> 
> WebKitWebView *webView -> WebKitWebView* webView

ok.

>> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:49
>>  {
> 
> This is not needed, there's using namespace WebCore; in this file

ok

>> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:59
>> +}
> 
> Why do you remove the comment?

fixed.

>> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:28
>> +#include <wtf/text/CString.h>
> 
> This should be
> 
> #include "config.h"
> #include "webkitwebusermedialist.h"
> 
> #if ENABLE(MEDIA_STREAM)
> #include "webkitglobalsprivate.h"
> #include "webkitwebusermedialistprivate.h"
> #include <wtf/text/CString.h>

fixed.

>> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:39
>> + * Since: 1.10.0
> 
> 1.10.0 was released already, this should be 2.0

I was thinking that it should be something like Since: TODO, and fix that after the approval.
But ok, changing to 2.0.0

>> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:59
>> +}
> 
> I don't think you need this, the vector will be released by the struct destructor in Finalize()

The whole dispose was removed.

>> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:69
>> +    GObjectClass* gobject_class = G_OBJECT_CLASS(listClass);
> 
> gobject_class -> gobjectClass

fixed.

>> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:95
>> + * Since: 1.10.0
> 
> 2.0

fixed,

>> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:114
>> + * Since: 1.10.0
> 
> Ditto.

ok.

>> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:135
>> + * Since: 1.10.0
> 
> Ditto.

ok

>> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:159
>> +    g_return_if_fail(index < core(list).size());
> 
> Extra new line there

ok

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:24
>> +#include "webkitwebusermediarequest.h"
> 
> ame issue here with the headers

fixed.

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:42
>> + * Since: 1.10.0
> 
> 2.0

ok

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:84
>> + * Return value: True if the request contains an audio request.
> 
> True -> %TRUE
> 
> True if the request contains an audio request or %FALSE otherwise.

ok

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:101
>> + * Return value: True if the request contains a video request.
> 
> Ditto

ok

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:133
>> +    g_return_val_if_fail(WEBKIT_IS_WEB_USER_MEDIA_REQUEST(request), 0);
> 
> This is not a public API mehod, use ASSERT or nothing instead of g_return macros.

ok

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:140
>> +    g_return_val_if_fail(request, 0);
> 
> Ditto.

ok

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5410
>> +    core(webRequest)->succeed(audioSources, videoSources);
> 
> I wonder why this is a method of WebView if it doesn't use the web view. Looks too me like a method of WebKitWebUserMediaRequest, something like webkit_web_user_media_request_accept();

vestige of an old implementation. fixed by now

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5427
>> +    core(webRequest)->fail();
> 
> Same here, webView is unused

ditto

[Danilo de Paula]

Created attachment 166945 [details]
Patch

[Tommy Widenflycht]

The general approach looks entirely reasonable to me, but I have zero experience with GTK and can't comment the specifics.

[Danilo de Paula]

Created attachment 167833 [details]
Patch

[Danilo de Paula]

Comment on attachment 167833 [details]
Patch

Changes:
 1 - added a default handler as suggested by mrobinson
 2 - removed the request-canceled signal

[WebKit Review Bot]

Attachment 167833 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebusermediarequest.h:68:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:163:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Extra space between WebKitWebUserMediaList and *audio  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Extra space between WebKitWebUserMediaList and *video  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:1201:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:1202:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:1203:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:1204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:1205:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:1206:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:1207:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2861:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 17 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 167833 [details]
Patch

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

Just a few general things since this patch is still evolving:

1. Use C++ style comments (// instead of /*)
2. It seems this API should not be synchronous, but asynchronous. It's okay if the default implementation is synchronous now, but it should be possible to choose the media devices asynchronously. To see how we typically do this look at http://webkitgtk.org/reference/webkit2gtk/unstable/WebKitPolicyDecision.html

And a couple specific things:

> Source/WebKit/gtk/webkit/webkitwebview.cpp:217
> +    CHOOSE_MEDIA_DEVICE,

I'm not certain, but adding this in the middle may be an ABI break.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1186
> +    GtkWidget* dialog;
> +    GtkWidget* contentArea;
> +    GtkWidget* actionArea;
> +    GtkWidget* frame;

In WebKit we typically declare variables the first time we use them instead of at the start of the method/function.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1201
>> +                                         GTK_WINDOW(gtk_widget_get_toplevel(GTK_WIDGET(webView))),
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

The style bot complains when we align things now, so just indent them 8 spaces here.

[Danilo de Paula]

Created attachment 168589 [details]
Patch

[WebKit Review Bot]

Attachment 168589 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.list.am', u'Sou..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebusermediarequest.h:68:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:163:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Extra space between WebKitWebUserMediaList and *audio  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Extra space between WebKitWebUserMediaList and *video  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2795:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 10 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

Created attachment 168591 [details]
Patch

[Danilo de Paula]

Comment on attachment 168591 [details]
Patch

a few comments:

1 - Fixed the current implementation to be async
2 - added a new GtkMediaChooserDialog dialog on webcore
    * this dialog exports a gtkwidget on the widget() method, so the view can connect to the 'response' signal. I don't know if it's the best option, is there any way to create a gsignal on a c++ class?

[WebKit Review Bot]

Attachment 168591 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.list.am', u'Sou..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebusermediarequest.h:68:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:163:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Extra space between WebKitWebUserMediaList and *audio  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Extra space between WebKitWebUserMediaList and *video  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2795:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 10 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

Created attachment 168593 [details]
Patch

[Danilo de Paula]

(In reply to comment #31)
> Created an attachment (id=168593) [details]
> Patch

1 - Fixed the current implementation to be async
2 - added a new GtkMediaChooserDialog dialog on webcore
    * this dialog exports a gtkwidget on the widget() method, so the view can connect to the 'response' signal. I don't know if it's the best option, is there any way to create a gsignal on a c++ class?
3 - Fixed changelog

[WebKit Review Bot]

Attachment 168593 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.list.am', u'Sou..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebusermediarequest.h:68:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:163:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Extra space between WebKitWebUserMediaList and *audio  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Extra space between WebKitWebUserMediaList and *video  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2795:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 10 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 168593 [details]
Patch

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

The API looks OK to me in general, but I'll try to review the spec later today and see if I can spot any problems.

> Source/WebKit/gtk/ChangeLog:6
> +        When the user make a getUserMedia call, webview will emit a sigial (user-media-requested)

make -> makes sigial -> signal

> Source/WebKit/gtk/ChangeLog:7
> +        to the application containing then request itself (webkitwebusermediarequest) and two

then -> the

> Source/WebKit/gtk/ChangeLog:8
> +        list with the available audio and video devices (webkitwebusermedialist).

list -> lists

> Source/WebKit/gtk/ChangeLog:17
> +        On the case the user reject the request, webkit_web_user_media_request_fail must

On the case -> In case?

> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:25
> +

No blank line

> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:39
> +    GtkWidget* vbox;
> +    GtkWidget* audioMessage = 0;
> +    GtkWidget* videoMessage = 0;

How about making this a GtkBuilder file that can more easily be changed if necessary?

> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:43
> +    gint audioListLength = audioSource.size();
> +    gint videoListLength = videoSource.size();

We tend not to use glib types for types that are simple C types.

> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:72
> +        audioMessage = gtk_label_new(hasAudio ? _("Select from available user audio") : _("No user audio available"));

These messages are quite cryptic for someone that doesn't know the spec I think. User audios are audio inputs? 'audio' should use the plural in this case I imagine?

> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:88
> +            gtk_combo_box_text_append_text(GTK_COMBO_BOX_TEXT(m_audioCombo), g_strdup(audioSource[i]->name().utf8().data()));

You're leaking the string that you create with g_strdup. Why g_strdup, if the function takes a const char*?

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:131
> + * webkit_web_user_media_request_succeed:

How about sticking to the allow/deny terminology we use for our other policy decisions?

[Danilo de Paula]

Created attachment 169016 [details]
Patch

[WebKit Review Bot]

Attachment 169016 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.list.am', u'Sou..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebusermediarequest.h:68:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:163:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Extra space between WebKitWebUserMediaList and *audio  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Extra space between WebKitWebUserMediaList and *video  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2795:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 10 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

Comment on attachment 168593 [details]
Patch

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

>> Source/WebKit/gtk/ChangeLog:6
>> +        When the user make a getUserMedia call, webview will emit a sigial (user-media-requested)
> 
> make -> makes sigial -> signal

fixed

>> Source/WebKit/gtk/ChangeLog:7
>> +        to the application containing then request itself (webkitwebusermediarequest) and two
> 
> then -> the

fixed

>> Source/WebKit/gtk/ChangeLog:8
>> +        list with the available audio and video devices (webkitwebusermedialist).
> 
> list -> lists

fixed.

>> Source/WebKit/gtk/ChangeLog:17
>> +        On the case the user reject the request, webkit_web_user_media_request_fail must
> 
> On the case -> In case?

fixed.

>> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:25
>> +
> 
> No blank line

fixed.

>> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:39
>> +    GtkWidget* videoMessage = 0;
> 
> How about making this a GtkBuilder file that can more easily be changed if necessary?

discussed on irc about it. Decided to keep simple while the dialog is simpler.
Will change that in case it gets much more complex.

>> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:43
>> +    gint videoListLength = videoSource.size();
> 
> We tend not to use glib types for types that are simple C types.

fixed.

>> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:72
>> +        audioMessage = gtk_label_new(hasAudio ? _("Select from available user audio") : _("No user audio available"));
> 
> These messages are quite cryptic for someone that doesn't know the spec I think. User audios are audio inputs? 'audio' should use the plural in this case I imagine?

fixed all the messages, including title.

>> Source/WebCore/platform/gtk/GtkMediaChooserDialog.cpp:88
>> +            gtk_combo_box_text_append_text(GTK_COMBO_BOX_TEXT(m_audioCombo), g_strdup(audioSource[i]->name().utf8().data()));
> 
> You're leaking the string that you create with g_strdup. Why g_strdup, if the function takes a const char*?

fixed.

>> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:131
>> + * webkit_web_user_media_request_succeed:
> 
> How about sticking to the allow/deny terminology we use for our other policy decisions?

in my first prototype the API was request_accept and request_reject.
I changed it back to succeed/fail to reflect the webkit internals (UserMediaRequest->succeed and fail).

What is best? Reflect the internal API or stick to what we have? To be honest I don't like succeed() and fail(), but I thought that using it would be safer.
Any second thought? Mrobinson?

[Danilo de Paula]

Created attachment 169431 [details]
Patch

[Danilo de Paula]

Comment on attachment 169431 [details]
Patch

fix build
changed API to use allow/deny convention

[WebKit Review Bot]

Attachment 169431 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.list.am', u'Sou..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebusermediarequest.h:68:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebusermedialist.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:163:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Extra space between WebKitWebUserMediaList and *audio  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:164:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Extra space between WebKitWebUserMediaList and *video  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.h:165:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:2795:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 10 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 169431 [details]
Patch

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

The API looks good to me, I'm just wondering about passing the list of chocies to allow; the spec says currently in its implementation suggestions: "A single call to getUserMedia() will always return a stream with either zero or one audio tracks, and either zero or one video tracks.", what do you think?

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:49
> +    RefPtr<UserMediaRequest> request = prpRequest;
> +    g_signal_emit_by_name(m_webView, "choose-media-device", kitNew(request.get()), kitNew(audioSource), kitNew(videoSource));

The spec says already in use devices should be considered busy and not shown, do you know whether we're doing that kind of filtering atm?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:32
> + * @Title: WebKitWebUserMediaList

From the looks of it the names here are case insensitive, but might as well be consistent and keep everything lower-case.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:70
> +    // placement new syntax

This comment's not very useful.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:141
> +void webkit_web_user_media_request_allow(WebKitWebUserMediaRequest* webRequest, WebKitWebUserMediaList* audioMediaList, WebKitWebUserMediaList* videoMediaList)

The implementation suggestion seems to say that there should be either 0 or 1 selected items in a reply to getUserMedia, currently, I'm wondering if using a list with possibly multiple selections is the way to go (http://dev.w3.org/2011/webrtc/editor/getusermedia.html#implementation-suggestions)

> Source/WebKit/gtk/webkit/webkitwebview.cpp:126
> +

Left over new line?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1217
> +    UserMediaSelectorData* data = g_new(UserMediaSelectorData, 1);

This looks like a good place to use g_slice_new =)

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1221
> +    data->request = (WebKitWebUserMediaRequest*) g_object_ref(request);
> +    data->audioMediaList = (WebKitWebUserMediaList*) g_object_ref(audioMediaList);
> +    data->videoMediaList = (WebKitWebUserMediaList*) g_object_ref(videoMediaList);

We usually do C++-style casts, static_cast<WebKit...*>()

> Source/WebKit/gtk/webkit/webkitwebview.h:162
> +    gboolean                   (* choose_media_device)    (WebKitWebView             *web_view,

Adding this to the middle of the class struct breaks ABI. That's not a problem if we decide to break ABI for 2.0, but something to keep in mind.

[Philippe Normand]

I think we should focus on the WK2 implementation of this feature and actually disable it in WK1 if possible.

[Philippe Normand]

Let's look at the bright future of WK2 :)