[Webkit-unassigned] [Bug 36291] placeholder text should be stripped from line breaks

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36291


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55102|review?                     |review-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2010-05-06 18:20:51 PST ---
(From update of attachment 55102)
> +    const AtomicString& attributeValue = getAttribute(placeholderAttr);
> +    if (attributeValue.isEmpty())
> +        return String();
> +    // According to the HTML5 specification, we need to remove CR and LF from
> +    // the attribute value.
> +    Vector<UChar> stripped;
> +    unsigned length = attributeValue.length();
> +    stripped.reserveCapacity(length);
> +    for (unsigned i = 0; i < length; ++i) {
> +        UChar character = attributeValue[i];
> +        if (character == newlineCharacter || character == carriageReturn)
> +            continue;
> +        stripped.append(character);
> +    }
> +    return String(stripped.data(), stripped.size());

Since most strings don't have line breaks, it would be good to have a more
efficient code path for the common case. A simple way to do that is to simply
search for newline character and carriage returns first with the contains
function and add an early return.

Further, we can make this code faster by removing one allocation by using:

    Vector<UChar, 256> stripped;

The storage will be allocated on the stack instead of the heap if it fits in
256 characters. Another possibility, if you decide not to do this and to stick
with Vector<UChar>, is to use String::adopt instead, which uses the same
pointer. The resulting string will use a bit more storage, but we avoid copying
all the characters.

> +        && !strippedPlaceholder().isEmpty();

Seems wasteful to calculate the stripped placeholder just to check if it's
empty. If this is at all hot I suggest using a separate function that does the
far more efficient emptiness test, stopping on the first character that is not
a newline or carriage return.

>      virtual ~HTMLTextFormControlElement();
>      virtual void dispatchFocusEvent();
>      virtual void dispatchBlurEvent();
> +    String strippedPlaceholder() const;

I think this function should be in its own paragraph, not in the same one as
dispatchFocusEvent and dispatchBlurEvent.

>  const UChar blackSquare = 0x25A0;
>  const UChar bullet = 0x2022;
> +const UChar carriageReturn = 0x000D;
>  const UChar ethiopicPrefaceColon = 0x1366;
>  const UChar hebrewPunctuationGeresh = 0x05F3;

The characters in CharacterNames.h are supposed to use the names from the
Unicode standard. So the name should be lineFeed, not newlineCharacter. The one
you are adding has the right name.

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

This is not the right assertion. You need to assert this is an HTMLInputElement
since that's what you are casting to.

    ASSERT(node()->hasTagName(inputTag));

review- because I think you should do at least some of the things I mentioned
above

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.