Bug 16489

Created attachment 21260 [details] Patch + testcases Here is an implementation... review welcomed :)

Comment on attachment 21260 [details] Patch + testcases The implementation looks fine. The test cases should be written as js test cases, following the form used by so many in fast/js (and other places). you can use make-js-test-wrappers to generate the final .html files once you add your .js files. (js test cases use our shouldBe functions, etc.) Also, these new functions on Element should be const. Otherwise it looks fine to me. I would r+ this but, AFAIK you don't have commit bit (so any edits you make need to go through a posted patch). So I'd like to see a patch with cleaner test cases before r+ing. Thanks!

Created attachment 21327 [details] Fixed patch I made the methods 'const'. I rewrite the test cases to match the "shouldBe" way... hope i unserstood how description() should be used.

Comment on attachment 21327 [details] Fixed patch This looks great! It will be nice to have these. + Node *n = this->firstChild(); As mentioned in the coding style guide, the * should go next to the typename. So it should be "Node* n", not "Node *n". + while (n && n->nodeType() != ELEMENT_NODE) I think it would be better to use isElementNode() here instead of nodeType(). +unsigned long Element::childElementCount() const The type "unsigned long" in IDL actually means a 32-bit unsigned integer. Thus in C++ the type should be "unsigned" (which is what we use for 32 bit), not "unsigned long" (which is what we use for 32 bit or 64 bit depending on the target architecture). Thus childElementCount should have a type of unsigned. + unsigned long count = 0L; There's no good reason to use "0L" here instead of "0". If you really wanted to make the constant have the right type, it would be "0UL", but just plain "0" works fine. I think it's unnecessary additional work to use firstElementChild and nextElementSIbling to implement childElementCount. It would be better to just iterate all the child nodes and do: count += n->isElementNode() More efficient and still quite clear. +successfullyParsed = true; \ No newline at end of file Please put newlines at the ends of these files. +description( +"This test checks the implementation of the childElementCount attribute (ElementTraversal API)." ++ "<div id='noChildren'></div>" ++ "<div id='noElementChildren'><!-- comment but not an element -->no elements here</div>" ++ "<div id='children'><p>i'm an element</p><!-- comment --><p>i'm an other element</p></div>" +); It is not normal practice to put the test content into the description. Instead, code to set up the test content should be separate. You could use clean DOM calls, or something more like innerHTML or document.write, but it should not be part of description. These are not JavaScript tests, but rather DOM tests. So they should be somewhere in fast/dom, not in fast/js. The tests in fast/js in theory should all work in a standalone JavaScript engine and not rely on anything outside the core language. I don't think all these tests need to be separate. I'd suggest a single test with more test cases. +shouldBe("null", "document.getElementById(\"noChildren\").firstElementChild"); These are backwards. The interesting expression should be on the left and the expected value on the right. This produces better output when you run the tests. The patch needs to include the expected.txt files that are generated running the tests.

Created attachment 21338 [details] New fixed patch I hope all is fine now :-) I fixed the type of 'count', the call to isElement(), the directory of the tests (merged in only one), and added the '-expected.txt'.

Comment on attachment 21338 [details] New fixed patch Are these functions supposed to be defined on Document objects, or just Element objects? + Node* n = this->firstChild(); + Node* n = this->lastChild(); + Node* n = this->previousSibling(); + Node* n = this->nextSibling(); + Node* n = this->firstChild(); No need for the explicit "this->" here. + * fast/js/ElementTraversal-childElementCount.html: Added. + * fast/js/ElementTraversal-firstElementChild.html: Added. + * fast/js/ElementTraversal-lastElementChild.html: Added. + * fast/js/ElementTraversal-nextElementSibling.html: Added. + * fast/js/ElementTraversal-previousElementSibling.html: Added. + * fast/js/resources/ElementTraversal-childElementCount.js: Added. + * fast/js/resources/ElementTraversal-firstElementChild.js: Added. + * fast/js/resources/ElementTraversal-lastElementChild.js: Added. + * fast/js/resources/ElementTraversal-nextElementSibling.js: Added. + * fast/js/resources/ElementTraversal-previousElementSibling.js: Added. This is no longer the accurate list of files added. r=me, but it would be better to fix the above issues

While landing this I removed the unneeded "this->", fixed the change log entries. I also discovered another test case that depends on the precise number of properties in the Element class. Since we've added new functions, the results of that test have changed, so I had to update it too. Next time you submit a patch, please run the regression tests. If you ran them you would have seen this failure in fast/dom/domListEnumeration.html. Committed revision 34452.

Hum... even after this has landed, we don't pass the WICD core test on the subject: http://www.w3.org/2004/CDF/TestSuite/WICD_CDR_WP1/test-element-traversal.xhtml We hit a "TypeError: null value" on line 19 of: http://www.w3.org/2004/CDF/TestSuite/WICD_CDR_WP1/test-element-traversal.svg var childElement = element.firstElementChild; while (childElement) { // THIS IS WHERE WE HIT THE ERROR TypeError: Null value counter0++; childElement = childElement.nextElementSibling; } Ah, looks like it's lack of xml:id support?

(In reply to comment #8) > Hum... even after this has landed, we don't pass the WICD core test on the > subject: > http://www.w3.org/2004/CDF/TestSuite/WICD_CDR_WP1/test-element-traversal.xhtml > > Ah, looks like it's lack of xml:id support? We've removed xml:id from the testcase. It now passes fine. Thank you.

Mass moving XML DOM bugs to the "DOM" Component.