Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-flexbox] Don't cache definiteness when there's an override container height #20137

Merged
merged 1 commit into from Nov 7, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Nov 6, 2019

See stack trace below. We set the override container logical height to -1
for the initial layout of a flex item so that we compute the correct size
for min-height. However, that messes with our cache for definite heights
because we would always set it to indefinite in such a case.

Instead, just don't cache these values. That way we will later compute the right
thing for resolving flex-basis, etc.

(FlexNG can't come soon enough...)

#0 blink::LayoutBox::ContainingBlockLogicalHeightForPercentageResolution (this=0x3dda8d434198,
out_cb=0x7f6e7d42d8c0, out_skipped_auto_height_containing_block=0x0)
at ../../third_party/blink/renderer/core/layout/layout_box.cc:3833
#1 0x00007f6ee84ad0a1 in blink::LayoutFlexibleBox::MainAxisLengthIsDefinite (this=0x3dda8d434010,
child=..., flex_basis=Length(0%, Percent), add_to_cb=false)
at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:762
#2 0x00007f6ee84af930 in blink::LayoutFlexibleBox::MainSizeIsDefiniteForPercentageResolution (
this=0x3dda8d434010, child=...)
at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:1125
#3 0x00007f6ee84ad7f5 in blink::LayoutFlexibleBox::UseOverrideLogicalHeightForPerentageResolution (
this=0x3dda8d434010, child=...)
at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:1137
#4 0x00007f6ee83f2b9d in blink::LayoutBlock::AvailableLogicalHeightForPercentageComputation (
this=0x3dda8d434198) at ../../third_party/blink/renderer/core/layout/layout_block.cc:2333
#5 0x00007f6ee845e745 in blink::LayoutBox::ContainingBlockLogicalHeightForPercentageResolution (
this=0x3dda8d4243d0, out_cb=0x0, out_skipped_auto_height_containing_block=0x0)
at ../../third_party/blink/renderer/core/layout/layout_box.cc:3830
#6 0x00007f6ee86dcc5c in blink::LayoutBoxUtils::AvailableLogicalHeight (box=..., cb=0x3dda8d434198)
at ../../third_party/blink/renderer/core/layout/ng/layout_box_utils.cc:64
#7 0x00007f6ee86eafea in blink::LayoutNGMixin<blink::LayoutBlockFlow>::ComputeIntrinsicLogicalWidths (
this=0x3dda8d4243d0, min_logical_width=0px, max_logical_width=0px)
at ../../third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc:48
#8 0x00007f6ee83ef53a in blink::LayoutBlock::ComputePreferredLogicalWidths (this=0x3dda8d4243d0)
at ../../third_party/blink/renderer/core/layout/layout_block.cc:1509
#9 0x00007f6ee8451f01 in blink::LayoutBox::MaxPreferredLogicalWidth (this=0x3dda8d4243d0)
at ../../third_party/blink/renderer/core/layout/layout_box.cc:1395
#10 0x00007f6ee84adba2 in blink::LayoutFlexibleBox::ComputeInnerFlexBaseSizeForChild (this=0x3dda8d434198,
child=..., main_axis_border_and_padding=0px, child_layout_type=blink::LayoutFlexibleBox::kForceLayout)
at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:890
#11 0x00007f6ee84ae5d1 in blink::LayoutFlexibleBox::ConstructAndAppendFlexItem (this=0x3dda8d434198,
algorithm=0x7f6e7d42ed70, child=..., layout_type=blink::LayoutFlexibleBox::kForceLayout)
at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:1203
#12 0x00007f6ee84aa27b in blink::LayoutFlexibleBox::LayoutFlexItems (this=0x3dda8d434198,
relayout_children=true, layout_scope=...)
at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:934
#13 0x00007f6ee84a9cff in blink::LayoutFlexibleBox::UpdateBlockLayout (this=0x3dda8d434198,
relayout_children=true) at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:369

Bug: 1019138
Change-Id: Ie94e69a5f3fe6accc3623d358315b174088d5597
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1902514
Commit-Queue: David Grogan <dgrogan@chromium.org>
Auto-Submit: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713296}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

…iner height

See stack trace below. We set the override container logical height to -1
for the initial layout of a flex item so that we compute the correct size
for min-height. However, that messes with our cache for definite heights
because we would always set it to indefinite in such a case.

Instead, just don't cache these values. That way we will later compute the right
thing for resolving flex-basis, etc.

(FlexNG can't come soon enough...)

 #0  blink::LayoutBox::ContainingBlockLogicalHeightForPercentageResolution (this=0x3dda8d434198,
    out_cb=0x7f6e7d42d8c0, out_skipped_auto_height_containing_block=0x0)
    at ../../third_party/blink/renderer/core/layout/layout_box.cc:3833
 #1  0x00007f6ee84ad0a1 in blink::LayoutFlexibleBox::MainAxisLengthIsDefinite (this=0x3dda8d434010,
    child=..., flex_basis=Length(0%, Percent), add_to_cb=false)
    at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:762
 #2  0x00007f6ee84af930 in blink::LayoutFlexibleBox::MainSizeIsDefiniteForPercentageResolution (
    this=0x3dda8d434010, child=...)
    at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:1125
 #3  0x00007f6ee84ad7f5 in blink::LayoutFlexibleBox::UseOverrideLogicalHeightForPerentageResolution (
    this=0x3dda8d434010, child=...)
    at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:1137
 #4  0x00007f6ee83f2b9d in blink::LayoutBlock::AvailableLogicalHeightForPercentageComputation (
    this=0x3dda8d434198) at ../../third_party/blink/renderer/core/layout/layout_block.cc:2333
 #5  0x00007f6ee845e745 in blink::LayoutBox::ContainingBlockLogicalHeightForPercentageResolution (
    this=0x3dda8d4243d0, out_cb=0x0, out_skipped_auto_height_containing_block=0x0)
    at ../../third_party/blink/renderer/core/layout/layout_box.cc:3830
 #6  0x00007f6ee86dcc5c in blink::LayoutBoxUtils::AvailableLogicalHeight (box=..., cb=0x3dda8d434198)
    at ../../third_party/blink/renderer/core/layout/ng/layout_box_utils.cc:64
 #7  0x00007f6ee86eafea in blink::LayoutNGMixin<blink::LayoutBlockFlow>::ComputeIntrinsicLogicalWidths (
    this=0x3dda8d4243d0, min_logical_width=0px, max_logical_width=0px)
    at ../../third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc:48
 #8  0x00007f6ee83ef53a in blink::LayoutBlock::ComputePreferredLogicalWidths (this=0x3dda8d4243d0)
    at ../../third_party/blink/renderer/core/layout/layout_block.cc:1509
 #9  0x00007f6ee8451f01 in blink::LayoutBox::MaxPreferredLogicalWidth (this=0x3dda8d4243d0)
    at ../../third_party/blink/renderer/core/layout/layout_box.cc:1395
 #10 0x00007f6ee84adba2 in blink::LayoutFlexibleBox::ComputeInnerFlexBaseSizeForChild (this=0x3dda8d434198,
    child=..., main_axis_border_and_padding=0px, child_layout_type=blink::LayoutFlexibleBox::kForceLayout)
    at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:890
 #11 0x00007f6ee84ae5d1 in blink::LayoutFlexibleBox::ConstructAndAppendFlexItem (this=0x3dda8d434198,
    algorithm=0x7f6e7d42ed70, child=..., layout_type=blink::LayoutFlexibleBox::kForceLayout)
    at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:1203
 #12 0x00007f6ee84aa27b in blink::LayoutFlexibleBox::LayoutFlexItems (this=0x3dda8d434198,
    relayout_children=true, layout_scope=...)
    at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:934
 #13 0x00007f6ee84a9cff in blink::LayoutFlexibleBox::UpdateBlockLayout (this=0x3dda8d434198,
    relayout_children=true) at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:369

Bug: 1019138
Change-Id: Ie94e69a5f3fe6accc3623d358315b174088d5597
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1902514
Commit-Queue: David Grogan <dgrogan@chromium.org>
Auto-Submit: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713296}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants