[webkit-reviews] review denied: [Bug 94373] [gtk] adding methods and signals for usermedia communication between webkit and browser : [Attachment 167833] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 10 13:05:33 PDT 2012


Martin Robinson <mrobinson at webkit.org> has denied Danilo Cesar Lemes de Paula
<danilo.cesar at collabora.co.uk>'s request for review:
Bug 94373: [gtk] adding methods and signals for usermedia communication between
webkit and browser
https://bugs.webkit.org/show_bug.cgi?id=94373

Attachment 167833: Patch
https://bugs.webkit.org/attachment.cgi?id=167833&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.


More information about the webkit-reviews mailing list