[webkit-reviews] review granted: [Bug 46953] Add simple API tester for WebKit2 : [Attachment 69413] Updated patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Sam Weinig
<sam at webkit.org>'s request for review:
Bug 46953: Add simple API tester for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=46953

Attachment 69413: Updated patch
https://bugs.webkit.org/attachment.cgi?id=69413&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=69413&action=review

I didn't look at the Perl very closely.

When should we integrate this into run-webkit-tests?

> WebKitTools/ChangeLog:24
> +	   * TestWebKitAPI: Added.

Maybe TestWebKit2API would be better?

> WebKitTools/TestWebKitAPI/PlatformWebView.h:30
> +#if __APPLE__
> +#if __OBJC__

#ifdef is better if you want to turn on the "warn for undefined macros" warning
someday.

> WebKitTools/TestWebKitAPI/PlatformWebView.h:38
> +typedef WKView* PlatformWKView;
> +typedef NSWindow* PlatformWindow;

Misplaced *s.

> WebKitTools/TestWebKitAPI/PlatformWebView.h:52
> +    PlatformWKView platformView() { return m_view; }

Can be a const member function.

> WebKitTools/TestWebKitAPI/ForwardingHeaders/wtf/ASCIICType.h:1
> +#include <JavaScriptCore/ASCIICType.h>

Can we just use WebCore's forwarding headers instead? They're copied into
WebCore's PrivateHeaders.

> WebKitTools/TestWebKitAPI/Tests/WTF/Vector1.cpp:38
> +TEST(Vector1)
> +{
> +    Vector<int> intVector;
> +    TEST_ASSERT(intVector.isEmpty());
> +    TEST_ASSERT(intVector.size() == 0);
> +    TEST_ASSERT(intVector.capacity() == 0);
> +}

WTF isn't really part of the WebKit2 API. Why are we testing it here?

> WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:52
> +    State* state = ((State*)clientInfo);

Extra parentheses here. And static_cast/const_cast would maybe be better.

> WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:57
> +    TEST_ASSERT(state->didDecidePolicyForNavigationAction);
> +    TEST_ASSERT(!state->didCommitLoadForFrame);
> +
> +    state->didStartProvisionalLoadForFrame = true;

Should we assert that didStartProvisionalLoadForFrame is false before this?

An enum for the states would be nicer, since we always expect them to be called
in the same order. It would make the order clearer!

> WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:107
> +    PlatformWebView webView(pageNamespace.get());

Having PlatformWebView both here and in WebKitTestRunner makes me think that
getting the view from the page (rather than vice-versa) might be better
overall.