Begin weaning GrClipMaskManager off of GrDrawTarget (take 2)

Begin weaning GrClipMaskManager off of GrDrawTarget (take 2)

GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1754563003

Committed: https://skia.googlesource.com/skia/+/86c60758e9e4f9e203d7462cb22b2a245a0f51bd

Committed: https://skia.googlesource.com/skia/+/391395dcfbff09a83f8f0e9d3e02d38c855ae2e9

[robertphillips, 2016-03-01 14:55:40]

Description was changed from

==========
Second attempt
==========

to

==========
Second attempt
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-03-01 17:21:20]

Description was changed from

==========
Second attempt
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Begin weaning GrClipMaskManager off of GrDrawTarget (take 2)

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-03-01 17:21:20]

robertphillips@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@chromium.org

[joshualitt, 2016-03-01 17:34:50]

joshualitt@google.com changed reviewers:
+ joshualitt@google.com

[joshualitt, 2016-03-01 17:34:50]

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
File include/gpu/GrDrawContext.h (right):

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
include/gpu/GrDrawContext.h:107: bool stencilRect(const SkIRect* scissorRect,
I really like the direction this CL is going, but do we want this to be on a
subclass of the DC?

to play devil's advocate to myself, what about DefaultPathRenderer? Doesn't he
need to use the stencil?  In any case do we want to limit this usage to internal
constructs?

[bsalomon, 2016-03-01 17:38:56]

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrClip.h
File include/gpu/GrClip.h (right):

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:117: void setScissor(const SkIRect& scissor) {
Can we call this setIRect()? Seems like this should operate at the level of
geometries without making assumptions about implementation.

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
File include/gpu/GrDrawContext.h (right):

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
include/gpu/GrDrawContext.h:107: bool stencilRect(const SkIRect* scissorRect,
I think the name here isn't quite right... it isn't just stenciling.
drawAndStencilClipRect? perfomClipRectCoverageAndStencil()?
addRectToCoverageClipMask?

https://codereview.chromium.org/1754563003/diff/20001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/1754563003/diff/20001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:232: SkASSERT(GrClip::kWideOpen_ClipType ==
pipelineBuilder.clip().clipType());
Maybe this warrants a separate entry with clearer semantics, e.g.
drawBatchClippedToRect which explicitly ignores the pipelinebuilder's clip?

Also, should we pull the clip off PB?

[bsalomon, 2016-03-01 17:40:06]

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
File include/gpu/GrDrawContext.h (right):

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
include/gpu/GrDrawContext.h:107: bool stencilRect(const SkIRect* scissorRect,
On 2016/03/01 17:34:50, joshualitt wrote:
> I really like the direction this CL is going, but do we want this to be on a
> subclass of the DC?
> 
> to play devil's advocate to myself, what about DefaultPathRenderer? Doesn't he
> need to use the stencil?  In any case do we want to limit this usage to
internal
> constructs?

I think some way of hiding this is in order (priv class, subclass, ...). Don't
want someone to call this from say an image filter impl. I think this is a
pretty fragile signature that should only exist for the ccm.

[robertphillips, 2016-03-01 20:11:58]

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrClip.h
File include/gpu/GrClip.h (right):

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:117: void setScissor(const SkIRect& scissor) {
On 2016/03/01 17:38:55, bsalomon wrote:
> Can we call this setIRect()? Seems like this should operate at the level of
> geometries without making assumptions about implementation.

Done.

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
File include/gpu/GrDrawContext.h (right):

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
include/gpu/GrDrawContext.h:107: bool stencilRect(const SkIRect* scissorRect,
On 2016/03/01 17:40:06, bsalomon wrote:
> On 2016/03/01 17:34:50, joshualitt wrote:
> > I really like the direction this CL is going, but do we want this to be on a
> > subclass of the DC?
> > 
> > to play devil's advocate to myself, what about DefaultPathRenderer? Doesn't
he
> > need to use the stencil?  In any case do we want to limit this usage to
> internal
> > constructs?
> 
> I think some way of hiding this is in order (priv class, subclass, ...). Don't
> want someone to call this from say an image filter impl. I think this is a
> pretty fragile signature that should only exist for the ccm.

Done. I have added GrDrawContextPriv.

https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
include/gpu/GrDrawContext.h:107: bool stencilRect(const SkIRect* scissorRect,
On 2016/03/01 17:38:55, bsalomon wrote:
> I think the name here isn't quite right... it isn't just stenciling.
> drawAndStencilClipRect? perfomClipRectCoverageAndStencil()?
> addRectToCoverageClipMask?

Done - for now. I was hoping to fold the stencil-only path into this call too
but we can re-rename it if that succeeds.

https://codereview.chromium.org/1754563003/diff/20001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/1754563003/diff/20001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:232: SkASSERT(GrClip::kWideOpen_ClipType ==
pipelineBuilder.clip().clipType());
On 2016/03/01 17:38:55, bsalomon wrote:
> Maybe this warrants a separate entry with clearer semantics, e.g.
> drawBatchClippedToRect which explicitly ignores the pipelinebuilder's clip?

I would like to hold off on that until the drawBatch/stencilPath/drawPathBatch
issue is resolved. Long term I would like a single choke point in the
GrDrawContext where the clip is set up.

> Also, should we pull the clip off PB?

Ultimately yes. Right now I think it would be premature.

[joshualitt, 2016-03-01 20:56:17]

On 2016/03/01 20:11:58, robertphillips wrote:
> https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrClip.h
> File include/gpu/GrClip.h (right):
> 
>
https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrClip.h#ne...
> include/gpu/GrClip.h:117: void setScissor(const SkIRect& scissor) {
> On 2016/03/01 17:38:55, bsalomon wrote:
> > Can we call this setIRect()? Seems like this should operate at the level of
> > geometries without making assumptions about implementation.
> 
> Done.
> 
>
https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
> File include/gpu/GrDrawContext.h (right):
> 
>
https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
> include/gpu/GrDrawContext.h:107: bool stencilRect(const SkIRect* scissorRect,
> On 2016/03/01 17:40:06, bsalomon wrote:
> > On 2016/03/01 17:34:50, joshualitt wrote:
> > > I really like the direction this CL is going, but do we want this to be on
a
> > > subclass of the DC?
> > > 
> > > to play devil's advocate to myself, what about DefaultPathRenderer?
Doesn't
> he
> > > need to use the stencil?  In any case do we want to limit this usage to
> > internal
> > > constructs?
> > 
> > I think some way of hiding this is in order (priv class, subclass, ...).
Don't
> > want someone to call this from say an image filter impl. I think this is a
> > pretty fragile signature that should only exist for the ccm.
> 
> Done. I have added GrDrawContextPriv.
> 
>
https://codereview.chromium.org/1754563003/diff/20001/include/gpu/GrDrawConte...
> include/gpu/GrDrawContext.h:107: bool stencilRect(const SkIRect* scissorRect,
> On 2016/03/01 17:38:55, bsalomon wrote:
> > I think the name here isn't quite right... it isn't just stenciling.
> > drawAndStencilClipRect? perfomClipRectCoverageAndStencil()?
> > addRectToCoverageClipMask?
> 
> Done - for now. I was hoping to fold the stencil-only path into this call too
> but we can re-rename it if that succeeds.
> 
> https://codereview.chromium.org/1754563003/diff/20001/src/gpu/GrDrawTarget.cpp
> File src/gpu/GrDrawTarget.cpp (right):
> 
>
https://codereview.chromium.org/1754563003/diff/20001/src/gpu/GrDrawTarget.cp...
> src/gpu/GrDrawTarget.cpp:232: SkASSERT(GrClip::kWideOpen_ClipType ==
> pipelineBuilder.clip().clipType());
> On 2016/03/01 17:38:55, bsalomon wrote:
> > Maybe this warrants a separate entry with clearer semantics, e.g.
> > drawBatchClippedToRect which explicitly ignores the pipelinebuilder's clip?
> 
> I would like to hold off on that until the drawBatch/stencilPath/drawPathBatch
> issue is resolved. Long term I would like a single choke point in the
> GrDrawContext where the clip is set up.
> 
> > Also, should we pull the clip off PB?
> 
> Ultimately yes. Right now I think it would be premature.

This CL is really nice.  I will let Brian handle an actual review, but it feels
like we are getting closer to killing pipelinebuilder entirely.

[bsalomon, 2016-03-02 14:15:34]

https://codereview.chromium.org/1754563003/diff/80001/src/gpu/GrDrawContextPr...
File src/gpu/GrDrawContextPriv.h (right):

https://codereview.chromium.org/1754563003/diff/80001/src/gpu/GrDrawContextPr...
src/gpu/GrDrawContextPriv.h:36: void internal_drawBatch(const GrPipelineBuilder&
pipelineBuilder,
Maybe this should drop the internal_ prefix now that it is behind the priv
interface? Otherwise, lgtm.

[robertphillips, 2016-03-02 15:28:22]

The CQ bit was checked by robertphillips@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-02 15:28:30]

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

[commit-bot: I haz the power, 2016-03-02 15:30:27]

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

[commit-bot: I haz the power, 2016-03-02 15:30:28]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[robertphillips, 2016-03-02 16:02:55]

https://codereview.chromium.org/1754563003/diff/80001/src/gpu/GrDrawContextPr...
File src/gpu/GrDrawContextPriv.h (right):

https://codereview.chromium.org/1754563003/diff/80001/src/gpu/GrDrawContextPr...
src/gpu/GrDrawContextPriv.h:36: void internal_drawBatch(const GrPipelineBuilder&
pipelineBuilder,
On 2016/03/02 14:15:34, bsalomon wrote:
> Maybe this should drop the internal_ prefix now that it is behind the priv
> interface? Otherwise, lgtm.

Done-ish. I've renamed it to testingOnly_drawBatch. I would like to avoid having
another generically named drawBatch entry point since I feel we already have too
many.

[robertphillips, 2016-03-02 16:03:02]

The CQ bit was checked by robertphillips@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-02 16:03:13]

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

[commit-bot: I haz the power, 2016-03-02 16:12:58]

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

[commit-bot: I haz the power, 2016-03-02 16:12:59]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-03-02 16:41:50]

The CQ bit was checked by robertphillips@google.com

[robertphillips, 2016-03-02 16:41:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1754563003/#ps120001
(title: "Fix indents")

[commit-bot: I haz the power, 2016-03-02 16:42:06]

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

[commit-bot: I haz the power, 2016-03-02 16:43:16]

Description was changed from

==========
Begin weaning GrClipMaskManager off of GrDrawTarget (take 2)

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Begin weaning GrClipMaskManager off of GrDrawTarget (take 2)

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/86c60758e9e4f9e203d7462cb22b2a245a0f51bd
==========

[commit-bot: I haz the power, 2016-03-02 16:43:17]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/86c60758e9e4f9e203d7462cb22b2a245a0f51bd

[robertphillips, 2016-03-02 16:52:51]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1754353002/ by robertphillips@google.com.

The reason for reverting is: fAuditTrail
.

[robertphillips, 2016-03-02 16:54:13]

Description was changed from

==========
Begin weaning GrClipMaskManager off of GrDrawTarget (take 2)

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/86c60758e9e4f9e203d7462cb22b2a245a0f51bd
==========

to

==========
Begin weaning GrClipMaskManager off of GrDrawTarget (take 2)

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/86c60758e9e4f9e203d7462cb22b2a245a0f51bd
==========

[robertphillips, 2016-03-02 17:03:48]

The CQ bit was checked by robertphillips@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-02 17:03:57]

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

[commit-bot: I haz the power, 2016-03-02 17:24:49]

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

[commit-bot: I haz the power, 2016-03-02 17:24:50]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-03-02 17:25:34]

The CQ bit was checked by robertphillips@google.com

[robertphillips, 2016-03-02 17:25:34]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1754563003/#ps140001
(title: "Fix fAuditTrail uses")

[commit-bot: I haz the power, 2016-03-02 17:25:39]

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

[commit-bot: I haz the power, 2016-03-02 17:26:51]

Description was changed from

==========
Begin weaning GrClipMaskManager off of GrDrawTarget (take 2)

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/86c60758e9e4f9e203d7462cb22b2a245a0f51bd
==========

to

==========
Begin weaning GrClipMaskManager off of GrDrawTarget (take 2)

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/86c60758e9e4f9e203d7462cb22b2a245a0f51bd

Committed:
https://skia.googlesource.com/skia/+/391395dcfbff09a83f8f0e9d3e02d38c855ae2e9
==========

[commit-bot: I haz the power, 2016-03-02 17:26:52]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/391395dcfbff09a83f8f0e9d3e02d38c855ae2e9