Fix PaintPropertyTreeBuilder for root layer scrolling.

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

Index: third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
index 1a2ac756ebe0ce1ecfac18df0090bdcd735442e3..6c04ca440c0f2fc887ced43d982d2c585dc2d3db 100644
--- a/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
@@ -21,6 +21,10 @@ namespace blink {
 
 void PaintPropertyTreeBuilder::buildTreeRootNodes(FrameView& rootFrame, PaintPropertyTreeBuilderContext& context)
 {
+    Settings* settings = rootFrame.frame().settings();
+    if (settings && settings->rootLayerScrolls())
+        return;
+
     if (!rootFrame.rootTransform() || rootFrame.rootTransform()->parent()) {
         rootFrame.setRootTransform(TransformPaintPropertyNode::create(nullptr, TransformationMatrix(), FloatPoint3D()));
         rootFrame.setRootClip(ClipPaintPropertyNode::create(nullptr, rootFrame.rootTransform(), FloatRoundedRect(LayoutRect::infiniteIntRect())));
@@ -37,9 +41,32 @@ void PaintPropertyTreeBuilder::buildTreeRootNodes(FrameView& rootFrame, PaintPro
 
 void PaintPropertyTreeBuilder::buildTreeNodes(FrameView& frameView, PaintPropertyTreeBuilderContext& context)
 {
-    // TODO(pdr): Creating paint properties for FrameView here will not be
-    // needed once settings()->rootLayerScrolls() is enabled.
-    // TODO(pdr): Make this conditional on the rootLayerScrolls setting.
+    Settings* settings = frameView.frame().settings();
+    if (settings && settings->rootLayerScrolls()) {
+        LayoutView* layoutView = frameView.layoutView();
+        if (!layoutView)
+            return;
+
+        if (!context.currentEffect) {
+            DCHECK(frameView.frame().isLocalRoot());
+            context.currentEffect = layoutView->getMutableForPainting().ensureObjectPaintProperties().createOrUpdateEffect(nullptr, 1.0);
+        }

[trchen, 2016/08/17 05:57:47]

I think we should not create effect nodes for LayoutView explicitly here. I
recall that accessibility folks are interested in applying filters on
LayoutView, so LayoutView may have real effect nodes in the future.

My mental model is that every LayoutObject that is a stacking context (or
stacked for being positioned) should create an effect node, but we don't
actually create them if they can be safely omitted. For the case of root
LayoutView, effect node can't be omitted because someone needs to create root
surface.

I'd rather do this in PaintPropertyTreeBuilder::updateEffect() line #202 instead
of here.

[szager1, 2016/08/17 19:21:34]

Done.

+
+        TransformationMatrix frameTranslate;
+        frameTranslate.translate(
+            frameView.x() + layoutView->location().x() + context.current.paintOffset.x(),
+            frameView.y() + layoutView->location().y() + context.current.paintOffset.y());

[trchen, 2016/08/17 05:57:47]

It feels weird. We should not include LayoutView location here as it defeats the
purpose of RLS --- eliminate cronyism between FrameView and LayoutView.

And doing FrameView location here is weird. I think these should stay in
frameView.preTranslation, as they belong to Widget, not LayoutView.

[szager1, 2016/08/17 19:21:34]

Once root layer scrolling is enabled, FrameView should not have any paint
properties at all.  Separately, wkorman@ and I are working on removing all
geometry from FrameView.  When that happens, the values that currently get
stuffed into FrameView::x() and FrameView::y() will instead go into
LayoutView::location(), and we will update this along with every other
invocation of FrameView geometry code.

[trchen, 2016/08/17 21:30:55]

That'd be great! I tried to do that 2 weeks ago but there are still many things
rely on the (broken) Widget-based coordinate conversion.

In the mean time I think the best strategy is to match old-world behavior.

[szager1, 2016/08/19 01:52:13]

This entire patch is intended match old-world behavior, while removing all paint
property nodes from FrameView when root layer scrolling is enabled, which is in
my opinion the only sane thing to do.

+        context.current.transform = layoutView->getMutableForPainting().ensureObjectPaintProperties().createOrUpdatePaintOffsetTranslation(
+            context.current.transform, frameTranslate, FloatPoint3D());
+        context.current.paintOffset = LayoutPoint();
+        context.current.clip = nullptr; // This will get set in updateOverflowClip().
+        context.current.renderingContextID = 0;
+        context.current.shouldFlattenInheritedTransform = true;
+        context.absolutePosition = context.current;
+        context.containerForAbsolutePosition = nullptr; // This will get set in updateOutOfFlowContext().
+        context.fixedPosition = context.current;
+        return;
+    }
 
     TransformationMatrix frameTranslate;
     frameTranslate.translate(frameView.x() + context.current.paintOffset.x(), frameView.y() + context.current.paintOffset.y());
@@ -98,6 +125,9 @@ void PaintPropertyTreeBuilder::updatePaintOffsetTranslation(const LayoutObject&
         }
     }
 
+    if (object.isLayoutView())

[pdr., 2016/08/17 04:30:41]

Can you add a comment here about why we don't clear the paint offset
translation, below?

[trchen, 2016/08/17 05:57:47]

I'd rather add CHECK(!object.isLayoutView()) on line #109 to make sure
LayoutView never have paint offset.

[szager1, 2016/08/17 19:21:34]

With root layer scrolling, every LayoutView must have a paint offset translation
which is initialized prior to calling deriveBorderBoxFromContainerContext. 
That's why it's created here, and that's why we don't clear it in
updatePaintOffsetTranslation.

[trchen, 2016/08/17 21:30:55]

Does that match the behavior of old-world compositing? I think we do clear
paintOffset and use TransformDisplayItem to do widget positioning even with RLS
on. See PartPainter.cpp:114 and FramePainter.cpp:61. I don't think RLS excluded
them.

I appreciate that you tried to propagate paintOffset into sub-frame. It will be
a good thing in the long term, if we can also propagate sub-pixel layout size
into sub-frames. However things are not ready yet today, and paint invalidation
may go mad if our paint offset doesn't match the old-world. :(

I think this explains some of the sub-pixel layout test failures.

[szager1, 2016/08/19 01:52:13]

I'm not sure what "long term" is here.  Chromium is broken with either spv2 or
rls enabled; with both of them enabled, it is doubly broken.  I don't see the
value of trying to make it less broken by compromising this patch, which is code
that only runs when both features are enabled.

To put it another way: if your primary concern is not to regress spv2,
independent of rls, then this patch is neutral.  But if you ever want rls and
spv2 to work together, then I would argue that this patch is the correct
approach, and if it causes test failures, then they need to be fixed elsewhere.

+        return;
+
     if (ObjectPaintProperties* properties = object.getMutableForPainting().objectPaintProperties())
         properties->clearPaintOffsetTranslation();
 }
@@ -169,6 +199,9 @@ void PaintPropertyTreeBuilder::updateTransform(const LayoutObject& object, Paint
 
 void PaintPropertyTreeBuilder::updateEffect(const LayoutObject& object, PaintPropertyTreeBuilderContext& context)
 {
+    if (object.isLayoutView())
+        return;
+
     if (!object.styleRef().hasOpacity()) {
         if (ObjectPaintProperties* properties = object.getMutableForPainting().objectPaintProperties())
             properties->clearEffect();
@@ -321,7 +354,14 @@ void PaintPropertyTreeBuilder::updateScrollTranslation(const LayoutObject& objec
         DCHECK(layer);
         DoubleSize scrollOffset = layer->getScrollableArea()->scrollOffset();
 
-        if (!scrollOffset.isZero() || layer->scrollsOverflow()) {
+        if (object.isLayoutView()) {

[skobes, 2016/08/17 01:38:24]

Why does the LayoutView have a separate path here?  It's a little unclear what
the difference is.

[trchen, 2016/08/17 05:57:47]

Yea... I can't spot any difference either.

[szager1, 2016/08/17 19:21:34]

The difference is that LayoutView will get a scroll transform even when the
scroll offset is (0, 0).  That makes it match the current behavior of
FrameView::scrollTranslation().  I refactored the code to eliminate redundancy.

+            Settings* settings = object.document().settings();
+            if (settings && settings->rootLayerScrolls()) {
+                context.current.transform = object.getMutableForPainting().ensureObjectPaintProperties().createOrUpdateScrollTranslation(context.current.transform, TransformationMatrix().translate(-scrollOffset.width(), -scrollOffset.height()), FloatPoint3D());
+                context.current.shouldFlattenInheritedTransform = false;
+                return;
+            }
+        } else if (!scrollOffset.isZero() || layer->scrollsOverflow()) {
             TransformationMatrix matrix = TransformationMatrix().translate(-scrollOffset.width(), -scrollOffset.height());
             context.current.transform = object.getMutableForPainting().ensureObjectPaintProperties().createOrUpdateScrollTranslation(
                 context.current.transform, matrix, FloatPoint3D(), context.current.shouldFlattenInheritedTransform, context.current.renderingContextID);
@@ -341,9 +381,15 @@ void PaintPropertyTreeBuilder::updateOutOfFlowContext(const LayoutObject& object
         context.containerForAbsolutePosition = &object;
     }
 
-    // TODO(pdr): Remove the !object.isLayoutView() condition when removing FrameView
-    // paint properties for rootLayerScrolls.
-    if (!object.isLayoutView() && object.canContainFixedPositionObjects()) {
+    if (object.isLayoutView()) {

[trchen, 2016/08/17 05:57:47]

It is unfortunate that canContainFixedPositionObjects() returns true for
LayoutView. I think LayoutView should behave as if
canContainFixedPositionObjects() being false, whether in RLS mode or not.

[szager1, 2016/08/17 19:21:34]

Be that as it may, I think that changing the return value of
canContainFixedPositionObjects would open a pretty big can of worms.  The body
element is not a fixed position container, so what than *would* be the top-level
fixed position container?

[trchen, 2016/08/17 21:30:55]

I think it should return nullptr and the element would know the viewport being
the fixed-pos container. It is plain wrong to treat LayoutView as fixed-pos
container especially in RLS mode, because we'd use the wrong box. For example:

<div id="fixed-container" style="overflow:scroll; transform:translateZ(0)">
    <div style="position:fixed"></div>
</div>

The transformed element would be the fixed-pos container for the inner fixed-pos
element. However the fixed-pos element stick to the SCROLLED padding box of it,
not the border box. In comparison, for the case of RLS LayoutView, fixed-pos
element actually wants to stick to the border box. This discrepancy caused a lot
of special-casing in our geometry conversion function, which can be avoided. :(

That said, I agree it is a big can of worms. I'd avoid changing
canContainFixedPositionObjects() at this moment. I think we can simply reuse
pdr's code that excludes LayoutView as fixed-pos container. What would break if
we simply treat LayoutView as regular containing block?

[szager1, 2016/08/19 01:52:12]

This is exactly the code that special-cases LayoutView for this reason.  At the
time that this code runs, context.current is the scroll offset of the container,
which is correct for scrollable div's, but incorrect for the LayoutView.  So,
this change grabs the pre-scroll-translation transform to use for
context.fixedPosition.

+        Settings* settings = object.document().settings();
+        if (settings && settings->rootLayerScrolls()) {
+            context.fixedPosition = context.current;
+            const TransformPaintPropertyNode* transform = object.objectPaintProperties()->paintOffsetTranslation();
+            DCHECK(transform);
+            context.fixedPosition.transform = transform;
+        }
+    } else if (object.canContainFixedPositionObjects()) {
         context.fixedPosition = context.current;
     } else if (object.getMutableForPainting().objectPaintProperties() && object.objectPaintProperties()->cssClip()) {
         // CSS clip applies to all descendants, even if this object is not a containing block