Bug 64038 – Pass -webkit-flex() values on to RenderStyle

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

Pass -webkit-flex() values on to RenderStyle

[Tony Chang]

Created attachment 99896 [details]
Patch

[Tony Chang]

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.

[Ojan Vafai]

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());
}

[Luke Macpherson]

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.

[Luke Macpherson]

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

[Tony Chang]

Created attachment 100359 [details]
Patch

[Tony Chang]

(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?

[Luke Macpherson]

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.

[Tony Chang]

Created attachment 100387 [details]
Patch

[Tony Chang]

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

[Tony Chang]

Trying to find a reviewer . . .

[Tony Chang]

Comment on attachment 100387 [details]
Patch

In an attempt to get this landed, I'm breaking this into smaller pieces.

[Luke Macpherson]

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

[Tony Chang]

Created attachment 101504 [details]
Patch

[Tony Chang]

Comment on attachment 101504 [details]
Patch

Other patches have landed.  Eric, care to review?

[Eric Seidel (no email)]

Comment on attachment 101504 [details]
Patch

OK.  How do we test this?

[Tony Chang]

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

[Tony Chang]

Committed r91410: <http://trac.webkit.org/changeset/91410>