Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1736893002:
Update drag images to paint from the nearest self painting layer (Closed)
DescriptionUpdate drag images to paint from the nearest self painting layer
Before this patch there were two codepaths for painting drag images:
1) Boxes were drawn using paintAsPseudoStackingContext which would
fail to paint composited children (https://crbug.com/585611).
2) Everything else was drawn using paintIntoDragImage which paints
starting at the view so any transparent regions of the drag element
would be filled in with the content behind the drag element.
This patch unifies drag image rendering under a single approach where
painting begins at the nearest self painting layer to the dragged
element. This has two nice properties:
1) self painting children are painted.
2) transparency is supported as long as the drag element has a layer.
Neither Safari nor Firefox really support these usecases so this
approach is a best-effort at supporting common usecases without
invasive surgery on the paint code. Some behavior is different such
as not including the dragged element's transform.
As part of this patch, paintIntoDragImage has been merged into
dragImageForSelection which is now the only caller.
BUG=585611
Committed: https://crrev.com/d1e45374111c047fc69c8719434bf14417f74d2e
Cr-Commit-Position: refs/heads/master@{#378021}
Patch Set 1 #Patch Set 2 : Fix test failures, add a test #
Total comments: 16
Patch Set 3 : Rebase and address reviewer comments #
Total comments: 2
Patch Set 4 : Add a performance note and prepare this ship for landing. #
Messages
Total messages: 33 (16 generated)
Description was changed from ========== Update drag images to begin painting from the nearest layer Before this patch there were two codepaths for painting drag images: 1) Boxes were drawn using paintAsPseudoStackingContext which would fail to paint composited children (https://crbug.com/585611). 2) Everything else was drawn using paintIntoDragImage which paints starting at the view so any transparent regions of the drag element would be filled in with the content behind the drag element. This patch unifies drag image rendering under a single approach where painting begins at the nearest paint layer to the dragged element. This has two nice properties: 1) composited children are painted. 2) transparency is supported as long as the drag element has a layer. Not yet ready for review until a test is written. BUG=585611 ========== to ========== Update drag images to begin painting from the nearest layer Before this patch there were two codepaths for painting drag images: 1) Boxes were drawn using paintAsPseudoStackingContext which would fail to paint composited children (https://crbug.com/585611). 2) Everything else was drawn using paintIntoDragImage which paints starting at the view so any transparent regions of the drag element would be filled in with the content behind the drag element. This patch unifies drag image rendering under a single approach where painting begins at the nearest paint layer to the dragged element. This has two nice properties: 1) composited children are painted. 2) transparency is supported as long as the drag element has a layer. As part of this patch, paintIntoDragImage has been merged into dragImageForSelection which is now the only caller. Not yet ready for review until a test is written. BUG=585611 ==========
pdr@chromium.org changed reviewers: + chrishtr@chromium.org
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736893002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736893002/1
pdr@chromium.org changed reviewers: + hyunjune.kim@samsung.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736893002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736893002/20001
Description was changed from ========== Update drag images to begin painting from the nearest layer Before this patch there were two codepaths for painting drag images: 1) Boxes were drawn using paintAsPseudoStackingContext which would fail to paint composited children (https://crbug.com/585611). 2) Everything else was drawn using paintIntoDragImage which paints starting at the view so any transparent regions of the drag element would be filled in with the content behind the drag element. This patch unifies drag image rendering under a single approach where painting begins at the nearest paint layer to the dragged element. This has two nice properties: 1) composited children are painted. 2) transparency is supported as long as the drag element has a layer. As part of this patch, paintIntoDragImage has been merged into dragImageForSelection which is now the only caller. Not yet ready for review until a test is written. BUG=585611 ========== to ========== Update drag images to begin painting from the nearest layer Before this patch there were two codepaths for painting drag images: 1) Boxes were drawn using paintAsPseudoStackingContext which would fail to paint composited children (https://crbug.com/585611). 2) Everything else was drawn using paintIntoDragImage which paints starting at the view so any transparent regions of the drag element would be filled in with the content behind the drag element. This patch unifies drag image rendering under a single approach where painting begins at the nearest painting layer to the dragged element. This has two nice properties: 1) composited children are painted. 2) transparency is supported as long as the drag element has a layer. Neither Safari nor Firefox really support these usecases so this approach is a best-effort at supporting common usecases without invasive surgery on the paint code. Some behavior is different such as not including the dragged element's transform. As part of this patch, paintIntoDragImage has been merged into dragImageForSelection which is now the only caller. BUG=585611 ==========
pdr@chromium.org changed reviewers: - hyunjune.kim@samsung.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:641: // Paint starting at the nearest painting layer, clipped to the object itself. s/painting layer/self-painting PaintLayer/. https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:644: // (see: ObjectPainter::paintAsPseudoStackingContext) but this would skip composited children. s/composited children/self-painting PaintLayer children/. Right? Also, if this fix is correct, also update the CL description. https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:647: while (layer && !(layer->isSelfPaintingLayer() || layer->hasSelfPaintingLayerDescendant())) How about putting this method on PaintLayer? PaintLayer::enclosingSelfPaintingLayer(). https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:650: IntRect absoluteBoundingBox = layoutObject->absoluteBoundingBoxRectIncludingDescendants(); 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. https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:672: m_view->paintContents(dragImageBuilder.context(), paintFlags, enclosingIntRect(paintingRect)); Here you're giving up on the common code solution?
https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:641: // Paint starting at the nearest painting layer, clipped to the object itself. On 2016/02/26 at 18:31:05, chrishtr wrote: > s/painting layer/self-painting PaintLayer/. 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? https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:650: IntRect absoluteBoundingBox = layoutObject->absoluteBoundingBoxRectIncludingDescendants(); On 2016/02/26 at 18:31:05, chrishtr wrote: > 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. 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. https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:672: m_view->paintContents(dragImageBuilder.context(), paintFlags, enclosingIntRect(paintingRect)); On 2016/02/26 at 18:31:05, chrishtr wrote: > Here you're giving up on the common code solution? 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.
https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:641: // Paint starting at the nearest painting layer, clipped to the object itself. On 2016/02/26 at 18:41:38, pdr wrote: > On 2016/02/26 at 18:31:05, chrishtr wrote: > > s/painting layer/self-painting PaintLayer/. > > 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? 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... https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:650: IntRect absoluteBoundingBox = layoutObject->absoluteBoundingBoxRectIncludingDescendants(); On 2016/02/26 at 18:41:38, pdr wrote: > On 2016/02/26 at 18:31:05, chrishtr wrote: > > 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. > > 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. 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() https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:672: m_view->paintContents(dragImageBuilder.context(), paintFlags, enclosingIntRect(paintingRect)); On 2016/02/26 at 18:41:38, pdr wrote: > On 2016/02/26 at 18:31:05, chrishtr wrote: > > Here you're giving up on the common code solution? > > 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. Ah ok. I think the refactoring as stated in the CL description is a little confusing though.
Description was changed from ========== Update drag images to begin painting from the nearest layer Before this patch there were two codepaths for painting drag images: 1) Boxes were drawn using paintAsPseudoStackingContext which would fail to paint composited children (https://crbug.com/585611). 2) Everything else was drawn using paintIntoDragImage which paints starting at the view so any transparent regions of the drag element would be filled in with the content behind the drag element. This patch unifies drag image rendering under a single approach where painting begins at the nearest painting layer to the dragged element. This has two nice properties: 1) composited children are painted. 2) transparency is supported as long as the drag element has a layer. Neither Safari nor Firefox really support these usecases so this approach is a best-effort at supporting common usecases without invasive surgery on the paint code. Some behavior is different such as not including the dragged element's transform. As part of this patch, paintIntoDragImage has been merged into dragImageForSelection which is now the only caller. BUG=585611 ========== to ========== Update drag images to paint from the nearest self painting layer Before this patch there were two codepaths for painting drag images: 1) Boxes were drawn using paintAsPseudoStackingContext which would fail to paint composited children (https://crbug.com/585611). 2) Everything else was drawn using paintIntoDragImage which paints starting at the view so any transparent regions of the drag element would be filled in with the content behind the drag element. This patch unifies drag image rendering under a single approach where painting begins at the nearest self painting layer to the dragged element. This has two nice properties: 1) self painting children are painted. 2) transparency is supported as long as the drag element has a layer. Neither Safari nor Firefox really support these usecases so this approach is a best-effort at supporting common usecases without invasive surgery on the paint code. Some behavior is different such as not including the dragged element's transform. As part of this patch, paintIntoDragImage has been merged into dragImageForSelection which is now the only caller. BUG=585611 ==========
Thx. PTAL https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:641: // Paint starting at the nearest painting layer, clipped to the object itself. On 2016/02/26 at 18:58:53, chrishtr wrote: > On 2016/02/26 at 18:41:38, pdr wrote: > > On 2016/02/26 at 18:31:05, chrishtr wrote: > > > s/painting layer/self-painting PaintLayer/. > > > > 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? > > 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... Cool, this makes sense. I've updated this cl to use our shiny new enclosingSelfPaintingLayer. https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:644: // (see: ObjectPainter::paintAsPseudoStackingContext) but this would skip composited children. On 2016/02/26 at 18:31:05, chrishtr wrote: > s/composited children/self-painting PaintLayer children/. Right? > > Also, if this fix is correct, also update the CL description. Done https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:647: while (layer && !(layer->isSelfPaintingLayer() || layer->hasSelfPaintingLayerDescendant())) On 2016/02/26 at 18:31:05, chrishtr wrote: > How about putting this method on PaintLayer? PaintLayer::enclosingSelfPaintingLayer(). Done. https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:650: IntRect absoluteBoundingBox = layoutObject->absoluteBoundingBoxRectIncludingDescendants(); On 2016/02/26 at 18:58:53, chrishtr wrote: > On 2016/02/26 at 18:41:38, pdr wrote: > > On 2016/02/26 at 18:31:05, chrishtr wrote: > > > 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. > > > > 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. > > 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() 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
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736893002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736893002/40001
lgtm https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1736893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:650: IntRect absoluteBoundingBox = layoutObject->absoluteBoundingBoxRectIncludingDescendants(); On 2016/02/26 at 19:28:02, pdr wrote: > On 2016/02/26 at 18:58:53, chrishtr wrote: > > On 2016/02/26 at 18:41:38, pdr wrote: > > > On 2016/02/26 at 18:31:05, chrishtr wrote: > > > > 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. > > > > > > 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. > > > > 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() > > 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 Oh, I see, you're leaving clipToDirtyRect true on PaintLayerPaintingInfo. https://codereview.chromium.org/1736893002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/1736893002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:613: PaintLayer* enclosingSelfPaintingLayer(); Add a note that this is O(depth), so callers should not call inside inner loops, and instead use optimizations like that in PaintInvalidationState.
Off to the CQ, and we made M50! https://codereview.chromium.org/1736893002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/1736893002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:613: PaintLayer* enclosingSelfPaintingLayer(); On 2016/02/26 at 19:36:27, chrishtr wrote: > Add a note that this is O(depth), so callers should not call inside inner loops, and instead use optimizations like that in PaintInvalidationState. Done
The CQ bit was checked by pdr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1736893002/#ps50001 (title: "Add a performance note and prepare this ship for landing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736893002/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736893002/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736893002/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736893002/50001
Message was sent while issue was closed.
Committed patchset #4 (id:50001)
Message was sent while issue was closed.
Description was changed from ========== Update drag images to paint from the nearest self painting layer Before this patch there were two codepaths for painting drag images: 1) Boxes were drawn using paintAsPseudoStackingContext which would fail to paint composited children (https://crbug.com/585611). 2) Everything else was drawn using paintIntoDragImage which paints starting at the view so any transparent regions of the drag element would be filled in with the content behind the drag element. This patch unifies drag image rendering under a single approach where painting begins at the nearest self painting layer to the dragged element. This has two nice properties: 1) self painting children are painted. 2) transparency is supported as long as the drag element has a layer. Neither Safari nor Firefox really support these usecases so this approach is a best-effort at supporting common usecases without invasive surgery on the paint code. Some behavior is different such as not including the dragged element's transform. As part of this patch, paintIntoDragImage has been merged into dragImageForSelection which is now the only caller. BUG=585611 ========== to ========== Update drag images to paint from the nearest self painting layer Before this patch there were two codepaths for painting drag images: 1) Boxes were drawn using paintAsPseudoStackingContext which would fail to paint composited children (https://crbug.com/585611). 2) Everything else was drawn using paintIntoDragImage which paints starting at the view so any transparent regions of the drag element would be filled in with the content behind the drag element. This patch unifies drag image rendering under a single approach where painting begins at the nearest self painting layer to the dragged element. This has two nice properties: 1) self painting children are painted. 2) transparency is supported as long as the drag element has a layer. Neither Safari nor Firefox really support these usecases so this approach is a best-effort at supporting common usecases without invasive surgery on the paint code. Some behavior is different such as not including the dragged element's transform. As part of this patch, paintIntoDragImage has been merged into dragImageForSelection which is now the only caller. BUG=585611 Committed: https://crrev.com/d1e45374111c047fc69c8719434bf14417f74d2e Cr-Commit-Position: refs/heads/master@{#378021} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d1e45374111c047fc69c8719434bf14417f74d2e Cr-Commit-Position: refs/heads/master@{#378021} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
