[webkit-reviews] review denied: [Bug 228283] Fix aspect-ratio-intrinsic-size-004.html : [Attachment 436759] Patch

[bugzilla-daemon at webkit.org]

Sergio Villar Senin <svillar at igalia.com> has denied  review:
Bug 228283: Fix aspect-ratio-intrinsic-size-004.html
https://bugs.webkit.org/show_bug.cgi?id=228283

Attachment 436759: Patch

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




--- Comment #16 from Sergio Villar Senin <svillar at igalia.com> ---
Comment on attachment 436759
  --> https://bugs.webkit.org/attachment.cgi?id=436759
Patch

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

I've serious doubts about two call sites added by this patch  and thus the r-.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:353
> +    dirtyForLayoutFromPercentageHeightDescendants();

This looks pretty interesting indeed. I haven't realized before that it's only
called from RenderBlockFlow and thus never for flexboxes. I wonder whether we
should call it from RenderFlexibleBox::layoutAndPlaceChildren() to kind of
mimic the location where it's called in RenderBlockFlow...

> Source/WebCore/rendering/RenderFlexibleBox.cpp:966
> +	       return percentageLogicalHeightIsResolvable();

Hmm I am not sure we could call percentageLogicalHeightIsResolvable() here.
This method could start the following sequence:

percentageLogicalHeightIsResolvable()->computePercentageLogicalHeight()->availa
bleLogicalHeightForPercentageComputation()->useChildOverridingLogicalHeightForP
ercentageResolution()->useChildOverridingMainSizeForPercentageResolution()->can
ComputePercentageFlexBasis()->computePercentageLogicalHeight()->availableLogica
lHeightForPercentageComputation() ...

entering a loop of calls. Do you have a test for this case?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1056
> +	       addPercentHeightDescendant(*child);

Doesn't look 100% correct. A content dependent size (like min-content,
max-content etc...) isn't also definite but it isn't a percentage.

Also it shouldn't be really called from RenderFlexibleBox, looks something
we're missing in RenderBox to handle the case of items with aspect ratio.