[webkit-reviews] review denied: [Bug 105180] [GTK] When the WebProcess crashes, a signal should be emitted : [Attachment 190991] process-crashed.diff

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Xan Lopez
<xan.lopez at gmail.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 190991: process-crashed.diff
https://bugs.webkit.org/attachment.cgi?id=190991&action=review

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


I think there's another processCrashed callback in the downloads client, that's
why we emit the signal in the web context indeed, we should also emit the
signal in that case, but we can do it in a separate patch with a new test case
for it.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:177
> +	* WebKitWebContext::process-crashed:
> +	* @context: the #WebKitWebContext
> +	*
> +	* This signal is emitted when a process crashes.

I think this is only emitted when the web process crashes, and as I commented
we might want to emit a signal for other processed, but I think it's unlikely.
So, we either, leave this as process-crashed and add an enum parameter with the
process type, or we name this web-process-crashed.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:102
> +    void (* process_crashed)  (WebKitWebContext *);
> +

This class is not inheritable in this moment, and you are not adding the offset
to this in the class definition anyway, so just remove it.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:-105
> -    void (*_webkit_reserved3) (void);

Don't remove the padding for now, we are not abi stable yet. I plan to add
padding to all classes before 2.0 release.

>> Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:49
>> +static void process_crashed_cb (WebKitWebContext* context, WebViewTest*
test)
> 
> Extra space before ( in function call  [whitespace/parens] [4]

This should be processCrashedCallback

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:75
> +    g_main_loop_run(test->m_mainLoop);
> +    g_assert(!result);
> +    g_assert(processCrashed == TRUE);

Check the result before running the main loop. Since the main loop is finished
in the callback we don't probably need the processCrashed variable, we can
leave it to make sure the main loop is not finished elsewhere but I think it's
redundant.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:86
> +    WebViewTest::add("WebKitWebExtension", "process-crashed",
testWebExtensionAbortProcess);

Nit: what we are testing is process-crashed signal so I would use
WebKitWebContext instead of WebKitWebExtension and testWebContextProcessCrashed
instead of testWebExtensionAbortProcess