[webkit-reviews] review denied: [Bug 65783] refactor box-ordinal-group handling so we don't timeout on large values : [Attachment 103099] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Tony Chang <tony at chromium.org>'s
request for review:
Bug 65783: refactor box-ordinal-group handling so we don't timeout on large
values
https://bugs.webkit.org/show_bug.cgi?id=65783

Attachment 103099: Patch
https://bugs.webkit.org/attachment.cgi?id=103099&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=103099&action=review


This is just way too slow. You'll sort all the kids every time you do a layout.
Keep in mind that flex-order is rarely ever going to be used.

A couple of ideas:

(1) Instead of caching only m_lastOrdinal, cache all the specified ordinals as
you encounter them when walking. Just make a little inthash.
PROS: Can just do a normal RenderObject walk when box-ordinal-group is 1.
CONS: Do one complete walk of the entire list for each value of
box-ordinal-group.
PERFORMANCE: O(n*m) where n is the # of children and m is the number of unique
box-ordinal-group values.
MEMORY: O(m) where m is the number of unique box-ordinal-group values stored in
the IntHash.

(2) Start off walking the children and only if you encounter a
box-ordinal-group value of 1 do you fault to being sorted.
PROS: Will perform better with box-ordinal-group values other than just 1.
CONS: Takes up a lot of extra memory when you only have a couple of
box-ordinal-group values (probably the common case).
PERFORMANCE: O(n) where n is the # of children.
MEMORY: O(n) where n is the number of children.

(3) Same as (2) but instead of a single sorted Vector, make an IntHash of
(box-ordinal-group) -> Vectors.
PROS: Easy to look only at a specific group.
CONS: Takes up a lot of extra memory still when you only have a couple of
box-ordinal-group values.
PERFORMANCE: O(n) where n is the number of children.
MEMORY: O(n) where n is the number of children.

Given the characteristics of flex-order, I'm inclined to favor approach #1. I
don't think doing multiple walks of the child list will be that big a deal.

> Source/WebCore/ChangeLog:10
> +	   vector and sorts them.  If box-oridinal-group is all the default

Typo. "ordinal"

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:85
> +    static bool compareFlexOrder(RenderBox* r1, RenderBox* r2)
> +    {
> +	   return r1->style()->boxOrdinalGroup() <
r2->style()->boxOrdinalGroup();
> +    }

I don't think compareFlexOrder is the right term. box-ordinal-group is about
re-ordering the items. I'd just call the function compare() :).