[webkit-reviews] review granted: [Bug 3492] TreeWalker implementation needs to be fixed (affects Acid3) : [Attachment 18894] much better patch -- not perfect on Acid3, but makes us pass 4 more tests (from 69 to 73 when I tried it)

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 3492: TreeWalker implementation needs to be fixed (affects Acid3)
http://bugs.webkit.org/show_bug.cgi?id=3492

Attachment 18894: much better patch -- not perfect on Acid3, but makes us pass
4 more tests (from 69 to 73 when I tried it)
http://bugs.webkit.org/attachment.cgi?id=18894&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
// FIXME: Should we add takeException as a member of ExecState?
IMO: yes.

if (exec->hadException()) {
    exception = takeException(exec);
    return NodeFilter::FILTER_REJECT;
}
These could be even shorter using a helper:
return rejectNodeWithException(exec);

:sigh:
We should file a bug about extending the js bindings mechanism to handle the
exception passing implemented by JSTreeWalkerCustom.cpp
[PassesException] or [RethrowsException]

const Node *n = this;
Node* traversePreviousSiblingPostOrder(const Node *stayWithin = 0) const;

Seems slightly silly to clear "exception" every time through the loop in Node*
NodeIterator::nextNode(ExceptionCode& ec, JSValue*& exception)

 153 void NodeIterator::notifyBeforeNodeRemoval(Node* removedNode)
has:
//updateForNodeRemoval(removedNode, m_candidateNode);

I think this check in TreeWalker::parentNode() to avoid the ref churn is
probably unnecessary:
    if (m_current == root())
 56	    return 0;

It might be worth adding a private helper function:
bool shouldAcceptNode(Node*, JSValue*& exception); due to how many times the
rather-long: acceptNode(m_candidateNode.node.get(), exception) ==
NodeFilter::FILTER_ACCEPT string used.

It would also be nice if this common hunk of code could be pushed into a common
function:
if (acceptNodeResult == NodeFilter::FILTER_ACCEPT) {
 280		 m_current = node.release();
 281		 return m_current.get();
 282	     }

Overall this looks great!  Certainly much much better than what's currently in
TOT.