Bug 57515 – [WK2] [Mac] Implement KeyDown function for WebKit2 EventSender

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chang Shu]

Start building framework using KeyDown event.

[Chang Shu]

Created attachment 87740 [details]
fix patch

This patch does the following things for KeyDown event:
1. passes function calls from WebKitTestRunner::InjectedBundle to WebKit2::WebProcess::InjectedBundle through WK interface 
2. encodes it as a binary buffer attached to a general InjectedBundle message.
3. sends out the message to UIProcess
4. UIProcess passes the general InjectedBundle message to WebKitTestRunner::EventSenderProxy.
5. EventSenderProxy calls the decoding mechanism through WK interface
5. EventSenderProxy further calls the platform-dependent code to trigger the event from the UIProcess side.

Note that the platform-dependent implementation on Mac has not been done yet for my lack of experience on objC development.

[Chang Shu]

I changed the title of the bug since my patch is 100% common code except the project files.

[Darin Adler]

Maciej told me an approach like this won’t work due to the behavior of CoreIPC.

[Darin Adler]

We can make it work by enhancing CoreIPC. I don’t know the details. Maybe he can fill us in.

Adding him to the cc list. In the mean time I am making a version of eventSender work entirely within the web process. We can still move to the round trip UI process version later, but this will get many tests running and testing what they intend to even though they will not test as much code as the multi-process version we eventually want.

[Early Warning System Bot]

Attachment 87740 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8315188

[Build Bot]

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

[Chang Shu]

(In reply to comment #3)
> Maciej told me an approach like this won’t work due to the behavior of CoreIPC.

Hi, Darin,
I am not sure which part of the code you are referring to. With the similar patch for bug 56485, I have made it work on Qt and fixed at least one test case. It will be good the enhance CoreIPC though and the WK interface will become neater.

[Darin Adler]

I am going to put my patches that work entirely in the web process up for review. I hope that does not make your task more difficult. I think your direction is fine. It’s Maciej and Anders that can explain the concerns. Please also fee free to replace my code with yours if mine lands and yours will work even better.

[Chang Shu]

Created attachment 87766 [details]
fix patch 2

fix build break

[WebKit Review Bot]

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

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

Tools/WebKitTestRunner/EventSenderProxy.h:30:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 30 files


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

[Chang Shu]

Created attachment 87770 [details]
fix patch 2

[Eric Seidel (no email)]

Comment on attachment 87770 [details]
fix patch 2

I'm told Maciej believes this can't work this way.  I'm sorry I don't have more infromation for you than that.

[Chang Shu]

(In reply to comment #12)
> (From update of attachment 87770 [details])
> I'm told Maciej believes this can't work this way.  I'm sorry I don't have more infromation for you than that.

Yes, I understand. Thanks

[Chang Shu]

I am back to this bug now. I had a concern that the synchronous message cannot hop from WebProcess to UIProcess and to WebProcess again. But after I investigated the code, it actually works.

[Chang Shu]

Created attachment 94789 [details]
fix patch 3

This patch allows key events being sent from WTR::InjectedBundle to WTR UIProcess and then to WebKit2::WebProcess again, all synchronously.

[Adam Roben (:aroben)]

*** Bug 62576 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 94789 [details]
fix patch 3

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

> Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:71
> +WK_EXPORT void WKPageSetSendEventSynchronously(WKPageRef page, bool sync);

The name here is not ideal. Given what this does, normally we’d name this WKPageSetShouldSendKeyboardEventsSynchronously.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1091
> +    // FIXME: Platform default behaviors should be performed during normal DOM event dispatch (in most cases, in default keydown event handler).

Do we really need this FIXME?

> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:36
> +    KeyEventSync(WebKit::WebKeyboardEvent event) -> (bool handled)

I think it would be best if there was some way to indicate this was intended for testing only.

> Tools/WebKitTestRunner/EventSenderProxy.h:36
> +    EventSenderProxy(TestController* testController) : m_testController(testController) { };

This semicolon is unnecessary.

> Tools/WebKitTestRunner/EventSenderProxy.h:38
> +    void keyDown(WKStringRef key, WKEventModifiers, unsigned int location, double timestamp);

It should be "unsigned", not "unsigned int".

> Tools/WebKitTestRunner/TestController.cpp:31
> +#if PLATFORM(MAC)
> +#include "EventSenderProxy.h"
> +#endif

Conditional includes should normally be separate and under unconditional includes.

> Tools/WebKitTestRunner/TestController.cpp:502
> +#if PLATFORM(MAC)

What is Mac-specific here?

> Tools/WebKitTestRunner/TestController.cpp:518
> +            unsigned int location = static_cast<unsigned int>(WKUInt64GetValue(static_cast<WKUInt64Ref>(WKDictionaryGetItemForKey(messageBodyDictionary, locationKey.get()))));

Should just be "unsigned" not "unsigned int".

[Chang Shu]

Thanks a lot for the review, Darin. I'll provide a new patch to accommodate your review and rebase the code.
(In reply to comment #17)
> (From update of attachment 94789 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94789&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:71
> > +WK_EXPORT void WKPageSetSendEventSynchronously(WKPageRef page, bool sync);
> 
> The name here is not ideal. Given what this does, normally we’d name this WKPageSetShouldSendKeyboardEventsSynchronously.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1091
> > +    // FIXME: Platform default behaviors should be performed during normal DOM event dispatch (in most cases, in default keydown event handler).
> 
> Do we really need this FIXME?
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:36
> > +    KeyEventSync(WebKit::WebKeyboardEvent event) -> (bool handled)
> 
> I think it would be best if there was some way to indicate this was intended for testing only.
> 
> > Tools/WebKitTestRunner/EventSenderProxy.h:36
> > +    EventSenderProxy(TestController* testController) : m_testController(testController) { };
> 
> This semicolon is unnecessary.
> 
> > Tools/WebKitTestRunner/EventSenderProxy.h:38
> > +    void keyDown(WKStringRef key, WKEventModifiers, unsigned int location, double timestamp);
> 
> It should be "unsigned", not "unsigned int".
> 
> > Tools/WebKitTestRunner/TestController.cpp:31
> > +#if PLATFORM(MAC)
> > +#include "EventSenderProxy.h"
> > +#endif
> 
> Conditional includes should normally be separate and under unconditional includes.
> 
> > Tools/WebKitTestRunner/TestController.cpp:502
> > +#if PLATFORM(MAC)
> 
> What is Mac-specific here?
> 
> > Tools/WebKitTestRunner/TestController.cpp:518
> > +            unsigned int location = static_cast<unsigned int>(WKUInt64GetValue(static_cast<WKUInt64Ref>(WKDictionaryGetItemForKey(messageBodyDictionary, locationKey.get()))));
> 
> Should just be "unsigned" not "unsigned int".

[Balazs Kelemen]

It's strange to me that this patch is a Mac-only fix. I guess the plan is to do the platform independent part and the Mac part in one and later fix it on other platforms, is that right?

[Chang Shu]

Exactly. I will change the title. Thanks.
(In reply to comment #19)
> It's strange to me that this patch is a Mac-only fix. I guess the plan is to do the platform independent part and the Mac part in one and later fix it on other platforms, is that right?

[Chang Shu]

Created attachment 107203 [details]
patch 4

[WebKit Review Bot]

Comment on attachment 107203 [details]
patch 4

Clearing flags on attachment: 107203

Committed r95039: <http://trac.webkit.org/changeset/95039>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.