[Webkit-unassigned] [Bug 105180] [GTK] When the WebProcess crashes, a signal should be emitted

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=105180


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #190991|review?                     |review-
               Flag|                            |




--- Comment #10 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-03-01 11:17:10 PST ---
(From update of attachment 190991)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.