Add documentation to several LayoutObject functions

Source/core/layout/LayoutObject.h

 /*
  * Copyright (C) 2000 Lars Knoll (knoll@kde.org)
  *           (C) 2000 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  *           (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 122 matching lines @@
 //
 // 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
 // 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 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);
 public:
     // Anonymous objects should pass the document as their node, and they will then automatically be
     // marked as anonymous in the constructor.
     explicit LayoutObject(Node*);
     ~LayoutObject() override;
 
     // Returns the name of the layout object.
@@ skipping 18 matching lines @@
             return children->firstChild();
         return nullptr;
     }
     LayoutObject* slowLastChild() const
     {
         if (const LayoutObjectChildList* children = virtualChildren())
             return children->lastChild();
         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; }
 
     LayoutObject* nextInPreOrder() const;
     LayoutObject* nextInPreOrder(const LayoutObject* stayWithin) const;
     LayoutObject* nextInPreOrderAfterChildren() const;
     LayoutObject* nextInPreOrderAfterChildren(const LayoutObject* stayWithin) const;
     LayoutObject* previousInPreOrder() const;
     LayoutObject* previousInPreOrder(const LayoutObject* stayWithin) const;
     LayoutObject* childAt(unsigned) const;
@@ skipping 136 matching lines @@
     void showLayoutTreeForThis() const;
     void showLineTreeForThis() const;
 
     void showLayoutObject() const;
     // We don't make printedCharacters an optional parameter so that
     // showLayoutObject can be called from gdb easily.
     void showLayoutObject(int printedCharacters) const;
     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.
     void* operator new(size_t);
     void operator delete(void*);
 
 public:
     bool isPseudoElement() const { return node() && node()->isPseudoElement(); }
 
     virtual bool isBoxModelObject() const { return false; }
@@ skipping 815 matching lines @@
 
     // Overrides should call the superclass at the end. m_style will be 0 the first time
     // this function will be called.
     virtual void styleWillChange(StyleDifference, const ComputedStyle& newStyle);
     // Overrides should call the superclass at the start. |oldStyle| will be 0 the first
     // time this function is called.
     virtual void styleDidChange(StyleDifference, const ComputedStyle* oldStyle);
     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();
     virtual void willBeRemovedFromTree();
 
     void setDocumentForAnonymous(Document* document) { ASSERT(isAnonymous()); m_node = document; }
 
     // Add hit-test rects for the layout tree rooted at this node to the provided collection on a
     // per-Layer basis.
     // currentLayer must be the enclosing layer, and layerOffset is the current offset within
@@ skipping 241 matching lines @@
         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);
+
+        // 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);
 
         ADD_BOOLEAN_BITFIELD(hasLayer, HasLayer);
         ADD_BOOLEAN_BITFIELD(hasOverflowClip, HasOverflowClip); // Set in the case of overflow:auto/scroll/hidden
         ADD_BOOLEAN_BITFIELD(hasTransformRelatedProperty, HasTransformRelatedProperty);
         ADD_BOOLEAN_BITFIELD(hasReflection, HasReflection);
 
         ADD_BOOLEAN_BITFIELD(hasCounterNodeMap, HasCounterNodeMap);
@@ skipping 309 matching lines @@
 void showTree(const blink::LayoutObject*);
 void showLineTree(const blink::LayoutObject*);
 void showLayoutTree(const blink::LayoutObject* object1);
 // We don't make object2 an optional parameter so that showLayoutTree
 // can be called from gdb easily.
 void showLayoutTree(const blink::LayoutObject* object1, const blink::LayoutObject* object2);
 
 #endif
 
 #endif // LayoutObject_h