[webkit-reviews] review denied: [Bug 22996] RenderTextControl unecessarily combines multi/single line text control rendering : [Attachment 26249] Initial patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 22996: RenderTextControl unecessarily combines multi/single line text
control rendering
https://bugs.webkit.org/show_bug.cgi?id=22996

Attachment 26249: Initial patch
https://bugs.webkit.org/attachment.cgi?id=26249&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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