Introduce LayoutObject::AncestorSkipInfo.

third_party/WebKit/Source/core/layout/LayoutObject.h

Index: third_party/WebKit/Source/core/layout/LayoutObject.h
diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.h b/third_party/WebKit/Source/core/layout/LayoutObject.h
index 3b28df12550cf8720dad571dd0bd6206e632dd34..518e6ed2f1d7c5a4b03675ea350b1ecbfa8c6c98 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.h
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.h
@@ -895,6 +895,34 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
     return isLayoutBlockFlow() || isLayoutButton();
   }
 
+  // May be optionally passed to container() and various other similar methods
+  // that search the ancestry for some sort of containing block.
+  struct AncestorSkipInfo {
+    AncestorSkipInfo() {}

[Xianzhu, 2017/01/15 23:05:05]

Is the above constructor needed?

[mstensho (USE GERRIT), 2017/01/16 09:34:04]

Maybe it's not used at the moment, but it should be okay to not specify an
ancestor (if you only want to know if you skipped filters, typically).

Maybe better to add a default value to |ancestor| to the other constructor
instead.

+    AncestorSkipInfo(const LayoutObject* ancestor) : ancestor(ancestor) {}
+
+    void resetOutput() {
+      ancestorSkipped = false;
+      filterSkipped = false;
+    }

[Xianzhu, 2017/01/15 19:41:13]

As we have default values for the fields, do we still need this method?

[mstensho (USE GERRIT), 2017/01/15 21:09:25]

There is a unit test that called container() twice in a row, with different
ancestors to look for. Maybe it's better to create two AncestorSkipInfo objects,
instead of allowing an AncestorSkipInfo object to be re-used? Looks like there's
currently no other use case for this method. So, yeah, I suggest removing it
again.

[Xianzhu, 2017/01/15 23:05:05]

Agreed (about setAncestor() :) ).

[mstensho (USE GERRIT), 2017/01/16 09:34:04]

Done. Removed.

+    void setAncestor(const LayoutObject* ancestor) {
+      this->ancestor = ancestor;
+      resetOutput();
+    }

[Xianzhu, 2017/01/15 19:41:13]

What's the purpose of this method?

[mstensho (USE GERRIT), 2017/01/15 21:09:25]

This will update this AncestorSkipInfo object based on the LayoutObject passed,
i.e. flip flags to true if we skipped something. Do you want some documentation
right here, or a better name (or both)? updateFromLayoutObject()?

[Xianzhu, 2017/01/15 23:05:05]

I meant setAncestor() here. Sorry for the confusion.

The name of update() looks fine.

+    void update(const LayoutObject&);

[Xianzhu, 2017/01/15 19:41:13]

I'm a bit concerned about performance of this method which looks a hot spot. I
think we should inline this method, and optimize hasFilterIncludingProperty()
(which previously was not called when filterSkipped == nullptr) perhaps by
backing it with with a bitfield which is updated during setStyle.

[mstensho (USE GERRIT), 2017/01/15 21:09:25]

I was kind of assuming that it wasn't the common case to want to know if we
skipped something, so that it would better to keep it non-inline, rather than
"bloating" each call site with code. But maybe I was wrong. Then again: Are
function calls really that expensive? Keep in mind that we now pass one pointer
instead of three on each call to contain*(). I don't know how pushing two
pointers on the stack compares to actually calling a function, though. :)

Good point about hasFilterIncludingProperty(); I failed to realize that it will
potentially be called way more often with this patch. If this is likely to
degrade performance, maybe AncestorSkipInfo needs to be told whether we want to
check for filters or not?

[Xianzhu, 2017/01/15 23:05:05]

The difference is that pushing two pointers on the stack happened only once for
each contain*() call, but now update() is called from loops in the functions. We
did observe that frequent calls to tiny functions affected performance and
inlining them helped (e.g. https://codereview.chromium.org/2550083002).

In my optimized build on x64 Linux, update() contains just 14 instructions (and
only 6 instructions if I make hasFilterInducingProperty() just access a
bitfield), so code bloating seems not an issue.
I prefer using LayoutObjectBitfields to store hasFilterInducingProperty:
  set the bitfield during setStyle, and
  void hasFilterInducingProperty() const { return
m_bitfields.hasFilterInducingProperty(); }

In this way, AncestorSkipInfo::update() will be only 6 instructions, and
checking for filters is only 3 instructions which is equal or more efficient
than using another field in AncestorSkipInfo to control whether we want to check
for filters.

[mstensho (USE GERRIT), 2017/01/16 09:34:04]

Done. Also made update() inline.

[mstensho (USE GERRIT), 2017/01/16 14:39:02]

This turned out to be tricky (see trybot failures from patch set 3).
hasFilterInducingProperty() calls hasReflection(), which changes value in the
styleDidChange() machinery. I also tried to just cache style()->hasFilter() as a
bit in LayoutObject, but I got problems even with that. It's probably
achievable, but I just got too scared of breaking something. So I ended up with
an input flag to AncestorSkipInfo instead, to tell whether or not to look for
filters.

+
+    // Input: A potential ancestor to look for. If we walk past this one while
+    // walking the ancestry in search of some containing block, ancestorSkipped
+    // will be set to true.
+    const LayoutObject* ancestor = nullptr;
+    // Output: Set to true if |ancestor| was walked past while walking the
+    // ancestry.
+    bool ancestorSkipped = false;
+    // Output: Set to true if we walked past a filter object. This will be set
+    // regardless of the value of |ancestor|.
+    bool filterSkipped = false;
+  };
+
   // This function returns the containing block of the object.
   // Due to CSS being inconsistent, a containing block can be a relatively
   // positioned inline, thus we can't return a LayoutBlock from this function.
@@ -914,23 +942,12 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
   // walk the containing block chain. See e.g. markContainerChainForLayout.
   // It is also used for correctly sizing absolutely positioned elements
   // (point 3 above).
-  //
-  // If |ancestor| and |ancestorSkipped| are not null, on return
-  // *ancestorSkipped is true if the layoutObject returned is an ancestor of
-  // |ancestor|.
-  LayoutObject* container(const LayoutBoxModelObject* ancestor = nullptr,
-                          bool* ancestorSkipped = nullptr,
-                          bool* filterSkipped = nullptr) const;
+  LayoutObject* container(AncestorSkipInfo* = nullptr) const;
   // Finds the container as if this object is fixed-position.
-  LayoutBlock* containerForFixedPosition(
-      const LayoutBoxModelObject* ancestor = nullptr,
-      bool* ancestorSkipped = nullptr,
-      bool* filterSkipped = nullptr) const;
+  LayoutBlock* containerForFixedPosition(AncestorSkipInfo* = nullptr) const;
   // Finds the containing block as if this object is absolute-position.
   LayoutBlock* containingBlockForAbsolutePosition(
-      const LayoutBoxModelObject* ancestor = nullptr,
-      bool* ancestorSkipped = nullptr,
-      bool* filterSkipped = nullptr) const;
+      AncestorSkipInfo* = nullptr) const;
 
   virtual LayoutObject* hoverAncestor() const { return parent(); }
 
@@ -1119,9 +1136,7 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
   //
   // See container() for the function that returns the containing block.
   // See LayoutBlock.h for some extra explanations on containing blocks.
-  LayoutBlock* containingBlock(const LayoutBoxModelObject* ancestor = nullptr,
-                               bool* ancestorSkipped = nullptr,
-                               bool* filterSkipped = nullptr) const;
+  LayoutBlock* containingBlock(AncestorSkipInfo* = nullptr) const;
 
   bool canContainAbsolutePositionObjects() const {
     return m_style->canContainAbsolutePositionObjects() ||
@@ -2035,10 +2050,7 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
   void invalidatePaintIncludingNonSelfPaintingLayerDescendantsInternal(
       const LayoutBoxModelObject& paintInvalidationContainer);
 
-  LayoutObject* containerForAbsolutePosition(
-      const LayoutBoxModelObject* ancestor = nullptr,
-      bool* ancestorSkipped = nullptr,
-      bool* filterSkipped = nullptr) const;
+  LayoutObject* containerForAbsolutePosition(AncestorSkipInfo* = nullptr) const;
 
   const LayoutBoxModelObject* enclosingCompositedContainer() const;