RESOLVED FIXED 220224
Support transferred min/max block size for aspect-ratio
https://bugs.webkit.org/show_bug.cgi?id=220224
Summary Support transferred min/max block size for aspect-ratio
Rob Buis
Reported 2021-01-02 02:51:10 PST
Support transferred min/max block size for aspect-ratio
Attachments
Patch (4.13 KB, patch)
2021-01-02 02:53 PST, Rob Buis
no flags
Patch (4.13 KB, patch)
2021-01-08 00:50 PST, Rob Buis
no flags
Patch (6.61 KB, patch)
2021-01-08 01:41 PST, Rob Buis
no flags
Patch (11.57 KB, patch)
2021-01-09 01:13 PST, Rob Buis
no flags
Patch (11.54 KB, patch)
2021-01-09 08:17 PST, Rob Buis
no flags
Patch (11.62 KB, patch)
2021-01-10 06:35 PST, Rob Buis
no flags
Patch (11.70 KB, patch)
2021-01-14 06:40 PST, Rob Buis
ews-feeder: commit-queue-
Patch (11.70 KB, patch)
2021-01-14 07:02 PST, Rob Buis
ews-feeder: commit-queue-
Patch (11.71 KB, patch)
2021-01-14 07:39 PST, Rob Buis
no flags
Patch (11.80 KB, patch)
2021-01-15 05:38 PST, Rob Buis
no flags
Patch (1.61 KB, patch)
2021-01-19 09:45 PST, Rob Buis
no flags
Patch (1.80 KB, patch)
2021-01-20 00:31 PST, Rob Buis
no flags
Rob Buis
Comment 1 2021-01-02 02:53:39 PST
Rob Buis
Comment 2 2021-01-08 00:50:47 PST
Rob Buis
Comment 3 2021-01-08 01:41:02 PST
Rob Buis
Comment 4 2021-01-09 01:13:36 PST
Radar WebKit Bug Importer
Comment 5 2021-01-09 02:52:14 PST
Rob Buis
Comment 6 2021-01-09 08:17:30 PST
Rob Buis
Comment 7 2021-01-10 06:35:20 PST
Rob Buis
Comment 8 2021-01-14 06:40:26 PST
Rob Buis
Comment 9 2021-01-14 07:02:36 PST
Rob Buis
Comment 10 2021-01-14 07:39:43 PST
Sam Weinig
Comment 11 2021-01-14 15:42:42 PST
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
Comment 12 2021-01-15 05:38:30 PST
Rob Buis
Comment 13 2021-01-15 09:07:17 PST
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 14 2021-01-15 17:31:12 PST
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
Comment 15 2021-01-15 23:58:01 PST
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 16 2021-01-16 10:22:41 PST
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 17 2021-01-16 10:59:31 PST
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 18 2021-01-19 09:39:57 PST
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
Comment 19 2021-01-19 09:45:42 PST
Reopening to attach new patch.
Rob Buis
Comment 20 2021-01-19 09:45:45 PST
Rob Buis
Comment 21 2021-01-19 09:49:25 PST
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
Comment 22 2021-01-19 13:01:38 PST
(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
Comment 23 2021-01-19 13:25:01 PST
(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
Comment 24 2021-01-19 13:27:13 PST
(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
Comment 25 2021-01-20 00:31:51 PST
EWS
Comment 26 2021-01-20 08:02:06 PST
Committed r271648: <https://trac.webkit.org/changeset/271648> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417947 [details].
Note You need to log in before you can comment on or make changes to this bug.