[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