Ensure display items are unique when multiple nested clip paths are used

Ensure display items are unique when multiple nested clip paths are used

This patch fixes a bug where nested clip paths (drawn as a mask) would
use the LayoutSVGResourceClipper as the display item client id. This
would crash when multiple elements referenced the same nested clip paths
because the LayoutSVGResourceClipper display item client would be
referenced multiple times.

This patch wraps clip path masks in a single display item.

BUG=598416
Committed: https://crrev.com/3ddca928b60a3ee16c3ffe945dd6b3916883e2ea
Cr-Commit-Position: refs/heads/master@{#383791}

[pdr., 2016-03-29 06:10:44]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, wkorman@chromium.org

[pdr., 2016-03-29 06:10:50]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 06:11:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1838983002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1838983002/20001

[commit-bot: I haz the power, 2016-03-29 07:48:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-29 07:48:12]

Dry run: This issue passed the CQ dry run.

[wkorman, 2016-03-29 17:56:33]

lgtm

https://codereview.chromium.org/1838983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGClipPainter.h (right):

https://codereview.chromium.org/1838983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGClipPainter.h:37: bool
drawClipAsMask(GraphicsContext&, const LayoutObject&, const FloatRect&
targetBoundingBox, const FloatRect& targetPaintInvalidationRect, const
AffineTransform&);
Please document, in particular the return value meaning.

[chrishtr, 2016-03-29 17:59:05]

https://codereview.chromium.org/1838983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.cpp
(left):

https://codereview.chromium.org/1838983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.cpp:161:
PassRefPtr<const SkPicture>
LayoutSVGResourceClipper::createContentPicture(AffineTransform&
contentTransformation, const FloatRect& targetBoundingBox,
SVGMaskPainter also calls this method, does that code need to be updated?

[pdr., 2016-03-29 18:06:15]

https://codereview.chromium.org/1838983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.cpp
(left):

https://codereview.chromium.org/1838983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.cpp:161:
PassRefPtr<const SkPicture>
LayoutSVGResourceClipper::createContentPicture(AffineTransform&
contentTransformation, const FloatRect& targetBoundingBox,
On 2016/03/29 at 17:59:04, chrishtr wrote:
> SVGMaskPainter also calls this method, does that code need to be updated?

We should be good there, as SVGMaskPainter only calls
LayoutSVGResourceMasker::createContentPicture.

https://codereview.chromium.org/1838983002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGClipPainter.h (right):

https://codereview.chromium.org/1838983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGClipPainter.h:37: bool
drawClipAsMask(GraphicsContext&, const LayoutObject&, const FloatRect&
targetBoundingBox, const FloatRect& targetPaintInvalidationRect, const
AffineTransform&);
On 2016/03/29 at 17:56:33, wkorman wrote:
> Please document, in particular the return value meaning.

Done

[pdr., 2016-03-29 18:06:24]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-03-29 18:06:24]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/1838983002/#ps40001
(title: "Make patch great again")

[commit-bot: I haz the power, 2016-03-29 18:06:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1838983002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1838983002/40001

[chrishtr, 2016-03-29 18:45:40]

https://codereview.chromium.org/1838983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGClipPainter.cpp (right):

https://codereview.chromium.org/1838983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGClipPainter.cpp:126:
SubtreeContentTransformScope contentTransformScope(contentTransform);
This used to be allocated only if m_clipContentPicture was null. Is it still
correct?

[pdr., 2016-03-29 18:56:46]

On 2016/03/29 at 18:45:40, chrishtr wrote:
>
https://codereview.chromium.org/1838983002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/SVGClipPainter.cpp (right):
> 
>
https://codereview.chromium.org/1838983002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/SVGClipPainter.cpp:126:
SubtreeContentTransformScope contentTransformScope(contentTransform);
> This used to be allocated only if m_clipContentPicture was null. Is it still
correct?

Yeah, still correct. It's only used for painting the children.

[chrishtr, 2016-03-29 19:05:32]

https://codereview.chromium.org/1838983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGClipPainter.cpp (right):

https://codereview.chromium.org/1838983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGClipPainter.cpp:126:
SubtreeContentTransformScope contentTransformScope(contentTransform);
On 2016/03/29 at 18:45:40, chrishtr wrote:
> This used to be allocated only if m_clipContentPicture was null. Is it still
correct?

Per offline conversation: SubtreeContentTransformScope has no effect here; it
just sets a hidden global
variable.  (gross).

Add a TODO in a followup to fix this?

[commit-bot: I haz the power, 2016-03-29 19:21:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-29 19:23:21]

Description was changed from

==========
Ensure display items are unique when multiple nested clip paths are used

This patch fixes a bug where nested clip paths (drawn as a mask) would
use the LayoutSVGResourceClipper as the display item client id. This
would crash when multiple elements referenced the same nested clip paths
because the LayoutSVGResourceClipper display item client would be
referenced multiple times.

This patch wraps clip path masks in a single display item.

BUG=598416
==========

to

==========
Ensure display items are unique when multiple nested clip paths are used

This patch fixes a bug where nested clip paths (drawn as a mask) would
use the LayoutSVGResourceClipper as the display item client id. This
would crash when multiple elements referenced the same nested clip paths
because the LayoutSVGResourceClipper display item client would be
referenced multiple times.

This patch wraps clip path masks in a single display item.

BUG=598416

Committed: https://crrev.com/3ddca928b60a3ee16c3ffe945dd6b3916883e2ea
Cr-Commit-Position: refs/heads/master@{#383791}
==========

[commit-bot: I haz the power, 2016-03-29 19:23:22]

Patchset 3 (id:??) landed as
https://crrev.com/3ddca928b60a3ee16c3ffe945dd6b3916883e2ea
Cr-Commit-Position: refs/heads/master@{#383791}