[webkit-reviews] review denied: [Bug 29681] [Qt] QWebPagePrivate::inputMethodEvent and QWebPage::inputMethodQuery methods are incomplete : [Attachment 40382] Proposed patch, input method handling

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Joseph Ligman
<joseph.ligman at nokia.com>'s request for review:
Bug 29681: [Qt] QWebPagePrivate::inputMethodEvent and
QWebPage::inputMethodQuery methods are incomplete
https://bugs.webkit.org/show_bug.cgi?id=29681

Attachment 40382: Proposed patch,  input method handling
https://bugs.webkit.org/attachment.cgi?id=40382&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
Thanks for the update and a big thanks for adding the good automated tests!

I've taken another look at the patch and I've also tried it out with Kristian
(CCed).

There are a few things that need to be fixed as commented below. Kristian is
going to upload
a revised version of your patch later with a few small fixes.


> +Q_DECLARE_METATYPE(QTextCharFormat)
[...]
> +	       case QInputMethodEvent::TextFormat: {
> +		   QTextCharFormat textCharFormat =
a.value.value<QTextCharFormat>();
> +		   QColor qcolor = textCharFormat.underlineColor();

This sequence can be simplified a bit. The text format stored in the QVariant
is _actually_ a QTextFormat. QVariant has native
support for storing these. So the Q_DECLARE_METATYPE can be removed and instead
the code should look like this:

    QTextCharFormat textCharFormat =
a.value.value<QCharFormat>().toCharFormat();

A coding style issue I noticed: The case portions of the switch() statement
should be indented at the same level
as the switch(). I recommend the use of WebKitTools/Scripts/check-webkit-style
to verify the coding style after
a change :-)


>  QVariant QWebPage::inputMethodQuery(Qt::InputMethodQuery property) const
[...]
>  {
> +    Frame *frame = d->page->focusController()->focusedFrame();
[...]
> +    WebCore::Editor *editor = frame->editor();

* placement :)


> +	   case Qt::ImCursorPosition: {
> +	       if (renderTextControl) {
> +		   if (editor->hasComposition()) {
> +		       PassRefPtr<Range> range = editor->compositionRange();

This should be a RefPtr, not a PassRefPtr. Same for the other cases below.

> +		       return QVariant(renderTextControl->selectionEnd() -
TextIterator::rangeLength(range.get()));
> +		   }
> +		   return QVariant(renderTextControl->selectionEnd());
> +	       }
> +	       return QVariant();
> +	   }
> +	   case Qt::ImSurroundingText: {
> +	       if (renderTextControl)
> +		   return QVariant(renderTextControl->text());
> +	       return QVariant();
> +	   }
> +	   case Qt::ImCurrentSelection: {
> +	       if (renderTextControl) {
> +		   int start = renderTextControl->selectionStart();
> +		   int end = renderTextControl->selectionEnd();
> +		   if (end > start)
> +		       return
QVariant(QString(renderTextControl->text()).mid(start,end-start));
> +	       }
> +	       return QVariant();
> +
> +	   }
> +#if QT_VERSION >= 0x040502

This should check for Qt version >= 4.6 instead of 4.5.2, otherwise the
compilation
on non-S60 platforms will break.


Another issue that Kristian noticed is that the query for ImSurroundingText
should _exclude_ the preedit text.

We'll see about getting that fixed in a revised patch :-)