[Webkit-unassigned] [Bug 109842] [WK2] Write a test to simulate crashed WebProcess followed by Window resize

[bugzilla-daemon at webkit.org]

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


Benjamin Poulain <benjamin at webkit.org> changed:

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




--- Comment #2 from Benjamin Poulain <benjamin at webkit.org>  2013-02-14 23:23:30 PST ---
(From update of attachment 188372)
View in context: https://bugs.webkit.org/attachment.cgi?id=188372&action=review

The test is great.

Try to be more careful with your comments.
Think about how someone else will read this code in 2 years when the test fails. You need to maximize the important information, while reducing the noise and redundant information.

> Tools/ChangeLog:14
> +        * TestWebKitAPI/GNUmakefile.am:
> +        * TestWebKitAPI/PlatformEfl.cmake:
> +        * TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp: Added.

What about the xcodeproject ?

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:36
> +static bool resizeAfterCrash = false;
> +static bool done = false;
> +static PlatformWebView *webView = 0;

WebKit style: *webView.

"done" is too generic in this case. Because when "done == true", the test is not finished!

I would prefer if you put those 3 in a struct and pass it as clientInfo.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:40
> +    // Loading a blank page worked, next we will kill WebProcess

This comment is true only in the if(). It should be in the brackets.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:43
> +        // Set it, otherwise the loop will get stuck and we won't move to the
> +        // next step.

No need to cut the line. You can even get rid of this comment.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:48
> +    // If the UIProcess was able to resize (without a bounded WebProcess),

"bounded WebProcess"?

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:56
> +    // TODO: use a smart/shared pointer here
> +    delete webView;

Use a OwnPtr in your Struct.

Don't leave a TODO in a test :)
We also don't use TODO in WebKit, but FIXME.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:68
> +
> +    // Could be any size, it is just a matter of trigging an event
> +    // that will force the port to re-act (and if it is well behaved,
> +    // it will test WebPageProxy data members state before accessing them).
> +    // Maybe a safer value would be current window size + n
> +    // (where n = any value).
> +    webView->resizeTo(800, 800);

This comment is too long. Here, you could state:
1) This is the part being tested: resize could cause the UIProcess to crash. The test succeed if we do not crash.
2) The reason of the crash: The view was accessing the dead web process.

For the size, I would not bother and just call resizeTo twice:
    webView->resizeTo(100, 200);
    webView->resizeTo(300, 400);
That was you are sure whatever the size was, resize was not skipped.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:69
> +    resizeAfterCrash = false;

It should just be initialize to false when creating the clientInfo struct.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:93
> +    // Let's try load a page and see what happens.
> +    WKPageLoadURL(webView->page(), url.get());
> +    Util::run(&resizeAfterCrash);

It is great you have this part.

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