Bug 22996 – RenderTextControl unecessarily combines multi/single line text control rendering

• Component: XML
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

As the summary says, RenderTextControl is quite bloated. It has several problems:
it's tied to HTMLInputElement/HTMLTextAreaElement - making it hard to reuse it for WML at the moment.
The memory usage for multi-line controls is unnecessarily high, all member variables related to popup menus / search fields are not needed for these controls.

I've created a patch splitting up RenderTextControl in RenderTextControlSingleLine / RenderTextControlMultiLine, I'd love to hear some comments.

[Nikolas Zimmermann]

Created attachment 26249 [details]
Initial patch

Initial patch implementing the split-up. Only touches a few lines of HTMLInput/TextAreaElement - the rest is all about splitting up RenderTextControl.

[Darin Adler]

Comment on attachment 26249 [details]
Initial patch

This looks like a good approach.

Please be careful about the copyright. Some files have only the Torch Mobile copyright, so please be sure there's nothing that came from the existing code there.

> -    RenderTextControl(Node*, bool multiLine);
> +    RenderTextControl(Node*);

I think it would be better to have the constructor be protected since RenderTextControl an abstract base class now.

> +    virtual void forwardEvent(Event*);

This RenderTextControl member function should be protected and non-virtual. There's no caller that needs to call this on a RenderTextControl*, so there's no need to make it a public virtual function rather than a protected non-virtual one.

> +    void updateInnerTextValue(const String&);

Why not name this setInnerTextValue?

> +    virtual void processStyleChange() { }

I'm not sure the name processStyleChange clearly distinguishes this new function from styleDidChange. Why can't the derived class override styleDidChange instead? If there's a good reason, maybe this new function's name could reflect that reason. Maybe it's the precise timing of the call, and it could reflect that?

> +    void createSubtreeIfNeeded(Node* innerBlock);

I think Element* is a better type to use here than Node*.

> +    bool nodeAtPoint(const HitTestRequest&, HitTestResult&, int x, int y, int tx, int ty, HitTestAction, Node* innerBlock);

I think it's too subtle to have this non-virtual helper function have the same name as the virtual function, and differ only in the extra parameter. I think we should find a different name for it. Also, I think that the innerBlock argument should be a TextControlInnerElement*, not a Node*.

> +    bool m_edited : 1;
> +    bool m_userEdited : 1;

It's inconsistent to change these to bit fields (implying we think the 8 bits per bool memory is precious enough that we want the slower bigger code -- or maybe it's 32 bits per bool?), but to put the different bits in the derived classes (implying we don't care about the extra 32 bits it costs to have two different sets of bit fields). I think it would be better to be consistent.

> +void RenderTextControlMultiLine::forwardEvent(Event* evt)

Is there a good reason to abbreviate the name "event"? I'd prefer not to.

> +    if (evt->type() == eventNames().blurEvent || evt->type() == eventNames().focusEvent)
> +        return;
> +
> +    RenderTextControl::forwardEvent(evt);

Could this behavior go in the base class? We never want to forward these blur and focus events.

> +void RenderTextControlMultiLine::calcExtraLineHeight(int lineHeight)

I'm not sure I'm happy with the name "calc" for a function that mutates the object. This function doesn't "calculate extra line height". Frankly, I don't know what it does, because I don't see any code calling it!

> +private:
> +    virtual int preferredWidth(float charWidth) const;

It's a little strange to call this "preferredWidth" when actually the preferredWidth includes the padding. We should look for a name that makes that more explicit.

> +class RenderTextControlSingleLine : public RenderTextControl
> +                                  , private PopupMenuClient {

We haven't done this formatting in most places in WebKit the past and I'd prefer not to do it unless we have consensus we should do it this way. It seems this case fit fine on one line.

>      RenderTextControl* renderer = static_cast<RenderTextControl*>(node()->shadowAncestorNode()->renderer());
> -    
> -    return RenderBlock::nodeAtPoint(request, result, x, y, tx, ty, renderer->placeholderIsVisible() ? HitTestBlockBackground : hitTestAction);
> +
> +    bool placeholderIsVisible = false;
> +    if (renderer->isTextField())
> +        placeholderIsVisible = static_cast<RenderTextControlSingleLine*>(renderer)->placeholderIsVisible();
> +
> +    return RenderBlock::nodeAtPoint(request, result, x, y, tx, ty, placeholderIsVisible ? HitTestBlockBackground : hitTestAction);

It seems to me that the line that sets up the renderer local variable doesn't need to cast to RenderTextControl* -- it can just use RenderObject*.

> -        if (input && input->renderer() && static_cast<RenderTextControl*>(input->renderer())->popupIsVisible())
> -            static_cast<RenderTextControl*>(input->renderer())->hidePopup();
> -        else if (input->maxResults() > 0)
> -            static_cast<RenderTextControl*>(input->renderer())->showPopup();
> +        RenderTextControlSingleLine* renderer = static_cast<RenderTextControlSingleLine*>(input->renderer());
> +        ASSERT(renderer);

I understand that the old code had unnecessary null checking of the renderer, but I'm not sure an assertion is all that helpful. Clearly the shadow DOM wouldn't exist if the renderer didn't exist, and I don't think the assert adds much.

> +        if (renderer->popupIsVisible())
> +            renderer->hidePopup();
> +        else if (input->maxResults() > 0) {
> +            ASSERT(!renderer->popupIsVisible());
> +            renderer->showPopup();
> +        }

This assertion you added seems non-helpful. I suggest just leaving it out.

review- because of the non-called calcExtraLineHeight function

[Nikolas Zimmermann]

Hey Darin,

forgot to say merry x-mas :-)
> This looks like a good approach.
That's good to hear, I was posting this patch in this early state, to get exactly the kind of comments you've provided - glad you took the time to check that big patch.
 
> Please be careful about the copyright. Some files have only the Torch Mobile
> copyright, so please be sure there's nothing that came from the existing code
> there.
IIRC only the new headers contain TorchMobile-only copyright? I thought that's ok, even if some methods existed in the other header before. If not, I can readd the Apple copyright in the new headers as well.
 
> > -    RenderTextControl(Node*, bool multiLine);
> > +    RenderTextControl(Node*);
> 
> I think it would be better to have the constructor be protected since
> RenderTextControl an abstract base class now.
Good catch!
 
> > +    virtual void forwardEvent(Event*);
> 
> This RenderTextControl member function should be protected and non-virtual.
> There's no caller that needs to call this on a RenderTextControl*, so there's
> no need to make it a public virtual function rather than a protected
> non-virtual one.
Right, my fault.

> > +    void updateInnerTextValue(const String&);
> 
> Why not name this setInnerTextValue?
Agreed.
 
> > +    virtual void processStyleChange() { }
> 
> I'm not sure the name processStyleChange clearly distinguishes this new
> function from styleDidChange. Why can't the derived class override
> styleDidChange instead? If there's a good reason, maybe this new function's
> name could reflect that reason. Maybe it's the precise timing of the call, and
> it could reflect that?
It affects timing, 'processStyleChange' has to be called after the base class didStyleChange method has been called. I tried just reimplementing styleDidChange in the subclasses, and it failed.. Though as this was some days ago, I'm going to retry it. Maybe it turns out overriding styleDidChange() works fine.

> 
> > +    void createSubtreeIfNeeded(Node* innerBlock);
> 
> I think Element* is a better type to use here than Node*.
Then we could directly use TextControlInnerElement, no?

> 
> > +    bool nodeAtPoint(const HitTestRequest&, HitTestResult&, int x, int y, int tx, int ty, HitTestAction, Node* innerBlock);
> 
> I think it's too subtle to have this non-virtual helper function have the same
> name as the virtual function, and differ only in the extra parameter. I think
> we should find a different name for it. Also, I think that the innerBlock
> argument should be a TextControlInnerElement*, not a Node*.
You're right, I didn't like the solution as well. Will think about this.

> 
> > +    bool m_edited : 1;
> > +    bool m_userEdited : 1;
> 
> It's inconsistent to change these to bit fields (implying we think the 8 bits
> per bool memory is precious enough that we want the slower bigger code -- or
> maybe it's 32 bits per bool?), but to put the different bits in the derived
> classes (implying we don't care about the extra 32 bits it costs to have two
> different sets of bit fields). I think it would be better to be consistent.
I _think_ it's 32 bit per bool, but I'd need to check it as well. I'll revert the bitfield change.
 
> > +void RenderTextControlMultiLine::forwardEvent(Event* evt)
> 
> Is there a good reason to abbreviate the name "event"? I'd prefer not to.
No reason, but 'Event* evt' is used all over WebCore, just like "Document* doc".
I'm all for changing this!
 
> > +    if (evt->type() == eventNames().blurEvent || evt->type() == eventNames().focusEvent)
> > +        return;
> > +
> > +    RenderTextControl::forwardEvent(evt);
> 
> Could this behavior go in the base class? We never want to forward these blur
> and focus events.
Well RenderTextControlSingleLine::forwardEvent() handles blur/focus events?
So I don't think that logic can go in the base class.

> 
> > +void RenderTextControlMultiLine::calcExtraLineHeight(int lineHeight)
> 
> I'm not sure I'm happy with the name "calc" for a function that mutates the
> object. This function doesn't "calculate extra line height". Frankly, I don't
> know what it does, because I don't see any code calling it!
RenderTextControl::calcHeight() uses this helper function. It's named in a really bad way - it was 5 am ;-) What it does, is restricting the passed lineHeight parameter, to the maximum possible line height (result/cancel button in a search field, may enlarge the lineHeight) and then it adds this contrained lineHeight to the m_height parameter. Text area elements multiply this lineHeight with the number of rows. Hm, could you suggest a better name?

> > +private:
> > +    virtual int preferredWidth(float charWidth) const;
> 
> It's a little strange to call this "preferredWidth" when actually the
> preferredWidth includes the padding. We should look for a name that makes that
> more explicit.
How about preferredAbsoluteWidth(IncludingPadding) ?
 
> > +class RenderTextControlSingleLine : public RenderTextControl
> > +                                  , private PopupMenuClient {
> 
> We haven't done this formatting in most places in WebKit the past and I'd
> prefer not to do it unless we have consensus we should do it this way. It seems
> this case fit fine on one line.
Okay, fine with me.

> 
> >      RenderTextControl* renderer = static_cast<RenderTextControl*>(node()->shadowAncestorNode()->renderer());
> > -    
> > -    return RenderBlock::nodeAtPoint(request, result, x, y, tx, ty, renderer->placeholderIsVisible() ? HitTestBlockBackground : hitTestAction);
> > +
> > +    bool placeholderIsVisible = false;
> > +    if (renderer->isTextField())
> > +        placeholderIsVisible = static_cast<RenderTextControlSingleLine*>(renderer)->placeholderIsVisible();
> > +
> > +    return RenderBlock::nodeAtPoint(request, result, x, y, tx, ty, placeholderIsVisible ? HitTestBlockBackground : hitTestAction);
> 
> It seems to me that the line that sets up the renderer local variable doesn't
> need to cast to RenderTextControl* -- it can just use RenderObject*.
Oh it was meant to save the cast below, I'll change that.
 
> > -        if (input && input->renderer() && static_cast<RenderTextControl*>(input->renderer())->popupIsVisible())
> > -            static_cast<RenderTextControl*>(input->renderer())->hidePopup();
> > -        else if (input->maxResults() > 0)
> > -            static_cast<RenderTextControl*>(input->renderer())->showPopup();
> > +        RenderTextControlSingleLine* renderer = static_cast<RenderTextControlSingleLine*>(input->renderer());
> > +        ASSERT(renderer);
> 
> I understand that the old code had unnecessary null checking of the renderer,
> but I'm not sure an assertion is all that helpful. Clearly the shadow DOM
> wouldn't exist if the renderer didn't exist, and I don't think the assert adds
> much.
Ok, most added ASSERTS just helped me while doing the split-up patch. I needed to be sure, I was interpreting the code correctly. Those can go away, you're right.

> 
> > +        if (renderer->popupIsVisible())
> > +            renderer->hidePopup();
> > +        else if (input->maxResults() > 0) {
> > +            ASSERT(!renderer->popupIsVisible());
> > +            renderer->showPopup();
> > +        }
> 
> This assertion you added seems non-helpful. I suggest just leaving it out.
Ok.

> 
> review- because of the non-called calcExtraLineHeight function
> 
Thanks a lot for the helpful comments!

Have a nice day,
Niko

[Darin Adler]

(In reply to comment #3)
> > > +    virtual void processStyleChange() { }
> > 
> > I'm not sure the name processStyleChange clearly distinguishes this new
> > function from styleDidChange. Why can't the derived class override
> > styleDidChange instead? If there's a good reason, maybe this new function's
> > name could reflect that reason. Maybe it's the precise timing of the call, and
> > it could reflect that?
>
> It affects timing, 'processStyleChange' has to be called after the base class
> didStyleChange method has been called. I tried just reimplementing
> styleDidChange in the subclasses, and it failed.. Though as this was some days
> ago, I'm going to retry it. Maybe it turns out overriding styleDidChange()
> works fine.

We can address this by changing the name if there is some order dependency. Once we understand what the dependency is then we can easily come up with a clear name.

> > > +    void createSubtreeIfNeeded(Node* innerBlock);
> > 
> > I think Element* is a better type to use here than Node*.
>
> Then we could directly use TextControlInnerElement, no?

I'm not sure the argument here is always a TextControlInnerElement. I think that in at least one case it's not. Maybe I read the code wrong.

> > > +    bool m_edited : 1;
> > > +    bool m_userEdited : 1;
> > 
> > It's inconsistent to change these to bit fields (implying we think the 8 bits
> > per bool memory is precious enough that we want the slower bigger code -- or
> > maybe it's 32 bits per bool?), but to put the different bits in the derived
> > classes (implying we don't care about the extra 32 bits it costs to have two
> > different sets of bit fields). I think it would be better to be consistent.
>
> I _think_ it's 32 bit per bool, but I'd need to check it as well. I'll revert
> the bitfield change.

Under gcc on Mac OS X it's 8 bits per bool.

> > > +void RenderTextControlMultiLine::forwardEvent(Event* evt)
> > 
> > Is there a good reason to abbreviate the name "event"? I'd prefer not to.
>
> No reason, but 'Event* evt' is used all over WebCore, just like "Document*
> doc".
> I'm all for changing this!

I never use "doc" or "evt" myself; no reason to abbreviate document and event in my opinion. But I do see both in many places. I did a quick grep and found "doc" in about 1000 places and "evt" in about 300 places. But "document" is used in about 3000 places and "event" is used in about 1200 places. So I think the abbreviations are losing out over time. (Although the full words get a boost from things like public APIs and comments, so the numbers aren't entirely fair.)

> > > +    if (evt->type() == eventNames().blurEvent || evt->type() == eventNames().focusEvent)
> > > +        return;
> > > +
> > > +    RenderTextControl::forwardEvent(evt);
> > 
> > Could this behavior go in the base class? We never want to forward these blur
> > and focus events.
>
> Well RenderTextControlSingleLine::forwardEvent() handles blur/focus events?
> So I don't think that logic can go in the base class.

Well, RenderTextControlSingleLine handles them and never calls the base class. So there's no harm in doing this check in the base class. And I think my comment is true -- we never want to forward these blur and focus events to the shadow nodes in either case.

It's a bit faster to not do this work in the base class, but it seems slightly cleaner to me to have the checks there.

> > > +void RenderTextControlMultiLine::calcExtraLineHeight(int lineHeight)
> > 
> > I'm not sure I'm happy with the name "calc" for a function that mutates the
> > object. This function doesn't "calculate extra line height". Frankly, I don't
> > know what it does, because I don't see any code calling it!
>
> RenderTextControl::calcHeight() uses this helper function. It's named in a
> really bad way - it was 5 am ;-) What it does, is restricting the passed
> lineHeight parameter, to the maximum possible line height (result/cancel button
> in a search field, may enlarge the lineHeight) and then it adds this contrained
> lineHeight to the m_height parameter. Text area elements multiply this
> lineHeight with the number of rows. Hm, could you suggest a better name?

I think adjustControlHeightBasedOnLineHeight might be a good name if a bit long; or maybe leave out the word "Control". The word "adjust" seems consistent with both looking at and modifying m_height.

> > > +private:
> > > +    virtual int preferredWidth(float charWidth) const;
> > 
> > It's a little strange to call this "preferredWidth" when actually the
> > preferredWidth includes the padding. We should look for a name that makes that
> > more explicit.
>
> How about preferredAbsoluteWidth(IncludingPadding) ?

I looked it up and the box inside the padding is called the content box, I suggest the name preferredContentWidth.

I don't think the word "absolute" is helpful here. We normally use that to distinguish document-relative coordinates from parent-relative coordinates.

[Nikolas Zimmermann]

Created attachment 26272 [details]
Updated patch

I think all of Darins issues are resolved with this new patch.

[Darin Adler]

Comment on attachment 26272 [details]
Updated patch

> -using namespace std;
> +using std::max;
> +using std::min;

Why that change? I like the old way better.

> +    return m_width - paddingLeft() - paddingRight() - borderLeft() - borderRight() -
> +           m_innerText->renderer()->paddingLeft() - m_innerText->renderer()->paddingRight();

We normally put the operator in the continuation line, and just indent one normal level rather than lining up, so it would be:

    return m_width - paddingLeft() - paddingRight() - borderLeft() - borderRight()
        - m_innerText->renderer()->paddingLeft() - m_innerText->renderer()->paddingRight();

The operator at the beginning of the line helps make it clear it's a continuation.

> +        ASSERT(ec == 0);

We should probably write these as ASSERT(!ec) in the future.

The history would probably work a little better if RenderTextControlSingleLine.h/cpp and RenderTextControlMultipleLine.h/cpp started out with "svn cp" of the RenderTextControl.h/cpp files. In fact I could then have reviewed the patch a little more carefully if that diff was in there.

r=me

[Nikolas Zimmermann]

(In reply to comment #6)
> (From update of attachment 26272 [details] [review])
> > -using namespace std;
> > +using std::max;
> > +using std::min;
> 
> Why that change? I like the old way better.
I prefer not to include all of 'std' namespace, but I'll leave it as is.
 
> > +    return m_width - paddingLeft() - paddingRight() - borderLeft() - borderRight() -
> > +           m_innerText->renderer()->paddingLeft() - m_innerText->renderer()->paddingRight();
> 
> We normally put the operator in the continuation line, and just indent one
> normal level rather than lining up, so it would be:
> 
>     return m_width - paddingLeft() - paddingRight() - borderLeft() -
> borderRight()
>         - m_innerText->renderer()->paddingLeft() -
> m_innerText->renderer()->paddingRight();
> 
> The operator at the beginning of the line helps make it clear it's a
> continuation. 
Fixed.

> > +        ASSERT(ec == 0);
> 
> We should probably write these as ASSERT(!ec) in the future.
Funny, I never questioned this style.. ASSERT(!ec) is much nicer. Fixed. Hm, thinking of 'ec' it's also an unecessary abbrevation, though it's handy & small. Though that's another topic.
 
> The history would probably work a little better if
> RenderTextControlSingleLine.h/cpp and RenderTextControlMultipleLine.h/cpp
> started out with "svn cp" of the RenderTextControl.h/cpp files. In fact I could
> then have reviewed the patch a little more carefully if that diff was in there.
Ok, I'll keep that in mind during the next split-up patches.

> r=me
Thanks!

[Nikolas Zimmermann]

Landed in r39490.

[Darin Adler]

(In reply to comment #7)
> I prefer not to include all of 'std' namespace, but I'll leave it as is.

I can understand the preference. It was my initial thought too. But I've since changed my mind. I think we should use "using namespace std" unless it creates a troublesome ambiguity -- then we can go to individual using statements instead.

> Hm,
> thinking of 'ec' it's also an unecessary abbrevation, though it's handy &
> small.

What would you prefer? Maybe "exception" or "exceptionCode"?

> > The history would probably work a little better if
> > RenderTextControlSingleLine.h/cpp and RenderTextControlMultipleLine.h/cpp
> > started out with "svn cp" of the RenderTextControl.h/cpp files. In fact I could
> > then have reviewed the patch a little more carefully if that diff was in there.
>
> Ok, I'll keep that in mind during the next split-up patches.

It's something you can do even after the fact. Just move the file aside, "svn cp", and then overwrite with your new file.

[Nikolas Zimmermann]

(In reply to comment #9)
> > Hm,
> > thinking of 'ec' it's also an unecessary abbrevation, though it's handy &
> > small.
> 
> What would you prefer? Maybe "exception" or "exceptionCode"?
Both sound fine IMHO - can we reuse the WebCore-rename script for changes like this? I've never looked at it so far.
 
> > > The history would probably work a little better if
> > > RenderTextControlSingleLine.h/cpp and RenderTextControlMultipleLine.h/cpp
> > > started out with "svn cp" of the RenderTextControl.h/cpp files. In fact I could
> > > then have reviewed the patch a little more carefully if that diff was in there.
> >
> > Ok, I'll keep that in mind during the next split-up patches.
> 
> It's something you can do even after the fact. Just move the file aside, "svn
> cp", and then overwrite with your new file.
Ouch, I didn't know that - I guess it's too late now to fix this, as I already landed it?

[Darin Adler]

(In reply to comment #10)
> > What would you prefer? Maybe "exception" or "exceptionCode"?
>
> Both sound fine IMHO - can we reuse the WebCore-rename script for changes like
> this? I've never looked at it so far.

We definitely could. But I worry a bit at replacing "ec" with "exceptionCode" because it's so much longer.

> > It's something you can do even after the fact. Just move the file aside, "svn
> > cp", and then overwrite with your new file.
>
> Ouch, I didn't know that - I guess it's too late now to fix this, as I already
> landed it?

Yes, too late for this time around.

[Nikolas Zimmermann]

(In reply to comment #11)
> We definitely could. But I worry a bit at replacing "ec" with "exceptionCode"
> because it's so much longer.
That's true - hm maybe we should special case it in the style rules ;-)