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

• Status: UNCONFIRMED
• Component: WebKitGTK

[Andre Moreira Magalhaes]

So I wanted to add a way to map between extensions and WebProcess/Context/View on webkit2gtk and discussing with Gustavo Noronha (kov) we thought a good way to achieve this would be to have a signal (web-process-started) in the WebKitWebView with a pid argument. 

This way extensions could for example use getpid() to register on DBus using an unique name (e.g. com.foo.Bar-PID) and the view would be able to watch for this name to know when the extension registered on D-Bus (I am using multiple contexts here - added webkit_web_context_new locally also).

Patch to come.

[Andre Moreira Magalhaes]

Created attachment 215109 [details]
Patch

The signal uses GPid* as parameter as there is no GType for GPid. Maybe we could use a custom GType (holding a GPid) for it.

Let me know what you think.

[Andre Moreira Magalhaes]

Created attachment 215114 [details]
Patch 

Rebased against upstream/master (was on top of 2.2)

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Gustavo Noronha (kov)]

Comment on attachment 215114 [details]
Patch 

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

> 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>())

> Source/WebKit2/UIProcess/WebContext.cpp:631
> +WebProcessProxy* WebContext::getWebProcessForWebConnection(const WebConnection* webConnection)

How about having a method to query the pid for the remote end of the connection instead? You could have that method in the base class so you don't need to cast to WebConnectionToWebProcess. Or maybe we should go back and rethink the identifier for processes, make it a long integer like the ones used for sub resources…

[Andre Moreira Magalhaes]

(In reply to comment #4)
> (From update of attachment 215114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215114&action=review
> > Source/WebKit2/UIProcess/WebContext.cpp:631
> > +WebProcessProxy* WebContext::getWebProcessForWebConnection(const WebConnection* webConnection)
> 
> How about having a method to query the pid for the remote end of the connection instead? You could have that method in the base class so you don't need to cast to WebConnectionToWebProcess. Or maybe we should go back and rethink the identifier for processes, make it a long integer like the ones used for sub resources…

Adding a processIdentifier virtual method on WebProcess would do it, but only the UIProcess (WebConnectionToWebProcess) would implement this method, so not sure. Another option would be to use a hash of WebConnection->WebProcess, this way the method (WebContext::getWebProcessForWebConnection) would be faster.

I believe using the pid is better here as we already have the info in the UIProcess, otherwise we would have to pass the identifier to the extension and UIProcess.

[Carlos Garcia Campos]

Comment on attachment 215114 [details]
Patch 

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.

[Andre Moreira Magalhaes]

(In reply to comment #6)

Thanks for the review.

> 
> > 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
> 
I tried to reuse the didCreateConnection callback from WKContextConnectionClient, which is already called from WebContext when the WebProcess starts. What would you suggest instead? (see comments above). I could modify WKContextConnectionClient::didCreateConnection callback to include a pid param but that would require changing other ports.

I will work on the other changes, please let me know what you think about the question above.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> 
> Thanks for the review.
> 
> > 
> > > 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
> > 
> I tried to reuse the didCreateConnection callback from WKContextConnectionClient, which is already called from WebContext when the WebProcess starts. What would you suggest instead? (see comments above). I could modify WKContextConnectionClient::didCreateConnection callback to include a pid param but that would require changing other ports.

I mean to move the code to get the pid to the callback right before calling webkitWebViewWebProcessStarted.

[Andre Moreira Magalhaes]

Created attachment 215666 [details]
Patch

Ok, this new patch should handle the review comments (unless I missed something :)) and also fix other issues like properly matching the views with the webprocess that started (one WebContext can have multiple WebProcess and each WebProcess can be associated with multiple views).

Summary of changes since last patch:
- Moved WKContextConnectionClient handling to WebKitWebContext
- Do not pass pid on web-process-started signal
- Added webkit_web_view_get_web_process_identifier
- Added webkit_web_view_is_web_process_running (and is-web-process-running property)
- Reset ProcessProxy::processIdentifier when process crashes
- Added tests for new API
- Properly match view with web process that started
- Other small changes and review fixes (e.g. doc changes)

Please let me know what you think.

[Adrian Perez]

Created attachment 218871 [details]
Patch

[Carlos Garcia Campos]

This should not block the network process.

[Andre Moreira Magalhaes]

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

The updated patch looks good to me.

[Michael Catanzaro]

Carlos, what do you think about this one?

[Carlos Garcia Campos]

I need to look at the patch in detail, but maybe this is possible with WebKitWebContext::initialize-web-extensions signal + webkit_web_context_set_web_extensions_initialization_user_data() nowadays. I think that's what we do in ephy

[Brady Eidson]

Comment on attachment 218871 [details]
Patch

Assuming that patches for review since 2013 are stale, r-