[Webkit-unassigned] [Bug 109980] [WK2] Calls to WKPageLoadURL() when WebProcess was terminated may fail

[bugzilla-daemon at webkit.org]

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


Benjamin Poulain <benjamin at webkit.org> changed:

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




--- Comment #2 from Benjamin Poulain <benjamin at webkit.org>  2013-02-18 18:32:45 PST ---
(From update of attachment 188966)
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.

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