Bug 94373

Summary: [gtk] adding methods and signals for usermedia communication between webkit and browser
Product: WebKit Reporter: Danilo de Paula <danilo.eu>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: cgarcia, eric.carlson, feature-media-reviews, gustavo, nick.diego, philn, pnormand, s.choi, tommyw, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 94361    
Bug Blocks: 87514    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch pnormand: review-

Description Danilo de Paula 2012-08-17 12:20:21 PDT
patching will be added in a few minutes.
Comment 1 Danilo de Paula 2012-08-17 12:21:51 PDT
Created attachment 159177 [details]
Patch
Comment 2 Danilo de Paula 2012-08-17 12:30:15 PDT
Created attachment 159179 [details]
Patch
Comment 3 Danilo de Paula 2012-08-20 07:49:55 PDT
I will add it's usage today to avoid dead code. So this patch doesn't need review yet.
Comment 4 Danilo de Paula 2012-08-21 09:55:59 PDT
Created attachment 159705 [details]
Patch
Comment 5 Carlos Garcia Campos 2012-09-19 08:39:05 PDT
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.
Comment 6 Danilo de Paula 2012-09-24 13:30:16 PDT
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.
Comment 7 Danilo de Paula 2012-09-26 12:40:34 PDT
Created attachment 165854 [details]
Patch
Comment 8 Danilo de Paula 2012-09-28 07:52:39 PDT
(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.
Comment 9 Danilo de Paula 2012-09-28 10:12:37 PDT
Created attachment 166270 [details]
Patch
Comment 10 WebKit Review Bot 2012-09-28 10:16:52 PDT
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.
Comment 11 Danilo de Paula 2012-09-28 10:59:36 PDT
Created attachment 166279 [details]
Patch
Comment 12 WebKit Review Bot 2012-09-28 11:02:48 PDT
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.
Comment 13 Danilo de Paula 2012-10-01 11:26:25 PDT
Comment on attachment 166279 [details]
Patch

adding request for commit
Comment 14 Danilo de Paula 2012-10-02 11:37:59 PDT
Created attachment 166722 [details]
Patch
Comment 15 Danilo de Paula 2012-10-02 11:40:18 PDT
Comment on attachment 166722 [details]
Patch

Asking for review and commit-queue.

Tests on fast/mediastream should cover this.
Comment 16 WebKit Review Bot 2012-10-02 11:44:52 PDT
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.
Comment 17 Danilo de Paula 2012-10-02 11:47:19 PDT
(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?
Comment 18 Carlos Garcia Campos 2012-10-03 06:48:46 PDT
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
Comment 19 Danilo de Paula 2012-10-03 11:14:43 PDT
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
Comment 20 Danilo de Paula 2012-10-03 13:12:35 PDT
Created attachment 166945 [details]
Patch
Comment 21 Tommy Widenflycht 2012-10-08 03:28:09 PDT
The general approach looks entirely reasonable to me, but I have zero experience with GTK and can't comment the specifics.
Comment 22 Danilo de Paula 2012-10-09 13:08:10 PDT
Created attachment 167833 [details]
Patch
Comment 23 Danilo de Paula 2012-10-09 13:12:48 PDT
Comment on attachment 167833 [details]
Patch

Changes:
 1 - added a default handler as suggested by mrobinson
 2 - removed the request-canceled signal
Comment 24 WebKit Review Bot 2012-10-09 13:13:06 PDT
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.
Comment 25 Martin Robinson 2012-10-10 13:06:13 PDT
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.
Comment 26 Danilo de Paula 2012-10-14 14:25:40 PDT
Created attachment 168589 [details]
Patch
Comment 27 WebKit Review Bot 2012-10-14 14:28:41 PDT
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.
Comment 28 Danilo de Paula 2012-10-14 14:31:47 PDT
Created attachment 168591 [details]
Patch
Comment 29 Danilo de Paula 2012-10-14 14:36:28 PDT
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?
Comment 30 WebKit Review Bot 2012-10-14 14:37:54 PDT
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.
Comment 31 Danilo de Paula 2012-10-14 14:45:46 PDT
Created attachment 168593 [details]
Patch
Comment 32 Danilo de Paula 2012-10-14 14:46:32 PDT
(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
Comment 33 WebKit Review Bot 2012-10-14 14:49:18 PDT
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.
Comment 34 Gustavo Noronha (kov) 2012-10-16 10:42:09 PDT
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?
Comment 35 Danilo de Paula 2012-10-16 13:47:52 PDT
Created attachment 169016 [details]
Patch
Comment 36 WebKit Review Bot 2012-10-16 13:53:18 PDT
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.
Comment 37 Danilo de Paula 2012-10-16 13:55:30 PDT
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?
Comment 38 Danilo de Paula 2012-10-18 10:17:13 PDT
Created attachment 169431 [details]
Patch
Comment 39 Danilo de Paula 2012-10-18 10:18:49 PDT
Comment on attachment 169431 [details]
Patch

fix build
changed API to use allow/deny convention
Comment 40 WebKit Review Bot 2012-10-18 10:21:21 PDT
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.
Comment 41 Gustavo Noronha (kov) 2012-10-19 10:07:30 PDT
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.
Comment 42 Philippe Normand 2013-10-07 06:39:15 PDT
I think we should focus on the WK2 implementation of this feature and actually disable it in WK1 if possible.
Comment 43 Philippe Normand 2013-10-15 08:55:34 PDT
Let's look at the bright future of WK2 :)