Bug 57515

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.

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

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

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.

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

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

(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.

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.

Created attachment 87766 [details] fix patch 2 fix build break

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.

Created attachment 87770 [details] fix patch 2

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.

(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

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.

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.

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

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".

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".

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?

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?

Created attachment 107203 [details] patch 4

Comment on attachment 107203 [details] patch 4 Clearing flags on attachment: 107203 Committed r95039: <http://trac.webkit.org/changeset/95039>

All reviewed patches have been landed. Closing bug.