[webkit-reviews] review granted: [Bug 88999] After Editor::setComposition is called, input should scroll to the end of the composition. : [Attachment 149188] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 88999: After Editor::setComposition is called, input should scroll to the
end of the composition.
https://bugs.webkit.org/show_bug.cgi?id=88999

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=149188&action=review


r=me provided you fix all nits below.

> Source/WebCore/ChangeLog:13
> +	   Reviewed by NOBODY (OOPS!).

Nit: This line should appear before the long description but after the bug URL.


> Source/WebCore/editing/Editor.cpp:2304
> -    m_frame->selection()->revealSelection(alignment);
> +    m_frame->selection()->revealSelection(alignment, revealExtentOption ==
RevealExtent);

We should also change the type of the second argument of
FrameSelection::revealSelection to use RevealExtentOption.

> LayoutTests/ChangeLog:9
> +	   Reviewed by NOBODY (OOPS!).

Ditto.

> LayoutTests/fast/forms/input-set-composition-scroll.html:1
> +<p>This tests whether an input field scrolls to the end of the new
composition when setComposition is called.</p>

Nit: No DOCTYPE?

> LayoutTests/fast/forms/input-set-composition-scroll.html:7
> +   
document.getElementById('log').appendChild(document.createTextNode(s+"\n"));

Nit: Spaces are missing around +.

> LayoutTests/fast/forms/input-set-composition-scroll.html:13
> +var inp = document.getElementById('inp');

Nit: Please don't use abbreviations like inp. Spell out input. You can also use
document.querySelector('input'); here.

> LayoutTests/fast/forms/input-set-composition-scroll.html:17
> +var TOLERANCE = 5;

Nit: I don't see a point in declaring this magic as a variable here. It would
have read much better if it were in the condition itself.

> LayoutTests/fast/forms/input-set-composition-scroll.html:22
> +if (maxScrollLeft - inp.scrollLeft < TOLERANCE) {
> +    log("SUCCESS: input has scrolled to the end of the composition");
> +} else {
> +    log("FAILED: input has not scrolled to the end of the composition.
scrollLeft=" + inp.scrollLeft);
> +}

Nit: No curly brackets around single statements.