[webkit-reviews] review denied: [Bug 35702] [Qt] Add more support for InputTextController : [Attachment 50152] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 10 14:03:51 PST 2010


Simon Hausmann <hausmann at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 35702: [Qt] Add more support for InputTextController
https://bugs.webkit.org/show_bug.cgi?id=35702

Attachment 50152: Patch
https://bugs.webkit.org/attachment.cgi?id=50152&action=review

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


> +QVariantList QWEBKIT_EXPORT qt_drt_selectedRange(QWebPage* page)
> +{
> +    WebCore::Frame *frame =
page->handle()->page->focusController()->focusedOrMainFrame();

Coding style nitpick, * placement :)

> +    WebCore::Frame *frame =
page->handle()->page->focusController()->focusedOrMainFrame();

Same here.

> +    int max = InputElement::s_maximumLength;
> +    if (frame->document() && frame->document()->focusedNode()) {
> +	   if
(frame->document()->focusedNode()->hasTagName(HTMLNames::inputTag)) {
> +	       HTMLInputElement* inputElement =
static_cast<HTMLInputElement*>(frame->document()->focusedNode());
> +	       max = inputElement->maxLength();
> +	   }
> +    }
> +
>      if (!ev->commitString().isEmpty())
>	   editor->confirmComposition(ev->commitString());
> -    else if (!ev->preeditString().isEmpty()) {
> +    else {
>	   QString preedit = ev->preeditString();
> -	   editor->setComposition(preedit, underlines, preedit.length(), 0);
> +	   editor->setComposition(preedit, underlines, (preedit.length() > max)
? max : preedit.length(), 0);
>      }

Instead of this logic it might be better to use InputElement::sanitizeValue
instead, which AFAICS
would provide a similar cut-off with the extra advantage of handling grapheme
clusters, i.e.
cutting off more characters than needed to avoid invalid character
combinations.

The cut-off sounds like a good idea in general, but I think this should be also
tested
with a unit-test. Sorry, I know this is asking for more than you intented to
actually
fix, but the input method handling is unfortunately fragile :(. On the upside
there are already
a few unit tests in tst_qwebpage.cpp.

> +#if QT_VERSION >= 0x040600
> +    if (renderTextControl && (selection.length > 0)) {

Probably more of a style nitpick, but I'd leave out the extra pair of
parentheses :)


More information about the webkit-reviews mailing list