RESOLVED FIXED223377
[css-flexbox] Make {main|cross}SizeForPercentageResolution() return booleans instead of actual sizes 
https://bugs.webkit.org/show_bug.cgi?id=223377
Summary [css-flexbox] Make {main|cross}SizeForPercentageResolution() return booleans ...
Sergio Villar Senin
Reported 2021-03-17 06:14:38 PDT
[css-flex] Make XXXForPercentageResolution() methods return booleans instead of actual sizes
Attachments
Patch (11.11 KB, patch)
2021-03-17 06:24 PDT, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Patch (11.13 KB, patch)
2021-03-18 08:51 PDT, Sergio Villar Senin 		rego: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2021-03-17 06:24:32 PDT
Created attachment 423479 [details] Patch
Manuel Rego Casasnovas
Comment 2 2021-03-18 05:33:27 PDT
Comment on attachment 423479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423479&action=review Nice cleanup, but I have a couple of comments. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1237 > +bool RenderFlexibleBox::childCrossSizeIsDefiniteForPercentageResolution(const RenderBox& child) I don't think this name and the next one are accurate. If a flex item has fixed width and height, its main/cross size is definite, but this might be returning false. This is checking if we should use or not the OverridingLogicalHeight to compute percentages, but the method name is confusing. > Source/WebCore/rendering/RenderFlexibleBox.h:73 > + bool childCrossSizeIsDefiniteForPercentageResolution(const RenderBox&); > + bool childMainSizeIsDefiniteForPercentageResolution(const RenderBox&); Why are you making these 2 methods public?
Sergio Villar Senin
Comment 3 2021-03-18 08:12:11 PDT
Comment on attachment 423479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423479&action=review >> Source/WebCore/rendering/RenderFlexibleBox.cpp:1237 >> +bool RenderFlexibleBox::childCrossSizeIsDefiniteForPercentageResolution(const RenderBox& child) > > I don't think this name and the next one are accurate. > > If a flex item has fixed width and height, its main/cross size is definite, but this might be returning false. > This is checking if we should use or not the OverridingLogicalHeight to compute percentages, but the method name is confusing. Right, I agree I'll rename then. >> Source/WebCore/rendering/RenderFlexibleBox.h:73 >> + bool childMainSizeIsDefiniteForPercentageResolution(const RenderBox&); > > Why are you making these 2 methods public? Ups, I just put them all together but they were away for a reason :)
Sergio Villar Senin
Comment 4 2021-03-18 08:51:46 PDT
Created attachment 423602 [details] Patch
Manuel Rego Casasnovas
Comment 5 2021-03-18 09:01:54 PDT
Comment on attachment 423602 [details] Patch r=me
Sergio Villar Senin
Comment 6 2021-03-19 02:15:07 PDT
Committed r274708 (235528@main): <https://commits.webkit.org/235528@main>
Radar WebKit Bug Importer
Comment 7 2021-03-19 02:16:15 PDT
<rdar://problem/75612783>
Note You need to log in before you can comment on or make changes to this bug.