[webkit-reviews] review denied: [Bug 34178] Add KeyboardTest to WebKit API tests : [Attachment 47433] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Yaar Schnitman
<yaar at chromium.org>'s request for review:
Bug 34178: Add KeyboardTest to WebKit API tests
https://bugs.webkit.org/show_bug.cgi?id=34178

Attachment 47433: Patch
https://bugs.webkit.org/attachment.cgi?id=47433&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebKit/chromium/tests/KeyboardTest.cpp

> +#include "../src/EditorClientImpl.h"
> +#include "../src/WebInputEventConversion.h"

how about just adding src to include_dirs in WebKit.gyp?


> +using WebCore::PlatformKeyboardEvent;
> +using WebCore::KeyboardEvent;
> +
> +using WebKit::EditorClientImpl;
> +using WebKit::PlatformKeyboardEventBuilder;
> +using WebKit::WebInputEvent;
> +using WebKit::WebKeyboardEvent;

let's just do 'using namespace Webcore' and 'using namespace WebKit'


> +class KeyboardTest : public testing::Test {
> +public:
> +    void SetUp()
> +    {
> +	   WTF::initializeThreading();

^^^ you should be able to delete this function.  RunAllTests.cpp takes care of
it.


> +    const char* InterpretKeyEvent(
> +    void SetupKeyDownEvent(WebKeyboardEvent* keyboardEvent,
> +    const char* InterpretOSModifierKeyPress(char keyCode)
> +    const char* InterpretCtrlKeyPress(char keyCode)
> +    const char* InterpretTab(int modifiers)
> +    const char* InterpretNewLine(int modifiers)

nit: change these to camelCase style


> +#if !defined(OS_MACOSX)

the OS_MACOSX macro is a chromium define.  i don't think we have access
to it here.  this should be changed to OS(DARWIN).