Update drag images to paint from the nearest self painting layer

third_party/WebKit/Source/core/frame/LocalFrame.cpp

Index: third_party/WebKit/Source/core/frame/LocalFrame.cpp
diff --git a/third_party/WebKit/Source/core/frame/LocalFrame.cpp b/third_party/WebKit/Source/core/frame/LocalFrame.cpp
index 15161faa4dcb7882692ff8645199a611fbf15140..f748404371bb61381e71044abcdbe87f080bad63 100644
--- a/third_party/WebKit/Source/core/frame/LocalFrame.cpp
+++ b/third_party/WebKit/Source/core/frame/LocalFrame.cpp
@@ -97,7 +97,7 @@ namespace {
 class DragImageBuilder {
     STACK_ALLOCATED();
 public:
-    DragImageBuilder(const LocalFrame* localFrame, const IntRect& bounds, Node* draggedNode, float opacity = 1)
+    DragImageBuilder(const LocalFrame* localFrame, const FloatRect& bounds, Node* draggedNode, float opacity = 1)
         : m_localFrame(localFrame)
         , m_draggedNode(draggedNode)
         , m_bounds(bounds)
@@ -140,7 +140,7 @@ public:
 private:
     RawPtrWillBeMember<const LocalFrame> m_localFrame;
     RawPtrWillBeMember<Node> m_draggedNode;
-    IntRect m_bounds;
+    FloatRect m_bounds;
     float m_opacity;
     OwnPtr<SkPictureBuilder> m_pictureBuilder;
 };
@@ -631,18 +631,6 @@ double LocalFrame::devicePixelRatio() const
     return ratio;
 }
 
-PassOwnPtr<DragImage> LocalFrame::paintIntoDragImage(const GlobalPaintFlags globalPaintFlags,
-    IntRect paintingRect, Node* draggedNode, float opacity)
-{
-    ASSERT(document()->isActive());
-    // Not flattening compositing layers will result in a broken image being painted.
-    ASSERT(globalPaintFlags & GlobalPaintFlattenCompositingLayers);
-
-    DragImageBuilder dragImageBuilder(this, paintingRect, draggedNode, opacity);
-    m_view->paintContents(dragImageBuilder.context(), globalPaintFlags, paintingRect);
-    return dragImageBuilder.createImage();
-}
-
 PassOwnPtr<DragImage> LocalFrame::nodeImage(Node& node)
 {
     m_view->updateAllLifecyclePhases();
@@ -650,24 +638,24 @@ PassOwnPtr<DragImage> LocalFrame::nodeImage(Node& node)
     if (!layoutObject)
         return nullptr;
 
-    // Directly paint boxes as if they are a stacking context.
-    if (layoutObject->isBox() && layoutObject->container()) {
-        IntRect boundingBox = layoutObject->absoluteBoundingBoxRectIncludingDescendants();
-        LayoutPoint paintOffset = boundingBox.location() - layoutObject->offsetFromContainer(layoutObject->container(), LayoutPoint());
-
-        DragImageBuilder dragImageBuilder(this, boundingBox, &node);
-        {
-            PaintInfo paintInfo(dragImageBuilder.context(), boundingBox, PaintPhase::PaintPhaseForeground, GlobalPaintFlattenCompositingLayers, 0);
-            ObjectPainter(*layoutObject).paintAsPseudoStackingContext(paintInfo, LayoutPoint(paintOffset));
-        }
-        return dragImageBuilder.createImage();
+    // Paint starting at the nearest painting layer, clipped to the object itself.

[chrishtr, 2016/02/26 18:31:05]

s/painting layer/self-painting PaintLayer/.

[pdr., 2016/02/26 18:41:38]

It's not necessarily the nearest self painting layer though, since it can be the
a paint layer which is not self painting but has self painting descendants.

I wish I could say there is a grand vision here but I'm just reverse engineering
the early-out in PaintLayerPainter::paintLayer. Do you have a better suggestion?

[chrishtr, 2016/02/26 18:58:53]

I don't think the hasSelfPaintingLayerDescendant() part is needed. The code in
PaintLayerPainter is just to optimize paint recursion cost for a self-painting
layer in the PaintLayer tree (no point recursing into PaintLayer children if
none of them will do anything):

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[pdr., 2016/02/26 19:28:02]

Cool, this makes sense. I've updated this cl to use our shiny new
enclosingSelfPaintingLayer.

+    // TODO(pdr): This will also paint the content behind the object if the object contains
+    // transparency but the layer is opaque. We could directly call layoutObject->paint(...)
+    // (see: ObjectPainter::paintAsPseudoStackingContext) but this would skip composited children.

[chrishtr, 2016/02/26 18:31:05]

s/composited children/self-painting PaintLayer children/. Right?

Also, if this fix is correct, also update the CL description.

[pdr., 2016/02/26 19:28:02]

Done

+    PaintLayer* layer = layoutObject->enclosingLayer();
+    // Find the first layer that will paint content, otherwise the paintLayer call will be a noop.
+    while (layer && !(layer->isSelfPaintingLayer() || layer->hasSelfPaintingLayerDescendant()))

[chrishtr, 2016/02/26 18:31:05]

How about putting this method on PaintLayer?
PaintLayer::enclosingSelfPaintingLayer().

[pdr., 2016/02/26 19:28:02]

Done.

+        layer = layer->parent();
+
+    IntRect absoluteBoundingBox = layoutObject->absoluteBoundingBoxRectIncludingDescendants();

[chrishtr, 2016/02/26 18:31:05]

How about layer->boundingBoxForCompositing() instead? This also should be more
correct, since the clip rect in the PaintLayerPaintingInfo should be in the
space of the layer, not layoutObject.

[pdr., 2016/02/26 18:41:38]

boundingBoxForCompositing would be the layer's bounding box, but we want the
bounding box of the layout object. For example, a span can be draggable and we
want to just paint the span in that case.
absoluteBoundingBoxRectIncludingDescendants is a quirky function but matches the
behavior we had before :/ I'd really like to call computePaintInvalidationRect
instead but this won't work for inlines.

[chrishtr, 2016/02/26 18:58:53]

boundingBoxForCompositing is the bounds of the LayoutObject which owns the
PaintLayer, unioned with the bounds of all LayoutObjects which paint into it (up
to compositing boundaries...hmm)

Alternatively, it seems ok to just call
layer->layoutObject()->absoluteBoundingBoxRectIncludingDescendants()

[pdr., 2016/02/26 19:28:02]

I don't think we can do this because text needs to be draggable too so we need
to use a tigher bounding box. For example: http://output.jsbin.com/kudokix

[chrishtr, 2016/02/26 19:36:27]

Oh, I see, you're leaving clipToDirtyRect true on PaintLayerPaintingInfo.

+    FloatRect boundingBox = layer->layoutObject()->absoluteToLocalQuad(FloatQuad(absoluteBoundingBox), UseTransforms).boundingBox();
+    DragImageBuilder dragImageBuilder(this, boundingBox, &node);
+    {
+        PaintLayerPaintingInfo paintingInfo(layer, LayoutRect(boundingBox), GlobalPaintFlattenCompositingLayers, LayoutSize(), 0);
+        PaintLayerFlags flags = PaintLayerHaveTransparency | PaintLayerAppliedTransform | PaintLayerUncachedClipRects;
+        PaintLayerPainter(*layer).paintLayer(dragImageBuilder.context(), paintingInfo, flags);
     }
-
-    // TODO(pdr): This will also paint the background if the object contains transparency. We can
-    // directly call layoutObject->paint(...) (see: ObjectPainter::paintAsPseudoStackingContext) but
-    // painters are inconsistent about which transform space they expect (see: svg, inlines, etc.)
-    // TODO(pdr): SVG and inlines are painted offset (crbug.com/579153, crbug.com/579158).
-    return paintIntoDragImage(GlobalPaintFlattenCompositingLayers, layoutObject->absoluteBoundingBoxRectIncludingDescendants(), &node);
+    return dragImageBuilder.createImage();
 }
 
 PassOwnPtr<DragImage> LocalFrame::dragImageForSelection(float opacity)
@@ -676,9 +664,13 @@ PassOwnPtr<DragImage> LocalFrame::dragImageForSelection(float opacity)
         return nullptr;
 
     m_view->updateAllLifecyclePhases();
+    ASSERT(document()->isActive());
 
-    return paintIntoDragImage(GlobalPaintSelectionOnly | GlobalPaintFlattenCompositingLayers,
-        enclosingIntRect(selection().bounds()), nullptr, opacity);
+    FloatRect paintingRect = FloatRect(selection().bounds());
+    DragImageBuilder dragImageBuilder(this, paintingRect, nullptr, opacity);
+    GlobalPaintFlags paintFlags = GlobalPaintSelectionOnly | GlobalPaintFlattenCompositingLayers;
+    m_view->paintContents(dragImageBuilder.context(), paintFlags, enclosingIntRect(paintingRect));

[chrishtr, 2016/02/26 18:31:05]

Here you're giving up on the common code solution?

[pdr., 2016/02/26 18:41:38]

This is just a small refactoring mentioned in the cl description.
dragImageForSelection is unrelated to this patch, but this patch removed the
other caller to paintIntoDragImage so I moved the code in this function.

[chrishtr, 2016/02/26 18:58:53]

Ah ok. I think the refactoring as stated in the CL description is a little
confusing though. 

+    return dragImageBuilder.createImage();
 }
 
 String LocalFrame::selectedText() const