[webkit-reviews] review denied: [Bug 29681] [Qt] QWebPagePrivate::inputMethodEvent and QWebPage::inputMethodQuery lack support for CoeFepInputContext : [Attachment 40075] patch to describe the changes need to sync up the s60 input context

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 25 04:06:53 PDT 2009


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 lack support for CoeFepInputContext
https://bugs.webkit.org/show_bug.cgi?id=29681

Attachment 40075: patch to describe the changes need to sync up the s60 input
context
https://bugs.webkit.org/attachment.cgi?id=40075&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

In principle this looks good, it's great that you're working on fixing the
input method handling!

I have a few minor issues (see below) and I'm also wondering if there's
anything we can do
to make this testable.

How about unit tests that programmatically set the focus, select text, etc. and
then call inputMethodQuery() 
or send synthetic input method events to verify this code?

> +
> +#if PLATFORM(SYMBIAN)
> +    for (int i = 0; i < ev->attributes().size(); ++i) {
> +	   const QInputMethodEvent::Attribute &a = ev->attributes().at(i);
> +	   if (a.type == QInputMethodEvent::Selection) {
> +	       RenderObject* renderer = 0;
> +	       if (frame->selection()->rootEditableElement())
> +		   renderer =
frame->selection()->rootEditableElement()->shadowAncestorNode()->renderer();
> +
> +	       if (renderer && renderer->isTextControl()) {
> +		   toRenderTextControl(renderer)->setSelectionStart(a.start);
> +		   toRenderTextControl(renderer)->setSelectionEnd(a.start +
a.length);
> +	       }
> +	       break;
> +	   };
>      }
> +#endif

Why is this code limited to PLATFORM(SYMBIAN)?
  

> @@ -1196,42 +1215,64 @@ bool QWebPagePrivate::handleScrolling(QK
[...]
>
> +    switch (property) { 
> +	   case Qt::ImMicroFocus: { 

Looks like the indentation is off here :)

> +		return QVariant(frame->selection()->absoluteCaretBounds()); 
> +	    return QVariant(); 
> +	} 
> +	case Qt::ImFont: { 
> +	    QWebView *webView = qobject_cast<QWebView *>(d->view); 
> +	    if (webView) 
> +		return QVariant(webView->font()); 
> +	    return QVariant(); 

This doesn't look correct. font() is a method of QWidget, it should be enough
to
simply use return d->view ? d->view->font() : QFont();

> +#if PLATFORM(SYMBIAN)
> +	case Qt::ImAnchorPosition: {
> +	   if (renderTextControl) {
> +	       if (editor->hasComposition()) {
> +		   PassRefPtr<Range> range = editor->compositionRange();
> +		   return QVariant(renderTextControl->selectionStart() -
TextIterator::rangeLength(range.get()));
> +	       }
> +	       return QVariant(renderTextControl->selectionStart());
>	   }
>	   return QVariant();
>      }
> -    case Qt::ImSurroundingText: {
> -	   Frame *frame = d->page->focusController()->focusedFrame();
> -	   if (frame) {
> -	       Document *document = frame->document();
> -	       if (document->focusedNode())
> -		   return QVariant(document->focusedNode()->nodeValue());
> +#endif

Same as earlier hunk, why is this only enabled for Symbian?


More information about the webkit-reviews mailing list