Bug 223377

Summary:

[css-flexbox] Make {main|cross}SizeForPercentageResolution() return booleans instead of actual sizes

Product:

WebKit

Reporter:

Sergio Villar Senin <svillar>

Component:

New Bugs

Assignee:

Sergio Villar Senin <svillar>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, changseok, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, pdr, rbuis, rego, simon.fraser, svillar, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	rego: review+

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	Details Formatted Diff Diff
Patch (11.13 KB, patch) 2021-03-18 08:51 PDT, Sergio Villar Senin	rego: review+	Details Formatted Diff Diff
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.