[css-flexbox] Improve & simplify the flex-basis computation
Created attachment 439157 [details] Patch
Gentle ping for reviewers
<rdar://problem/83769781>
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.
Any other comment regarding this patch?
(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
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?
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?
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.
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.
👍
Created attachment 441350 [details] Patch for landing
Committed r284359 (243144@main): <https://commits.webkit.org/243144@main>
Reverted in https://trac.webkit.org/changeset/285045/webkit This patch makes Twitter CPU usage 100%.
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.
Created attachment 443788 [details] Patch
Committed r285623 (244124@main): <https://commits.webkit.org/244124@main>
This broke the IRS payment page. see bug 240068.