[webkit-reviews] review denied: [Bug 56485] [Qt] Implement KeyDown function for WebKit2 EventSender : [Attachment 107361] patch 2

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Chang Shu
<cshu at webkit.org>'s request for review:
Bug 56485: [Qt] Implement KeyDown function for WebKit2 EventSender
https://bugs.webkit.org/show_bug.cgi?id=56485

Attachment 107361: patch 2
https://bugs.webkit.org/attachment.cgi?id=107361&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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.