Support transferred min/max block size for aspect-ratio

• Component: CSS
• Status: RESOLVED FIXED

[Rob Buis]

Support transferred min/max block size for aspect-ratio

[Rob Buis]

Created attachment 416893 [details]
Patch

[Rob Buis]

Created attachment 417250 [details]
Patch

[Rob Buis]

Created attachment 417257 [details]
Patch

[Rob Buis]

Created attachment 417339 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/72961058>

[Rob Buis]

Created attachment 417342 [details]
Patch

[Rob Buis]

Created attachment 417349 [details]
Patch

[Rob Buis]

Created attachment 417615 [details]
Patch

[Rob Buis]

Created attachment 417618 [details]
Patch

[Rob Buis]

Created attachment 417619 [details]
Patch

[Sam Weinig]

Comment on attachment 417619 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:5058
> +void RenderBox::computeMinMaxLogicalWidthFromAspectRatio(LayoutUnit& transferredMinSize, LayoutUnit& transferredMaxSize) const

This should return a pair (or a bespoke struct) rather than using out parameters.

[Rob Buis]

Created attachment 417693 [details]
Patch

[Rob Buis]

Comment on attachment 417619 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:5058
>> +void RenderBox::computeMinMaxLogicalWidthFromAspectRatio(LayoutUnit& transferredMinSize, LayoutUnit& transferredMaxSize) const
> 
> This should return a pair (or a bespoke struct) rather than using out parameters.

Done.

[Darin Adler]

Comment on attachment 417693 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:650
> +        logicalWidth = std::max(logicalMinWidth, std::min(logicalWidth, logicalMaxWidth));

I’m fond of using std::clamp for this sort of thing, but it’s maybe not *exactly* the same as the std::max/min combo here.

[EWS]

Committed r271554: <https://trac.webkit.org/changeset/271554>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417693 [details].

[Sam Weinig]

Comment on attachment 417693 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:649
> +        LayoutUnit logicalMinWidth, logicalMaxWidth;
> +        std::tie(logicalMinWidth, logicalMaxWidth) = computeMinMaxLogicalWidthFromAspectRatio();

No need to change this, but in the future, you can write these two lines as:

auto [logicalMinWidth, logicalMaxWidth] = computeMinMaxLogicalWidthFromAspectRatio();

You still have to use std::tie in some circumstances, like if you have existing variables, but in cases like this the structured binding approach is quite clean.

>> Source/WebCore/rendering/RenderBox.cpp:650
>> +        logicalWidth = std::max(logicalMinWidth, std::min(logicalWidth, logicalMaxWidth));
> 
> I’m fond of using std::clamp for this sort of thing, but it’s maybe not *exactly* the same as the std::max/min combo here.

It should be exactly the same I think.

[Rob Buis]

Comment on attachment 417693 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:649
>> +        std::tie(logicalMinWidth, logicalMaxWidth) = computeMinMaxLogicalWidthFromAspectRatio();
> 
> No need to change this, but in the future, you can write these two lines as:
> 
> auto [logicalMinWidth, logicalMaxWidth] = computeMinMaxLogicalWidthFromAspectRatio();
> 
> You still have to use std::tie in some circumstances, like if you have existing variables, but in cases like this the structured binding approach is quite clean.

I tried it first but I got a compile error (on Big sur) at the place where the other std::tie is used:
   if (shouldComputeLogicalHeightFromAspectRatio())
        std::tie(transferredMinSize, transferredMaxSize) = computeMinMaxLogicalWidthFromAspectRatio();

You can try for yourself if you are interested, I forgot the exact error.

>>> Source/WebCore/rendering/RenderBox.cpp:650
>>> +        logicalWidth = std::max(logicalMinWidth, std::min(logicalWidth, logicalMaxWidth));
>> 
>> I’m fond of using std::clamp for this sort of thing, but it’s maybe not *exactly* the same as the std::max/min combo here.
> 
> It should be exactly the same I think.

I am not sure, but mainly I think it would like a non-intuitive and maybe suspicious clamp, i.e. why clamp between max, min range instead of usual min, max. Maybe we need a mathematician to chime in here, Igalia has a few of them...

[Darin Adler]

Comment on attachment 417693 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderBox.cpp:650
>>>> +        logicalWidth = std::max(logicalMinWidth, std::min(logicalWidth, logicalMaxWidth));
>>> 
>>> I’m fond of using std::clamp for this sort of thing, but it’s maybe not *exactly* the same as the std::max/min combo here.
>> 
>> It should be exactly the same I think.
> 
> I am not sure, but mainly I think it would like a non-intuitive and maybe suspicious clamp, i.e. why clamp between max, min range instead of usual min, max. Maybe we need a mathematician to chime in here, Igalia has a few of them...

There is a small difference, one I learned when I researched clamp. The difference is when minimum > maximum. You can carefully use std::max/min so the minimum takes precedence over the maximum. But in std::clamp, the maximum takes precedence. I have no idea what this has to do with mathematicians! I love std::clamp for its clarity, clamp a value between low and high, rather than using std::max/min where it’s harder to read.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 417881 [details]
Patch

[Rob Buis]

Comment on attachment 417693 [details]
Patch

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

>>>>> Source/WebCore/rendering/RenderBox.cpp:650
>>>>> +        logicalWidth = std::max(logicalMinWidth, std::min(logicalWidth, logicalMaxWidth));
>>>> 
>>>> I’m fond of using std::clamp for this sort of thing, but it’s maybe not *exactly* the same as the std::max/min combo here.
>>> 
>>> It should be exactly the same I think.
>> 
>> I am not sure, but mainly I think it would like a non-intuitive and maybe suspicious clamp, i.e. why clamp between max, min range instead of usual min, max. Maybe we need a mathematician to chime in here, Igalia has a few of them...
> 
> There is a small difference, one I learned when I researched clamp. The difference is when minimum > maximum. You can carefully use std::max/min so the minimum takes precedence over the maximum. But in std::clamp, the maximum takes precedence. I have no idea what this has to do with mathematicians! I love std::clamp for its clarity, clamp a value between low and high, rather than using std::max/min where it’s harder to read.

I probably misinterpreted the code. Let's see if it turns up green using std:clamp, I also prefer it.

[Darin Adler]

(In reply to Rob Buis from comment #17)
> I tried it first but I got a compile error (on Big sur) at the place where
> the other std::tie is used:
>    if (shouldComputeLogicalHeightFromAspectRatio())
>         std::tie(transferredMinSize, transferredMaxSize) =
> computeMinMaxLogicalWidthFromAspectRatio();
> 
> You can try for yourself if you are interested, I forgot the exact error.

That’s exactly what Sam meant when he said, "You still have to use std::tie in some circumstances, like if you have existing variables." These are existing variables.

[Rob Buis]

(In reply to Darin Adler from comment #22)
> (In reply to Rob Buis from comment #17)
> > I tried it first but I got a compile error (on Big sur) at the place where
> > the other std::tie is used:
> >    if (shouldComputeLogicalHeightFromAspectRatio())
> >         std::tie(transferredMinSize, transferredMaxSize) =
> > computeMinMaxLogicalWidthFromAspectRatio();
> > 
> > You can try for yourself if you are interested, I forgot the exact error.
> 
> That’s exactly what Sam meant when he said, "You still have to use std::tie
> in some circumstances, like if you have existing variables." These are
> existing variables.

I see, thanks for the clarification! I think I will use the std::tie usage as-is since otherwise it feels a bit inconsistent.

[Darin Adler]

(In reply to Rob Buis from comment #23)
> I think I will use the std::tie usage
> as-is since otherwise it feels a bit inconsistent.

I’m a fan of combining both since the structured binding is so much more terse when we are both defining and initializing at once.

[Rob Buis]

Created attachment 417947 [details]
Patch

[EWS]

Committed r271648: <https://trac.webkit.org/changeset/271648>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417947 [details].