Add some better documentation to LayoutObject

Source/core/layout/LayoutObject.h

Index: Source/core/layout/LayoutObject.h
diff --git a/Source/core/layout/LayoutObject.h b/Source/core/layout/LayoutObject.h
index 06e8d0270fba1af8f836169364b4e2f30ebf0d3b..3ea7d0b6aae0d8b7d6f420cae103fbb974c3d541 100644
--- a/Source/core/layout/LayoutObject.h
+++ b/Source/core/layout/LayoutObject.h
@@ -115,7 +115,25 @@ typedef WTF::HashMap<const DeprecatedPaintLayer*, Vector<LayoutRect>> LayerHitTe
 const int showTreeCharacterOffset = 39;
 #endif
 
-// Base class for all layout tree objects.
+// LayoutObject is the base class for all layout tree objects.
+//

[pdr., 2015/08/27 04:09:48]

It may be useful to add a blurb about the layout tree being rooted at a
LayoutView which roughly corresponds to the Document node.

[Julien - ping for review, 2015/08/27 18:56:55]

Added:

// The root of the LayoutObject tree is the LayoutView, which is the
LayoutObject associated
// with the Document.

+// LayoutObjects form a tree structure that is a close mapping of the DOM tree.
+// Some LayoutObjects don't have an associated Node and are called "anonymous" (see the constructor

[skobes, 2015/08/27 00:55:03]

Also mention that some Nodes don't have an associated LayoutObject (e.g.
display:none).

[Julien - ping for review, 2015/08/27 18:56:54]

Added:

// Also some Node don't have an associated LayoutObjects e.g. if display: none
is set. For more
// detail, see LayoutObject::createObject that creates the right LayoutObject
based on the style.

+// below). Anonymous LayoutObjects exists for several purposes but are usually required by CSS. A
+// good example is anonymous table parts (http://www.w3.org/TR/CSS21/tables.html#anonymous-boxes).
+//
+// Because the SVG and CSS classes both inherit from this object, functions can belong to either
+// realm and sometimes to both.
+//
+// The purpose of the layout tree is to do layout (aka reflow) and store its results for painting and
+// hit-testing.
+// Layout is the process of sizing and positioning Nodes on the page. In Blink, layouts always start

[skobes, 2015/08/27 00:55:03]

Shouldn't that be "sizing and positioning LayoutObjects"?

[Julien - ping for review, 2015/08/27 18:56:55]

Conceptually either phrasing works and I don't feel strongly about either.

I thought about layout in terms of what the author writes (html, which is
translated into the DOM). The Layout tree is just our internal structure to
handle laying out and is an implementation detail from this perspective.

+// from a relayout boundary (see objectIsRelayoutBoundary in LayoutObject.cpp). As such, we need to mark
+// the ancestors all the way to the enclosing relayout boundary in order to do a correct layout.
+//
+// Due to the hight cost of layout, a lot of effort is done to avoid doing full layouts of nodes.

[skobes, 2015/08/27 00:55:03]

typo: "high cost"

[Julien - ping for review, 2015/08/27 18:56:55]

Corrected.

+// This is why there are several types of layout available to bypass the complex operations. See the
+// comments on the layout booleans in LayoutObjectBitfields below about the different layouts.
 class CORE_EXPORT LayoutObject : public ImageResourceClient {
     friend class LayoutBlock;
     friend class LayoutBlockFlow;
@@ -1275,16 +1293,16 @@ private:
     public:
         LayoutObjectBitfields(Node* node)
             : m_selfNeedsLayout(false)
-            , m_shouldInvalidateOverflowForPaint(false)
-            , m_childShouldCheckForPaintInvalidation(false)
-            , m_mayNeedPaintInvalidation(false)
-            , m_shouldInvalidateSelection(false)
-            , m_neededLayoutBecauseOfChildren(false)
             , m_needsPositionedMovementLayout(false)
             , m_normalChildNeedsLayout(false)
             , m_posChildNeedsLayout(false)
             , m_needsSimplifiedNormalFlowLayout(false)
             , m_preferredLogicalWidthsDirty(false)
+            , m_shouldInvalidateOverflowForPaint(false)
+            , m_childShouldCheckForPaintInvalidation(false)
+            , m_mayNeedPaintInvalidation(false)
+            , m_shouldInvalidateSelection(false)
+            , m_neededLayoutBecauseOfChildren(false)
             , m_floating(false)
             , m_selfNeedsOverflowRecalcAfterStyleChange(false)
             , m_childNeedsOverflowRecalcAfterStyleChange(false)
@@ -1319,21 +1337,65 @@ private:
         }
 
         // 32 bits have been used in the first word, and 16 in the second.
+
+        // Self needs layout means that this layout object is marked for a full layout.
+        // This is the default layout but it is expensive as it recomputes everything.

[dsinclair, 2015/08/27 14:09:24]

It won't, necessarily, re-layout children if they aren't marked thought,
correct?

[Julien - ping for review, 2015/08/27 18:56:55]

That's not correct. I added a comment to LayoutObject::layout, which should
answer your question:

// By default, layout only lays out the children that are marked for layout.
// In some cases, layout has to force laying out more children. An example is
// when the width of the LayoutObject changes as this impacts children with
// 'width' set to auto.

+        // For CSS boxes, this includes the width (laying out the line boxes again), the margins
+        // (due to block collapsing margins), the positions, the height and the potential overflow.
         ADD_BOOLEAN_BITFIELD(selfNeedsLayout, SelfNeedsLayout);
-        ADD_BOOLEAN_BITFIELD(shouldInvalidateOverflowForPaint, ShouldInvalidateOverflowForPaint); // TODO(wangxianzhu): Remove for slimming paint v2.
-        ADD_BOOLEAN_BITFIELD(childShouldCheckForPaintInvalidation, ChildShouldCheckForPaintInvalidation);
-        ADD_BOOLEAN_BITFIELD(mayNeedPaintInvalidation, MayNeedPaintInvalidation);
-        ADD_BOOLEAN_BITFIELD(shouldInvalidateSelection, ShouldInvalidateSelection); // TODO(wangxianzhu): Remove for slimming paint v2.
-        ADD_BOOLEAN_BITFIELD(neededLayoutBecauseOfChildren, NeededLayoutBecauseOfChildren); // TODO(wangxianzhu): Remove for slimming paint v2.
+
+        // A positioned movement layout is a specialized type of layout used on positioned objects
+        // that only visually moved. This layout is used when changing 'top'/'left' on a positioned
+        // element or margins on an out-of-flow one. Because the following operations don't impact
+        // the size of the object or sibling LayoutObjects, this layout is very lightweight.
+        //
+        // Positioned movement layout is implemented in LayoutBlock::simplifiedLayout.
         ADD_BOOLEAN_BITFIELD(needsPositionedMovementLayout, NeedsPositionedMovementLayout);
+
+        // This boolean is set when a normal flow ('position' == static || relative) child requires
+        // layout (but this object doesn't). Due to the nature of CSS, relaying out a child can cause

[pdr., 2015/08/27 04:09:48]

Nit: "relaying" was a little confusing to me.

Maybe: Laying out a child can cause the parent to resize (e.g., if 'height' is
auto).

[Julien - ping for review, 2015/08/27 18:56:54]

Applied your suggestion.

+        // this object to resize (e.g. if 'height' is auto).
         ADD_BOOLEAN_BITFIELD(normalChildNeedsLayout, NormalChildNeedsLayout);
+
+        // This boolean is set when an out-of-flow positioned ('position' == fixed || absolute) child
+        // requires layout (but this object doesn't).
         ADD_BOOLEAN_BITFIELD(posChildNeedsLayout, PosChildNeedsLayout);
+
+        // Simplified normal flow layout only relayouts the normal flow children, ignoring the
+        // out-of-flow descendants.
+        //
+        // The implementation of this layout is in Simplified LayoutBlock::simplifiedNormalFlowLayout.

[dsinclair, 2015/08/27 14:09:24]

nit: Is that Simplified supposed to be there?

[Julien - ping for review, 2015/08/27 18:56:55]

Hell no! Removed.

         ADD_BOOLEAN_BITFIELD(needsSimplifiedNormalFlowLayout, NeedsSimplifiedNormalFlowLayout);
-        ADD_BOOLEAN_BITFIELD(preferredLogicalWidthsDirty, PreferredLogicalWidthsDirty);
-        ADD_BOOLEAN_BITFIELD(floating, Floating);
+
+        // Some properties only have a visual impact and don't impact the actual layout position and
+        // sizes of the object. An example of this is the 'transform' property, who doesn't modify the
+        // layout but gets applied at paint time.
+        // Setting this flag only recompute the overflow information.

[wkorman, 2015/08/27 05:02:30]

recomputes

[Julien - ping for review, 2015/08/27 18:56:54]

Done.

         ADD_BOOLEAN_BITFIELD(selfNeedsOverflowRecalcAfterStyleChange, SelfNeedsOverflowRecalcAfterStyleChange);
+
+        // This flag is set on the ancestor of a LayoutObject needing
+        // selfNeedsOverflowRecalcAfterStyleChange. This is needed as a descendant overflow can
+        // bleed into its containing block's so we have to recompute it in some case.

[wkorman, 2015/08/27 05:02:30]

cases

[Julien - ping for review, 2015/08/27 18:56:55]

Done.

         ADD_BOOLEAN_BITFIELD(childNeedsOverflowRecalcAfterStyleChange, ChildNeedsOverflowRecalcAfterStyleChange);
 
+        // The preferred logical widths are the intrinsic sizes of this element.
+        // Intrinsic means that they depend purely on the content, not on the style

[skobes, 2015/08/27 00:55:03]

This seems wrong, some styles certainly affect the intrinsic size (e.g.
font-size).  Maybe this should say "depend purely on the content, not on the
space available from the container"?

[Julien - ping for review, 2015/08/27 18:56:55]

You're right that it's not accurate. Only referring to the available space is
too restrictive IMO as it seems to oppose it to the normal width that includes
it (but also the layout algorithm selected with 'display'). CSS uses the
following definition but it seems hand-wavy too:

"Intrinsic sizing determines sizes based on the contents of an element, without
regard for its context."

I improved the original text by changing it to:

// Intrinsic sizes depend mostly on the content and a limited set of style
// properties (e.g. any font-related property for text,
'min-width'/'max-width').
//
// Those widths are used to determine the final layout logical width, which
// depends on the layout algorithm used and the available logical width.

+        // set on the elements.
+        //
+        // Blink stores them in LayoutBox (m_minPreferredLogicalWidth and
+        // m_maxPreferredLogicalWidth). They are lazily computed during layout.
+        //
+        // Setting this boolean marks both widths for recomputation when
+        // LayoutBox::minPreferredLogicalWidth() or maxPreferredLogicalWidth() is called.
+        ADD_BOOLEAN_BITFIELD(preferredLogicalWidthsDirty, PreferredLogicalWidthsDirty);
+
+        ADD_BOOLEAN_BITFIELD(shouldInvalidateOverflowForPaint, ShouldInvalidateOverflowForPaint); // TODO(wangxianzhu): Remove for slimming paint v2.
+        ADD_BOOLEAN_BITFIELD(childShouldCheckForPaintInvalidation, ChildShouldCheckForPaintInvalidation);
+        ADD_BOOLEAN_BITFIELD(mayNeedPaintInvalidation, MayNeedPaintInvalidation);
+        ADD_BOOLEAN_BITFIELD(shouldInvalidateSelection, ShouldInvalidateSelection); // TODO(wangxianzhu): Remove for slimming paint v2.
+        ADD_BOOLEAN_BITFIELD(neededLayoutBecauseOfChildren, NeededLayoutBecauseOfChildren); // TODO(wangxianzhu): Remove for slimming paint v2.
+        ADD_BOOLEAN_BITFIELD(floating, Floating);
+
         ADD_BOOLEAN_BITFIELD(isAnonymous, IsAnonymous);
         ADD_BOOLEAN_BITFIELD(isText, IsText);
         ADD_BOOLEAN_BITFIELD(isBox, IsBox);