Introduce LayoutObject::AncestorSkipInfo.

third_party/WebKit/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
@@ skipping 877 matching lines @@
   // implemented using flex box but should still support things like
   // first-line, first-letter and text-overflow.
   // The flex box and grid specs require that flex box and grid do not
   // support first-line|first-letter, though.
   // TODO(cbiesinger): Remove when buttons are implemented with align-items
   // instead of flex box. crbug.com/226252.
   bool behavesLikeBlockContainer() const {
     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.
   //
   // This method is extremely similar to containingBlock(), but with a few
   // notable exceptions.
   // (1) For normal flow elements, it just returns the parent.
   // (2) For absolute positioned elements, it will return a relative
   //     positioned inline. containingBlock() simply skips relpositioned inlines
   //     and lets an enclosing block handle the layout of the positioned object.
   //     This does mean that computePositionedLogicalWidth and
   //     computePositionedLogicalHeight have to use container().
   //
   // Note that floating objects don't belong to either of the above exceptions.
   //
   // This function should be used for any invalidation as it would correctly
   // 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(); }
 
   Element* offsetParent(const Element* = nullptr) const;
 
   void markContainerChainForLayout(bool scheduleRelayout = true,
                                    SubtreeLayoutScope* = nullptr);
   void setNeedsLayout(LayoutInvalidationReasonForTracing,
                       MarkingBehavior = MarkContainerChain,
                       SubtreeLayoutScope* = nullptr);
@@ skipping 168 matching lines @@
   //   non-anonymous LayoutBlock.
   //   Note that in the previous examples, the returned LayoutBlock has no
   //   logical relationship to the original element.
   //
   // LayoutBlocks are the one that handle laying out positioned elements,
   // thus this function is important during layout, to insert the positioned
   // elements into the correct LayoutBlock.
   //
   // 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() ||
            canContainFixedPositionObjects();
   }
   bool canContainFixedPositionObjects() const {
     return isLayoutView() || isSVGForeignObject() ||
            (isLayoutBlock() && m_style->canContainFixedPositionObjects());
   }
 
@@ skipping 893 matching lines @@
   // LayoutBox recomputeOverflow-capable. crbug.com/437012 and crbug.com/434700.
   inline void markAncestorsForOverflowRecalcIfNeeded();
 
   inline void markAncestorsForPaintInvalidation();
 
   inline void invalidateContainerPreferredLogicalWidths();
 
   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;
 
   LayoutFlowThread* locateFlowThreadContainingBlock() const;
   void removeFromLayoutFlowThreadRecursive(LayoutFlowThread*);
 
   ComputedStyle* cachedFirstLineStyle() const;
   StyleDifference adjustStyleDifference(StyleDifference) const;
 
   Color selectionColor(int colorProperty, const GlobalPaintFlags) const;
@@ skipping 618 matching lines @@
 CORE_EXPORT void showLineTree(const blink::LayoutObject*);
 CORE_EXPORT void showLayoutTree(const blink::LayoutObject* object1);
 // We don't make object2 an optional parameter so that showLayoutTree
 // can be called from gdb easily.
 CORE_EXPORT void showLayoutTree(const blink::LayoutObject* object1,
                                 const blink::LayoutObject* object2);
 
 #endif
 
 #endif  // LayoutObject_h