Add support for <ol reversed>

• Component: Forms
• Status: RESOLVED FIXED

[Jakub Wieczorek]

The spec says:
"The reversed attribute is a boolean attribute. If present, it indicates that the list is a descending list (..., 3, 2, 1). If the attribute is omitted, the list is an ascending list (1, 2, 3, ...)."

Currently WebKit does not support this attribute.

[Jakub Wieczorek]

Created attachment 51863 [details]
patch

[Jakub Wieczorek]

I tried to write a text-only test but unfortunately such a test would not dump the list markers, which is basically the main point of the test. :(

Also, the patch might look unnecessarily complicated but I had no idea how to efficiently stay up to date with the list items without adding all those private members.

[Darin Adler]

Comment on attachment 51863 [details]
patch

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.

> +        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.

> +        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.

> +    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.

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

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.

> +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? Do such cases exist?

> +    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.

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.

[Jakub Wieczorek]

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.

[Jakub Wieczorek]

Created attachment 51968 [details]
patch

[Jakub Wieczorek]

Created attachment 51970 [details]
patch

Rebased against ToT.

[Darin Adler]

(In reply to comment #4)
> 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).

We need correct behavior even with "broken" HTML. Dynamic changes should not give different results than the same markup in non-dynamic form. I think we do need to do the equivalent of listening to changes in the entire subtree. Maybe we can find an efficient way to do it. I suspect we can.

> 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.

I was thinking instead that the function would take a node and return the list item marker text for the renderer corresponding to that node, or null if there is no list item.

> 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.

Maybe I can help you improve the tests. I was thinking we could use the function I had in mind and a little JavaScript code to generate a nice, readable outline in plain text.

> 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.

That's surprising. I suggest we find a way to do it and use the RenderListItem::markerText() function.

[Jakub Wieczorek]

> We need correct behavior even with "broken" HTML. Dynamic changes should not
> give different results than the same markup in non-dynamic form. I think we do
> need to do the equivalent of listening to changes in the entire subtree. Maybe
> we can find an efficient way to do it. I suspect we can.

Any specific idea? :)

> I was thinking instead that the function would take a node and return the list
> item marker text for the renderer corresponding to that node, or null if there
> is no list item.

> Maybe I can help you improve the tests. I was thinking we could use the
> function I had in mind and a little JavaScript code to generate a nice,
> readable outline in plain text.

Yes, that seems to make much more sense. I'll look into it.

> > 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.
> 
> That's surprising. I suggest we find a way to do it and use the
> RenderListItem::markerText() function.

Yes. With your approach, my comment would become outdated anyway though.

[Darin Adler]

(In reply to comment #8)
> > We need correct behavior even with "broken" HTML. Dynamic changes should not
> > give different results than the same markup in non-dynamic form. I think we do
> > need to do the equivalent of listening to changes in the entire subtree. Maybe
> > we can find an efficient way to do it. I suspect we can.
> 
> Any specific idea? :)

You could take a look at the mechanisms that NodeList uses to keep its lists up to date. The requirements of a TagNodeList are quite similar to what a list would need to do to keep track of its list items. The functions notifyNodeListsAttributeChanged and notifyNodeListsChildrenChanged are part of what makes node lists work.

We may need to do more of this on the render tree side instead of the DOM tree. An element with "display: list-item" contributes to list numbering too. In fact, I think that may be something wrong with the current code and patch. We need some test cases that use CSS to create list items or to turn list items into non-list-item content.

[Jakub Wieczorek]

Comment on attachment 51970 [details]
patch

> You could take a look at the mechanisms that NodeList uses to keep its lists up
> to date. The requirements of a TagNodeList are quite similar to what a list
> would need to do to keep track of its list items. The functions
> notifyNodeListsAttributeChanged and notifyNodeListsChildrenChanged are part of
> what makes node lists work.
>
> We may need to do more of this on the render tree side instead of the DOM tree.
> An element with "display: list-item" contributes to list numbering too. In
> fact, I think that may be something wrong with the current code and patch. We
> need some test cases that use CSS to create list items or to turn list items
> into non-list-item content.

Great, thanks for the hints!

Clearing the review flag for now.

[Jakub Wieczorek]

Created attachment 54798 [details]
patch

[Darin Adler]

Comment on attachment 54798 [details]
patch

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.

[Jakub Wieczorek]

Thanks for the review. I'm going to upload a new patch shortly.

(In reply to comment #12)
> (From update of attachment 54798 [details])
> 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.

updateItemValues() does not renumber the markers. In fact it does exactly what you suggest it should do - it marks the items to be renumbered but the actual computation takes place at the next layout:

void RenderListItem::updateValue()
{
    if (!m_hasExplicitValue) {
        m_isValueUpToDate = false;
        if (m_marker)
            m_marker->setNeedsLayoutAndPrefWidthsRecalc();
    }
}

> > +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.

Yes.

"The default value, used if the attribute is missing or if the value cannot be converted to a number according to the referenced algorithm, is 1 if the element has no reversed attribute, and is the number of child li elements otherwise."

"The start IDL attribute must reflect the value of the start content attribute."

> > +    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.

Yeah, I agree with that. I'm going to refactor it into a RenderList class that will serve as a renderer for lists.

> 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.

[Darin Adler]

(In reply to comment #13)
> "The default value, used if the attribute is missing or if the value cannot be
> converted to a number according to the referenced algorithm, is 1 if the
> element has no reversed attribute, and is the number of child li elements
> otherwise."

This start attribute is an example, then, of something in the DOM API that requires that the render tree be up to date to give a correct response. Thus I think we'll need a call to updateLayout for the same reason that functions like Element::offsetHeight do.

Except that it specifically says the "number of child li elements", which ignores the CSS "display: list-item".

If the text is really intentional about stating the number of child li elements, then we should remove all the code and smarts that use the render tree and create test cases that demonstrate the incorrect looking numbering you get when there are "display: list-item" items that are not li elements.

> > 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.
> 
> Yeah, I agree with that. I'm going to refactor it into a RenderList class that
> will serve as a renderer for lists.

Now I’m a little worried about my suggestion. If there is not already a RenderList class, then allocating a renderer of that class instead of the renderer type that we are already creating might be a problem for lists that have CSS "display: inline" or something like that applied to them.

[Jakub Wieczorek]

(In reply to comment #14)
> (In reply to comment #13)
> > "The default value, used if the attribute is missing or if the value cannot be
> > converted to a number according to the referenced algorithm, is 1 if the
> > element has no reversed attribute, and is the number of child li elements
> > otherwise."
> 
> This start attribute is an example, then, of something in the DOM API that
> requires that the render tree be up to date to give a correct response. Thus I
> think we'll need a call to updateLayout for the same reason that functions like
> Element::offsetHeight do.
> 
> Except that it specifically says the "number of child li elements", which
> ignores the CSS "display: list-item".
> 
> If the text is really intentional about stating the number of child li
> elements, then we should remove all the code and smarts that use the render
> tree and create test cases that demonstrate the incorrect looking numbering you
> get when there are "display: list-item" items that are not li elements.

Oh, I overlooked it talking about li elements rather than just list items.

If the text is really intentional, then I think it's just wrong. At least it seems to be in conflict with CSS in this matter. I am not convinced we should follow this particular recommendation.

> > > 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.
> > 
> > Yeah, I agree with that. I'm going to refactor it into a RenderList class that
> > will serve as a renderer for lists.
> 
> Now I’m a little worried about my suggestion. If there is not already a
> RenderList class, then allocating a renderer of that class instead of the
> renderer type that we are already creating might be a problem for lists that
> have CSS "display: inline" or something like that applied to them.

Yes, I think you're right.

From what I can see, that would mean we can't really move this logic to the rendering tree. A list on the rendering side can be virtually anything whereas on the DOM side we exactly know that the only place where we want to keep track of how many list items a list contains is HTMLOListElement - this might change in the future as the need arises but I think that is true now.

Therefore I suggest we get back to the previous approach. The second major change you suggested - putting the renumbering logic in one place should be still doable with a static function in RenderListItem (I agree, not a perfect place for this).

[Ian 'Hixie' Hickson]

Not depending on CSS is intentional. Doing so would be a layering violation. What the spec is defining is just the ordinal value of the list items; it doesn't require that this has anything to do with the rendering. All the spec says is that by default, user agents are expected to use the ordinal value to render the list item of <li>s in <ol>s; that's just a suggestion, basically (though one we should follow).

It wouldn't make sense to define the ordinal value in terms of the CSS; there might not be any CSS. CSS is an optional technology not supported by all HTML user agents.

[Jakub Wieczorek]

(In reply to comment #16)
> Not depending on CSS is intentional. Doing so would be a layering violation. 

Right. Maybe I didn't express my thoughts clearly. I was not trying to say that it should depend on CSS but simply define this matter in a way that would not interfere with CSS. But now I see that's not the point of that definition.

> What the spec is defining is just the ordinal value of the list items; it doesn't require that this has anything to do with the rendering. All the spec says is that by default, user agents are expected to use the ordinal value to render the list item of <li>s in <ol>s;

OK, that makes sense. 

> that's just a suggestion, basically (though one we should follow).

Well, it can't be followed because of CSS.

It appears that the correct solution here would be to keep track of list item count in terms of HTML as well as CSS. The former for the sake of the DOM interface and the latter for rendering.

Thanks for the clarification.

[Ian 'Hixie' Hickson]

> Well, it can't be followed because of CSS.

Why not?

[Jakub Wieczorek]

(In reply to comment #18)
> > Well, it can't be followed because of CSS.
> 
> Why not?

Because the semantic value of a list item may not match the rendered one when e.g. CSSish list-items come into play.

[Ian 'Hixie' Hickson]

Certainly CSS can override the default rendering, but I don't see why the default rendering can't be what HTML describes.

[Alexis Menard (darktears)]

Created attachment 119049 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #21)
> Created an attachment (id=119049) [details]
> Patch

I'm trying to keep that bug going on. I rebased the patch, fixed some feedback from Darin. I also preserved the original author Jakub Wieczorek, just added my name as I modified the original code.

Now we can continue so we can land this feature hopefully :D...I can do the changes if Jakub is busy/away as he seems to be.

[WebKit Review Bot]

Comment on attachment 119049 [details]
Patch

Attachment 119049 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10848490

[Darin Adler]

Comment on attachment 119049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119049&action=review

> Source/WebCore/html/HTMLOListElement.cpp:106
> +void HTMLOListElement::setIsReversed(bool reversed)
> +{
> +    setAttribute(reversedAttr, reversed ? "" : 0);
> +}

Why is this function needed? I don’t see any calls to it.

> Source/WebCore/html/HTMLOListElement.cpp:114
> +void HTMLOListElement::updateItemValues()
> +{
> +    for (RenderObject* child = renderer(); child; child = child->nextInPreOrder(renderer())) {
> +        if (child->isListItem())
> +            toRenderListItem(child)->updateValue();
> +    }
> +}

Is it really necessary to walk the entire tree under the <ol>? It seems unfortunate that this will ask to update all the values in all lists nested inside this one. Maybe this should have the <ul>/<ol> check that RenderListItem::itemCount has. Perhaps we should package the iteration in a helper function that these two functions can share, since both want to do something for each list item in a list and skip other renderers.

It sort of seems like nextListItem may be that function!

> Source/WebCore/rendering/RenderListItem.cpp:104
> +// FIXME: We keep the numbering code close but this should really be in a representation of the list in the render tree.

What does “we keep the numbering code close” mean?

[Darin Adler]

I did not have time to complete a review yet. Those are just the comments from the first part of reviewing the patch.

[Alexis Menard (darktears)]

Created attachment 119213 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 119214 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #27)
> Created an attachment (id=119214) [details]
> Patch

Second version should fix the Win EWS error.

[WebKit Review Bot]

Comment on attachment 119214 [details]
Patch

Attachment 119214 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10873251

New failing tests:
editing/execCommand/switch-list-type-with-orphaned-li.html
editing/execCommand/5575101-1.html
editing/execCommand/insert-list-nested-with-orphaned.html
editing/execCommand/5575101-2.html
editing/execCommand/break-out-of-empty-list-item.html

[WebKit Review Bot]

Comment on attachment 119214 [details]
Patch

Attachment 119214 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10830312

New failing tests:
editing/execCommand/switch-list-type-with-orphaned-li.html
editing/execCommand/5575101-1.html
editing/execCommand/insert-list-nested-with-orphaned.html
editing/execCommand/5575101-2.html
editing/execCommand/break-out-of-empty-list-item.html

[Alexis Menard (darktears)]

Created attachment 119270 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #31)
> Created an attachment (id=119270) [details]
> Patch

It should fix the regressions.

[Benjamin Poulain]

Maybe add one test of XHTML in addition to the HTML tests? Boolean attributes are invalid in XHML.

[Benjamin Poulain]

Comment on attachment 119270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119270&action=review

> Source/WebCore/html/HTMLOListElement.cpp:85
> +        bool hasExplicitStart;
> +        int start = attr->value().toInt(&hasExplicitStart);
> +        if (start == m_start && hasExplicitStart == m_hasExplicitStart)
>              return;
>          m_start = start;

This might not be incorrect but I think it is a bit hard to follow.
If you had m_hasExplicitStart == true, and you cannot parse the new value, you assign m_start = start. The value "start" being from something you cannot parse.

I think the name "hasExplicitStart" is missleading in  int start = attr->value().toInt(&hasExplicitStart);

> Source/WebCore/html/HTMLOListElement.h:54
> +    mutable unsigned m_itemCount;

unsigned -> size_t

> Source/WebCore/html/HTMLOListElement.h:55
>      int m_start;
> +    bool m_hasExplicitStart;
> +    bool m_isReversed;
> +
> +    mutable unsigned m_itemCount;
> +    mutable bool m_shouldRecalculateItemCount;

A bit field would be nice to avoid using too much space for those bool.

[Benjamin Poulain]

Comment on attachment 119270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119270&action=review

> Source/WebCore/html/HTMLOListElement.cpp:85
> +        bool hasExplicitStart;
> +        int start = attr->value().toInt(&hasExplicitStart);
> +        if (start == m_start && hasExplicitStart == m_hasExplicitStart)
>              return;
>          m_start = start;

This might not be incorrect but I think it is a bit hard to follow.
If you had m_hasExplicitStart == true, and you cannot parse the new value, you assign m_start = start. The value "start" being from something you cannot parse.

I think the name "hasExplicitStart" is missleading in  int start = attr->value().toInt(&hasExplicitStart);

> Source/WebCore/html/HTMLOListElement.h:54
> +    mutable unsigned m_itemCount;

unsigned -> size_t

> Source/WebCore/html/HTMLOListElement.h:55
>      int m_start;
> +    bool m_hasExplicitStart;
> +    bool m_isReversed;
> +
> +    mutable unsigned m_itemCount;
> +    mutable bool m_shouldRecalculateItemCount;

A bit field would be nice to avoid using too much space for those bool.

[Alexis Menard (darktears)]

Created attachment 119281 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #36)
> Created an attachment (id=119281) [details]
> Patch

Took into account the comment of Benjamin.

[Darin Adler]

Comment on attachment 119270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119270&action=review

>>> Source/WebCore/html/HTMLOListElement.h:54
>>> +    mutable unsigned m_itemCount;
>> 
>> unsigned -> size_t
> 
> unsigned -> size_t

I don’t think size_t is a good idea for this. It will make the object bigger in 64-bit-compiled WebKit and have no real-world benefit.

[Alexis Menard (darktears)]

Created attachment 119414 [details]
Patch

[Andreas Kling]

(In reply to comment #38)
> (From update of attachment 119270 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119270&action=review
> 
> >>> Source/WebCore/html/HTMLOListElement.h:54
> >>> +    mutable unsigned m_itemCount;
> >> 
> >> unsigned -> size_t
> > 
> > unsigned -> size_t
> 
> I don’t think size_t is a good idea for this. It will make the object bigger in 64-bit-compiled WebKit and have no real-world benefit.

Indeed. I think we should prefer 'unsigned' for any "web-facing" numbers, since that keeps behavior consistent between 32 and 64-bit builds of WebKit.

[Darin Adler]

Comment on attachment 119414 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119414&action=review

review- because of the mistake in HTMLOListElement::parseMappedAttribute. Otherwise looking good.

> Source/WebCore/html/HTMLOListElement.cpp:90
> -        if (!canParse)
> -            start = 1;
> +        if (!canParse) {
> +            m_start = 1;
> +            m_hasExplicitStart = false;
> +        }
>          if (start == m_start)
>              return;
>          m_start = start;
> -        for (RenderObject* child = renderer(); child; child = child->nextInPreOrder(renderer())) {
> -            if (child->isListItem())
> -                toRenderListItem(child)->updateValue();
> -        }
> +        m_hasExplicitStart = canParse;

The code in the current patch will not update item values when the start changes from "1" to "" on a reversed list, but we need to update item values in that case. May even need a test case.

I think the right way to write this is as follows:

    int oldStart = start();
    bool canParse;
    int parsedStart = attr->value().toInt(&canParse);
    m_hasExplicitStart = canParse;
    m_start = canParse ? parsedStart : 0xBADBEEF;
    if (oldStart == start())
        return;
    updateItemValues();

Or we could use the value 1 or 0 or 10000 rather than 0xBADBEEF.

> Source/WebCore/html/HTMLOListElement.cpp:113
> +    RenderListItem* listItem = RenderListItem::nextListItem(renderer());
> +    while (listItem) {
> +        listItem->updateValue();
> +        listItem = RenderListItem::nextListItem(renderer(), listItem);
> +    }

This has the structure of a for loop and I would like to write it that way; sadly the function names are long so the for statement will be long.

> Source/WebCore/html/HTMLOListElement.cpp:119
> +unsigned HTMLOListElement::itemCount() const
> +{
> +    if (!m_shouldRecalculateItemCount)
> +        return m_itemCount;

A cool way to write a function like this is to have an inline part:

    if (m_shouldRecalculateItemCount)
        recalculateItemCount();
    return m_itemCount;

And then a non-inline recalculateItemCount function. I also think it makes the code read better.

> Source/WebCore/html/HTMLOListElement.cpp:127
> +    RenderListItem* listItem = RenderListItem::nextListItem(renderer());
> +    while (listItem) {
> +        m_itemCount++;
> +        listItem = RenderListItem::nextListItem(renderer(), listItem);
> +    }

Ditto.

> Source/WebCore/html/HTMLOListElement.cpp:137
> +void HTMLOListElement::itemCountChanged()
> +{
> +    m_shouldRecalculateItemCount = true;
> +}

Should be inlined.

> Source/WebCore/rendering/RenderListItem.cpp:468
> +    RenderListItem* item = isListReversed ? previousListItem(list, this) : nextListItem(list, this);
> +    while (item) {

Could initialize item to list and then put the expression inside the while. That would avoid repeating the isListReversed logic.

Or could write as a for loop if you made a helper function that does previous/nextListItem to avoid writing out the ?: each time.

[Alexis Menard (darktears)]

Created attachment 119485 [details]
Patch

[Darin Adler]

Comment on attachment 119485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119485&action=review

> Source/WebCore/html/HTMLOListElement.h:50
> +            const_cast<HTMLOListElement*>(this)->recalculateItemCount();

Why did you switch to const_cast rather than mutable? I thought earlier versions of the patch used mutable instead.

> Source/WebCore/rendering/RenderListItem.cpp:122
> +    RenderObject* renderer = item ? item->nextInPreOrder(list) : list->nextInPreOrder(list);
> +
> +    while (renderer) {
> +        if (renderer->node() && isList(renderer->node())) {
> +            // We've found a nested, independent list: nothing to do here.
> +            renderer = renderer->nextInPreOrderAfterChildren(list);
> +            continue;
> +        }
> +
> +        if (renderer->isListItem())
> +            return toRenderListItem(renderer);
> +
> +        renderer = renderer->nextInPreOrder(list);
> +    }

We can tighten up this loop:

    RenderObject* renderer = item ? item : list;
    while ((renderer = renderer->nextInPreOrder(list)))

I think that reads better than repeating the nextInPreOrder before and at the end of the loop.

> Source/WebCore/rendering/RenderListItem.h:52
> +    static RenderListItem* nextListItem(RenderObject*, const RenderListItem* = 0);

The first argument here needs an argument name. It’s not at all obvious that it’s the list’s renderer. It would be easy to make the mistake of passing a RenderListItem instead.

[Alexis Menard (darktears)]

Comment on attachment 119485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119485&action=review

Thanks darin for the review. I'll update the patch and will do a safe landing tomorrow when I arrive in the office. In the meantime I clear the cq+ if you don't have any objections (let's land directly the good version).

>> Source/WebCore/html/HTMLOListElement.h:50
>> +            const_cast<HTMLOListElement*>(this)->recalculateItemCount();
> 
> Why did you switch to const_cast rather than mutable? I thought earlier versions of the patch used mutable instead.

Just because I thought that recalculateItemCount() const would not look nice. You expect that function to modify the object. I have no strong opinion.

>> Source/WebCore/rendering/RenderListItem.cpp:122
>> +    }
> 
> We can tighten up this loop:
> 
>     RenderObject* renderer = item ? item : list;
>     while ((renderer = renderer->nextInPreOrder(list)))
> 
> I think that reads better than repeating the nextInPreOrder before and at the end of the loop.

Will fix it when landing.

>> Source/WebCore/rendering/RenderListItem.h:52
>> +    static RenderListItem* nextListItem(RenderObject*, const RenderListItem* = 0);
> 
> The first argument here needs an argument name. It’s not at all obvious that it’s the list’s renderer. It would be easy to make the mistake of passing a RenderListItem instead.

Good point.

[Darin Adler]

Comment on attachment 119485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119485&action=review

> Source/WebCore/html/HTMLOListElement.cpp:85
> +        m_start = canParse ? parsedStart : 0xBADBEEF;

I’m feeling a little remorse over my 0xBADBEEF suggestion here. If we set it to 0xBADBEEF here, we should probably set it to 0xBADBEEF in the constructor too. It’s hard to decide about things that truly don’t matter!

>>> Source/WebCore/html/HTMLOListElement.h:50
>>> +            const_cast<HTMLOListElement*>(this)->recalculateItemCount();
>> 
>> Why did you switch to const_cast rather than mutable? I thought earlier versions of the patch used mutable instead.
> 
> Just because I thought that recalculateItemCount() const would not look nice. You expect that function to modify the object. I have no strong opinion.

I think I like the way you did it. Please don’t change back to mutable.

[Alexis Menard (darktears)]

Created attachment 119596 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 119596 [details]
Patch for landing

Rejecting attachment 119596 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/10895711

[Alexis Menard (darktears)]

Created attachment 119598 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 119598 [details]
Patch for landing

Attachment 119598 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10895728

New failing tests:
http/tests/inspector/extensions-headers.html
http/tests/inspector/extensions-useragent.html
http/tests/inspector/extensions-ignore-cache.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-cd.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector/console-websocket-error.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/compiler-source-mapping.html
http/tests/inspector-enabled/database-open.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector/change-iframe-src.html

[WebKit Review Bot]

Created attachment 119601 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexis Menard (darktears)]

(In reply to comment #49)
> (From update of attachment 119598 [details])
> Attachment 119598 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10895728
> 
> New failing tests:
> http/tests/inspector/extensions-headers.html
> http/tests/inspector/extensions-useragent.html
> http/tests/inspector/extensions-ignore-cache.html
> http/tests/inspector/console-xhr-logging.html
> http/tests/inspector/console-cd.html
> http/tests/inspector/console-cd-completions.html
> http/tests/inspector/resource-har-headers.html
> http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html
> http/tests/inspector/extensions-network-redirect.html
> http/tests/inspector/console-websocket-error.html
> http/tests/inspector/inspect-iframe-from-different-domain.html
> http/tests/inspector/network-preflight-options.html
> http/tests/inspector/compiler-source-mapping.html
> http/tests/inspector-enabled/database-open.html
> http/tests/inspector/compiler-source-mapping-debug.html
> http/tests/inspector/resource-har-conversion.html
> http/tests/inspector/console-resource-errors.html
> http/tests/inspector/resource-parameters.html
> http/tests/inspector-enabled/console-log-before-frame-navigation.html
> http/tests/inspector/change-iframe-src.html

It seems unrelated to my change. The Qt port mark some of them as flaky btw.

[Alexis Menard (darktears)]

Committed r103062: <http://trac.webkit.org/changeset/103062>

[Csaba Osztrogonác]

No, it isn't unrelated. Chromium EWS said you will break them. And you did it ... Check the Qt bot. Could you roll it out?

[Alexis Menard (darktears)]

Created attachment 119610 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 119610 [details]
Patch for landing

Clearing flags on attachment: 119610

Committed r103074: <http://trac.webkit.org/changeset/103074>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexis Menard (darktears)]

(In reply to comment #56)
> All reviewed patches have been landed.  Closing bug.

Sorry for the bumpy landing.