[webkit-reviews] review denied: [Bug 36291] placeholder text should be stripped from line breaks : [Attachment 55391] Proposed patch (rev.3)

Fri May 7 18:21:30 PDT 2010

Darin Adler <darin at apple.com> has denied Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 36291: placeholder text should be stripped from line breaks
https://bugs.webkit.org/show_bug.cgi?id=36291

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

------- Additional Comments from Darin Adler <darin at apple.com>
I looked in HTML5 and could not find any text saying that we need to remove CR
and LF from attribute values. What I saw was text telling HTML5 content authors
they must not include CR and LF in their attribute values, but nothing that
tells HTML5 browser implementations what to do.

> +<head>
> +<link rel="stylesheet"
href="../../../../fast/js/resources/js-test-style.css"
> +</head>

This is strange. Why do you have an unterminated <link> element here? I think
you should just leave out this <head> entirely. Do you need this style sheet at
all?

> +static bool isNewLineCharacter(UChar ch) { return ch == newlineCharacter ||
ch == carriageReturn; }
> +
> +static bool isNotNewLineCharacter(UChar ch) { return ch != newlineCharacter
&& ch != carriageReturn; }

Why these particular function names? Are these two characters called "new line
characters"? Is that terminology from the HTML5 specification? I'd prefer a
different name given that one of these characters is called "newlineCharacter".
The function name seems too close to that.

> +    if (attributeValue.string().find(isNewLineCharacter) == -1)
> +	   return attributeValue;

I'm sure it's much more efficient to call contains on a UChar twice than to
call the version of find that takes a character-matching function once. I
suggest using contains.

> +bool HTMLTextFormControlElement::isPlaceholderEmpty() const
> +{
> +    const AtomicString& attributeValue = getAttribute(placeholderAttr);
> +    if (attributeValue.isEmpty())
> +	   return true;
> +    return attributeValue.string().find(isNotNewLineCharacter) == -1;
> +}

I don't think you need a special case for the empty string here. The find
function is already quite fast for empty strings, and will give the correct
result.

> +	   // node() should be an HTMLInputElement. WMLInputElement doesn't
support placeholder.

The phrase "should be" is not strong enough. I would say "must be".