Bug 230755 – [css-flexbox] Improve & simplify the flex-basis computation

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

[Sergio Villar Senin]

[css-flexbox] Improve & simplify the flex-basis computation

[Sergio Villar Senin]

Created attachment 439157 [details]
Patch

[Sergio Villar Senin]

Gentle ping for reviewers

[Radar WebKit Bug Importer]

<rdar://problem/83769781>

[Rob Buis]

Comment on attachment 439157 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1016
> +            m_child.mutableStyle().setLogicalWidth(Length(m_originalMainAxisLength));

Temp Length not needed.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1018
> +            m_child.mutableStyle().setLogicalHeight(Length(m_originalMainAxisLength));

Ditto.

[Sergio Villar Senin]

Any other comment regarding this patch?

[Sergio Villar Senin]

(In reply to Rob Buis from comment #4)
> Comment on attachment 439157 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439157&action=review
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:1016
> > +            m_child.mutableStyle().setLogicalWidth(Length(m_originalMainAxisLength));
> 
> Temp Length not needed.

You actually need that or a WTFMove

[Rob Buis]

Comment on attachment 439157 [details]
Patch

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

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1016
>>> +            m_child.mutableStyle().setLogicalWidth(Length(m_originalMainAxisLength));
>> 
>> Temp Length not needed.
> 
> You actually need that or a WTFMove

Ah I see! WTFMove would be more efficient here, right?

[Manuel Rego Casasnovas]

Comment on attachment 439157 [details]
Patch

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

Nice patch, I have some comments and questions inline.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:999
> +class ScopedFlexBasisAsMainSize {

Wow, this is quite a hack I guess we don't have a better alternative though. :-(

I'm not really sure about the class name, would be better ScopedFlexBasisAsChildSize? Or something else, not sure...

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1007
> +            m_child.mutableStyle().setLogicalWidth(Length(flexBasis));

Does this works fine if the element has something like "max-width: 100px;" or "min-width: 1000px;".
Not sure if preferred widths are recomputed or not after this call.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1023
> +    Length m_originalMainAxisLength;

Why not just "m_originalSize" or "m_originalChildSize"?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1040
> +    // 9.3.2 E. Otherwise, size the item into the available space using its used flex basis in place of its main size.

I miss C) and D) before getting to E). Maybe we could add a comment.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1047
> +        if (childHasIntrinsicMainAxisSize(child))
> +            child.setNeedsLayout(MarkOnlyThis);

Why we need this?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1048
> +        child.layoutIfNeeded();

Ditto about this like, do we need this if we haven't gone through the previous if statement?

[Sergio Villar Senin]

Comment on attachment 439157 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1007
>> +            m_child.mutableStyle().setLogicalWidth(Length(flexBasis));
> 
> Does this works fine if the element has something like "max-width: 100px;" or "min-width: 1000px;".
> Not sure if preferred widths are recomputed or not after this call.

This is a nice question. When computing the flex-base size the min|max constrains should not apply. We are not doing that at the moment, however that is properly addressed in https://bugs.webkit.org/show_bug.cgi?id=225590. That bug landed and caused some regression, I have a local patch that implements https://bugs.webkit.org/show_bug.cgi?id=225590 on top of this patch and that does not regress anything.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1023
>> +    Length m_originalMainAxisLength;
> 
> Why not just "m_originalSize" or "m_originalChildSize"?

OK, I don't have a strong opinion here.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1040
>> +    // 9.3.2 E. Otherwise, size the item into the available space using its used flex basis in place of its main size.
> 
> I miss C) and D) before getting to E). Maybe we could add a comment.

They've not been implemented yet.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1047
>> +            child.setNeedsLayout(MarkOnlyThis);
> 
> Why we need this?

We were already doing this in the code that was removed from constructFlexItem(). At this point we know that we need to return the logical height of the item so we must ensure that we have an updated value for the height before. The only way to do that is laying out the item

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1048
>> +        child.layoutIfNeeded();
> 
> Ditto about this like, do we need this if we haven't gone through the previous if statement?

Yep, because you don't have the height until you layout the item.

[Manuel Rego Casasnovas]

Comment on attachment 439157 [details]
Patch

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

r=me

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:999
>> +class ScopedFlexBasisAsMainSize {
> 
> Wow, this is quite a hack I guess we don't have a better alternative though. :-(
> 
> I'm not really sure about the class name, would be better ScopedFlexBasisAsChildSize? Or something else, not sure...

Could we add some brief documentation about this class? Is it not very big, but it's a kind of very special case, so it's worth explaining it for the future.

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1040
>>> +    // 9.3.2 E. Otherwise, size the item into the available space using its used flex basis in place of its main size.
>> 
>> I miss C) and D) before getting to E). Maybe we could add a comment.
> 
> They've not been implemented yet.

I'd add a comment for clarity, otherwise it's kind of confusing.

[zalan]

👍

[Sergio Villar Senin]

Created attachment 441350 [details]
Patch for landing

[Sergio Villar Senin]

Committed r284359 (243144@main): <https://commits.webkit.org/243144@main>

[Yusuke Suzuki]

Reverted in https://trac.webkit.org/changeset/285045/webkit
This patch makes Twitter CPU usage 100%.

[Antti Koivisto]

Comment on attachment 441350 [details]
Patch for landing

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1012
> +        m_child.mutableStyle().setLogicalHeight(Length(flexBasis));

This sort of hacks are not ok.

[Sergio Villar Senin]

Created attachment 443788 [details]
Patch

[Sergio Villar Senin]

Committed r285623 (244124@main): <https://commits.webkit.org/244124@main>

[zalan]

This broke the IRS payment page. see bug 240068.