Bug 123292 - [GTK][WK2] Add webview signal for web-process-started
Summary: [GTK][WK2] Add webview signal for web-process-started
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-24 14:28 PDT by Andre Moreira Magalhaes
Modified: 2017-03-11 11:02 PST (History)
9 users (show)

See Also:


Attachments
Patch (11.08 KB, patch)
2013-10-24 15:04 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch (11.09 KB, patch)
2013-10-24 15:59 PDT, Andre Moreira Magalhaes
cgarcia: review-
cgarcia: commit-queue-
Details | Formatted Diff | Diff
Patch (30.06 KB, patch)
2013-10-31 12:42 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch (18.93 KB, patch)
2013-12-10 07:11 PST, Adrian Perez
beidson: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Moreira Magalhaes 2013-10-24 14:28:17 PDT
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.
Comment 1 Andre Moreira Magalhaes 2013-10-24 15:04:15 PDT
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.
Comment 2 Andre Moreira Magalhaes 2013-10-24 15:59:38 PDT
Created attachment 215114 [details]
Patch 

Rebased against upstream/master (was on top of 2.2)
Comment 3 WebKit Commit Bot 2013-10-24 16:00:54 PDT
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
Comment 4 Gustavo Noronha (kov) 2013-10-30 06:17:56 PDT
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…
Comment 5 Andre Moreira Magalhaes 2013-10-30 06:42:23 PDT
(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.
Comment 6 Carlos Garcia Campos 2013-10-30 07:05:15 PDT
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.
Comment 7 Andre Moreira Magalhaes 2013-10-30 11:35:40 PDT
(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.
Comment 8 Carlos Garcia Campos 2013-10-31 01:02:49 PDT
(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.
Comment 9 Andre Moreira Magalhaes 2013-10-31 12:42:19 PDT
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.
Comment 10 Adrian Perez 2013-12-10 07:11:15 PST
Created attachment 218871 [details]
Patch
Comment 11 Carlos Garcia Campos 2014-01-15 05:43:45 PST
This should not block the network process.
Comment 12 Andre Moreira Magalhaes 2014-02-23 16:37:16 PST
(In reply to comment #10)
> Created an attachment (id=218871) [details]
> Patch

The updated patch looks good to me.
Comment 13 Michael Catanzaro 2016-01-02 09:10:12 PST
Carlos, what do you think about this one?
Comment 14 Carlos Garcia Campos 2016-01-02 09:51:40 PST
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
Comment 15 Brady Eidson 2016-05-24 22:05:43 PDT
Comment on attachment 218871 [details]
Patch

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