Bug 92041 - changing -webkit-order should change the paint order of flex items
Summary: changing -webkit-order should change the paint order of flex items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on: 92042
Blocks: 62048
  Show dependency treegraph
 
Reported: 2012-07-23 15:55 PDT by Tony Chang
Modified: 2012-07-27 00:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.53 KB, patch)
2012-07-24 16:29 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (1.01 MB, application/zip)
2012-07-24 16:49 PDT, WebKit Review Bot
no flags Details
Patch for landing (6.50 KB, patch)
2012-07-26 13:38 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-07-23 15:55:42 PDT
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."
Comment 1 Tony Chang 2012-07-24 16:29:36 PDT
Created attachment 154169 [details]
Patch
Comment 2 Tony Chang 2012-07-24 16:30:58 PDT
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).
Comment 3 WebKit Review Bot 2012-07-24 16:49:46 PDT
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
Comment 4 WebKit Review Bot 2012-07-24 16:49:50 PDT
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
Comment 5 Ojan Vafai 2012-07-25 07:12:27 PDT
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.
Comment 6 Tony Chang 2012-07-25 15:04:08 PDT
(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.
Comment 7 Ojan Vafai 2012-07-26 08:21:41 PDT
(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.
Comment 8 Tony Chang 2012-07-26 13:38:13 PDT
Created attachment 154740 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-07-27 00:28:08 PDT
Comment on attachment 154740 [details]
Patch for landing

Clearing flags on attachment: 154740

Committed r123842: <http://trac.webkit.org/changeset/123842>
Comment 10 WebKit Review Bot 2012-07-27 00:28:12 PDT
All reviewed patches have been landed.  Closing bug.