Implement collection of out-of-flow descendants

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"
 
@@ skipping 10 matching lines @@
   NGFragmentBuilder& SetDirection(TextDirection);
 
   NGFragmentBuilder& SetInlineSize(LayoutUnit);
   NGFragmentBuilder& SetBlockSize(LayoutUnit);
 
   NGFragmentBuilder& SetInlineOverflow(LayoutUnit);
   NGFragmentBuilder& SetBlockOverflow(LayoutUnit);
 
   NGFragmentBuilder& AddChild(NGFragmentBase*, NGLogicalOffset);
 
-  NGFragmentBuilder& SetOutOfFlowDescendants(WeakBoxList&,
-                                             Vector<NGLogicalOffset>&);
+  // Builder is not dumb when handling out of flow descendants.

[ikilpatrick, 2016/12/02 17:47:06]

Maybe:

The builder isn't "simple" when handling out of flow (OOF) descendants. It will
collect OOF descendants from:
 - AddChild (will add all descendants from the appended fragment).
 - AddOutOfFlowDescendantCandidate (directs adds a descendant).

[atotic, 2016/12/02 19:55:15]

entire comment revised for clarity

+  // Out of flow descendants API should be used like this:
+  // for (child : children)
+  //   if (child->position == (Absolute or Fixed)

[ikilpatrick, 2016/12/02 17:47:06]

missing closing ')'

[atotic, 2016/12/02 19:55:15]

entire comment revised for clarity

+  //     builder->AddOutOfFlowCandidateChild(child);
+  //   else
+  //     child->Layout()

[ikilpatrick, 2016/12/02 17:47:06]

i'd write:
fragment = child->Layout()

instead. (make clear that not adding input node).

[atotic, 2016/12/02 19:55:16]

done

+  //     builder_->AddChild(child)
+  // end
+  // builder->SetInlineSize/SetBlockSize
+  // while (builder->GetOutOfFlowDescendantCandidates(oof_candidates)

[ikilpatrick, 2016/12/02 17:47:06]

while loop bad?

[atotic, 2016/12/02 19:55:16]

entire comment revised for clarity

+  //    && oof_candidates.size() > 0)
+  // {
+  //   for (candidate : oof_candidates)
+  //     if (CanPosition(candidate)
+  //        candidate->Layout();
+  //        builder_->AddChild();
+  //     else
+  //        builder_->AddOutOfFlowDescendant();
+  // }

[ikilpatrick, 2016/12/02 17:47:06]

There are two cases:
 - The descendant came from a child fragment.
 - The descendant is a direct child of the current fragment.

[atotic, 2016/12/02 19:55:15]

entire comment revised for clarity

+  NGFragmentBuilder& AddOutOfFlowCandidateChild(NGBlockNode*, NGLogicalOffset);

[ikilpatrick, 2016/12/02 17:47:06]

AddOutOfFlowDescendantCandidate for consistency.

[atotic, 2016/12/02 19:55:15]

It can only be a child, so will not rename to Descendant.

Did rename to AddOutOfFlowChildCandidate for consistency with 
AddOutOfFlowDescendant, AddChild

+
+  void GetOutOfFlowDescendantCandidates(WeakBoxList&, Vector<NGCorner>&);
+
+  NGFragmentBuilder& AddOutOfFlowDescendant(NGBlockNode*, const NGCorner&);
 
   // Sets MarginStrut for the resultant fragment.
   NGFragmentBuilder& SetMarginStrutBlockStart(const NGMarginStrut& from);
   NGFragmentBuilder& SetMarginStrutBlockEnd(const NGMarginStrut& from);
 
   // 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();
 
   DECLARE_VIRTUAL_TRACE();
 
  private:
+  // Descendant offset information

[ikilpatrick, 2016/12/02 17:47:06]

Maybe:

Descendant offset information. We need information from two places to calculate
the NGCorner of the descendant correctly.
 - The inner physical offset (child_offset). This is set if the descendant came
from a child fragment. It is the offset within that fragment.
 - The outer logical offset (descendant_corner). This is set in both cases (see
above). It is the offset to the descendant, or the child fragment as
appropriate. We store the outer logical offset here separately as it is
impossible to convert to physical without the fragment size.  

[atotic, 2016/12/02 19:55:16]

Renamed OutOfFlowOffset to OutOfFlowPlacement for clarity, and wrote a lengthy
comment.

+  struct OutOfFlowOffset {
+    NGLogicalOffset child_offset;
+    NGCorner descendant_corner;
+  };
+
   NGPhysicalFragmentBase::NGFragmentType type_;
   NGWritingMode writing_mode_;
   TextDirection direction_;
 
   NGLogicalSize size_;
   NGLogicalSize overflow_;
 
   NGMarginStrut margin_strut_;
 
   HeapVector<Member<NGPhysicalFragmentBase>> children_;
   Vector<NGLogicalOffset> offsets_;
+
+  WeakBoxList out_of_flow_descendant_candidates_;
+  Vector<OutOfFlowOffset> oof_candidate_offsets_;

[ikilpatrick, 2016/12/02 17:47:06]

s/oof/out_of_flow/

to be consistent with rest of variable naming.

[atotic, 2016/12/02 19:55:15]

Will do.

Note: this is an example where "explicit variable naming" and "80 cols"
guidelines produce less readable code. out_of_flow_of_candidate_placements_ is
34 characters. This leaves only 47 characters left for other code, causing
lots and 
lots of
line 
wrapping

+
   WeakBoxList out_of_flow_descendants_;
-  Vector<NGLogicalOffset> out_of_flow_offsets_;
+  Vector<NGCorner> out_of_flow_corners_;
 };
 
 }  // namespace blink
 
 #endif  // NGFragmentBuilder