[webkit-reviews] review granted: [Bug 21465] Yet another querySelectorAll speedup : [Attachment 24187] Fixed

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Smith <catfish.man at gmail.com>'s
request for review:
Bug 21465: Yet another querySelectorAll speedup
https://bugs.webkit.org/show_bug.cgi?id=21465

Attachment 24187: Fixed
https://bugs.webkit.org/attachment.cgi?id=24187&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+	 for (CSSSelector* tempSelector = querySelector; !newRoot &&
tempSelector; tempSelector = tempSelector->m_tagHistory) {

I think the word "temp" in "tempSelector" is confusing here; there's nothing
temporary about the selector. Maybe just "selector" would be a better name. Or
maybe "idSelector", because that's what it is once we get past that first if
statement in the loop.

+	     if (tempSelector == querySelector && (rootNode->isDocumentNode()
|| newRoot->isDescendantOf(rootNode)) &&
selectorChecker.checkSelector(querySelector, newRoot)) {

What is the purpose of the "rootNode->isDocumentNode()" here? Can we just leave
that out? Do we have a test case that would fail if we left it out?

Do we have a test suite that covers all the cases here? I know this patch
doesn't change behavior, but I'm worried about whether there's enough coverage.
I'd like to know that there's at least one case for each if statement and case
in switch statement.

I'm going to say r=me, but I'd like to be reassured about test coverage.