Auto merge of #29957 - Loirooriol:optimize-collapsible-margin-lookahead, r=mrobinson

Layout 2020: Optimize collapsible margin lookahead

Every time that we would lay out a block box that could collapse its top margin with its contents, we would do a lookahead to compute the resulting margin in order to place floats correctly.

The problem is that this lookahead could iterate several descendants, but then when laying these we would run the lookahead again.

This patch restricts the lookahead to boxes that either aren't collapsing their top margin with their parent, or that have 'clear' different than 'none' (since clearance prevents collapsing margins with the parent).

Since the lookahead stops iterating when it finds a box that doesn't collapse its top margin with its parent, or whose 'clear' isn't 'none', this should ensure that lookahead never handles the same box twice.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because there shouldn't be any change in behavior

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

1 files changed, 25 insertions, 5 deletions

diff --git a/components/layout_2020/flow/mod.rs b/components/layout_2020/flow/mod.rs
index 7f76abd9db8..e881ed61347 100644
--- a/components/layout_2020/flow/mod.rs
+++ b/components/layout_2020/flow/mod.rs
@@ -67,8 +67,6 @@ impl BlockLevelBox {
         collected_margin: &mut CollapsedMargin,
         containing_block: &ContainingBlock,
     ) -> bool {
-        // TODO(mrobinson,Loirooriol): Cache margins here so that we don't constantly
-        // have to keep looking forward when dealing with sequences of floats.
         let style = match self {
             BlockLevelBox::SameFormattingContextBlock { ref style, .. } => &style,
             BlockLevelBox::OutOfFlowAbsolutelyPositionedBox(_) |
@@ -399,6 +397,7 @@ fn layout_block_level_children_in_parallel(
                 &mut child_positioning_context,
                 containing_block,
                 /* sequential_layout_state = */ None,
+                /* collapsible_with_parent_start_margin = */ None,
             );
             (fragment, child_positioning_context)
         })
@@ -448,6 +447,9 @@ fn layout_block_level_children_sequentially(
                 &mut child_positioning_context,
                 containing_block,
                 Some(&mut *sequential_layout_state),
+                Some(CollapsibleWithParentStartMargin(
+                    placement_state.next_in_flow_margin_collapses_with_parent_start_margin,
+                )),
             );
 
             placement_state.place_fragment(&mut fragment, Some(sequential_layout_state));
@@ -474,6 +476,7 @@ impl BlockLevelBox {
         positioning_context: &mut PositioningContext,
         containing_block: &ContainingBlock,
         sequential_layout_state: Option<&mut SequentialLayoutState>,
+        collapsible_with_parent_start_margin: Option<CollapsibleWithParentStartMargin>,
     ) -> Fragment {
         match self {
             BlockLevelBox::SameFormattingContextBlock {
@@ -493,6 +496,7 @@ impl BlockLevelBox {
                         style,
                         NonReplacedContents::SameFormattingContextBlock(contents),
                         sequential_layout_state,
+                        collapsible_with_parent_start_margin,
                     )
                 },
             )),
@@ -529,6 +533,7 @@ impl BlockLevelBox {
                                     non_replaced,
                                 ),
                                 sequential_layout_state,
+                                collapsible_with_parent_start_margin,
                             )
                         },
                     ))
@@ -591,6 +596,7 @@ fn layout_in_flow_non_replaced_block_level(
     style: &Arc<ComputedValues>,
     block_level_kind: NonReplacedContents,
     mut sequential_layout_state: Option<&mut SequentialLayoutState>,
+    collapsible_with_parent_start_margin: Option<CollapsibleWithParentStartMargin>,
 ) -> BoxFragment {
     let pbm = style.padding_border_margin(containing_block);
     let box_size = style.content_box_size(containing_block, &pbm);
@@ -671,13 +677,27 @@ fn layout_in_flow_non_replaced_block_level(
         None => parent_containing_block_position_info = None,
         Some(ref mut sequential_layout_state) => {
             let mut block_start_margin = CollapsedMargin::new(margin.block_start);
-            if start_margin_can_collapse_with_children {
+
+            // The block start margin may collapse with content margins,
+            // compute the resulting one in order to place floats correctly.
+            // Only need to do this if the element isn't also collapsing with its parent,
+            // otherwise we should have already included the margin in an ancestor.
+            // Note this lookahead stops when finding a descendant whose `clear` isn't `none`
+            // (since clearance prevents collapsing margins with the parent).
+            // But then we have to decide whether to actually add clearance or not,
+            // so look forward again regardless of `collapsible_with_parent_start_margin`.
+            // TODO: This isn't completely right: if we don't add actual clearance,
+            // the margin should have been included in the parent (or some ancestor).
+            // The lookahead should stop for actual clearance, not just for `clear`.
+            let collapsible_with_parent_start_margin = collapsible_with_parent_start_margin.expect(
+                "We should know whether we are collapsing the block start margin with the parent \
+                when laying out sequentially",
+            ).0 && style.get_box().clear == Clear::None;
+            if !collapsible_with_parent_start_margin && start_margin_can_collapse_with_children {
                 if let NonReplacedContents::SameFormattingContextBlock(
                     BlockContainer::BlockLevelBoxes(child_boxes),
                 ) = block_level_kind
                 {
-                    // The block start margin may collapse with content margins,
-                    // compute the resulting one in order to place floats correctly.
                     BlockLevelBox::find_block_margin_collapsing_with_parent_from_slice(
                         child_boxes,
                         &mut block_start_margin,