WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(4.13 KB, patch)
2021-01-08 00:50 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(6.61 KB, patch)
2021-01-08 01:41 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(11.57 KB, patch)
2021-01-09 01:13 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(11.54 KB, patch)
2021-01-09 08:17 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(11.62 KB, patch)
2021-01-10 06:35 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(11.70 KB, patch)
2021-01-14 06:40 PST
,
Rob Buis
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(11.70 KB, patch)
2021-01-14 07:02 PST
,
Rob Buis
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(11.71 KB, patch)
2021-01-14 07:39 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(11.80 KB, patch)
2021-01-15 05:38 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(1.61 KB, patch)
2021-01-19 09:45 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(1.80 KB, patch)
2021-01-20 00:31 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
Rob Buis
Comment 1
2021-01-02 02:53:39 PST
Created
attachment 416893
[details]
Patch
Rob Buis
Comment 2
2021-01-08 00:50:47 PST
Created
attachment 417250
[details]
Patch
Rob Buis
Comment 3
2021-01-08 01:41:02 PST
Created
attachment 417257
[details]
Patch
Rob Buis
Comment 4
2021-01-09 01:13:36 PST
Created
attachment 417339
[details]
Patch
Radar WebKit Bug Importer
Comment 5
2021-01-09 02:52:14 PST
<
rdar://problem/72961058
>
Rob Buis
Comment 6
2021-01-09 08:17:30 PST
Created
attachment 417342
[details]
Patch
Rob Buis
Comment 7
2021-01-10 06:35:20 PST
Created
attachment 417349
[details]
Patch
Rob Buis
Comment 8
2021-01-14 06:40:26 PST
Created
attachment 417615
[details]
Patch
Rob Buis
Comment 9
2021-01-14 07:02:36 PST
Created
attachment 417618
[details]
Patch
Rob Buis
Comment 10
2021-01-14 07:39:43 PST
Created
attachment 417619
[details]
Patch
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
Created
attachment 417693
[details]
Patch
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
Created
attachment 417881
[details]
Patch
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
Created
attachment 417947
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug