Support transferred min/max block size for aspect-ratio
Created attachment 416893 [details] Patch
Created attachment 417250 [details] Patch
Created attachment 417257 [details] Patch
Created attachment 417339 [details] Patch
<rdar://problem/72961058>
Created attachment 417342 [details] Patch
Created attachment 417349 [details] Patch
Created attachment 417615 [details] Patch
Created attachment 417618 [details] Patch
Created attachment 417619 [details] Patch
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.
Created attachment 417693 [details] Patch
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.
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.
Committed r271554: <https://trac.webkit.org/changeset/271554> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417693 [details].
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.
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...
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.
Reopening to attach new patch.
Created attachment 417881 [details] Patch
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.
(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.
(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.
(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.
Created attachment 417947 [details] Patch
Committed r271648: <https://trac.webkit.org/changeset/271648> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417947 [details].