[SPInvalidation] Fix paint offset issue of descendants of composited svg root

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

 // Copyright 2016 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/PaintInvalidator.h"
 
 #include "core/editing/FrameSelection.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Settings.h"
@@ skipping 60 matching lines @@
         rect.moveBy(FloatPoint(context.treeBuilderContext.current.paintOffset));
 
         GeometryPropertyTreeState currentTreeState(context.treeBuilderContext.current.transform, context.treeBuilderContext.current.clip, context.treeBuilderContext.currentEffect);
         GeometryPropertyTreeState containerTreeState;
         const ObjectPaintProperties* containerPaintProperties = context.paintInvalidationContainer->objectPaintProperties();
         containerPaintProperties->getContentsProperties(containerTreeState);
 
         bool success = false;
         result = LayoutRect(geometryMapper.mapToVisualRectInDestinationSpace(rect, currentTreeState, containerTreeState, success));
         DCHECK(success);
-        result.moveBy(-containerPaintProperties->localBorderBoxProperties()->paintOffset);
+
+        // Exclude container's paint offset from the result to get it in container's local coordinates,

[pdr., 2016/09/29 21:00:54]

If possible I'd like to avoid having to do this kind of code. It's best if we
can rely on the property tree being exactly right so we don't need special cases
in all the users of the tree. Can we refactor how the property tree is organized
so this special case is not needed? For example, maybe zero out the paint offset
for foreign object?

[Xianzhu, 2016/09/29 22:18:06]

We do zero out the paint offset in PaintPropertyTreeBuilderContext, but that'd
be incorrect to zero out it in the container's localBorderBoxProperties. We need
the special case because of the difference between the state of the border box
space and the contents space needed by paint invalidation.

Tried to make it clearer by adding an out parameter paintOffsetFromState for
ObjectPaintProperties::getContentsPropertyTreeState() to indicate the paint
offset that is not baked into the property tree state yet.

[pdr., 2016/09/29 23:31:54]

Your latest patch does make this much clearer but I think it has exposed that we
made a mistake in the updateSvgLocalToBorderBoxTransform patch.

For html elements, we bake the paint offset into a transform node in
updatePaintOffsetTranslation. Could we do that for foreign object too, instead
of using svgLocalToBorderBoxTransform? Is there an issue in the ordering of the
paint offset and transform nodes?

Can you think of a way to fix this bug by modifying the property trees without
changing PaintInvalidator.cpp?

[Xianzhu, 2016/09/30 00:20:24]

I think the root cause of the concept "paintOffsetFromState" in
getContentsPropertyTreeState() is the difference between the state of
localBorderBoxProperties (for the object itself, which records the paint offset)
and contentsPropertyTreeState.

SvgLocalToBorderBoxTransform is not the root cause, but happens to be the thing
affected by the above difference: the transform doesn't apply to the object
itself, but for children, and it resets paint offset (required), so if it's the
target state of geometry mapping, the result doesn't include paint offset.

Current the paint property tree state is not accurate because it doesn't
represent paint offset. The latest patch added paint offset parameter to remedy
that. We could avoid the special treaty of paint offset by always emit paint
offsets as transform nodes, so that we can always get an accurate paint property
tree state of the container for its contents, but this would cause too many
transform nodes, and also breaks subpixel accumulation.

[pdr., 2016/09/30 04:06:00]

Aha! I think I understand it now. In other words, an SVGRoot's
LocalBorderBoxProperties has a paint offset that is different from the paint
offset of its contents (children of the root).

What do you think of a final design for ObjectPaintProperties like this:
struct BoxProperties {
    LayoutPoint paintOffset;
    PaintPropertyTreeState propertyTreeState;
}
const BoxProperties* borderBoxProperties() const { return
m_localBorderBoxProperties.get(); }
BoxProperties contentsBoxProperties() { return basically what your latest patch
does for getContentsPropertyTreeState }

If that design looks good to you, I'd like to land your latest patch as-is.
After your patch lands, I will rebase my refactoring (replace
GeometryPropertyTreeState with just PropertyTreeState) on top of your patch so
we'll have this design.

[Xianzhu, 2016/09/30 04:10:24]

The design looks great! Thanks!

+        // except for svgLocalToBorderBoxTransform which is applied after the container's paint offset.
+        if (containerTreeState.transform != containerPaintProperties->svgLocalToBorderBoxTransform())
+            result.moveBy(-containerPaintProperties->localBorderBoxProperties()->paintOffset);
     }
 
     if (context.paintInvalidationContainer->layer()->groupedMapping())
         PaintLayer::mapRectInPaintInvalidationContainerToBacking(*context.paintInvalidationContainer, result);
     return result;
 }
 
 void PaintInvalidatorContext::mapLocalRectToPaintInvalidationBacking(const LayoutObject& object, LayoutRect& rect) const
 {
     GeometryMapper geometryMapper;
@@ skipping 221 matching lines @@
     object.getMutableForPainting().clearPaintInvalidationFlags();
 }
 
 void PaintInvalidator::processPendingDelayedPaintInvalidations()
 {
     for (auto target : m_pendingDelayedPaintInvalidations)
         target->getMutableForPainting().setShouldDoFullPaintInvalidation(PaintInvalidationDelayedFull);
 }
 
 } // namespace blink