Add documentation to several LayoutObject functions

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 588f37a12c2ea11997f45725ae62949f512d0f11..97b8e3d3b30b8440e8c4ac6f44d4e944c8db939f 100644
--- a/Source/core/layout/LayoutObject.h
+++ b/Source/core/layout/LayoutObject.h
@@ -140,6 +140,10 @@ const int showTreeCharacterOffset = 39;
 // Due to the high cost of layout, a lot of effort is done to avoid doing full layouts of nodes.
 // 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.
+//
+// To save memory, especially for the common child class LayoutText, LayoutObject doesn't provide
+// storage for children. Descendant classes that do allow children have to have a LayoutObjectChildList
+// member that stores the actual children and override virtualChildren().
 class CORE_EXPORT LayoutObject : public ImageResourceClient {
     friend class LayoutObjectChildList;
     WTF_MAKE_NONCOPYABLE(LayoutObject);
@@ -178,6 +182,7 @@ public:
         return nullptr;
     }
 
+    // See comment in the class description as to why there is no child.
     virtual LayoutObjectChildList* virtualChildren() { return nullptr; }
     virtual const LayoutObjectChildList* virtualChildren() const { return nullptr; }
 
@@ -334,6 +339,21 @@ public:
     void showLayoutTreeAndMark(const LayoutObject* markedObject1 = nullptr, const char* markedLabel1 = nullptr, const LayoutObject* markedObject2 = nullptr, const char* markedLabel2 = nullptr, int depth = 0) const;
 #endif
 
+    // This function is used to create the appropriate LayoutObject based
+    // on the style, in particular 'display' and 'content'.
+    // "display: none" is the only time this function will return nullptr.
+    //
+    // For renderer creation, the inline-* values create the same renderer
+    // as the non-inline version. The difference is that inline-* sets
+    // m_isInline below. This means that "display: inline-table" creates

[mstensho (USE GERRIT), 2015/08/31 19:48:07]

"below", as in "during initialization"?

[Julien - ping for review, 2015/08/31 22:38:53]

"below" as in ↓ (unicode FTW!)

Changed anyway as during initialization is a better phrasing.

[mstensho (USE GERRIT), 2015/09/01 07:56:23]

☑

+    // a LayoutTable, like "display: table".
+    //
+    // Ideally every Element::createLayoutObject would call this function to

[mstensho (USE GERRIT), 2015/08/31 19:48:07]

I either don't understand or don't agree.

Isn't it great that we have Element::createLayoutObject overrides, so that we
don't have to cram everything in here, and only worry about the generics? If you
don't think so, come over here, and I'll show you the Presto code. :)

[Julien - ping for review, 2015/08/31 22:38:53]

No Presto code open showroom please :)

We have a lot of bugs where people don't understand why display: block on
<iframe> doesn't work. Granted it's probably a corner-case but there should be a
cleaner mid-way between cramming everything in createObject (Presto) and not
replying to display change half of the time (Blink).

Happy to rephrase the idea is that we should be consistent with our treatment of
'display' by having createLayoutObject systematically call into this (probably
passing in some context along to handle the tag specific information).

[mstensho (USE GERRIT), 2015/09/01 07:56:23]

At the same time, those people should be thankful that display:table-row doesn't
work on <iframe>. :)

Right, so LayoutIFrame, for instance, forgets to set m_isInline = false? Hm..
no, that doesn't seem to be the case. This just works:

before
<iframe src="http://bitten.servebeer.com/" style="display:block;"></iframe>
after
In my mind, createObject() can only do harm if the Element has a better idea
about what to create. If we introduce a separation between display-outer and
display-inner, it could make sense to let createObject() deal with the
display-outer part, though. Then we could make things like <input
style="display:table-cell;"> work.

[Julien - ping for review, 2015/09/01 16:18:29]

Actually that's what I am advocating. Currently we only extract the inline-level
/ block-level information from the style, which confuses people.
<3 :)
I think we agree but somehow talk past each other: I am not advocating for
adding more logic to LayoutObject::createObject but making sure Element's
createLayoutObject take style into account by calling createObject (while
keeping their element specific logic). I haven't outlined a plan to make that
work but put that more as a statement of intention / design note.

I do think this comment represents the consensus on createObject and what should
be done but maybe I am misinterpreting your objections. Should I just remove it
or can we amend it in some ways?

[mstensho (USE GERRIT), 2015/09/01 18:15:28]

Okay, I cannot think of a lot else to extract, but
HTMLRubyElement::createLayoutObject() may be an interesting case, for instance.
We should at least be able to display ruby as inline-block, but we only handle
"inline" and "block". Then again, one might argue that the whole approach here
is wrong and that we should just implement the Ruby CSS spec instead and support
ruby specific display values and just add ruby { display:ruby; } to html.css, so
maybe yet another bad example.

Anyway, my brain starts to act up when I think of <iframe
style="display:table-row;">, but let's not talk more about that for now. :)
Let's just keep it as it is.

+    // respond to 'display' but there are deep rooted assumptions about
+    // which LayoutObject is created on a fair number of Elements. This
+    // function also doesn't handle the default association between a tag
+    // and its renderer (e.g. <iframe> creates a LayoutIFrame even if the
+    // initial 'display' value is inline).
     static LayoutObject* createObject(Element*, const ComputedStyle&);
 
     // LayoutObjects are allocated out of the rendering partition.
@@ -1169,6 +1189,18 @@ protected:
     void propagateStyleToAnonymousChildren(bool blockChildrenOnly = false);
 
 protected:
+    // This function is called before calling the destructor so that some clean-up
+    // can happen regardless of whether they call a virtual function or not. As a
+    // rule of thumb, this function should be preferred to the destructor. See
+    // destroy() that is the one calling willBeDestroyed().
+    //
+    // There are 2 types of destructions: regular destructions and tree tear-down.
+    // Regular destructions happen when the renderer is not needed anymore (e.g.
+    // 'display' changed or the DOM Node was removed).
+    // Tree tear-down is when the whole tree destroyed during navigation. It is
+    // handled in the code by checking if documentBeingDestroyed() returns 'true'.
+    // In this case, the code skips some unneeded expensive operations as we know
+    // the tree is not reused (e.g. avoid clearing the containing block's line box).
     virtual void willBeDestroyed();
 
     virtual void insertedIntoTree();
@@ -1430,7 +1462,12 @@ private:
         ADD_BOOLEAN_BITFIELD(isAnonymous, IsAnonymous);
         ADD_BOOLEAN_BITFIELD(isText, IsText);
         ADD_BOOLEAN_BITFIELD(isBox, IsBox);
+
+        // This boolean represents whether the LayoutObject is laid out as part
+        // of the current line (inline) or generate a new formatting context

[mstensho (USE GERRIT), 2015/08/31 19:48:07]

Doesn't necessarily generate a new formatting context if it's false. Example:
<div>

To say something CSS lingo compatible here, I could suggest "inline-level" vs.
"block-level".

[Julien - ping for review, 2015/08/31 22:38:53]

Fair point, changed to some better CSS lingo:

// This boolean represents whether the LayoutObject is 'inline-level'
// (a CSS concept). Inline-level boxes are laid out inside a line. If
// unset, the box is 'block-level' and thus stack on top of its
// siblings (think of paragraph).

[mstensho (USE GERRIT), 2015/09/01 07:56:23]

Nice!

+        // (e.g. block, grid, flex ...).
         ADD_BOOLEAN_BITFIELD(isInline, IsInline);
+
         ADD_BOOLEAN_BITFIELD(isReplaced, IsReplaced);
         ADD_BOOLEAN_BITFIELD(horizontalWritingMode, HorizontalWritingMode);
         ADD_BOOLEAN_BITFIELD(isDragging, IsDragging);