[webkit-reviews] review denied: [Bug 5776] Generated numbers of ordered lists (OL) are not calculated right : [Attachment 5796] Proposed patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Andrew Wellington
<proton at wiretapped.net>'s request for review:
Bug 5776: Generated numbers of ordered lists (OL) are not calculated right
http://bugzilla.opendarwin.org/show_bug.cgi?id=5776

Attachment 5796: Proposed patch
http://bugzilla.opendarwin.org/attachment.cgi?id=5796&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This looks really good! A few comments:

To compare if two nodes are same, code should use "==". The isSameNode function
is for DOM bindings, not internal use. For most people working on the code, the
isSameNode calls will be harder to read.

On this line:

+	 } else if (n->firstChild() && (!dontDescend ||
!n->isSameNode(dontDescend))) {

can just do n != dontDescend, since we're guaranteed n is not 0.

I think it would be best to change getPrevListNode so that it's iterative
rather than recursive when following parentNode pointers -- should be a
straightforward change.

Some minor style things:

    1) we don't usually put else after return
    2) the while loops in getPrevListNode and prevListNode look well suited to
be for loops instead
    3) the "found" variable should be declared right where it's used; there's
no need to declare it outside the loop

This:

-	 if (listNode && listNode->hasTagName(olTag) &&
!m_render->previousSibling()) {
-	     HTMLOListElementImpl *ol = static_cast<HTMLOListElementImpl
*>(listNode);
-	     render->setValue(ol->start());
+	 if (listNode && listNode->hasTagName(olTag)) {
+	     NodeImpl *prevNode = prevListNode();

should just be:

+	 if (listNode && listNode->hasTagName(olTag) && prevListNode()) {

No reason to break it into multiple lines and add a local variable.

Also, should that be prevListNode() or listNode->prevListNode()?

In render_list.cpp, these spaces in the parentheses don't match our coding
style:

+    while ( o ) {
+	 if ( o->isListItem() && o->style()->listStyleType() != LNONE )

In this line of code:

+	 NodeImpl *n = ((HTMLLIElementImpl *)node())->prevListNode();

what do we do to be sure that node is an HTMLLIElement? The reason I ask is
that these assumptions about the DOM tree by the render tree have caused
crashing bugs in the past. Often there are CSS ways to get a particular type of
renderer with an unexpectedly different type of DOM node, which is one reason
renderers can't alway assume the type of DOM element. But in this case there
may be no issue -- it's hard to see locally in the calcListValue function.

We try to use static_cast syntax rather than C-style casts when possible.