[webkit-reviews] review denied: [Bug 109842] [WK2] Write a test to simulate crashed WebProcess followed by Window resize : [Attachment 188372] Missing XCode buildsystem changes

[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 109842: [WK2] Write a test to simulate crashed WebProcess followed by
Window resize
https://bugs.webkit.org/show_bug.cgi?id=109842

Attachment 188372: Missing XCode buildsystem changes
https://bugs.webkit.org/attachment.cgi?id=188372&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.