Bug 92041 – changing -webkit-order should change the paint order of flex items

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tony Chang]

http://dev.w3.org/csswg/css3-flexbox/#order-property says:
"A flex container will lay out its content starting from the lowest numbered ordinal group and going up. Items with the same ordinal group are laid out in the order they appear in the source document. This also affects the painting order [CSS21], exactly as if the elements were reordered in the document."

[Tony Chang]

Created attachment 154169 [details]
Patch

[Tony Chang]

In a follow up change, we should stop passing around OrderIterator as a parameter.

No hurry on this.  I want to get some more perf data about bug 92042 first (so far it looks fine, but a few more cycles won't hurt).

[WebKit Review Bot]

Comment on attachment 154169 [details]
Patch

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

New failing tests:
http/tests/security/script-crossorigin-loads-correctly.html

[WebKit Review Bot]

Created attachment 154174 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Ojan Vafai]

Comment on attachment 154169 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:262
> +    m_orderIterator = adoptPtr(new OrderIterator(this, orderValues));

Do we really need the member variable? All this saves is the vector copy + sort in paintChildren, which will typically be next to 0 cost.

[Tony Chang]

(In reply to comment #5)
> (From update of attachment 154169 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154169&action=review
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:262
> > +    m_orderIterator = adoptPtr(new OrderIterator(this, orderValues));
> 
> Do we really need the member variable? All this saves is the vector copy + sort in paintChildren, which will typically be next to 0 cost.

If we don't save the vector orderValues, we would have to walk all the flexitems to figure out what the possible order: values are.  We could also just save the vector, but that didn't seem significantly better than just saving the iterator.

[Ojan Vafai]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 154169 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=154169&action=review
> > 
> > > Source/WebCore/rendering/RenderFlexibleBox.cpp:262
> > > +    m_orderIterator = adoptPtr(new OrderIterator(this, orderValues));
> > 
> > Do we really need the member variable? All this saves is the vector copy + sort in paintChildren, which will typically be next to 0 cost.
> 
> If we don't save the vector orderValues, we would have to walk all the flexitems to figure out what the possible order: values are.  We could also just save the vector, but that didn't seem significantly better than just saving the iterator.

Oh right. Makes sense.

[Tony Chang]

Created attachment 154740 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 154740 [details]
Patch for landing

Clearing flags on attachment: 154740

Committed r123842: <http://trac.webkit.org/changeset/123842>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.