[webkit-reviews] review denied: [Bug 105180] [GTK] When the WebProcess crashes, a signal should be emitted : [Attachment 179743] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Joaquim Rocha
<jrocha at igalia.com>'s request for review:
Bug 105180: [GTK] When the WebProcess crashes, a signal should be emitted
https://bugs.webkit.org/show_bug.cgi?id=105180

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

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


We need unit tests for this too.

>> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:122
>> +	webkitWebViewWebProcessCrashed(WEBKIT_WEB_VIEW(m_viewWidget));
> 
> Is it safe to assume that m_viewWidget is a WebKitWebView here? For instance,
when using the C API it could be a WebKitWebViewBase.

Right. I think this could be used for internal stuff that we might want to do
when the web process crashes, or just keep it unimplemented. To expose a signal
in the API, we could use the processDidCrash callback of the Page Loader
client.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:118
> +    WEB_PROCESS_CRASHED,
> +

I'm not sure the web view is the best place for the signal. You might start a
download from a web context and get a crash (there's a different callback for
that in the download client), without having a view to emit the signal. Maybe
we could have two different signals, one in the web view and the other in the
web context or even the download object. In a multi web process model we
definitely want to know which web page caused the crash to only show the crash
message in the appropriate tab in the browser.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1280
> +	* WebKitWebView::web-process-crashed:
> +	* @web_view: the #WebKitWebView on which the signal is emitted
> +	*

For now this signal is only emitted when the web process crashes, I wonder what
is going to happen when the network process crashes, for example, once the
network process is implemented. Maybe we could call the signal process-crashed,
and pass the process that crashed as an enum. If eventually more type of
processed are added and we want to notify when they crash we just need to add
the process type to the enum.