| 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: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Sergio Villar Senin
2021-03-17 06:14:38 PDT
Created attachment 423479 [details]
Patch
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? 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 :) Created attachment 423602 [details]
Patch
Comment on attachment 423602 [details]
Patch
r=me
Committed r274708 (235528@main): <https://commits.webkit.org/235528@main> |