[S.P.] Move svg mask painting to SVGMaskPainter

[S.P.] Move mask painting to SVGMaskPainter.

BUG=462077
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191196

[Erik Dahlström (inactive), 2015-03-02 08:49:52]

ed@opera.com changed reviewers:
+ fs@opera.com, pdr@chromium.org

[Erik Dahlström (inactive), 2015-03-02 08:49:53]

A first attempt at breaking out the painting parts of <mask>, PTAL.

[fs, 2015-03-02 13:24:51]

https://codereview.chromium.org/967543002/diff/20001/Source/core/layout/svg/L...
File Source/core/layout/svg/LayoutSVGResourceMasker.cpp (right):

https://codereview.chromium.org/967543002/diff/20001/Source/core/layout/svg/L...
Source/core/layout/svg/LayoutSVGResourceMasker.cpp:26: #include
"core/paint/TransformRecorder.h"
Still needed? Same for DrawingDisplayItem.h.

https://codereview.chromium.org/967543002/diff/20001/Source/core/layout/svg/L...
File Source/core/layout/svg/LayoutSVGResourceMasker.h (right):

https://codereview.chromium.org/967543002/diff/20001/Source/core/layout/svg/L...
Source/core/layout/svg/LayoutSVGResourceMasker.h:58: friend class
SVGMaskPainter;
With the suggestion in SVGMaskPainter I don't think this should be needed.

https://codereview.chromium.org/967543002/diff/20001/Source/core/paint/SVGMas...
File Source/core/paint/SVGMaskPainter.cpp (right):

https://codereview.chromium.org/967543002/diff/20001/Source/core/paint/SVGMas...
Source/core/paint/SVGMaskPainter.cpp:10: #include
"core/layout/svg/SVGLayoutSupport.h"
Not needed?

https://codereview.chromium.org/967543002/diff/20001/Source/core/paint/SVGMas...
Source/core/paint/SVGMaskPainter.cpp:78: if (!m_mask.m_maskContentPicture) {
Maybe this block (transform+content picture setup) should remain in
LayoutSVGResourceMasker, and just return a picture and a transform.

[pdr., 2015-03-02 18:57:35]

https://codereview.chromium.org/967543002/diff/20001/Source/core/paint/SVGMas...
File Source/core/paint/SVGMaskPainter.cpp (right):

https://codereview.chromium.org/967543002/diff/20001/Source/core/paint/SVGMas...
Source/core/paint/SVGMaskPainter.cpp:78: if (!m_mask.m_maskContentPicture) {
On 2015/03/02 at 13:24:51, fs wrote:
> Maybe this block (transform+content picture setup) should remain in
LayoutSVGResourceMasker, and just return a picture and a transform.

+1

[Erik Dahlström (inactive), 2015-03-03 08:42:58]

The remaining call to (private method)
LayoutSVGResourceMasker.clearInvalidationMask() was converted into an assert
checking that the invalidationmask is already cleared.

https://codereview.chromium.org/967543002/diff/20001/Source/core/layout/svg/L...
File Source/core/layout/svg/LayoutSVGResourceMasker.cpp (right):

https://codereview.chromium.org/967543002/diff/20001/Source/core/layout/svg/L...
Source/core/layout/svg/LayoutSVGResourceMasker.cpp:26: #include
"core/paint/TransformRecorder.h"
On 2015/03/02 13:24:51, fs wrote:
> Still needed? Same for DrawingDisplayItem.h.

Done.

https://codereview.chromium.org/967543002/diff/20001/Source/core/layout/svg/L...
File Source/core/layout/svg/LayoutSVGResourceMasker.h (right):

https://codereview.chromium.org/967543002/diff/20001/Source/core/layout/svg/L...
Source/core/layout/svg/LayoutSVGResourceMasker.h:58: friend class
SVGMaskPainter;
On 2015/03/02 13:24:51, fs wrote:
> With the suggestion in SVGMaskPainter I don't think this should be needed.

Done.

https://codereview.chromium.org/967543002/diff/20001/Source/core/paint/SVGMas...
File Source/core/paint/SVGMaskPainter.cpp (right):

https://codereview.chromium.org/967543002/diff/20001/Source/core/paint/SVGMas...
Source/core/paint/SVGMaskPainter.cpp:10: #include
"core/layout/svg/SVGLayoutSupport.h"
On 2015/03/02 13:24:51, fs wrote:
> Not needed?

Done.

https://codereview.chromium.org/967543002/diff/20001/Source/core/paint/SVGMas...
Source/core/paint/SVGMaskPainter.cpp:78: if (!m_mask.m_maskContentPicture) {
On 2015/03/02 13:24:51, fs wrote:
> Maybe this block (transform+content picture setup) should remain in
> LayoutSVGResourceMasker, and just return a picture and a transform.

Done.

[Erik Dahlström (inactive), 2015-03-03 10:29:26]

On 2015/03/03 08:42:58, Erik Dahlström wrote:
> The remaining call to (private method)
> LayoutSVGResourceMasker.clearInvalidationMask() was converted into an assert
> checking that the invalidationmask is already cleared.

That should teach me not to just compile release ;) 

I did a testrun locally with this assert and it failed on one of the mask
repaint tests, so I've made clearInvalidationMask public instead.

[fs, 2015-03-03 12:04:37]

lgtm

https://codereview.chromium.org/967543002/diff/60001/Source/core/layout/svg/L...
File Source/core/layout/svg/LayoutSVGResourceMasker.h (right):

https://codereview.chromium.org/967543002/diff/60001/Source/core/layout/svg/L...
Source/core/layout/svg/LayoutSVGResourceMasker.h:58: PassRefPtr<const SkPicture>
setupDrawMaskForRenderer(AffineTransform&, const FloatRect&);
I think I would've called this "getContentPicture" or something along those
lines instead. (Maybe it should just be pushed into createContentPicture and
then that would be exposed instead - sounds like somethat could be done later on
though...)

https://codereview.chromium.org/967543002/diff/60001/Source/core/paint/SVGMas...
File Source/core/paint/SVGMaskPainter.h (right):

https://codereview.chromium.org/967543002/diff/60001/Source/core/paint/SVGMas...
Source/core/paint/SVGMaskPainter.h:19: class LayoutSVGShape;
Looks like this list could be pruned a bit.

[Erik Dahlström (inactive), 2015-03-03 13:08:58]

https://codereview.chromium.org/967543002/diff/60001/Source/core/layout/svg/L...
File Source/core/layout/svg/LayoutSVGResourceMasker.h (right):

https://codereview.chromium.org/967543002/diff/60001/Source/core/layout/svg/L...
Source/core/layout/svg/LayoutSVGResourceMasker.h:58: PassRefPtr<const SkPicture>
setupDrawMaskForRenderer(AffineTransform&, const FloatRect&);
On 2015/03/03 12:04:37, fs wrote:
> I think I would've called this "getContentPicture" or something along those
> lines instead. (Maybe it should just be pushed into createContentPicture and
> then that would be exposed instead - sounds like somethat could be done later
on
> though...)

Ok, changed the name for now.

https://codereview.chromium.org/967543002/diff/60001/Source/core/paint/SVGMas...
File Source/core/paint/SVGMaskPainter.h (right):

https://codereview.chromium.org/967543002/diff/60001/Source/core/paint/SVGMas...
Source/core/paint/SVGMaskPainter.h:19: class LayoutSVGShape;
On 2015/03/03 12:04:37, fs wrote:
> Looks like this list could be pruned a bit.

Done.

[Erik Dahlström (inactive), 2015-03-03 13:09:04]

The CQ bit was checked by ed@opera.com

[Erik Dahlström (inactive), 2015-03-03 13:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/967543002/#ps80001 (title:
"review fixes")

[commit-bot: I haz the power, 2015-03-03 13:10:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/967543002/80001

[commit-bot: I haz the power, 2015-03-03 15:29:46]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191196