[webkit-reviews] review denied: [Bug 33165] [Qt] onKeyPress/Down/Up event handlers cannot be used when the text is entered through InputMethod : [Attachment 54168] patch file

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Jay Tucker
<jay.tucker at nokia.com>'s request for review:
Bug 33165: [Qt] onKeyPress/Down/Up event handlers cannot be used when the text
is entered through InputMethod
https://bugs.webkit.org/show_bug.cgi?id=33165

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 58169)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> +2010-04-23  Tucker Jay  <jay.tucker at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [Qt] onKeyPress/Down/Up event handlers cannot be used when the text
is entered through InputMethod
> +	   https://bugs.webkit.org/show_bug.cgi?id=33165
> +
> +	   Fixed bug preventing alpha-only and numeric-only text entry
> +	   fields from working on Symbian platform.
> +
> +	   * Api/qwebpage.cpp:
> +	   (QWebPagePrivate::inputMethodEvent):
> +
>  2010-04-22  John Pavan  <john.pavan at nokia.com>
>  
>	   Reviewed by Laszlo Gombos.
> Index: WebKit/qt/Api/qwebpage.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebpage.cpp	(revision 58169)
> +++ WebKit/qt/Api/qwebpage.cpp	(working copy)
> @@ -1188,9 +1188,26 @@ void QWebPagePrivate::inputMethodEvent(Q
>	   }
>      }
>  
> -    if (!ev->commitString().isEmpty())
> -	   editor->confirmComposition(ev->commitString());
> -    else if (!ev->preeditString().isEmpty()) {
> +    if (!ev->commitString().isEmpty()) {
> +	   QString cmtedit = ev->commitString();
> +	   //delete the selection
> +	   editor->confirmComposition("");
> +	   //simulate keypress and keyrelease event for QWebpage to fire the
javascript key event listeners
> +	   //some input method can input more than 1 character at one time like
Chinese input method, so a loop is needed.
> +	   for (int i = 0; i < cmtedit.length(); ++i) {
> +	       QChar echar = cmtedit.at(i);
> +	       //if it's a ascii char, just transform it to key event to fire
the javascript key event listeners
> +	       if (echar >= 0 && echar <= 255) {
> +		   QKeyEvent* keypressevent = new QKeyEvent(QEvent::KeyPress,
echar.toAscii(), Qt::NoModifier, echar);
> +		   qApp->sendEvent(q, keypressevent);
> +		   QKeyEvent* keyreleaseevent = new
QKeyEvent(QEvent::KeyRelease, echar.toAscii(), Qt::NoModifier, echar);
> +		   qApp->sendEvent(q, keyreleaseevent);
> +	       } else {
> +		   //if it's an unicode char, insert it to the editor directly,
since unicode char cannot be from a keyevent.
> +		   editor->confirmComposition(QString(echar));
> +	     }
> +	   }
> +    } else if (!ev->preeditString().isEmpty()) {
>	   QString preedit = ev->preeditString();
>	   editor->setComposition(preedit, underlines, preedit.length(), 0);
>      }
WebKit/qt/Api/qwebpage.cpp:1193
 +	    //delete the selection
Please add a space after the comment sign

WebKit/qt/Api/qwebpage.cpp:1194
 +	    editor->confirmComposition("");
Please use String()'s default constructor instead of passing "", which does an
implicit conversion from ascii and also allocate a StringImpl object on the
heap for no reason.

WebKit/qt/Api/qwebpage.cpp:1206
 +		    //if it's an unicode char, insert it to the editor
directly, since unicode char cannot be from a keyevent.
This comment doesn't make sense to me. Why can't be a unicode char be sent from
a key event? QKeyEvent takes a QString, and certainly there are keyboards out
there that produce characters outside the ASCII range.

WebKit/qt/Api/qwebpage.cpp:1201
 +		    QKeyEvent* keypressevent = new QKeyEvent(QEvent::KeyPress,
echar.toAscii(), Qt::NoModifier, echar);
This QKeyEvent is allocated on the heap, but it is never deleted. Please
allocate it on the stack instead.


Please also add a test for this. For example you could write a unit test that
generates synthetic input method events and before sending them initialize a
snippet of JS that registers event listeners. Then you can verify if they were
called correctly during the synthetic composition sequence or not.