Ignore zero-height fragments during margin collapsing

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NGFragmentBuilder_h
 #define NGFragmentBuilder_h
 
 #include "core/layout/ng/ng_fragment.h"
 #include "core/layout/ng/ng_units.h"
 
 namespace blink {
 
 class CORE_EXPORT NGFragmentBuilder final
     : public GarbageCollectedFinalized<NGFragmentBuilder> {
  public:
   NGFragmentBuilder(NGPhysicalFragmentBase::NGFragmentType);
 
   NGFragmentBuilder& SetWritingMode(NGWritingMode);
   NGFragmentBuilder& SetDirection(NGDirection);
 
   NGFragmentBuilder& SetInlineSize(LayoutUnit);
   NGFragmentBuilder& SetBlockSize(LayoutUnit);
 
   NGFragmentBuilder& SetInlineOverflow(LayoutUnit);
   NGFragmentBuilder& SetBlockOverflow(LayoutUnit);
 
   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.
 
   // Creates the fragment. Can only be called once.
   NGPhysicalFragment* ToFragment();
 
   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_;
 
   NGLogicalSize size_;
   NGLogicalSize overflow_;
 
   NGMarginStrut margin_strut_;
 
   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
 
 #endif  // NGFragmentBuilder