Start the framework with KeyDown function.
Created attachment 85967 [details] fix patch
It isn't even implemented on Mac yet, right?
(In reply to comment #2) > It isn't even implemented on Mac yet, right? Strictly speaking, not yet. However, 90% of the code in the patch is common code and will be used for Mac. Once this patch is in, all I have to do for Mac and other platforms are just changing projects files and add one function to handle keyDown locally, aka in UIProcess. So I really appreciate comments and reviews from Apple guys, too.
When looking at bug 44016, I was very unsure about the approach taken there. Instead of working with real platform events, it dispatched something cross-platform. For eventSender, it's very important to test WebKit layer, not just WebCore.
(In reply to comment #4) > When looking at bug 44016, I was very unsure about the approach taken there. Instead of working with real platform events, it dispatched something cross-platform. For eventSender, it's very important to test WebKit layer, not just WebCore. Yes, instead, what I did in my patch is to 1. pass function calls from WebKitTestRunner::InjectedBundle to WebKit2::WebProcess::InjectedBundle through WK interface (something similar to 44016) 2. encode it as a binary buffer attached to a general InjectedBundle message. 3. send out the message to UIProcess 4. UIProcess decode the general InjectedBundle message and passes it to WebKitTestRunner::EventSenderProxy. 5. EventSenderProxy further decodes the message and calls the platform-dependent code to trigger the event from the UIProcess side. This makes the event a true platform event. Hopefully, my patch follows the right WebKit2 architecture. Thanks.
One thing I have noticed is EventSendingControllerQt.cpp is not needed. Everything there is platform-independent code and can be put into EventSendingController.cpp. I will make the change in the future patch.
There is another thing I am not so sure. In my patch, I put the decoding of keydown message in WebKitTestRunner/EventSenderProxy. As a result, I have to expose CoreIPC from WebKit2 to WebKitTestRunner. Alternatively, in order to avoid exposing CoreIPC, I have to put the decoding logic inside WebKit2/UIProcess. But that's not something wanted by all WebKit2 clients. Is there a better solution?
Wow, this is a lot more elaborate than what I was working on, which was a temporary version entirely inside the web process.
Created attachment 107361 [details] patch 2
Comment on attachment 107361 [details] patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=107361&action=review What are we doing for WebKit1? Isn't there some way to share this code or make it at least cleaner > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:46 > + Qt::KeyboardModifiers modifs = 0; Why not just write it out? modifiers > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:60 > +void EventSenderProxy::keyDown(WKStringRef keyRef, WKEventModifiers modifiersRef, unsigned int location, double timestamp) I think we just write unsigned in webkit, not unsigned int, but I could be wrong. Anyway in WebKit2 is mostly see uint32_t or uint64_t. > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:65 > + QString s = string; s is a bit too short variable name. This 's' seems like the resultString or so. Do you really need both string and s? (sorry I only skimmed that part of the code) > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:74 > + // qDebug() << ">>>>>>>>> keyDown" << code << (char)code; Shouldn't we remove this from the patch? > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:175 > + } else if (string == QLatin1String("home")) { > + s = QString(); > + code = Qt::Key_Home; > + } else if (string == QLatin1String("end")) { Don't we already have this mapping somewhere? Anyway, a map would be a better solution.
> What are we doing for WebKit1? Isn't there some way to share this code or make it at least cleaner Thanks for the review. The code in EventSenderProxyQt looks a bit unpolished mainly because I copy&pasted from WebKit1's EventSenderQt.cpp and tried to show the new implementation didn't have to change a single line of the original one. But it seems better to improve the code. > > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:46 > > + Qt::KeyboardModifiers modifs = 0; > > Why not just write it out? modifiers modifiers is already used. But I can use something like modifiersQt. > > > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:60 > > +void EventSenderProxy::keyDown(WKStringRef keyRef, WKEventModifiers modifiersRef, unsigned int location, double timestamp) > > I think we just write unsigned in webkit, not unsigned int, but I could be wrong. Anyway in WebKit2 is mostly see uint32_t or uint64_t. Ok, will use unsigned. > > > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:65 > > + QString s = string; > > s is a bit too short variable name. This 's' seems like the resultString or so. Do you really need both string and s? (sorry I only skimmed that part of the code) That's how the original code was written. I may be able to improve it. > > > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:74 > > + // qDebug() << ">>>>>>>>> keyDown" << code << (char)code; > > Shouldn't we remove this from the patch? same. Just keep the original code. > > > Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:175 > > + } else if (string == QLatin1String("home")) { > > + s = QString(); > > + code = Qt::Key_Home; > > + } else if (string == QLatin1String("end")) { > > Don't we already have this mapping somewhere? Anyway, a map would be a better solution. This is the current implementation in WebKit1. I intended to keep them the same.
Can't we share the implementation somehow? Ie, we already have DumpRenderTreeSupport for Qt in WebCore I believe. Could it be added there? or in a similar class?
(In reply to comment #12) > Can't we share the implementation somehow? Ie, we already have DumpRenderTreeSupport for Qt in WebCore I believe. Could it be added there? or in a similar class? There is a lot of things that are topic of sharing across WebKit1&2 and we don't share them. I think the architecture of wk2 makes it impossible in most cases. So if there is no way of sharing this logic (I don't say there is no way) it is acceptable to duplicate it for wk2.
Well if the code is in WebCore and has a private API it can be shared. We are already doing this for the orientation events (on our branch at least)
(In reply to comment #14) > Well if the code is in WebCore and has a private API it can be shared. We are already doing this for the orientation events (on our branch at least) Hey Kenneth, could you share - in IRC, mailinglist, .. - with us some details about your WK2 plans and tendencies like this sharing implementation approach? Branch upstreaming plans, etc. I think it could improve our work effectivity. Thanks! Zoltan
(In reply to comment #12) > Can't we share the implementation somehow? Ie, we already have DumpRenderTreeSupport for Qt in WebCore I believe. Could it be added there? or in a similar class? Are you talking about WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt, Kenneth? I didn't seem to find DumpRenderTreeSupport under WebCore. Besides, the functionality in this patch is a part of the DRT test tool. It should not belong to WebKit/WebCore component.
(In reply to comment #16) > (In reply to comment #12) > > Can't we share the implementation somehow? Ie, we already have DumpRenderTreeSupport for Qt in WebCore I believe. Could it be added there? or in a similar class? > > Are you talking about WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt, Kenneth? I didn't seem to find DumpRenderTreeSupport under WebCore. Besides, the functionality in this patch is a part of the DRT test tool. It should not belong to WebKit/WebCore component. Ah sure, this is part of the DRT itself. Hmm, would be nice to share it somehow, but if it is too complicated, fine.
> > Are you talking about WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt, Kenneth? I didn't seem to find DumpRenderTreeSupport under WebCore. Besides, the functionality in this patch is a part of the DRT test tool. It should not belong to WebKit/WebCore component. > > Ah sure, this is part of the DRT itself. Hmm, would be nice to share it somehow, but if it is too complicated, fine. Thanks! A new patch will be submitted soon.
Created attachment 107517 [details] patch 3
Comment on attachment 107517 [details] patch 3 r=me, though at some point we should definitely clean this up and share it between WK1/WK2.
Comment on attachment 107517 [details] patch 3 Thanks for the review. We can come up with a task for sharing WK1/WK2 code.
Comment on attachment 107517 [details] patch 3 the patch needs rebaseline, i think.
Comment on attachment 107517 [details] patch 3 Clearing flags on attachment: 107517 Committed r95626: <http://trac.webkit.org/changeset/95626>
All reviewed patches have been landed. Closing bug.