[Webkit-unassigned] [Bug 46953] Add simple API tester for WebKit2

[bugzilla-daemon at webkit.org]

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


Adam Roben (aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69413|review?                     |review+
               Flag|                            |




--- Comment #5 from Adam Roben (aroben) <aroben at apple.com>  2010-10-01 11:54:51 PST ---
(From update of attachment 69413)
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.

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