[Webkit-unassigned] [Bug 5776] Generated numbers of ordered lists (OL) are not calculated right

[bugzilla-daemon at opendarwin.org]

http://bugzilla.opendarwin.org/show_bug.cgi?id=5776


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #5796|review?                     |review-
               Flag|                            |




------- Comment #7 from darin at apple.com  2006-01-21 10:48 -------
(From update of attachment 5796)
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.


-- 
Configure bugmail: http://bugzilla.opendarwin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.