Bug 47870 - Remove RenderTextControl::setSelectionRange
Summary: Remove RenderTextControl::setSelectionRange
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 47865
  Show dependency treegraph
 
Reported: 2010-10-18 18:55 PDT by Ryosuke Niwa
Modified: 2010-10-29 16:16 PDT (History)
8 users (show)

See Also:


Attachments
fixes the bug (12.87 KB, patch)
2010-10-18 19:06 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per Darin's comment (8.09 KB, patch)
2010-10-29 11:24 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed some details (7.98 KB, patch)
2010-10-29 11:32 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
More fixes per Darin's comment (10.34 KB, patch)
2010-10-29 12:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
qt build fix (10.36 KB, patch)
2010-10-29 13:23 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-10-18 18:55:39 PDT
Remove enderTextControl::setSelectionRange because it instantiates VisiblePosition.
Comment 1 Ryosuke Niwa 2010-10-18 19:06:46 PDT
Created attachment 71115 [details]
fixes the bug
Comment 2 Ryosuke Niwa 2010-10-28 16:06:08 PDT
Can someone review this patch?
Comment 3 Darin Adler 2010-10-28 16:14:41 PDT
Comment on attachment 71115 [details]
fixes the bug

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.
Comment 4 Ryosuke Niwa 2010-10-29 11:24:37 PDT
Created attachment 72353 [details]
Fixed per Darin's comment
Comment 5 Ryosuke Niwa 2010-10-29 11:32:51 PDT
Created attachment 72355 [details]
Fixed some details
Comment 6 Ryosuke Niwa 2010-10-29 11:34:09 PDT
(In reply to comment #4)
> Created an attachment (id=72353) [details]
> Fixed per Darin's comment

I'm not sure if this approach is necessarily cleaner because we end up having 3-4 global functions like that by the time I finish removing all selection-related functions from RenderTextControl.

(In reply to comment #3)
> 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.

In some sense, Element was the LCD of all classes that called setSelectionRange. But I agree that Element having this function seems quite arbitrary.

> 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.

I did this in my new patch but I'm not sure if this is a good approach.  I'm not a big fun of converting member functions to global functions...

> > 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.

Good point.  Fixed.

> > 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.

Fixed.

> > 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.

I made setSelectionRange a friend of RenderTextControl so that I don't have to make cacheSelection public and add this member function.
Comment 7 Darin Adler 2010-10-29 11:44:10 PDT
Comment on attachment 72353 [details]
Fixed per Darin's comment

View in context: https://bugs.webkit.org/attachment.cgi?id=72353&action=review

review- because of some of the null check stuff

> WebCore/dom/InputElement.cpp:111
> +    if (toRenderTextControl(element->renderer()))
> +        setSelectionRange(element, start, end);

There is no need call toRenderTextControl just to check something for null. There’s no need for a null check here, since setSelectionRange already takes care of it.

> WebCore/html/HTMLFormControlElement.cpp:551
> +    WebCore::setSelectionRange(this, start, std::max(start, selectionEnd()));

I think you can use just "max", not "std::max", here. Also, seems fine to just call setSelectionRange, since it turns around and calls WebCore::setSelectionRange, both here and in the rest of the functions in this file.

> WebCore/html/HTMLFormControlElement.cpp:556
> +    WebCore::setSelectionRange(this, std::min(end, selectionStart()), end);

I think you can use just "min", not "std::min", here.

> WebCore/html/HTMLFormControlElement.cpp:562
> +    if (RenderTextControl* textControl = toRenderTextControl(renderer()))
> +        WebCore::setSelectionRange(this, 0, textControl->text().length());

I think instead you can write:

    setSelectionRange(this, 0, numeric_limits<int>::max())

Avoid the need for a typecast or a local variable.

> WebCore/rendering/RenderTextControl.cpp:219
> +void setSelectionRange(Node* node, int start, int end)

Do we need to ref/deref node? Is there any danger that this code results in JavaScript code running that could delete the node?

> WebCore/rendering/RenderTextControl.cpp:230
> +    RenderTextControl* textControl = toRenderTextControl(node->renderer());

I would name this local variable just “control” for brevity.

> WebCore/rendering/RenderTextControl.cpp:234
> +    if (textControl->style()->visibility() == HIDDEN || !textControl->m_innerText
> +        || !textControl->m_innerText->renderer() || !textControl->m_innerText->renderBox()->height()) {
> +        textControl->cacheSelection(start, end);

I find this formatting, where the || lines up with the code below it, so hard to read that I go out of my way to avoid this sort of thing. Maybe we can factor all or part of this check into an inline helper function to avoid the long body for the if.
Comment 8 Early Warning System Bot 2010-10-29 12:02:30 PDT
Attachment 72353 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4775068
Comment 9 Early Warning System Bot 2010-10-29 12:11:34 PDT
Attachment 72355 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4785072
Comment 10 Ryosuke Niwa 2010-10-29 12:25:31 PDT
+robert@webkit.org

(In reply to comment #8)
> Attachment 72353 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/4775068

This failure is happening in QWebPagePrivate::inputMethodEvent in WebKit/qt/qwebpage.cpp:

        case QInputMethodEvent::Selection: {
            hasSelection = true;
            // A selection in the inputMethodEvent is always reflected in the visible text
            if (renderer && renderer->isTextControl()) {
                renderTextControl->setSelectionStart(qMin(a.start, (a.start + a.length)));
                renderTextControl->setSelectionEnd(qMax(a.start, (a.start + a.length)));
            }

I don't understand why WebKit layer is accessing RenderTextControl.  These functions aren't even exported.
Comment 11 Ryosuke Niwa 2010-10-29 12:38:45 PDT
Created attachment 72373 [details]
More fixes per Darin's comment
Comment 12 Ryosuke Niwa 2010-10-29 12:42:23 PDT
(In reply to comment #7)
> > WebCore/dom/InputElement.cpp:111
> > +    if (toRenderTextControl(element->renderer()))
> > +        setSelectionRange(element, start, end);
> 
> There is no need call toRenderTextControl just to check something for null. There’s no need for a null check here, since setSelectionRange already takes care of it.

Oops, I missed that one.  Thanks for noticing that.

> > WebCore/html/HTMLFormControlElement.cpp:551
> > +    WebCore::setSelectionRange(this, start, std::max(start, selectionEnd()));
> 
> I think you can use just "max", not "std::max", here. Also, seems fine to just call setSelectionRange, since it turns around and calls WebCore::setSelectionRange, both here and in the rest of the functions in this file.

> I think you can use just "min", not "std::min", here.

Done.

> > WebCore/html/HTMLFormControlElement.cpp:562
> > +    if (RenderTextControl* textControl = toRenderTextControl(renderer()))
> > +        WebCore::setSelectionRange(this, 0, textControl->text().length());
> 
> I think instead you can write:
>     setSelectionRange(this, 0, numeric_limits<int>::max())

I don't think so. Neither setSelectionRange nor visiblePositionForIndex adjusts the offset.

> > WebCore/rendering/RenderTextControl.cpp:219
> > +void setSelectionRange(Node* node, int start, int end)
> 
> Do we need to ref/deref node? Is there any danger that this code results in JavaScript code running that could delete the node?

No.  Although selection change may trigger some events to fire, this is done at the very end of function at which point the existance of node is irrelevant.

> > WebCore/rendering/RenderTextControl.cpp:230
> > +    RenderTextControl* textControl = toRenderTextControl(node->renderer());
> 
> I would name this local variable just “control” for brevity.

Sorry, I forgot to fix that.

> > WebCore/rendering/RenderTextControl.cpp:234
> > +    if (textControl->style()->visibility() == HIDDEN || !textControl->m_innerText
> > +        || !textControl->m_innerText->renderer() || !textControl->m_innerText->renderBox()->height()) {
> > +        textControl->cacheSelection(start, end);
> 
> I find this formatting, where the || lines up with the code below it, so hard to read that I go out of my way to avoid this sort of thing. Maybe we can factor all or part of this check into an inline helper function to avoid the long body for the if.

Added hasVisibleTextArea back.
Comment 13 Early Warning System Bot 2010-10-29 13:17:17 PDT
Attachment 72373 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4769075
Comment 14 Ryosuke Niwa 2010-10-29 13:23:13 PDT
Created attachment 72381 [details]
qt build fix
Comment 15 Darin Adler 2010-10-29 13:58:46 PDT
(In reply to comment #12)
> > > WebCore/html/HTMLFormControlElement.cpp:562
> > > +    if (RenderTextControl* textControl = toRenderTextControl(renderer()))
> > > +        WebCore::setSelectionRange(this, 0, textControl->text().length());
> > 
> > I think instead you can write:
> >     setSelectionRange(this, 0, numeric_limits<int>::max())
> 
> I don't think so. Neither setSelectionRange nor visiblePositionForIndex adjusts the offset.

I think you are wrong.

The visiblePositionForIndex function handles large offsets correctly, because the CharacterIterator is limited to the range of the node contents. The only thing done with the index is that it’s passed to CharacterIterator, which will handle a massive number just fine.
Comment 16 Darin Adler 2010-10-29 14:02:38 PDT
Comment on attachment 72381 [details]
qt build fix

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.
Comment 17 Ryosuke Niwa 2010-10-29 14:06:24 PDT
(In reply to comment #15)
> I think you are wrong.
> 
> The visiblePositionForIndex function handles large offsets correctly, because the CharacterIterator is limited to the range of the node contents. The only thing done with the index is that it’s passed to CharacterIterator, which will handle a massive number just fine.

Ah, I didn't know that.

(In reply to comment #16)
> (From update of attachment 72381 [details])
> 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.

Will try.

> > 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.

Will do.

> > 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.

I think we still do need to check renderer->node() here because setSelectionRange only assert that node is not null.  Or should we add early exit to setSelectionRange when node == 0?
Comment 18 Ryosuke Niwa 2010-10-29 16:16:11 PDT
Committed r70945: <http://trac.webkit.org/changeset/70945>