[SkSVGDevice] Initial clipping support

[SkSVGDevice] Initial clipping support

Implement SVG clips based on clip stack flattening -
which is now exposed in SkClipStack::asPath() and shared
with SkCanvas's simplify-clip code.

R=reed@google.com,mtklein@google.com

Committed: https://skia.googlesource.com/skia/+/1a481fe4bf632ed4f76cb337691236fabfd4ab03

[reed1, 2015-02-04 03:54:18]

I think using path-ops is a fine way to create clips.
I do not want to propagate the fact that canvas has a clipstack.

I would prefer that SVG canvas/device does this itself: when it sees a clip all,
it does whatever it needs/wants (e.g. simplifies, etc.).

I have wanted to hide clipstack for a while, and have the gpu code-path manage
the stack themselves.

[f(malita), 2015-02-04 14:25:50]

On 2015/02/04 03:54:18, reed1 wrote:
> I think using path-ops is a fine way to create clips.
> I do not want to propagate the fact that canvas has a clipstack.
> 
> I would prefer that SVG canvas/device does this itself: when it sees a clip
all,
> it does whatever it needs/wants (e.g. simplifies, etc.).
> 
> I have wanted to hide clipstack for a while, and have the gpu code-path manage
> the stack themselves.

Is the plan to drop clipstack completely (from SkCanvas), or to make it optional
based on the backing device preference?

If the former, does that mean SkGPUDevice, SkSVGDevice and SkPDFDevice will each
have to implement their own canvas frontend?

[reed1, 2015-02-04 14:30:25]

On 2015/02/04 14:25:50, f(malita) wrote:
> On 2015/02/04 03:54:18, reed1 wrote:
> > I think using path-ops is a fine way to create clips.
> > I do not want to propagate the fact that canvas has a clipstack.
> > 
> > I would prefer that SVG canvas/device does this itself: when it sees a clip
> all,
> > it does whatever it needs/wants (e.g. simplifies, etc.).
> > 
> > I have wanted to hide clipstack for a while, and have the gpu code-path
manage
> > the stack themselves.
> 
> Is the plan to drop clipstack completely (from SkCanvas), or to make it
optional
> based on the backing device preference?
> 
> If the former, does that mean SkGPUDevice, SkSVGDevice and SkPDFDevice will
each
> have to implement their own canvas frontend?

Not sure. My first goal is to not make it public. My second is to make it only
exist when a given backend wants it (i.e. not raster, not picture, not analysis,
etc.).

How about this for the present. We effect your change, but make the new api
private, and just friend your device... to buy us time to think about this.

[mtklein, 2015-02-04 14:40:37]

https://codereview.chromium.org/876923003/diff/40001/experimental/svg/SkSVGDe...
File experimental/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/876923003/diff/40001/experimental/svg/SkSVGDe...
experimental/svg/SkSVGDevice.cpp:290: if (draw.fClipStack->isWideOpen()) {
Might want to put this logic in one place.  Since you've got to do the check in
addResources() anyway, why not have it only call addShaderResources /
addClipResources if the early-exit conditions for each are not true?

https://codereview.chromium.org/876923003/diff/40001/experimental/svg/SkSVGDe...
experimental/svg/SkSVGDevice.cpp:295: draw.fClipStack->asPath(&clipPath);
Can't hurt to add a (void) prefix?

[f(malita), 2015-02-04 14:53:00]

On 2015/02/04 14:30:25, reed1 wrote:
> Not sure. My first goal is to not make it public. My second is to make it only
> exist when a given backend wants it (i.e. not raster, not picture, not
analysis,
> etc.).
> 
> How about this for the present. We effect your change, but make the new api
> private, and just friend your device... to buy us time to think about this.

Sounds good, although I'm a bit confused about the *new api* being private - is
this referring to SkClipStack::asPath()? Making that method private doesn't seem
to help encapsulate the clip stack much, as it implies the client already has
access to one.

[reed1, 2015-02-04 15:18:06]

The CQ bit was checked by reed@google.com

[reed1, 2015-02-04 15:18:06]

lgtm

[commit-bot: I haz the power, 2015-02-04 15:18:21]

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

[f(malita), 2015-02-04 15:19:09]

The CQ bit was unchecked by fmalita@chromium.org

[f(malita), 2015-02-04 15:19:31]

New patchsets have been uploaded after l-g-t-m from reed@google.com

[f(malita), 2015-02-04 15:20:35]

https://codereview.chromium.org/876923003/diff/40001/experimental/svg/SkSVGDe...
File experimental/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/876923003/diff/40001/experimental/svg/SkSVGDe...
experimental/svg/SkSVGDevice.cpp:290: if (draw.fClipStack->isWideOpen()) {
On 2015/02/04 14:40:36, mtklein wrote:
> Might want to put this logic in one place.  Since you've got to do the check
in
> addResources() anyway, why not have it only call addShaderResources /
> addClipResources if the early-exit conditions for each are not true?

Done.

https://codereview.chromium.org/876923003/diff/40001/experimental/svg/SkSVGDe...
experimental/svg/SkSVGDevice.cpp:295: draw.fClipStack->asPath(&clipPath);
On 2015/02/04 14:40:36, mtklein wrote:
> Can't hurt to add a (void) prefix?

Done.

[f(malita), 2015-02-04 15:24:31]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-02-04 15:25:21]

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

[commit-bot: I haz the power, 2015-02-04 15:30:11]

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

[commit-bot: I haz the power, 2015-02-04 15:30:12]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[f(malita), 2015-02-04 15:33:00]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-02-04 15:33:30]

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

[commit-bot: I haz the power, 2015-02-04 15:39:39]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/1a481fe4bf632ed4f76cb337691236fabfd4ab03