Bug 58048 – Implement keyDown in WebKitTestRunner

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Chang Shu]

A WebProcess-only version.

[Chang Shu]

Darin, I am going to work on this. Let me know if you already have done some work. Thanks.

[Darin Adler]

No, I didn’t do anything yet and I won’t have time to work on this for weeks, so please go for it!

[Chang Shu]

It seems calling handleKeyEvent in WebProcess/WebPage/WebPage.cpp will cause a UIProcess crash as WebProcess is trying to send back a message while UIProcess does not expect it. We should either handle the crash on UIProcess side or suppress the sending on WebProcess side.

Part of the call stack looks like this:

0   com.apple.WebKit2             	0x0000000100024053 WebKit::PageClientImpl::interpretKeyEvent(WebKit::NativeWebKeyboardEvent const&, WebKit::TextInputState const&, WTF::Vector<WebCore::KeypressCommand, 0ul>&) + 11 (PageClientImpl.mm:293)
1   com.apple.WebKit2             	0x00000001000cdb51 WebKit::WebPageProxy::interpretQueuedKeyEvent(WebKit::TextInputState const&, bool&, WTF::Vector<WebCore::KeypressCommand, 0ul>&) + 53 (WebPageProxyMac.mm:226)
2   com.apple.WebKit2             	0x00000001000a9989 void CoreIPC::handleMessage<Messages::WebPageProxy::InterpretQueuedKeyEvent, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebKit::TextInputState const&, bool&, WTF::Vector<WebCore::KeypressCommand, 0ul>&)>(CoreIPC::ArgumentDecoder*, CoreIPC::ArgumentEncoder*, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(WebKit::TextInputState const&, bool&, WTF::Vector<WebCore::KeypressCommand, 0ul>&)) + 134 (ArgumentCoder.h:39)
3   com.apple.WebKit2             	0x00000001000a6bd0 WebKit::WebPageProxy::didReceiveSyncWebPageProxyMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*, CoreIPC::ArgumentEncoder*) + 1452 (WebPageProxyMessageReceiver.cpp:475)
4   com.apple.WebKit2             	0x00000001000700ec WebKit::WebProcessProxy::didReceiveSyncMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*, CoreIPC::ArgumentEncoder*) + 242 (WebProcessProxy.cpp:308)
5   com.apple.WebKit2             	0x00000001000145c9 CoreIPC::Connection::dispatchSyncMessage(CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 149

[Alexey Proskuryakov]

handleKeyEvent should only be called as a response to keyDown, because there needs to be a native NSEvent queued in the UI process.

[Chang Shu]

(In reply to comment #4)
> handleKeyEvent should only be called as a response to keyDown, because there needs to be a native NSEvent queued in the UI process.

that's the issue we have now for the webprocess-only approach. Interestingly, it's not a problem for mouse events. Anyway, based on your comments, it seems better to fix the problem on the webprocess side, but the code would look pretty hacky.

[Alexey Proskuryakov]

I still think that it's not very useful for eventSender to work from web process side. It's the interaction of both sides that is primarily being tested.

[Darin Adler]

(In reply to comment #6)
> I still think that it's not very useful for eventSender to work from web process side. It's the interaction of both sides that is primarily being tested.

I don’t agree with your comment.

I do agree that it is even better to have an eventSender that tests the entire code path including the UI process. But what is primarily being tested is lots of WebCore code that works the same way in any case. You are overstating the case when you say “the interaction of both sides” is the primary things we are testing.

It would be quite helpful to have a working implementation that was web process side only even if we plan to do a UI process side implementation later.

[Chang Shu]

The cross-process approach would be the ultimate solution but I do see the two important benefits of webprocess-only approach: simplicity and platform-independence. I would see how many tests could pass after I make it work.

[Chang Shu]

Created attachment 89385 [details]
fix patch

[Build Bot]

Attachment 89385 [details] did not build on win:
Build output: http://queues.webkit.org/results/8398291

[Adele Peterson]

Comment on attachment 89385 [details]
fix patch

r- because the win-EWS bot says this will fail on Windows

[Chang Shu]

Created attachment 91980 [details]
fix patch 2: fix build break

fix win-EWS and some code refactoring.

[WebKit Review Bot]

Attachment 91980 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2404:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 91980 [details]
fix patch 2: fix build break

View in context: https://bugs.webkit.org/attachment.cgi?id=91980&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2283
> +#define KEYCODE_DEL         127
> +#define KEYCODE_BACKSPACE   8
> +#define KEYCODE_LEFTARROW   0xf702
> +#define KEYCODE_RIGHTARROW  0xf703
> +#define KEYCODE_UPARROW     0xf700
> +#define KEYCODE_DOWNARROW   0xf701

Normally we’d use C++ const instead of #define for something like this.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2285
> +WebKeyboardEvent WebPage::createWebKeyboardEventFromKeyString(WebKeyboardEvent::Type type, const String& key, WKEventModifiers modifiers, int location)

I’m not sure we should really have all this code to make from a key string to a keyboard event inside this function. It seems out of place and more related to the testing machinery and perhaps should be in the test runner instead of WebKit2. Maybe this function should instead take more arguments.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2287
> +    String text, unmodifiedText, keyIdentifier;

We normally declare only one function per line.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2307
> +            if (modifiers == kWKEventModifiersAltKey)
> +                modifiers = kWKEventModifiersControlKey;

Why?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2309
> +        } else if (code == 'y' && modifiers == kWKEventModifiersControlKey) {

Why is Control-Y the letter C? What is going on here?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2310
> +            text = String("c");

No need for the explicit String() here.

[Alexey Proskuryakov]

Comment on attachment 91980 [details]
fix patch 2: fix build break

View in context: https://bugs.webkit.org/attachment.cgi?id=91980&action=review

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2283
>> +#define KEYCODE_DOWNARROW   0xf701
> 
> Normally we’d use C++ const instead of #define for something like this.

It seems unfortunate that we have to duplicate constants from Cocoa/NSEvent.h.

[Chang Shu]

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2285
> > +WebKeyboardEvent WebPage::createWebKeyboardEventFromKeyString(WebKeyboardEvent::Type type, const String& key, WKEventModifiers modifiers, int location)
> 
> I’m not sure we should really have all this code to make from a key string to a keyboard event inside this function. It seems out of place and more related to the testing machinery and perhaps should be in the test runner instead of WebKit2. Maybe this function should instead take more arguments.

Good point. I will move this conversion code to WTR.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2307
> > +            if (modifiers == kWKEventModifiersAltKey)
> > +                modifiers = kWKEventModifiersControlKey;
> 
> Why?
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2309
> > +        } else if (code == 'y' && modifiers == kWKEventModifiersControlKey) {
> 
> Why is Control-Y the letter C? What is going on here?

Sorry the code wasn't polished enough. They were basically copied from EventSenderQt.cpp. It seems I should make a more cross-platform version even that means I should throw away some platform-specific support.

[Chang Shu]

(In reply to comment #16)
> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2285
> > > +WebKeyboardEvent WebPage::createWebKeyboardEventFromKeyString(WebKeyboardEvent::Type type, const String& key, WKEventModifiers modifiers, int location)
> > 
> > I’m not sure we should really have all this code to make from a key string to a keyboard event inside this function. It seems out of place and more related to the testing machinery and perhaps should be in the test runner instead of WebKit2. Maybe this function should instead take more arguments.
> 
> Good point. I will move this conversion code to WTR.

There's some difficulty about this as constants like VK_RETURN are defined inside WebCore, which are not accessible from WTR.

[Chang Shu]

> > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2285
> > > > +WebKeyboardEvent WebPage::createWebKeyboardEventFromKeyString(WebKeyboardEvent::Type type, const String& key, WKEventModifiers modifiers, int location)
> > > 
> > > I’m not sure we should really have all this code to make from a key string to a keyboard event inside this function. It seems out of place and more related to the testing machinery and perhaps should be in the test runner instead of WebKit2. Maybe this function should instead take more arguments.

Does it make sense to move the function to WebKit2/InjectedBundle?

[Chang Shu]

Created attachment 93288 [details]
fix patch 3

[Chang Shu]

Comment on attachment 93288 [details]
fix patch 3

I am getting close to a more complete implementation of keydown event (bug 57515). It will most likely make this patch obsolete.

[Chang Shu]

This bug is no longer valid.