[webkit-reviews] review denied: [Bug 21248] Support placeholder on textarea : [Attachment 32114] Proposed patch (rev.3)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 21248: Support placeholder on textarea
https://bugs.webkit.org/show_bug.cgi?id=21248

Attachment 32114: Proposed patch (rev.3)
https://bugs.webkit.org/attachment.cgi?id=32114&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks really good. Just a few comments.

> +String HTMLTextAreaElement::placeholder() const
> +{
> +    return getAttribute(placeholderAttr).string();
> +}
> +
> +void HTMLTextAreaElement::setPlaceholder(const String& value)
> +{
> +    setAttribute(placeholderAttr, value);
> +}

These are not great. There's no need to convert to a plain string here, and in
fact it's bad for performance. The right way to do this nowadays is to just put
[Reflect] in the IDL file and not define a setPlaceholder function in the .h
and .cpp file at all, so that should be done here.

Since there are a couple call sites that do want placeholder, it would be OK to
keep it but it could return const AtomicString& instead of String to avoid a
bit of reference count churn and you probably would only have to use the
string() function in one place.

> +    if ((oldPlaceholderShouldBeVisible != m_placeholderShouldBeVisible ||
placeholderValueChanged)
> +	   && renderer())
> +	  
static_cast<RenderTextControlMultiLine*>(renderer())->updatePlaceholderVisibili
ty();

Indenting here is awkward.

> +    bool placeholderShouldBeVisible() const { return
m_placeholderShouldBeVisible; }

I think it would be a better design for the HTMLTextAreaElement to tell the 

> +    virtual void dispatchFocusEvent();
> +    virtual void dispatchBlurEvent();

These overrides should be private, not public, because we generally want all
functions to be as private as possible.

> +    void setPlaceholderShouldBeVisible(bool visible) {
m_placeholderShouldBeVisible = visible; }

I don't think you need this setter function. It doesn't seem to add any
abstraction.

> +		    attribute  DOMString	    placeholder;

Just adding the [Reflect] attribute here will do the trick.

> +void RenderTextControl::updatePlaceholderVisibility()
> +{
> +    RefPtr<RenderStyle> textBlockStyle =
createInnerTextStyle(parentStyle());
> +    HTMLElement* innerText = innerTextElement();
> +    innerText->renderer()->setStyle(textBlockStyle);
> +
> +    for (Node* n = innerText->firstChild(); n; n =
n->traverseNextNode(innerText)) {
> +	   if (RenderObject* renderer = n->renderer())
> +	       renderer->setStyle(textBlockStyle);
> +    }
> +
> +    updateFromElement();
> +}

I know you must moved this from one class to another, but this function needs
some comments. It's unclear why updatePlaceholderVisibility needs to update the
style of all the renderers and then call updateFromElement. A comment could
explain the relationship. Also, we could consider moving code to set the text
block style into a separate function, since the name of that function would
help document it.

> -    if (result.innerNode() == node() || result.innerNode() ==
innerTextElement())
> +    HTMLTextAreaElement* textArea =
static_cast<HTMLTextAreaElement*>(node());
> +    if (result.innerNode() == node()
> +	   || !textArea->placeholderShouldBeVisible() && result.innerNode() ==
innerTextElement()
> +	   || textArea->placeholderShouldBeVisible() &&
result.innerNode()->isDescendantOf(innerTextElement()))
>	   hitInnerTextElement(result, x, y, tx, ty);

The way the formatting makes the clauses of the if statement line up with its
body makes things like this hard to read.

I think you could consider fixing that by using a boolean local variable; the
name of the boolean could even help document a bit. I think the code also needs
a comment. The rule here is slightly unclear unless the reader really aware of
how placeholders should behave.

I'm not so sure that it's great to move the m_placeholderVisible data member
into the RenderTextControl base class. While it's true that both derived
classes use it, the base class does not.

> +RenderStyle* RenderTextControlMultiLine::parentStyle()

I don't understand the use of "parent" in this name. This isn't really the
style of the renderer's parent, because if so it wouldn't need to be a virtual
function. I know the local variable was named that, but a local variable has
less scope than a member function, and naming is slightly less critical when
the name is that tightly scoped. But could we give this function a name that's
more precise? Sorry, I don't have a specific name to suggest.

> +protected:
> +    virtual RenderStyle* parentStyle();

This can and should be private instead of protected. You can override a
function with a private function, and we should make things as private as
possible. If we ever did want to derive from this class we could make it
protected at that time.

> +protected:
> +    virtual RenderStyle* parentStyle();

Ditto.

review- because at least some of the changes above should be made