Bug 64038 - Pass -webkit-flex() values on to RenderStyle
Summary: Pass -webkit-flex() values on to RenderStyle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on: 64765
Blocks: 62048
  Show dependency treegraph
 
Reported: 2011-07-06 15:35 PDT by Tony Chang
Modified: 2011-07-20 15:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch (33.42 KB, patch)
2011-07-06 15:39 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (35.13 KB, patch)
2011-07-11 14:00 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (34.94 KB, patch)
2011-07-11 17:23 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (4.87 KB, patch)
2011-07-20 13:39 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 2011-07-06 15:35:12 PDT
Pass -webkit-flex() values on to RenderStyle
Comment 1 Tony Chang 2011-07-06 15:39:52 PDT
Created attachment 99896 [details]
Patch
Comment 2 Tony Chang 2011-07-07 11:15:39 PDT
Luke: Can you take a look at the changes to CSSStyleApplyProperty.cpp and see if that's what you had in mind?  I added 2 params to the template.
Comment 3 Ojan Vafai 2011-07-07 16:39:35 PDT
Comment on attachment 99896 [details]
Patch

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

Seems fine to me. Would be good to have someone with more experience with this code to r+ though.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:247
> +            if (canBeFlexWidth && value->isFlexValue()) {
> +                CSSFlexValue* flexValue = static_cast<CSSFlexValue*>(value);
> +                selector->style()->setFlexboxWidthPositiveFlex(flexValue->positiveFlex());
> +                selector->style()->setFlexboxWidthNegativeFlex(flexValue->negativeFlex());
> +                applyValue(selector, flexValue->preferredSize());
> +            } else if (canBeFlexHeight && value->isFlexValue()) {
> +                CSSFlexValue* flexValue = static_cast<CSSFlexValue*>(value);
> +                selector->style()->setFlexboxHeightPositiveFlex(flexValue->positiveFlex());
> +                selector->style()->setFlexboxHeightNegativeFlex(flexValue->negativeFlex());
> +                applyValue(selector, flexValue->preferredSize());
> +            }

Possible cleanup nit:
if (!canBeFlexWidth && !canBeFlexHeight)
    return;

if (!value->isFlexValue())
    return;

CSSFlexValue* flexValue = static_cast<CSSFlexValue*>(value);
applyValue(selector, flexValue->preferredSize());

if (canBeFlexWidth) {
    selector->style()->setFlexboxWidthPositiveFlex(flexValue->positiveFlex());
    selector->style()->setFlexboxWidthNegativeFlex(flexValue->negativeFlex());
} else if (canBeFlexHeight) {
    selector->style()->setFlexboxHeightPositiveFlex(flexValue->positiveFlex());
    selector->style()->setFlexboxHeightNegativeFlex(flexValue->negativeFlex());
}
Comment 4 Luke Macpherson 2011-07-10 23:15:50 PDT
Comment on attachment 99896 [details]
Patch

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

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:247

> 
> Possible cleanup nit:
> if (!canBeFlexWidth && !canBeFlexHeight)
>     return;
> 
> if (!value->isFlexValue())
>     return;
> 
> CSSFlexValue* flexValue = static_cast<CSSFlexValue*>(value);
> applyValue(selector, flexValue->preferredSize());
> 
> if (canBeFlexWidth) {
>     selector->style()->setFlexboxWidthPositiveFlex(flexValue->positiveFlex());
>     selector->style()->setFlexboxWidthNegativeFlex(flexValue->negativeFlex());
> } else if (canBeFlexHeight) {
>     selector->style()->setFlexboxHeightPositiveFlex(flexValue->positiveFlex());
>     selector->style()->setFlexboxHeightNegativeFlex(flexValue->negativeFlex());
> }

Now that applyValue can recurse, is there any guarantee that preferredSize is not also a flex width or a bound on the depth of recursion?

> Source/WebCore/css/CSSStyleApplyProperty.cpp:653
> +    setPropertyHandler(CSSPropertyWidth, new ApplyPropertyLength<AutoEnabled, IntrinsicEnabled, MinIntrinsicEnabled, NoneDisabled, UndefinedDisabled, FlexWidthEnabled>(&RenderStyle::width, &RenderStyle::setWidth, &RenderStyle::initialSize));

Minor point - if you reorder the new template parameters you'll shorten these slightly by using the disabled-by-default pattern.
Comment 5 Luke Macpherson 2011-07-10 23:17:38 PDT
(In reply to comment #2)
> Luke: Can you take a look at the changes to CSSStyleApplyProperty.cpp and see if that's what you had in mind?  I added 2 params to the template.

Sure, this area is basically LGTM. A few minor suggestions, but I'd still r+ it if I were the reviewer.
Comment 6 Tony Chang 2011-07-11 14:00:55 PDT
Created attachment 100359 [details]
Patch
Comment 7 Tony Chang 2011-07-11 14:05:21 PDT
(In reply to comment #4)
> (From update of attachment 99896 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99896&action=review
> 
> >> Source/WebCore/css/CSSStyleApplyProperty.cpp:247
> 
> Now that applyValue can recurse, is there any guarantee that preferredSize is not also a flex width or a bound on the depth of recursion?

Good question.  I changed preferredSize() to be a CSSPrimitiveValue to make it more clear that this will only recurse once.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:653
> > +    setPropertyHandler(CSSPropertyWidth, new ApplyPropertyLength<AutoEnabled, IntrinsicEnabled, MinIntrinsicEnabled, NoneDisabled, UndefinedDisabled, FlexWidthEnabled>(&RenderStyle::width, &RenderStyle::setWidth, &RenderStyle::initialSize));
> 
> Minor point - if you reorder the new template parameters you'll shorten these slightly by using the disabled-by-default pattern.

Do you mean to put LengthFlex* before LengthNone and LengthUndefined in the template?  If I do that, I have to add extra params to CSSPropertyMaxHeight and CSSPropertyMaxWidth, no?

I also merged Ojan's suggested code.  Maybe Eric or Hyatt can take a look or suggest someone who would be a good reviewer?
Comment 8 Luke Macpherson 2011-07-11 17:03:13 PDT
Comment on attachment 100359 [details]
Patch

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

> Source/WebCore/css/CSSStyleApplyProperty.cpp:217
> +enum LengthFlexHeight {FlexHeightDisabled = 0, FlexHeightEnabled = 1};

Since these two are mutually exclusive, it might make sense to use a single enum instead. Eg. FlexDirection {FlexDisabled = 0, FlexWidth, FlexHeight};

> Source/WebCore/css/CSSStyleApplyProperty.cpp:243
> +            applyValue(selector, flexValue->preferredSize());

Now that preferredSize returns a primitive value you can just set primitiveValue and fall through, avoiding the recursion entirely.
Comment 9 Tony Chang 2011-07-11 17:23:52 PDT
Created attachment 100387 [details]
Patch
Comment 10 Tony Chang 2011-07-11 17:24:56 PDT
(In reply to comment #8)
> (From update of attachment 100359 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100359&action=review
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:217
> > +enum LengthFlexHeight {FlexHeightDisabled = 0, FlexHeightEnabled = 1};
> 
> Since these two are mutually exclusive, it might make sense to use a single enum instead. Eg. FlexDirection {FlexDisabled = 0, FlexWidth, FlexHeight};

Good idea, done.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:243
> > +            applyValue(selector, flexValue->preferredSize());
> 
> Now that preferredSize returns a primitive value you can just set primitiveValue and fall through, avoiding the recursion entirely.

Done, although it makes the !ENABLED(CSS3_FLEXBOX) case a bit tricky, so I just pulled it out.  It'll simplify once we're done with flexbox and can remove the #if.
Comment 11 Tony Chang 2011-07-15 11:48:54 PDT
Trying to find a reviewer . . .
Comment 12 Tony Chang 2011-07-18 16:25:32 PDT
Comment on attachment 100387 [details]
Patch

In an attempt to get this landed, I'm breaking this into smaller pieces.
Comment 13 Luke Macpherson 2011-07-18 17:20:08 PDT
(In reply to comment #12)
> (From update of attachment 100387 [details])
> In an attempt to get this landed, I'm breaking this into smaller pieces.

So sad that it has come to that.
Comment 14 Tony Chang 2011-07-20 13:39:23 PDT
Created attachment 101504 [details]
Patch
Comment 15 Tony Chang 2011-07-20 13:39:51 PDT
Comment on attachment 101504 [details]
Patch

Other patches have landed.  Eric, care to review?
Comment 16 Eric Seidel (no email) 2011-07-20 13:40:59 PDT
Comment on attachment 101504 [details]
Patch

OK.  How do we test this?
Comment 17 Tony Chang 2011-07-20 13:44:08 PDT
(In reply to comment #16)
> (From update of attachment 101504 [details])
> OK.  How do we test this?

It'll get tested by tests that are added with RenderFlexibleBox.  If we're not copying the values from css to the render style properly, we won't be able to flex properly.
Comment 18 Tony Chang 2011-07-20 15:00:39 PDT
Committed r91410: <http://trac.webkit.org/changeset/91410>