Element Traversal is not just Elements anymore
Created attachment 252760 [details] Patch
Committed r184034: <http://trac.webkit.org/changeset/184034>
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?
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
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.
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;
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.
> 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.
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.
(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.