[Webkit-unassigned] [Bug 7761] Tabs in class attribute not treated as whitespace

[bugzilla-daemon at opendarwin.org]

http://bugzilla.opendarwin.org/show_bug.cgi?id=7761


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7064|review?                     |review-
               Flag|                            |




------- Comment #3 from darin at apple.com  2006-03-14 15:46 PDT -------
(From update of attachment 7064)
Looks great. We wanted to get this class off of QStringList anyway, so hooray
for that!

OK, let me now be crazy and make premature optimization comments:

+        while (classAttr[sPos] && (classAttr[sPos] == ' ' || classAttr[sPos]
== '\r' || classAttr[sPos] == '\n' || classAttr[sPos] == '\t'))
+            sPos++;

The code above is a little inefficient. First of all, it checks that the
character is non-null before comparing it with three specific characters. The
check for 0 is not needed because if it's ' ' or '\r' or '\n' or '\t', then
it's definitely not 0.

Each call to the [] operator does a separate comparison with the length of the
string so it would be nicer if we didn't have to index over and over again. If
you made a little helper function then you could write:

    while (isClassWhitespace(classAttr[sPos]))
        sPos++;

which might be clearer anyway.

Checking for the null character to detect the end of the string is
intrinsically inefficient for our Platform::String class, because it has a
length stored, not a trailing null. So if you were to take classAttr.unicode()
and classAttr.length() and work directly, you'd end up with something more
efficient. Something like this:

    str = classAttr.unicode();

    while (sPos < length && isClassWhitespace(str[sPos]))
        ++sPos;
    if (sPos >= length)
        break;
    int ePos = sPos;
    while (ePos < length && !isClassWhitespace(str[ePos]))
        ++sPos;

But I can't tell whether this kind of tweaking makes the code better or worse.

ePos should not be declared outside the loop. Instead it should be declared
right where it's used.

Do we want to optimize the common case where sPos is 0 and ePos is 1 so that we
don't create a new atomic string for that case?

Not sure what to do about these nitpicks. The code is good as is. I'm going to
mark this review-, but it's practically a review+.


-- 
Configure bugmail: http://bugzilla.opendarwin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.