[line-clamp] Fix clamping by height with nested height constraints [chromium/src : main]
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
line_clamp_data_.data.clamp_bfc_offset != LayoutUnit::Max()) {If possible can you change this to a std::optional<LayoutUnit> instead of relying on the LayoutUnit::Max sentinel value?
Related details
- Andreu Botella
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreu Botella (Gerrit)
Andreu Botella added 1 comment![]( "Open in Gerrit")
line_clamp_data_.data.clamp_bfc_offset != LayoutUnit::Max()) {If possible can you change this to a std::optional<LayoutUnit> instead of relying on the LayoutUnit::Max sentinel value?
Maybe it'd be better to have an additional value in `LineClampData::State` only for counting the number of lines in a box. Since both of the checks I added for `LayoutUnit::Max()` also check that we're clamping by height, we could just move that into the state check.
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
line_clamp_data_.data.clamp_bfc_offset != LayoutUnit::Max()) {Andreu BotellaIf possible can you change this to a std::optional<LayoutUnit> instead of relying on the LayoutUnit::Max sentinel value?
Maybe it'd be better to have an additional value in `LineClampData::State` only for counting the number of lines in a box. Since both of the checks I added for `LayoutUnit::Max()` also check that we're clamping by height, we could just move that into the state check.
- Andreu Botella
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 3 comments![]( "Open in Gerrit")
MinMaxSizes size_constraints;do you want to initialize this to LayoutUnit(), LayoutUnit::Max() ?
if (size_constraints.min_size >= size_constraints.max_size) {The ">" shouldn't happen due to:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/length_utils.cc;l=658;drc=845730f7e24a5e2b58532ae54b478afdbcb107ba
should this just be "==" ?
block_size_constraints_(we typically call these block_min_max_sizes
Related details
- Andreu Botella
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreu Botella (Gerrit)
Andreu Botella added 1 comment![]( "Open in Gerrit")
line_clamp_data_.data.clamp_bfc_offset != LayoutUnit::Max()) {Andreu BotellaIf possible can you change this to a std::optional<LayoutUnit> instead of relying on the LayoutUnit::Max sentinel value?
Ian KilpatrickMaybe it'd be better to have an additional value in `LineClampData::State` only for counting the number of lines in a box. Since both of the checks I added for `LayoutUnit::Max()` also check that we're clamping by height, we could just move that into the state check.
That sounds good.
I'm thinking that maybe I should do that in a follow-up CL. For this one, I could keep Max as a sentinel value, or alternatively I could set the size constraints to the fixed size. Both should give the same result.
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreu Botella (Gerrit)
Andreu Botella added 4 comments![]( "Open in Gerrit")
line_clamp_data_.data.clamp_bfc_offset != LayoutUnit::Max()) {Andreu BotellaIf possible can you change this to a std::optional<LayoutUnit> instead of relying on the LayoutUnit::Max sentinel value?
Ian KilpatrickMaybe it'd be better to have an additional value in `LineClampData::State` only for counting the number of lines in a box. Since both of the checks I added for `LayoutUnit::Max()` also check that we're clamping by height, we could just move that into the state check.
Andreu BotellaThat sounds good.
I'm thinking that maybe I should do that in a follow-up CL. For this one, I could keep Max as a sentinel value, or alternatively I could set the size constraints to the fixed size. Both should give the same result.
Made this change, setting `block_min_max_sizes` to the fixed size in this CL.
do you want to initialize this to LayoutUnit(), LayoutUnit::Max() ?
Not relevant anymore in this CL. Will take that into account for the follow-up CL.
if (size_constraints.min_size >= size_constraints.max_size) {The ">" shouldn't happen due to:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/length_utils.cc;l=658;drc=845730f7e24a5e2b58532ae54b478afdbcb107bashould this just be "==" ?
Not relevant anymore in this CL. Will take that into account for the follow-up CL.
we typically call these block_min_max_sizes
Done
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |