[webkit-reviews] review denied: [Bug 109980] [WK2] Calls to WKPageLoadURL() when WebProcess was terminated may fail : [Attachment 188966] First version (not for commit, tests are timing out ATM)

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Adenilson Cavalcanti Silva
<savagobr at yahoo.com>'s request for review:
Bug 109980: [WK2] Calls to WKPageLoadURL() when WebProcess was terminated may
fail
https://bugs.webkit.org/show_bug.cgi?id=109980

Attachment 188966: First version (not for commit, tests are timing out ATM)
https://bugs.webkit.org/attachment.cgi?id=188966&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188966&action=review


> Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp:39
> +

No need for the blank line.

> Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp:97
> +void didCrash(WKPageRef page, const void* clientInfo)
> +{
> +    WebKit2CrashLoader* testHelper =
const_cast<WebKit2CrashLoader*>(static_cast<const
WebKit2CrashLoader*>(clientInfo));
> +
> +    // On second crash, we try to load from the crash handler.
> +    EXPECT_TRUE(testHelper->firstSuccessfulLoad);
> +    testHelper->loadUrl();
> +}

I would move this definition to just above LoadPageOnCrashHandler.
This would reduce the span between definition and usage.

> Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp:99
> +TEST_F(WebKit2CrashLoader, LoadPageAfterCrash)

I am not a fan of TEST_F. I'd prefer something explicit with less GoogleTest
magic.

The test case name should also be strictly WebKit2 in order to easily select
tests for running.

> Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp:110
> +TEST_F(WebKit2CrashLoader, LoadPageOnCrashHandler)

LoadPageInCrashHandler?

> Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp:118
> +{
> +    // We install a crash handler, it should load a page.
> +    loaderClient.processDidCrash = didCrash;
> +    loadUrl();
> +    Util::run(&firstSuccessfulLoad);
> +    crashWebProcess();
> +    Util::run(&secondSuccessfulLoad);
> +}

This tests requires a bit more comments. It is not obvious how the test work.