Bug 149126 – Document.createNodeIterator(null) / Document.createTreeWalker(null) should throw a TypeError

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Document.createNodeIterator(null) / Document.createTreeWalker(null) should throw a TypeError:
https://dom.spec.whatwg.org/#interface-document

This is because the parameter is not nullable and Web IDL says we should throw a TypeError in this case.

Firefox and Chrome throw an exception in this case.

[Chris Dumez]

rdar://problem/22564891

[Chris Dumez]

Created attachment 261134 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 261134 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261134&action=review

> Source/WebCore/dom/Document.h:540
> +    RefPtr<NodeIterator> createNodeIterator(Node* root, unsigned long whatToShow, RefPtr<NodeFilter>&&, bool expandEntityReferences, ExceptionCode&);
> +    RefPtr<NodeIterator> createNodeIterator(Node* root, unsigned long whatToShow, RefPtr<NodeFilter>&&, ExceptionCode&);
> +    RefPtr<NodeIterator> createNodeIterator(Node* root, unsigned long whatToShow, ExceptionCode&);
> +    RefPtr<NodeIterator> createNodeIterator(Node* root, ExceptionCode&);
> +

We can't rely on default arguments!?
Can we somehow fix the bidding code generator to take care of this instead?

> LayoutTests/imported/w3c/web-platform-tests/dom/traversal/TreeWalker-basic-expected.txt:7
> -FAIL Give an invalid root node to document.createTreeWalker(). assert_throws: function "function () { document.createTreeWalker(null); }" did not throw
> +PASS Give an invalid root node to document.createTreeWalker(). 

Does this test all the combinations?
If not, it might be worth adding a new test case.

[Chris Dumez]

Comment on attachment 261134 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261134&action=review

>> Source/WebCore/dom/Document.h:540
>> +
> 
> We can't rely on default arguments!?
> Can we somehow fix the bidding code generator to take care of this instead?

Well, the issue is that the function now takes an ExceptionCode& so we cannot use default arguments. Longer term, my plan is to use WTF::Optional for optional parameters. This will probably be needed when we supposed passing undefined for optional parameters, e.g.
document.createNodeIterator(node, undefined, filter, false);

In this case, we should use the default value for whatToShow parameter.

>> LayoutTests/imported/w3c/web-platform-tests/dom/traversal/TreeWalker-basic-expected.txt:7
>> +PASS Give an invalid root node to document.createTreeWalker(). 
> 
> Does this test all the combinations?
> If not, it might be worth adding a new test case.

All combinations? I only changed the behavior of the first argument of createNodeIterator() / createTreeWalker().
Now that I look at it though, there is no coverage for createNodeIterator(null) but I'll add a test before landing.

[Chris Dumez]

Created attachment 261152 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 261152 [details]
Patch

Clearing flags on attachment: 261152

Committed r189765: <http://trac.webkit.org/changeset/189765>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 261152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261152&action=review

> Source/WebCore/dom/Document.cpp:1744
> +    return createNodeIterator(root, 0xFFFFFFFF, nullptr, false, ec);

Would have been nice if this 0xFFFFFFFF was a constant somewhere. It is used in a couple places.