Avoid false-positives of paint offset change detection

third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/paint/PaintPropertyTreeBuilder.h"
 
 #include "core/dom/DOMNodeIds.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Settings.h"
@@ skipping 198 matching lines @@
   context.fixedPosition = context.current;
   context.fixedPosition.transform = fixedTransformNode;
   context.fixedPosition.scroll = fixedScrollNode;
 
   std::unique_ptr<PropertyTreeState> contentsState(new PropertyTreeState(
       context.current.transform, context.current.clip, context.currentEffect));
   frameView.setTotalPropertyTreeStateForContents(std::move(contentsState));
 }
 
 void PaintPropertyTreeBuilder::updatePaintOffsetTranslation(
-    const LayoutObject& object,
+    const LayoutBoxModelObject& object,
     PaintPropertyTreeBuilderContext& context) {
   bool usesPaintOffsetTranslation = false;
   if (RuntimeEnabledFeatures::rootLayerScrollingEnabled() &&
       object.isLayoutView()) {
     // Root layer scrolling always creates a translation node for LayoutView to
     // ensure fixed and absolute contexts use the correct transform space.
     usesPaintOffsetTranslation = true;
-  } else if (object.isBoxModelObject() &&
-             context.current.paintOffset != LayoutPoint()) {
-    PaintLayer* layer = toLayoutBoxModelObject(object).layer();
-    if (layer &&
-        layer->paintsWithTransform(GlobalPaintFlattenCompositingLayers))
-      usesPaintOffsetTranslation = true;
+  } else if (object.hasLayer() &&
+             context.current.paintOffset != LayoutPoint() &&
+             object.layer()->paintsWithTransform(
+                 GlobalPaintFlattenCompositingLayers)) {
+    usesPaintOffsetTranslation = true;
   }
 
   // We should use the same subpixel paint offset values for snapping
   // regardless of whether a transform is present. If there is a transform
   // we round the paint offset but keep around the residual fractional
   // component for the transformed content to paint with.  In spv1 this was
   // called "subpixel accumulation". For more information, see
   // PaintLayer::subpixelAccumulation() and
   // PaintLayerPainter::paintFragmentByApplyingTransform.
   IntPoint roundedPaintOffset = roundedIntPoint(context.current.paintOffset);
   LayoutPoint fractionalPaintOffset =
       LayoutPoint(context.current.paintOffset - roundedPaintOffset);
 
-  if (object.needsPaintPropertyUpdate() || context.forceSubtreeUpdate) {
-    if (usesPaintOffsetTranslation) {
-      auto& properties = object.getMutableForPainting().ensurePaintProperties();
-      context.forceSubtreeUpdate |= properties.updatePaintOffsetTranslation(
-          context.current.transform,
-          TransformationMatrix().translate(roundedPaintOffset.x(),
-                                           roundedPaintOffset.y()),
-          FloatPoint3D(), context.current.shouldFlattenInheritedTransform,
-          context.current.renderingContextId);
-    } else {
-      if (auto* properties = object.getMutableForPainting().paintProperties())
-        context.forceSubtreeUpdate |= properties->clearPaintOffsetTranslation();
-    }
+  if (usesPaintOffsetTranslation) {

[pdr., 2017/02/14 05:18:41]

Do we need to still early-out here if !(object.needsPaintPropertyUpdate() ||
context.forceSubtreeUpdate)?

[Xianzhu, 2017/02/14 19:23:34]

No, because paint offset can change without needsPaintPropertyUpdate().

The other choice (https://codereview.chromium.org/2688413005/ -- closed in favor
of this CL) is to ensure needsPaintPropertyUpdate() for any paint property
change including paint offset changes, so that any paint property update is
under needsPaintPropertyUpdate() || context.forceSubtreeUpdate condition.
However it seems hard to ensure the completeness and that CL is still incomplete
for now.

Note that we still depend on paint invalidation flags (mainly
mayNeedsPaintInvalidation) to ensure we'll reach any object whose paint offset
will change during PrePaintTreeWalk. Because we call
updateContextForBoxPosition() before checking the flags, we always walk and
update position one level more than the flags flagged. This might be unnecessary
and I would like to follow-up.

+    auto& properties = object.getMutableForPainting().ensurePaintProperties();
+    context.forceSubtreeUpdate |= properties.updatePaintOffsetTranslation(
+        context.current.transform,
+        TransformationMatrix().translate(roundedPaintOffset.x(),
+                                         roundedPaintOffset.y()),
+        FloatPoint3D(), context.current.shouldFlattenInheritedTransform,
+        context.current.renderingContextId);
+  } else {
+    if (auto* properties = object.getMutableForPainting().paintProperties())
+      context.forceSubtreeUpdate |= properties->clearPaintOffsetTranslation();
   }
 
   const auto* properties = object.paintProperties();
   if (properties && properties->paintOffsetTranslation()) {
     context.current.transform = properties->paintOffsetTranslation();
     context.current.paintOffset = fractionalPaintOffset;
     if (RuntimeEnabledFeatures::rootLayerScrollingEnabled() &&
         object.isLayoutView()) {
       context.absolutePosition.transform = properties->paintOffsetTranslation();
       context.fixedPosition.transform = properties->paintOffsetTranslation();
@@ skipping 587 matching lines @@
       return;
     }
   }
 
   if (object.needsPaintPropertyUpdate() || context.forceSubtreeUpdate) {
     if (auto* properties = object.getMutableForPainting().paintProperties())
       context.forceSubtreeUpdate |= properties->clearCssClipFixedPosition();
   }
 }
 
-void PaintPropertyTreeBuilder::updateContextForBoxPosition(
-    const LayoutObject& object,
+void PaintPropertyTreeBuilder::updatePaintOffset(
+    const LayoutBoxModelObject& object,
     PaintPropertyTreeBuilderContext& context) {
-  if (!object.isBoxModelObject())
-    return;
-
-  const LayoutBoxModelObject& boxModelObject = toLayoutBoxModelObject(object);
-
-  if (boxModelObject.isFloating())
+  if (object.isFloating())
     context.current.paintOffset = context.paintOffsetForFloat;
 
   // Multicolumn spanners are painted starting at the multicolumn container (but
   // still inherit properties in layout-tree order) so reset the paint offset.
-  if (boxModelObject.isColumnSpanAll())
-    context.current.paintOffset = boxModelObject.container()->paintOffset();
+  if (object.isColumnSpanAll())
+    context.current.paintOffset = object.container()->paintOffset();
 
   switch (object.styleRef().position()) {
     case EPosition::kStatic:
       break;
     case EPosition::kRelative:
-      context.current.paintOffset += boxModelObject.offsetForInFlowPosition();
+      context.current.paintOffset += object.offsetForInFlowPosition();
       break;
     case EPosition::kAbsolute: {
-      DCHECK(context.containerForAbsolutePosition ==
-             boxModelObject.container());
+      DCHECK(context.containerForAbsolutePosition == object.container());
       context.current = context.absolutePosition;
 
       // Absolutely positioned content in an inline should be positioned
       // relative to the inline.
       const LayoutObject* container = context.containerForAbsolutePosition;
       if (container && container->isInFlowPositioned() &&
           container->isLayoutInline()) {
         DCHECK(object.isBox());
         context.current.paintOffset +=
             toLayoutInline(container)->offsetForInFlowPositionedInline(
                 toLayoutBox(object));
       }
       break;
     }
     case EPosition::kSticky:
-      context.current.paintOffset += boxModelObject.offsetForInFlowPosition();
+      context.current.paintOffset += object.offsetForInFlowPosition();
       break;
     case EPosition::kFixed:
       context.current = context.fixedPosition;
       break;
     default:
       ASSERT_NOT_REACHED();
   }
 
-  if (boxModelObject.isBox()) {
+  if (object.isBox()) {
     // TODO(pdr): Several calls in this function walk back up the tree to
     // calculate containers (e.g., physicalLocation, offsetForInFlowPosition*).
     // The containing block and other containers can be stored on
     // PaintPropertyTreeBuilderContext instead of recomputing them.
-    context.current.paintOffset.moveBy(
-        toLayoutBox(boxModelObject).physicalLocation());
+    context.current.paintOffset.moveBy(toLayoutBox(object).physicalLocation());
     // This is a weird quirk that table cells paint as children of table rows,
     // but their location have the row's location baked-in.
     // Similar adjustment is done in LayoutTableCell::offsetFromContainer().
-    if (boxModelObject.isTableCell()) {
-      LayoutObject* parentRow = boxModelObject.parent();
+    if (object.isTableCell()) {
+      LayoutObject* parentRow = object.parent();
       DCHECK(parentRow && parentRow->isTableRow());
       context.current.paintOffset.moveBy(
           -toLayoutBox(parentRow)->physicalLocation());
     }
   }
+}
+
+void PaintPropertyTreeBuilder::updateContextForBoxPosition(
+    const LayoutObject& object,
+    PaintPropertyTreeBuilderContext& context) {
+  if (!object.isBoxModelObject()) {
+    object.getMutableForPainting().setPaintOffset(context.current.paintOffset);
+    return;
+  }
+
+  updatePaintOffset(toLayoutBoxModelObject(object), context);
+  updatePaintOffsetTranslation(toLayoutBoxModelObject(object), context);
+
+  if (object.paintOffset() == context.current.paintOffset)
+    return;
 
   // Many paint properties depend on paint offset so we force an update of
   // the entire subtree on paint offset changes.
-  if (object.paintOffset() != context.current.paintOffset)
-    context.forceSubtreeUpdate = true;
+  context.forceSubtreeUpdate = true;
+
+  if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
+    object.getMutableForPainting().setShouldDoFullPaintInvalidation(
+        PaintInvalidationLocationChange);
+  }
+  object.getMutableForPainting().setPaintOffset(context.current.paintOffset);
 }
 
 void PaintPropertyTreeBuilder::updatePropertiesForSelf(
     const LayoutObject& object,
     PaintPropertyTreeBuilderContext& context) {
 #if DCHECK_IS_ON()
   FindObjectPropertiesNeedingUpdateScope checkNeedsUpdateScope(object, context);
 #endif
 
   if (object.isBoxModelObject() || object.isSVG()) {
-    updatePaintOffsetTranslation(object, context);
     updateTransform(object, context);
     if (RuntimeEnabledFeatures::slimmingPaintV2Enabled())
       updateEffect(object, context);
     updateCssClip(object, context);
     updateLocalBorderBoxContext(object, context);
     if (RuntimeEnabledFeatures::slimmingPaintV2Enabled())
       updateScrollbarPaintOffset(object, context);
   }
-
-  if (object.needsPaintPropertyUpdate() || context.forceSubtreeUpdate) {
-    if (RuntimeEnabledFeatures::slimmingPaintV2Enabled() &&
-        object.paintOffset() != context.current.paintOffset) {
-      object.getMutableForPainting().setShouldDoFullPaintInvalidation(
-          PaintInvalidationLocationChange);
-    }
-    object.getMutableForPainting().setPaintOffset(context.current.paintOffset);
-  }
 }
 
 void PaintPropertyTreeBuilder::updatePropertiesForChildren(
     const LayoutObject& object,
     PaintPropertyTreeBuilderContext& context) {
 #if DCHECK_IS_ON()
   FindObjectPropertiesNeedingUpdateScope checkNeedsUpdateScope(object, context);
 #endif
 
   if (!object.isBoxModelObject() && !object.isSVG())
     return;
 
   updateOverflowClip(object, context);
   updatePerspective(object, context);
   updateSvgLocalToBorderBoxTransform(object, context);
   updateScrollAndScrollTranslation(object, context);
   updateOutOfFlowContext(object, context);
 
   context.forceSubtreeUpdate |= object.subtreeNeedsPaintPropertyUpdate();
 }
 
 }  // namespace blink