[webkit-reviews] review granted: [Bug 47870] Remove RenderTextControl::setSelectionRange : [Attachment 72381] qt build fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 47870: Remove RenderTextControl::setSelectionRange
https://bugs.webkit.org/show_bug.cgi?id=47870

Attachment 72381: qt build fix
https://bugs.webkit.org/attachment.cgi?id=72381&action=review

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

> WebCore/html/HTMLFormControlElement.cpp:563
>      if (RenderTextControl* renderer = textRendererAfterUpdateLayout())
> -	   renderer->select();
> +	   setSelectionRange(0, renderer->text().length());

I still stand by my earlier suggestion. You can call setSelectionRange(0,
numeric_limits<int>::max()) and it will work fine; seems better than calling
textRendererAfterUpdateLayout here. Too bad numeric_limits is so ugly.

> WebCore/rendering/RenderTextControl.h:113
> +    bool hasVisibleTextArea() const;
> +    friend void setSelectionRange(Node*, int start, int end);

I’d like to see these up with the private function members rather than at the
bottom after the private data members.

> WebKit/qt/Api/qwebpage.cpp:1057
> +	       if (renderer && renderer->isTextControl() && renderer->node())
> +		   setSelectionRange(renderer->node(), qMin(a.start, (a.start +
a.length)), qMax(a.start, (a.start + a.length)));

No need for the isTextControl() or node() checks in this if statement. The
setSelectionRange function does those checks already.