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

[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: REGRESSION: YAHOO: Generated numbers of ordered lists (OL) are not
calculated right
http://bugzilla.opendarwin.org/show_bug.cgi?id=5776

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

------- Additional Comments from Darin Adler <darin at apple.com>
Looking great. I love this patch!

There are still some slight improvements that could be made. In fact, I thought
of one major one.

1) I believe that the entire prevListNode function can be simplified, all
recursion removed, the two levels of loop removed, and all this folded back
into prevListNode by using the traversePreviousNode() function. Something like
this:

    static NodeImpl* parentList(NodeList* node)
    {
	for (NodeImpl* n = node; n; n = n->parentNode()) {
	    if (n->hasTagName(ulTag) || n->hasTagName(olTag))
		return n;
	return 0;
    }

    HTMLLIElementImpl* HTMLLIElementImpl::previousListItem()
    {
	NodeImpl* list = parentList(this);
	if (!list)
	    return 0;
	for (NodeImpl* n = this; n != list; n = n->traversePreviousNode())
	    if (n->hasTagName(liTag)) {
		NodeImpl* otherList = parentList(n);
		if (list == otherList)
		    // This item is part of our current list, so it's what
we're looking for.
		    return static_cast<HTMLLIElementImpl *>(n);
		// We found ourself inside another list; lets skip the rest of
it.
		if (otherList)
		    n = otherList;
	    }
	return 0;
    }

The parentList function could also be changed to be a member of
HTMLLIElementImpl instead of a local helper -- should work either way -- and we
might want to give it a different name since "parent" doesn't really express
the relationship. It's "the" list for a list item.

The logic is a little less nice than I'd like it, because it would be nicer to
not descend into nested list subtrees at all. But to do that we'd have to visit
parents before children, and to do that we'd use traversePreviousNodePostOrder.
But if we did that traversePreviousNodePostOrder would return an outer <li>
node *before* an <li> element inside it, which is out of document order. I
think that's a real case we need to handle, so I recommend just using
traversePreviousNode and using a different technique to avoid nodes in other
lists; should still be efficient enough. If I'm wrong about that <li> element
nesting issue, we could improve this further. We should make sure the test
cases include something like that (they probably do).

I have other comments about the existing patch's code's loop structure, but
I'll omit them since if you take my suggestion those loops will be gone.

2) I suggest renaming prevListNode to either previousListItem,
previousItemInList, previousLIElement, previousListItemElement, or something
like that.

3) If you follow my suggestion (1) above, then HTMLLIElementImpl::attach can
also use the parentList function. Then, since HTMLLIElementImpl::attach already
has the list pointer, you could factor out the "find previous <li> in a
particular list" part of the code I wrote above into a separate function and
call that from HTMLLIElementImpl::attach to avoid finding the parentList twice.


4) We've been putting the * next to the type, as in "NodeImpl*", in code
recently. Although I believe the coding style guide has not yet been updated,
I'd really like this patch to be that way, if it's not too much trouble.

5) Here, there's no need for isHTMLElement:

+	 if (node()->isHTMLElement() && node()->hasTagName(liTag))

That's because liTag includes both the tag name and the HTML namespace, so this
includes the check that this is an HTML node. Some older code in our tree seems
to not now this.

6) There's extra parentheses here:

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

One nice thing about static_cast syntax is that it provides parentheses so you
don't need that extra set.

7) We *might* need a null-check of node(). Not sure; there can sometimes be
render objects without corresponding DOM nodes, but this probably can't happen
for list items. We can probably land the patch without determining this.

8) Given my comments in item (3), we need some test cases where one <li> is
nested inside another (both for the same list). If those cases really don't
list, we can improve on (1) as described under (3).

9) We might want to consider changing responsibilities so the "start value"
code is not shared between both the render and DOM tree code. For example, the
DOM tree could always set the value, which could be named "listStartValue" and
we could only look at the value in the case where there's no previous list
item. Or we could explicitly fetch the value from the DOM tree inside
RenderListItem::calcListValue, since we already have to locate the list element
as a part of finding the previous list items. If you do the latter, then the
previousListItem function can be entirely local to render_list.cpp and need not
be added to the DOM tree code publicly at all.

10) This code seems to compute marker values even for the three list style
types that are symbols and not integers: DISC, CIRCLE, and SQUARE, although it
does have a special case for LNONE. That seems like unnecessary work. I think
we need test cases that mix list style types. Is it really true that a list
item with no marker resets the count to 0, but a list item with a disc gets an
invisible number? That's what the current code seems to do.

Thanks for tackling this!