[Webkit-unassigned] [Bug 36724] Add support for <ol reversed>

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36724


Darin Adler <darin at apple.com> changed:

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




--- Comment #12 from Darin Adler <darin at apple.com>  2010-04-30 10:36:10 PST ---
(From update of attachment 54798)
As I say below, I think that too much of this code is be on the DOM side. It
should consistently be in the rendering code, not the DOM.

LayoutTests/fast/lists/ol-reversed-simple.html:89
 +  \ No newline at end of file
We normally put newlines at ends of HTML files in the tests directory.

WebCore/html/HTMLOListElement.cpp:38
 +      , m_reversed(false)
This is a boolean member, we normally use the "list element <xxx>" style
naming. So this would be "m_isReversed" or "m_numbersItemValuesBackwards" or
something along those lines.

WebCore/html/HTMLOListElement.cpp:77
 +          updateItemValues();
Updating item values immediately is not good for performance. It means that if
multiple changes are made to the start attribute or the reversed attribute we
will renumber everything repeatedly. Ideally we would use a sort of "dirty"
system where the actual renumbering waits until the numbers are needed. But
this is not a regression, so I guess we can live with it for now.

> +int HTMLOListElement::start() const
> +{
> +    return m_hasExplicitStart ? m_start : (m_reversed ? itemCount() : 1);
> +}

I'm surprised that the result of the DOM start function is the item count when
the reversed flag is set. None of the test cases test this behavior. Does HTML5
specify this? Please add a test case if it does.

> +    if (m_shouldRecalculateItemCount) {

In the WebKit project we normally use early return for things like this instead
of nesting the entire function inside an if statement.

> +        RenderObject* child = renderer()->nextInPreOrder(renderer());
> +        while (child) {
> +            Node* node = child->node();
> +            if (node && (node->hasTagName(ulTag) || node->hasTagName(olTag))) {
> +                // Skip any nested lists.
> +                child = child->nextInPreOrderAfterChildren(renderer());
> +                continue;
> +            }
> +
> +            if (child->isListItem())
> +                ++m_itemCount;
> +
> +            child = child->nextInPreOrder(renderer());
> +        }

It's not good here that the numbering rules are in two places. The
HTMLOListElement code knows how to do the numbering so it can get the total
count. But RenderListItem separately knows how to do the numbering to get the
values of individual items. The code to do both should be side by side so they
can be kept in sync. That means we might even use a static member function so
the functions needed by the list and list item classes in the render tree can
be side by side. I suggest putting the functions into the list class and having
the item class call through, since numbering seems to be something the list
“does to the items” rather than the other way around.

It's also not great that the item count is stored in the DOM tree when it's
dependent on the rendering. The count should be stored in the render tree even
if the DOM tree might access it. I think this belongs in the renderer class for
the list not the DOM class.

I ran out of time and did not review the RenderListItem.cpp part of this file,
and I promise to do so next time, but I think what I already discovered is
enough to say review- and ask for another cut at this.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.