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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Jakub Wieczorek <jwieczorek at webkit.org>  2010-03-29 15:00:28 PST ---
Thanks for reviewing this!

> The test has nowhere near enough coverage. Lots of the new code isn't needed at
> all for the simple test case we're adding. We need code that covers all the
> edge cases of the new code, especially dynamic changes of various types,
> because that's the part that's easy to get wrong.

I agree. I added a couple of tests more. See below.

> > +        int start = attr->value().toInt(&m_hasExplicitStart);
> >          if (start == m_start)
> >              return;
> >          m_start = start;
> 
> The code above incorrectly returns early if the start changes from "start of 0"
> to "no explicit start" or changes from "no explicit start" to "start of 1". We
> need test cases that cover those dynamic changes, and the code needs to be
> corrected.

Fixed.

> > +        for (RenderObject* child = renderer(); child; child = child->nextInPreOrder(renderer())) {
> > +            if (child->isListItem())
> > +                toRenderListItem(child)->updateValue();
> > +        }
> 
> Instead of copying and pasting this code, I would prefer that we add a helper
> function to do this, so it can be shared by the startAttr and reversedAttr
> cases.

Fixed.

> > +    if (m_shouldRecalcItemCount) {
> > +        unsigned itemCount = 0;
> > +        for (RenderObject* child = renderer(); child; child = child->nextInPreOrder(renderer())) {
> > +            if (child->isListItem())
> > +                ++itemCount;
> > +        }
> > +
> > +        const_cast<HTMLOListElement*>(this)->m_itemCount = itemCount;
> > +     }
> 
> Rather than using const_cast, the m_itemCount member should be marked mutable.

Fixed.

> The code above does not set m_shouldRecalcItemCount to false, which is a major
> performance bug.

Oops! Fixed.

> I believe this function will not work correctly for nested lists. You need a
> test case involving nested lists, and need to correct the code to correctly
> skip nested lists.

Fixed.

> > +void HTMLOListElement::childrenChanged(bool changedByParser, Node* beforeChange, Node* afterChange, int childCountDelta)
> > +{
> > +    m_shouldRecalcItemCount = true;
> > +    HTMLElement::childrenChanged(changedByParser, beforeChange, afterChange, childCountDelta);
> > +}
> 
> Will this work properly if the list items are not immediate children of the
> <ol> element? 

Indeed, it won't.

> Do such cases exist?

Even though it is against the specification, I can imagine that such code as:

<ol>
    <li>foo</li>
    <p><li>bar</li></p>
</ol>

is used. It is supported but doesn't quite work with dynamic changes even now
(without the patch). The last test case in the
fast/lists/ol-reversed-dynamic.html is failing both with the patch as well as
without it (when the list is not reversed).

I'm not sure how to fix that. Because that basically means listening for
changes in the entire subtree or throwing the whole optimization away (which I
don't think is a particularly good idea). And I wonder if it's even worth it
(it's a broken HTML code anyway).

> > +    bool m_shouldRecalcItemCount;
> 
> Since this is used in only one or two places, I would prefer to not abbreviate
> the word "recalculate" in this name.

Fixed.

> review- because some of the problems above are serious enough that we should
> not land this as-is.
> 
> This would be much better with a cross-platform test. We should add the hooks
> to DumpRenderTree so we can get list item labels and make cross-platform plain
> text tests for list numbering.

I went for that and added a function to the LayoutTestController that returns
an array of item markers for a given list and rewrote the tests to plain text.

While it's definitely better to share the same test result across ports, in
this case the tests are kinda less readable, at least in its current shape. 

Also, the added function needs to know the logic of how list elements and item
elements interact with each other, which makes it more error-prone and if it
has bugs itself, it can hide other bugs in the implementation.

I realize though that with the current test infrastructure, that's the best
what we can do.

Please let me know if you have any suggestions on how to improve the tests.

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