[webkit-reviews] review denied: [Bug 20709] Implement HTML 5's HTMLElement.classList property : [Attachment 68606] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 68606: Patch
https://bugs.webkit.org/attachment.cgi?id=68606&action=review

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

Great work. Looks close to ready!

I am going to say review- but only because of the inefficient use of String
here in the add and remove functions. If we add some test cases to prove we
don’t have O(n^2) algorithms there then I think we’re ready to go.

> WebCore/dom/Element.cpp:1598
> +    if (hasRareData()) {
> +	   ElementRareData* data = rareData();
> +	   if (data && data->m_classList)
> +	       data->m_classList->reset(newClassName);
> +    }

We’e normally use early return here:

    if (!hasRareData())
	return;
    ElementRareData* data = rareData();
    if (!data || !data->m_classList)
	return;
    data->m_classLiast->reset(newClassName);

But also, you could use optionalClassList for this:

    DOMTokenList* classList = optionalClassList();
    if (!classList)
	return;
    classList->reset(newClassName);

I think that’s the best way to write it. Or maybe even put that code in-line at
the one call site and not have that function at all.

> WebCore/dom/StyledElement.cpp:224
> +    }
>      else {

Braces go on the same line as the else in the WebKit coding style.

> WebCore/html/DOMTokenList.cpp:38
> +namespace {
> +
> +bool validateToken(const AtomicString& token, ExceptionCode& ec)

WebKit code typically uses “static” to get internal linkage rather than an
anonymous namespace, but perhaps this is something that we should change?

> WebCore/html/DOMTokenList.cpp:45
> +    const unsigned length = token.length();

WebKit code normally does not use const for local scalar variables. I see the
attraction of doing so to state that a particular variable won’t change, but
doing in sometimes and not others seems untidy, and it has no effect on
generated code. I’d prefer that we not do it here, just to stay consistent.

> WebCore/html/DOMTokenList.cpp:62
> +	   m_classNames = new
SpaceSplitString(m_element->getAttribute(classAttr), false);

This can use fastGetAttribute.

> WebCore/html/DOMTokenList.cpp:91
> +	return containsInternal(token);

This line of code has an extra space.

> WebCore/html/DOMTokenList.cpp:108
> +    const AtomicString oldClassName(m_element->getAttribute(classAttr));

This can use const AtomicString& so it doesn’t ref and then deref.

This can use fastGetAttribute.

> WebCore/html/DOMTokenList.cpp:110
> +    if (oldClassName.isEmpty())
> +	   m_element->setAttribute(classAttr, token);

Don’t we also want to do this when the old value consists entirely of
whitespace? Is there a test case covering this?

> WebCore/html/DOMTokenList.cpp:112
> +	   m_element->setAttribute(classAttr, oldClassName + " " + token);

This is the cleanest way to write the code, but it is slow and reallocates the
string twice. You should use Vector<UChar> or StringBuilder for better
efficiency.

> WebCore/html/DOMTokenList.cpp:133
> +    String output; // 3

Building up a String a character at a time is pathologically slow. We need to
use Vector<UChar> instead of String here. I’d like to see a test case with a
long class name or a class attribute with a lot of class names to verify the
algorithm is not O(n^2).

> WebCore/html/DOMTokenList.cpp:185
> +    return m_element->getAttribute(classAttr);

Can use fastGetAttribute.

> WebCore/html/DOMTokenList.h:45
> +    ~DOMTokenList() { }

There is no need to explicitly declare and define this constructor.

> WebCore/html/DOMTokenList.idl:40
> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +	   [DontEnum] DOMString toString();
> +#endif

I don’t think this is the right way to make toString work. Isn’t it built in to
the language already?