Bug 56485 – [Qt] Implement KeyDown function for WebKit2 EventSender

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chang Shu]

Start the framework with KeyDown function.

[Chang Shu]

Created attachment 85967 [details]
fix patch

[Alexey Proskuryakov]

It isn't even implemented on Mac yet, right?

[Chang Shu]

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

[Alexey Proskuryakov]

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.

[Chang Shu]

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

[Chang Shu]

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.

[Chang Shu]

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?

[Darin Adler]

Wow, this is a lot more elaborate than what I was working on, which was a temporary version entirely inside the web process.

[Chang Shu]

Created attachment 107361 [details]
patch 2

[Kenneth Rohde Christiansen]

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.

[Chang Shu]

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

[Kenneth Rohde Christiansen]

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?

[Balazs Kelemen]

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

[Kenneth Rohde Christiansen]

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)

[Zoltan Horvath]

(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

[Chang Shu]

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

[Kenneth Rohde Christiansen]

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

[Chang Shu]

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

[Chang Shu]

Created attachment 107517 [details]
patch 3

[Andreas Kling]

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.

[Chang Shu]

Comment on attachment 107517 [details]
patch 3

Thanks for the review. We can come up with a task for sharing WK1/WK2 code.

[Chang Shu]

Comment on attachment 107517 [details]
patch 3

the patch needs rebaseline, i think.

[WebKit Review Bot]

Comment on attachment 107517 [details]
patch 3

Clearing flags on attachment: 107517

Committed r95626: <http://trac.webkit.org/changeset/95626>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.