[webkit-reviews] review denied: [Bug 16489] WebKit does not support ElementTraversal specification : [Attachment 21327] Fixed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Vincent Ricard
<magic at magicninja.org>'s request for review:
Bug 16489: WebKit does not support ElementTraversal specification
http://bugs.webkit.org/show_bug.cgi?id=16489

Attachment 21327: Fixed patch
http://bugs.webkit.org/attachment.cgi?id=21327&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.