[webkit-reviews] review granted: [Bug 128806] computeSelectionStart and computeSelectionEnd shouldn't trigger synchronous layout : [Attachment 224295] Fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 128806: computeSelectionStart and computeSelectionEnd shouldn't trigger
synchronous layout
https://bugs.webkit.org/show_bug.cgi?id=128806

Attachment 224295: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=224295&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=224295&action=review


> Source/WebCore/html/HTMLTextFormControlElement.cpp:333
> +    int index = indexForPosition(position.deepEquivalent());

Why int rather than unsigned?

> Source/WebCore/html/HTMLTextFormControlElement.cpp:601
> +		   index += std::min(length,
static_cast<unsigned>(passedPosition.offsetInContainerNode()));

Instead of static_cast<unsigned>, I suggest writing std::min<unsigned>.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:616
> +#ifndef NDEBUG
> +    VisiblePosition visiblePosition = passedPosition;
> +    unsigned indexComputedByVisiblePosition = 0;
> +    if (visiblePosition.isNotNull())
> +	   indexComputedByVisiblePosition =
WebCore::indexForVisiblePosition(innerText, visiblePosition, false /*
forSelectionPreservation */);
> +    ASSERT(index == indexComputedByVisiblePosition);
> +#endif

I think an assertion like this should be written with a helper function so it
doesn’t need to be inside an #if. Also can consider ASSERT_DISABLED instead of
NDEBUG.