[webkit-reviews] review denied: [Bug 7761] Tabs in class attribute not treated as whitespace : [Attachment 7064] Treat \t and \r as whitespace

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 7761: Tabs in class attribute not treated as whitespace
http://bugzilla.opendarwin.org/show_bug.cgi?id=7761

Attachment 7064: Treat \t and \r as whitespace
http://bugzilla.opendarwin.org/attachment.cgi?id=7064&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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+.