[webkit-reviews] review denied: [Bug 19933] nodeIterator with filter fails on documents not in a frame : [Attachment 22139] Patch to pass ExecState down to node filters

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Simon Fraser
<simon.fraser at apple.com>'s request for review:
Bug 19933: nodeIterator with filter fails on documents not in a frame
https://bugs.webkit.org/show_bug.cgi?id=19933

Attachment 22139: Patch to pass ExecState down to node filters
https://bugs.webkit.org/attachment.cgi?id=22139&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Great!

I would have made the ExecState* the first argument because that's the usual
convention in code that involves ExecState, but I think it's OK to have it as
the last one.

+    if (!exec)
+	 return NodeFilter::FILTER_REJECT;

I think this merits a comment. Specifically that this case will only arise when
calling from a non-JavaScript language to a JavaScript filter, when the
document in question is not associated with a frame.

     if (exec->hadException()) {
-	 exception = takeException(exec);
	 return NodeFilter::FILTER_REJECT;
     }

One-line if statement bodies don't get braces in the WebKit coding style.
Please remove the braces in these cases.

+	 virtual short acceptNode(Node*, KJS::ExecState* exec) const;

We don't give the argument name in function declarations when the type makes
the argument's purpose clear. So the name "exec" should be omitted here and in
the various other similar places.

+    short result = impl()->acceptNode(toNode(args[0]), exec);
     return jsNumber(exec, result);

Why not merge this into a single line?

+    if (exec->hadException()) {
+	 exec->clearException();
	 return jsUndefined();
     }

The clearException here is incorrect. We do want to pass the exception on to
the caller. This mistake should have made tests fail. I don't know why it
didn't. I guess we don't have sufficient test coverage for our existing
exception behavior!

+short ObjCNodeFilterCondition::acceptNode(Node* node, ExecState* exec) const

Although we haven't turned the warning on, we do omit the names of unused
arguments. Please do that here.

+    if (node && node->document()) {
+	 Frame* frame = node->document()->frame();
+	 if (frame)
+	     return frame->script()->globalObject()->globalExec();
+    }
+    return 0;

We normally prefer the early return style. I'd write it like this:

    if (!node)
	return 0;
    Document* document = node->document();
    if (!document)
	return 0;
    Frame* frame = document->frame();
    if (!frame)
	return 0;
    return frame->script()->globalObject()->globalExec();

Also, I'm not sure we're guaranteed globalObject() can't be 0. What happens if
JavaScript is disabled?

I think you need to do the execStateFromNode thing for NodeIterator as well as
TreeWalker. You should move that function to Traversal and do it for both. Also
I think it can be a static member function. But also, since you're using it for
NodeFilter too, maybe you can find a place to put it where it can be one shared
copy.

+	 exec->clearException();  // rignt thing to do?

This is not the right thing to do, although it will do no harm. You can just
leave this line of code out (and fix the typo that way).

+    if (exec)
+	 exec->clearException();

Similarly, these are not needed, but will do no harm.

Please check Acid3 to make sure we still have 100/100.

review- for the incorrect clearException calls (the ones that will do harm) and
the lack of execStateFromNode in NodeIterator.