Bug 144822 – Element Traversal is not just Elements anymore

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Element Traversal is not just Elements anymore

[Sam Weinig]

Created attachment 252760 [details]
Patch

[Sam Weinig]

Committed r184034: <http://trac.webkit.org/changeset/184034>

[Darin Adler]

Comment on attachment 252760 [details]
Patch

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

> Source/WebCore/dom/CharacterData.idl:44
> +    // From the NonDocumentTypeChildNode interface - https://dom.spec.whatwg.org/#nondocumenttypechildnode
> +    // FIXME: Move this to a seperate NonDocumentTypeChildNode IDL file when one exists.
> +    readonly attribute Element previousElementSibling;
> +    readonly attribute Element nextElementSibling;

Is there a reason not to put these in Node.idl instead?

There’s a misspelling in all these comments, by the way, separate with an e instead of an a.

> Source/WebCore/dom/ContainerNode.cpp:916
> +    ASSERT(is<Document>(*this) || is<DocumentFragment>(*this) || is<Element>(*this));

Why do we need to assert this?

> Source/WebCore/dom/ContainerNode.cpp:923
> +    ASSERT(is<Document>(*this) || is<DocumentFragment>(*this) || is<Element>(*this));

Why do we need to assert this?

> Source/WebCore/dom/ContainerNode.cpp:930
> +    ASSERT(is<Document>(*this) || is<DocumentFragment>(*this) || is<Element>(*this));

Why do we need to assert this?

> Source/WebCore/dom/ContainerNode.cpp:937
> +    Node* n = firstChild();
> +    while (n) {
> +        count += n->isElementNode();
> +        n = n->nextSibling();
> +    }

Since you moved this, how about a for loop instead of a while loop?

> Source/WebCore/dom/Node.cpp:435
> +    ASSERT(is<CharacterData>(*this) || is<Element>(*this));

Why do we need to assert this?

> Source/WebCore/dom/Node.cpp:442
> +    ASSERT(is<CharacterData>(*this) || is<Element>(*this));

Why do we need to assert this?

[Andreas Kling]

This broke js/dom/dom-static-property-for-in-iteration.html in a mostly harmless looking way:

https://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r184034%20(14583)/js/dom/dom-static-property-for-in-iteration-pretty-diff.html

[Chris Dumez]

Comment on attachment 252760 [details]
Patch

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

>> Source/WebCore/dom/CharacterData.idl:44
>> +    readonly attribute Element nextElementSibling;
> 
> Is there a reason not to put these in Node.idl instead?
> 
> There’s a misspelling in all these comments, by the way, separate with an e instead of an a.

Based on the name, these should not be exposed on Document so having them in Node.idl would cause this. However, I don't understand why we don't create this new NonDocumentTypeChildNode interface. I believe I added support for implements idl statements to our Bindings generator iirc so it should be trivial and it would avoid the IDL code duplication.

[Chris Dumez]

Comment on attachment 252760 [details]
Patch

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

>> Source/WebCore/dom/ContainerNode.cpp:937
>> +    }
> 
> Since you moved this, how about a for loop instead of a while loop?

for (auto& element : childrenOfType<Element>(*this))
  ++count;

[Chris Dumez]

Comment on attachment 252760 [details]
Patch

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

> Source/WebCore/dom/Element.idl:130
> +    // From the ParentNode interface - https://dom.spec.whatwg.org/#interface-parentnode

We already have ChildNode.idl, we should definitely add ParentNode.idl and avoid the IDL duplication.

[Alexey Proskuryakov]

> This broke js/dom/dom-static-property-for-in-iteration.html in a mostly harmless looking way:

And EWS would have told you that.

Landed new results in r184036. Please consider addressing comments from Darin and Chris though.

[Chris Dumez]

Comment on attachment 252760 [details]
Patch

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

>>> Source/WebCore/dom/CharacterData.idl:44
>>> +    readonly attribute Element nextElementSibling;
>> 
>> Is there a reason not to put these in Node.idl instead?
>> 
>> There’s a misspelling in all these comments, by the way, separate with an e instead of an a.
> 
> Based on the name, these should not be exposed on Document so having them in Node.idl would cause this. However, I don't understand why we don't create this new NonDocumentTypeChildNode interface. I believe I added support for implements idl statements to our Bindings generator iirc so it should be trivial and it would avoid the IDL code duplication.

I'll do the split in https://bugs.webkit.org/show_bug.cgi?id=144825.

>> Source/WebCore/dom/Element.idl:130
>> +    // From the ParentNode interface - https://dom.spec.whatwg.org/#interface-parentnode
> 
> We already have ChildNode.idl, we should definitely add ParentNode.idl and avoid the IDL duplication.

I'll do the split in https://bugs.webkit.org/show_bug.cgi?id=144825.

[Sam Weinig]

(In reply to comment #3)
> Comment on attachment 252760 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252760&action=review
> 
> > Source/WebCore/dom/CharacterData.idl:44
> > +    // From the NonDocumentTypeChildNode interface - https://dom.spec.whatwg.org/#nondocumenttypechildnode
> > +    // FIXME: Move this to a seperate NonDocumentTypeChildNode IDL file when one exists.
> > +    readonly attribute Element previousElementSibling;
> > +    readonly attribute Element nextElementSibling;
> 
> Is there a reason not to put these in Node.idl instead?

They should not be exposed on all Nodes as per the DOM spec.

> 
> There’s a misspelling in all these comments, by the way, separate with an e
> instead of an a.
> 
> > Source/WebCore/dom/ContainerNode.cpp:916
> > +    ASSERT(is<Document>(*this) || is<DocumentFragment>(*this) || is<Element>(*this));
> 
> Why do we need to assert this?
> 
> > Source/WebCore/dom/ContainerNode.cpp:923
> > +    ASSERT(is<Document>(*this) || is<DocumentFragment>(*this) || is<Element>(*this));
> 
> Why do we need to assert this?

Since these functions are really only for exposing to the web (internally, we use our own traversal and iteration functions, these assert the intent of what type of Nodes these should be exposed on.  Another way to do this, which I might do at some point, is to add new classes that represent the ParentNode and NonDocumentTypeChildNode interfaces and use inheritance to enforce this. 

> 
> > Source/WebCore/dom/ContainerNode.cpp:937
> > +    Node* n = firstChild();
> > +    while (n) {
> > +        count += n->isElementNode();
> > +        n = n->nextSibling();
> > +    }
> 
> Since you moved this, how about a for loop instead of a while loop?

Seems like a good idea. Or use our modern iterators.