[webkit-reviews] review granted: [Bug 96446] Element::classAttributeChanged should use characters8/16 to find first non-whitespace : [Attachment 163489] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 96446: Element::classAttributeChanged should use characters8/16 to find
first non-whitespace
https://bugs.webkit.org/show_bug.cgi?id=96446

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=163489&action=review


Looks like a good idea to me.

> Source/WebCore/dom/Element.cpp:765
>      for (i = 0; i < length; ++i) {
>	   if (isNotHTMLSpace(characters[i]))
>	       break;
>      }

If perf is important, this could become a do {} while() since we know length >
0.
Add ASSERT(length) if you do that change.

> Source/WebCore/dom/Element.cpp:773
> +    bool hasClass = !!length && (newClassString.is8Bit() ?
classStringHasClassName(newClassString.characters8(), length) :
classStringHasClassName(newClassString.characters16(), length));

I am not a fan of this long line.

I would change classStringHasClassName(AtomicString*) to call a
classStringHasClassNameImpl<>(CharacterType, length).
That way the function can stay just as clean.