[webkit-reviews] review denied: [Bug 123292] [GTK][WK2] Add webview signal for web-process-started : [Attachment 215114] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Andre Moreira Magalhaes
<andrunko at gmail.com>'s request for review:
Bug 123292: [GTK][WK2] Add webview signal for web-process-started
https://bugs.webkit.org/show_bug.cgi?id=123292

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215114&action=review


Thanks for the patch. You should also add a new unit tests to test the new API.


> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:198
> +    WKContextConnectionClient wkConnectionClient = {
> +	   kWKContextConnectionClientCurrentVersion,
> +	   webView, // clientInfo
> +	   processDidStart
> +    };
> +    WebKitWebContext* webContext = webkit_web_view_get_context(webView);
> +   
WKContextSetConnectionClient(toAPI(webkitWebContextGetContext(webContext)),
&wkConnectionClient);

Why is this in WebKitLoaderClient? I think this should be in WebKitWebContext,
and the context should notify all its web views.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1408
> +	* This signal is emitted when the web process started.

You should explain this a bit more, is this also emitted when the web process
is respawned after a crash?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1409
> +	*/

Since: 2.4

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3142
> +void webkitWebViewWebProcessStarted(WebKitWebView* webView, WebConnection*
connection)

This would be easier if it receives the pid directly instead of the connection

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3148
>> +	g_signal_emit(webView, signals[WEB_PROCESS_STARTED], 0, (gpointer)
&pid);
> 
> You should use a c++-style cast if it is indeed needed
(static_cast<gpointer>())

We could avoid this by not passing the pid as a signal parameter, and adding
webkit_web_view_get_web_process_identifier(). This way you can also get the pid
without having to connect to this signal (if you already know the process has
been launched already).

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:252
> +    gboolean   (* web_process_started)	  (WebKitWebView	      
*web_view,

This should be void instead of gboolean.

> Source/WebKit2/UIProcess/WebContext.cpp:637
> +	       result = m_processes[i].get();
> +	       break;

You can simply return m_processes[i].get() here and you don't need the local
variable nor the break.

> Source/WebKit2/UIProcess/WebContext.cpp:640
> +    return result;

And return 0 here.