Bug 96446 - Element::classAttributeChanged should use characters8/16 to find first non-whitespace
Summary: Element::classAttributeChanged should use characters8/16 to find first non-wh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-11 16:24 PDT by Michael Saboff
Modified: 2012-09-12 14:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.70 KB, patch)
2012-09-11 17:43 PDT, Michael Saboff
benjamin: review+
msaboff: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2012-09-11 16:24:28 PDT
Element::classAttributeChanged should be changed to use characters8() or characters16() based on string bitness.
Comment 1 Michael Saboff 2012-09-11 17:43:22 PDT
Created attachment 163489 [details]
Patch
Comment 2 Benjamin Poulain 2012-09-11 18:01:36 PDT
Comment on attachment 163489 [details]
Patch

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.
Comment 3 Michael Saboff 2012-09-12 14:40:07 PDT
Committed r128363: <http://trac.webkit.org/changeset/128363>