Summary: | [GTK][WK2] Add webview signal for web-process-started | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andre Moreira Magalhaes <andrunko> | ||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | UNCONFIRMED --- | ||||||||||||
Severity: | Normal | CC: | bugs-noreply, cdumez, cgarcia, commit-queue, gustavo, gyuyoung.kim, mcatanzaro, mrobinson, rakuco | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Andre Moreira Magalhaes
2013-10-24 14:28:17 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.
Created attachment 215114 [details]
Patch
Rebased against upstream/master (was on top of 2.2)
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 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… (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 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. (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. (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. 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.
Created attachment 218871 [details]
Patch
This should not block the network process. (In reply to comment #10) > Created an attachment (id=218871) [details] > Patch The updated patch looks good to me. Carlos, what do you think about this one? 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 on attachment 218871 [details]
Patch
Assuming that patches for review since 2013 are stale, r-
|