[webkit-reviews] review granted: [Bug 35702] [Qt] Add more support for InputTextController : [Attachment 53889] Updated Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted 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 53889: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=53889&action=review

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

> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,24 @@
> +2010-04-20  Robert Hogan  <robert at webkit.org>
> +
> +	   [Qt] Add more support for textInputController
> +

This part of the ChangeLog is missing the reviewed by bit ;-)

> +	   Add support for selectedRange(), setMarkedText(), insertText(),
> +	   and firstRectForCharacterRange().
> +
> +	   Unskip tests:
> +
> +	   fast/forms/input-maxlength-ime-preedit.html
> +	   fast/forms/input-maxlength-ime-completed.html
> +	   fast/text/international/thai-cursor-position.html
> +	   fast/events/ime-composition-events-001.html
> +	   editing/selection/5825350-1.html
> +	   editing/selection/5825350-2.html
> +	   editing/selection/mixed-editability-10.html
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=35702
> +
> +	   * platform/qt/Skipped:
> +
>  2010-04-20  James Robinson  <jamesr at chromium.org>
>  
>	   Unreviewed.	Update chromium test expectations to reflect changes

>      WebCore::Frame *frame = page->focusController()->focusedOrMainFrame();
>      WebCore::Editor *editor = frame->editor();
> +#if QT_VERSION >= 0x040600
> +    QInputMethodEvent::Attribute selection(QInputMethodEvent::Selection, 0,
0, QVariant());
> +#endif
>  
>      if (!editor->canEdit()) {
>	   ev->ignore();
> @@ -1264,6 +1267,7 @@ void
QWebPagePrivate::inputMethodEvent(QInputMethodEvent *ev)
>	   renderTextControl = toRenderTextControl(renderer);
>  
>      Vector<CompositionUnderline> underlines;
> +    bool hasSelection = false;

I suggest to move the boolean next to the definition of the "selection"
variable, for clarity.

> @@ -1299,10 +1301,25 @@ void
QWebPagePrivate::inputMethodEvent(QInputMethodEvent *ev)
>  
>      if (!ev->commitString().isEmpty())
>	   editor->confirmComposition(ev->commitString());
> -    else if (!ev->preeditString().isEmpty()) {
> +    else {
> +	   // 1. empty preedit with a selection attribute, and start/end of 0
cancels composition
> +	   // 2. empty preedit with a selection attribute, and start/end of
non-0 updates selection of current preedit text
> +	   // 3. populated preedit with a selection attribute, and start/end of
0 or non-0 updates selection of supplied preedit text
> +	   // 4. otherwise event is updating supplied pre-edit text
>	   QString preedit = ev->preeditString();
> -	   editor->setComposition(preedit, underlines, preedit.length(), 0);
> +#if QT_VERSION >= 0x040600
> +	   if (hasSelection) {
> +	       QString text = (renderTextControl) ?
QString(renderTextControl->text()) : QString();

I think you can leave out the parenthese around renderTextControl.


r=me. Feel free to fix my nitpicks when landing :)