[webkit-reviews] review granted: [Bug 128058] Speed up DatasetDOMStringMap::item() when the element has multiple attributes : [Attachment 222900] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 128058: Speed up DatasetDOMStringMap::item() when the element has multiple
attributes
https://bugs.webkit.org/show_bug.cgi?id=128058

Attachment 222900: Patch
https://bugs.webkit.org/attachment.cgi?id=222900&action=review

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


> Source/WebCore/dom/DatasetDOMStringMap.cpp:111
> +static inline AtomicString convertPropertyNameToAttributeName(const
StringImpl* name)

Should take StringImpl& rather than StringImpl* since you already check for
null in the caller. No need for const since a StringImpl is an immutable class.


> Source/WebCore/dom/DatasetDOMStringMap.cpp:113
> +    ASSERT(name);

This assertion is usually the clue that you should be passing a reference
rather than a pointer.

> Source/WebCore/dom/DatasetDOMStringMap.cpp:122
> +    for (unsigned i = 0, length = name->length(); i < length; ++i) {

Since name->length() is also used a couple lines earlier I suggest declaring
this local variable outside the loop, and using it up there.

> Source/WebCore/dom/DatasetDOMStringMap.cpp:123
> +	   CharacterType character = name->getCharacters<CharacterType>()[i];

I think we should call name->getCharacters outside the loop and put the result
into a local variable.

> Source/WebCore/dom/DatasetDOMStringMap.cpp:138
> +    StringImpl* nameImpl = name.impl();

This should be a StringImpl&.

> Source/WebCore/dom/DatasetDOMStringMap.cpp:180
> +	       AtomicString attributeName =
convertPropertyNameToAttributeName(propertyName);

I’m not sure that using the AtomicString here really pays off. What happens if
you use a String for the result of convertPropertyNameToAttributeName and do a
string equality check each time instead of an AtomicString pointer check?