[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





--- Comment #5 from Kent Tamura <tkent at chromium.org>  2010-05-07 10:09:57 PST ---
Thank you for reviewing!

(In reply to comment #2)
> (From update of attachment 55102 [details])
> > +    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.

Ok, i added code to check CR/LF existence before copying.

> 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.

I chose String::adopt().

> > +        && !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.

Ok, I added new function; isPlaceholderEmpty().

> >      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.

Added an empty line.

> >  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.

No.  The right name for U+000D is CARRIAGE RETURN according to the Unicode
standard.
LINE FEED is U+000A.  Note that we already have a wrong name 'newlineCharacter'
for U+000A in CharacterNames.h.

> > +        // 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.

No.
 - WMLInputElement also has inputTag.
 - HTMLInputElement represents <isindex> too, and we support placeholder for it
(See LayoutTests/fast/forms/isindex-placeholder.html)

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