[webkit-reviews] review granted: [Bug 20709] Implement HTML 5's HTMLElement.classList property : [Attachment 68696] Trying to get this to build on GTK

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 20709: Implement HTML 5's HTMLElement.classList property
https://bugs.webkit.org/show_bug.cgi?id=20709

Attachment 68696: Trying to get this to build on GTK
https://bugs.webkit.org/attachment.cgi?id=68696&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68696&action=review

Please consider my comments. You may want to do one more round of refinements
before landing this.

> WebCore/html/DOMTokenList.cpp:60
> +    if (m_element->document()->inQuirksMode())
> +	   m_classNames = new
SpaceSplitString(m_element->fastGetAttribute(classAttr), false);

I’m surprised this compiles without an explicit adoptPtr. Soon I will be
turning on the “script OwnPtr” flag and then the adoptPtr will definitely be
needed. Please use adoptPtr here.

> WebCore/html/DOMTokenList.cpp:135
> +    const unsigned inputLength = input.length();

This is another use of const like the one I asked you to remove earlier.

> WebCore/html/DOMTokenList.cpp:137
> +    Vector<UChar> tokenVector;
> +    append(tokenVector, token);

We could easily write a function that compares a vector to an AtomicString so
you would not have to copy the characters into a Vector. That function could go
into AtomicString.h or for now could just be here in this source file and moved
to AtomicString if we need it later.

> WebCore/html/DOMTokenList.cpp:138
> +    Vector<UChar> output; // 3

We could probably improve performance when doing this on a long string by
calling reserveCapacity on output, guessing that its size will be inputLength.
I don’t know exactly what it takes to remove excess capacity at String::adopt
time but even if we don’t do that the performance tradeoff still might be good.


> WebCore/html/DOMTokenList.cpp:177
> +    if (!validateToken(token,  ec))

Extra space here after the comma.

> WebCore/html/DOMTokenList.cpp:196
> +    if (m_element->document()->inQuirksMode())
> +	   m_classNames->set(newClassName, false);

I suggest having this do if (m_classNames) instead of if (inQuirksMode).

> WebCore/html/DOMTokenList.cpp:202
> +    if (m_element->document()->inQuirksMode())
> +	   return m_classNames.get();

I suggest having this do if (m_classNames) instead of if (inQuirksMode).

> WebCore/html/DOMTokenList.h:42
> +	   return new DOMTokenList(element);

This line of code should call adoptPtr.

> WebCore/html/DOMTokenList.h:61
> +    DOMTokenList(Element* element);

You should omit the argument name “element” here.

> WebCore/html/DOMTokenList.h:70
> +    OwnPtr<SpaceSplitString> m_classNames;

It might be better to name this in a way that makes it more clear it’s used
only in a particular case. Even a name such as
m_caseSensitiveClassNamesForQuirksMode would not be too crazy, given that this
is only used in three places and it would be a mistake to use it anywhere else.


Given that SpaceSplitString already uses a pointer-to-implementation idiom,
using an OwnPtr is unnecessary. In fact, it costs one extra memory block for no
good reason. Another way to do this is to use have m_classNames be a
SpaceSplitString.

If stop using OwnPtr, then my other comments above about switching to a null
check of m_classNames would require adding an isNull() function to
SpaceSplitString, which I think would be OK. Or you could check quirks mode
each time as you are doing now.

If stop using OwnPtr, you won’t have to worry about adoptPtr in the
constructor. Instead you would use the SpaceSplitString set function.