[webkit-reviews] review granted: [Bug 219538] [css-flex] Implement 9.8.1 Definite and Indefinite Sizes : [Attachment 415414] Patch

[bugzilla-daemon at webkit.org]

Manuel Rego Casasnovas <rego at igalia.com> has granted  review:
Bug 219538: [css-flex] Implement 9.8.1 Definite and Indefinite Sizes
https://bugs.webkit.org/show_bug.cgi?id=219538

Attachment 415414: Patch

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




--- Comment #6 from Manuel Rego Casasnovas <rego at igalia.com> ---
Comment on attachment 415414
  --> https://bugs.webkit.org/attachment.cgi?id=415414
Patch

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

r=me

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:768
>>> +	     ASSERT(containerCrossSizeLength.isFixed());
>> 
>> Why style.height() is going to be fixed? I don't understand this assert,
couldn't it be auto for example?
> 
> Right, it's far from obvious. I'll try to explain it .This method is only
called if useChildAspectRatio() is true. That only happens when
childCrossSizeIsDefinite() returns true as well. So far so good.
> 
> If the child cross size was auto (before this change) then the child cross
size would be consider indefinite and thus this method wouldn't ever called.
After this change this method could be called if the cross size is auto if and
only if the cross size of the container is fixed (see the changes to
childCrossSizeIsDefinite()). As I mention in a comment in the latter, we should
include more cases where a length is definite but we're starting with fixed
sizes for simplicity.

Ok, thanks for the explanation, I got it now.

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:828
>>> +		 if (auto& crossSize = isHorizontalFlow() ? style().height() :
style().width(); crossSize.isFixed())
>> 
>> Never seen this kind of things in a single "check" before, I'd move the
crossSize variable to the previous line, I don't see any benefit from not doing
it.
>> 
>> BTW, there's another FIXME a few lines below. Is this related?
>> 826842    // FIXME: Eventually we should support other types of sizes here.
>> 827843    // Requires updating computeMainSizeFromAspectRatioUsing.
> 
> The if statement with initializer is a C++17 feature that is becoming popular
inside WebKit, there are several other instances of it.
> 
> WRT the FIXME bellow is not related, it's about supporting intrinsic sizes.

I think it's harder to read than a variable first and then the if in a
different line. I see a few other examples in other WebKit code, I guess I
should get used to it at some point.