[webkit-reviews] review granted: [Bug 232617] [css-grid] Track sizing algorithm not repeated even if used flex fraction would change : [Attachment 444812] Patch

[bugzilla-daemon at webkit.org]

Javier Fernandez <jfernandez at igalia.com> has granted zsun at igalia.com's request
for review:
Bug 232617: [css-grid] Track sizing algorithm not repeated even if used flex
fraction would change
https://bugs.webkit.org/show_bug.cgi?id=232617

Attachment 444812: Patch

https://bugs.webkit.org/attachment.cgi?id=444812&action=review




--- Comment #8 from Javier Fernandez <jfernandez at igalia.com> ---
Comment on attachment 444812
  --> https://bugs.webkit.org/attachment.cgi?id=444812
Patch

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

r=me 

I think the patch looks good and it's correct. Bear in mind my comments, but
non of them are blockers for landing the patch.

> Source/WebCore/rendering/RenderGrid.cpp:168
> +

Nit: remove this empty line

> Source/WebCore/rendering/RenderGrid.cpp:169
> +    if (m_hasAnyOrthogonalItem ||
m_trackSizingAlgorithm.hasAnyPercentSizedRowsIndefiniteHeight() ||
(m_trackSizingAlgorithm.hasAnyFlexibleMaxTrackBreadth() &&
!m_hasAnyBaselineAlignmentItem) || m_hasAspectRatioBlockSizeDependentItem) {

Why we are avoiding the new run of the track sizing algorithm if there is any
grid item with baseline alignment ONLY in the case of using flex max-sizing
functions ? Wouldn't be possible to affect the grid item's participation in
baseline alignment due to the presence of percent-sized rows and indefinite
height ? 

I know that we weren't considering that case in the current code, but I'd like
to understand better the effect of avoiding the algorithm repetition in that
case as well (maybe we that would fix some additional test failures). We could
at least add a FIXME comment and try to investigate it in a follow up patch.

> Source/WebCore/rendering/RenderGrid.cpp:207
> +	   m_hasAnyBaselineAlignmentItem = false;

Perhaps we could just initialize the class field after completing the loop
below, just checking out whether there is any value in the
cacheBaselineAlignedItem. 

Idea: we could do the same for the m_hasAnyOrthogonalItem class field.