Ignore zero-height fragments during margin collapsing

third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h

Index: third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h
diff --git a/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h b/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h
index 998a2aa8fe1e10f6d3164c892b70c839608de132..0b5010f66f386188e80e73f8819c11d33bb12561 100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h
+++ b/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h
@@ -26,11 +26,12 @@ class CORE_EXPORT NGFragmentBuilder final
 
   NGFragmentBuilder& AddChild(NGFragment*, NGLogicalOffset);
 
-  // Sets MarginStrut for the resultant fragment.
-  // These 2 methods below should be only called once as we update the
-  // fragment's MarginStrut block start/end from first/last children MarginStrut
-  NGFragmentBuilder& SetMarginStrutBlockStart(const NGMarginStrut& from);
-  NGFragmentBuilder& SetMarginStrutBlockEnd(const NGMarginStrut& from);
+  // Updates block-{start|end} of the currently constructed fragment.
+  //
+  // This method is supposed to be called on every child but it only updates
+  // the block-start once(on the first non-zero height child fragment) and keeps

[cbiesinger, 2016/09/19 17:05:30]

Please add a space before ( here and on the next line

[Gleb Lanbin, 2016/09/19 17:39:52]

Done.

+  // updating block-end(on every non-zero height child).
+  NGFragmentBuilder& UpdateMarginStrut(const NGMarginStrut& from);

[ikilpatrick, 2016/09/19 17:04:19]

Is there another way that we can do this without splitting the logic between the
two classes?

[Gleb Lanbin, 2016/09/19 17:39:52]

that was my concern as well. I decided to keep it in this way in favor of clear
NGFragmentBuilder's API and readability of CollapseMargins method.

We need to find the first/last non-zero height children to update their parent's
margin strut. So we still need to use a some sort of flag to know whether the
parent's margin strut block-start has been already updated.

We already have this flag in NGFragmentBuilder and I don't like to overload the
implementation with IF statements.

[ikilpatrick, 2016/09/20 11:00:29]

I have a preference to leave this in a separate function inside BlockFlowLayout
(i.e. move UpdateMarginStrut to that class and introduce the flag).

Simply from a "leave the builder as simple as possible" POV. I'm just concerned
that additional complexity for some reason will end up here eventually.

 
   // Offsets are not supposed to be set during fragment construction, so we
   // do not provide a setter here.
@@ -41,6 +42,9 @@ class CORE_EXPORT NGFragmentBuilder final
   DEFINE_INLINE_VIRTUAL_TRACE() { visitor->trace(children_); }
 
  private:
+  void SetMarginStrutBlockStart(const NGMarginStrut& from);
+  void SetMarginStrutBlockEnd(const NGMarginStrut& from);
+
   NGPhysicalFragmentBase::NGFragmentType type_;
   NGWritingMode writing_mode_;
   NGDirection direction_;
@@ -53,10 +57,8 @@ class CORE_EXPORT NGFragmentBuilder final
   HeapVector<Member<NGPhysicalFragmentBase>> children_;
   Vector<NGLogicalOffset> offsets_;
 
-  // Whether MarginStrut block start/end was updated.
-  // It's used for DCHECK safety check.
+  // Whether MarginStrut block start is updated.
   bool is_margin_strut_block_start_updated_ : 1;
-  bool is_margin_strut_block_end_updated_ : 1;
 };
 
 }  // namespace blink