[Webkit-unassigned] [Bug 123292] [GTK][WK2] Add webview signal for web-process-started

[bugzilla-daemon at webkit.org]

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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #215114|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-10-30 07:05:15 PST ---
(From update of attachment 215114)
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.

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