[webkit-reviews] review denied: [Bug 47870] Remove RenderTextControl::setSelectionRange : [Attachment 71115] fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 71115: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=71115&action=review

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

Looks good. I think we can do this without putting RenderTextControl code into
Element.

> WebCore/accessibility/AccessibilityRenderObject.cpp:2013
>      if (isNativeTextControl()) {
> -	   RenderTextControl* textControl = toRenderTextControl(m_renderer);
> -	   textControl->setSelectionRange(range.start, range.start +
range.length);
> +	  
static_cast<Element*>(m_renderer->node())->setSelectionRange(range.start,
range.start + range.length);
>	   return;
>      }

Presumably what makes the cast here safe is that we know this is an Element
since all native text controls are Elements. It seems a little arbitrary to
have this call be a member of the Element class. Since it already has to check
the type of the renderer, perhaps it should just be a function that takes a
Node*, and not a member function at all. Then we wouldn’t need the typecast.

> WebCore/dom/Element.cpp:1402
> +void Element::setSelectionRange(int start, int end)

I see why this function takes a Node* argument, but not why it’s a member of
Element (as I said above).

I think this function could be in the RenderTextControl.cpp file, it just needs
to be changed so it takes a Node* rather than a RenderTextControl*, and thus
either should be free function or a static member function. By leaving it in
RenderTextControl.cpp you can even make the diff muhc smaller.

> WebCore/dom/Element.cpp:1407
> +    end = max(end, 0);
> +    start = min(max(start, 0), end);

Why do this before checking renderer()? I would do it later after the early
exits.

> WebCore/html/HTMLFormControlElement.cpp:561
> +    RenderTextControl* textControl = toRenderTextControl(renderer());

I’d just use the word “control” for this local variable. Needs to be a word,
but it’s local so there’s no need to be specific.

> WebCore/rendering/RenderTextControl.cpp:206
> +bool RenderTextControl::isVisible() const
> +{
> +    return style()->visibility() != HIDDEN && m_innerText &&
m_innerText->renderer() && m_innerText->renderBox()->height();
> +}

I think that given what this function does, it should be called
hasVisibleTextArea or something along those lines.