Move clip off of draw target

Move clip off of draw target

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/44701df5ce572ac3cccec785cf52103d3d5d14a5

[joshualitt, 2015-02-20 16:07:18]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@google.com, jvanverth@google.com

[joshualitt, 2015-02-20 16:07:19]

A vast shallow change to remove clip from drawtarget.  Brian, please pay special
attention to GrClipData and GrClipMaskManager.

Jim added for trivial changes to text context

[joshualitt, 2015-02-20 18:01:49]

On 2015/02/20 16:07:19, joshualitt wrote:
> A vast shallow change to remove clip from drawtarget.  Brian, please pay
special
> attention to GrClipData and GrClipMaskManager.
> 
> Jim added for trivial changes to text context

feedback incorporated

[joshualitt, 2015-02-23 17:26:36]

feedback incorporated

[bsalomon, 2015-02-23 19:31:30]

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClip.cpp
File src/gpu/GrClip.cpp (right):

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClip.cpp#newco...
src/gpu/GrClip.cpp:29: *isIntersectionOfRects = false;
I think we should say true here... it is effectively just an intersect with
SkRect::MakeWH(width, height);

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClip.cpp#newco...
src/gpu/GrClip.cpp:35: *isIntersectionOfRects = false;
true

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:232: GrClip::kIRect_ClipType == clip.clipType();
This seems unsafe... can't we just make the CMM not install an IRect clip rather
than ignoring it here?

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrDrawTarget.h#n...
src/gpu/GrDrawTarget.h:326: void drawSimpleRect(GrPipelineBuilder* ds,
A lot of the lines in this change are just wrapping params, but we don't have a
style guide that wraps every param and there are many places where we don't

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrPipelineBuilde...
File src/gpu/GrPipelineBuilder.cpp (right):

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrPipelineBuilde...
src/gpu/GrPipelineBuilder.cpp:73: // Enable the clip bit
change/remove comment?

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrTextContext.h
File src/gpu/GrTextContext.h (right):

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrTextContext.h#...
src/gpu/GrTextContext.h:42: const GrClip*              fClip;
realign these?

https://codereview.chromium.org/947443003/diff/80001/src/gpu/SkGpuDevice.h
File src/gpu/SkGpuDevice.h (right):

https://codereview.chromium.org/947443003/diff/80001/src/gpu/SkGpuDevice.h#ne...
src/gpu/SkGpuDevice.h:138: SkAutoTUnref<const SkClipStack> fClipStack;
alignment

[joshualitt, 2015-02-23 19:42:52]

On 2015/02/23 19:31:30, bsalomon wrote:
> https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClip.cpp
> File src/gpu/GrClip.cpp (right):
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClip.cpp#newco...
> src/gpu/GrClip.cpp:29: *isIntersectionOfRects = false;
> I think we should say true here... it is effectively just an intersect with
> SkRect::MakeWH(width, height);
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClip.cpp#newco...
> src/gpu/GrClip.cpp:35: *isIntersectionOfRects = false;
> true
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
> File src/gpu/GrClipMaskManager.cpp (right):
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
> src/gpu/GrClipMaskManager.cpp:232: GrClip::kIRect_ClipType == clip.clipType();
> This seems unsafe... can't we just make the CMM not install an IRect clip
rather
> than ignoring it here?
> 
> https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrDrawTarget.h
> File src/gpu/GrDrawTarget.h (right):
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrDrawTarget.h#n...
> src/gpu/GrDrawTarget.h:326: void drawSimpleRect(GrPipelineBuilder* ds,
> A lot of the lines in this change are just wrapping params, but we don't have
a
> style guide that wraps every param and there are many places where we don't
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrPipelineBuilde...
> File src/gpu/GrPipelineBuilder.cpp (right):
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrPipelineBuilde...
> src/gpu/GrPipelineBuilder.cpp:73: // Enable the clip bit
> change/remove comment?
> 
> https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrTextContext.h
> File src/gpu/GrTextContext.h (right):
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrTextContext.h#...
> src/gpu/GrTextContext.h:42: const GrClip*              fClip;
> realign these?
> 
> https://codereview.chromium.org/947443003/diff/80001/src/gpu/SkGpuDevice.h
> File src/gpu/SkGpuDevice.h (right):
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/SkGpuDevice.h#ne...
> src/gpu/SkGpuDevice.h:138: SkAutoTUnref<const SkClipStack> fClipStack;
> alignment

feedback incorporated

[joshualitt, 2015-02-23 20:06:27]

Whoops, meant to publish this comment

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:232: GrClip::kIRect_ClipType == clip.clipType();
On 2015/02/23 19:31:29, bsalomon wrote:
> This seems unsafe... can't we just make the CMM not install an IRect clip
rather
> than ignoring it here?

IRect code doesn't work now yet anyways because we use the origin in the code
which follows.  CMM is the only guy who uses the IRect, and I'll fix this all in
a follow on CL.  If its really important to fix this now, then I have to add 0,0
origin to IRects which I plan to do anyways, but I think its more dangerous to
start using the IRect code path when it hasn't been tested.

[bsalomon, 2015-02-23 20:48:23]

On 2015/02/23 20:06:27, joshualitt wrote:
> Whoops, meant to publish this comment
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
> File src/gpu/GrClipMaskManager.cpp (right):
> 
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
> src/gpu/GrClipMaskManager.cpp:232: GrClip::kIRect_ClipType == clip.clipType();
> On 2015/02/23 19:31:29, bsalomon wrote:
> > This seems unsafe... can't we just make the CMM not install an IRect clip
> rather
> > than ignoring it here?
> 
> IRect code doesn't work now yet anyways because we use the origin in the code
> which follows.  CMM is the only guy who uses the IRect, and I'll fix this all
in
> a follow on CL.  If its really important to fix this now, then I have to add
0,0
> origin to IRects which I plan to do anyways, but I think its more dangerous to
> start using the IRect code path when it hasn't been tested.

Maybe the IRect code should be added in a follow on CL then?

[joshualitt, 2015-02-23 21:37:17]

On 2015/02/23 20:48:23, bsalomon wrote:
> On 2015/02/23 20:06:27, joshualitt wrote:
> > Whoops, meant to publish this comment
> > 
> >
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
> > File src/gpu/GrClipMaskManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/947443003/diff/80001/src/gpu/GrClipMaskManage...
> > src/gpu/GrClipMaskManager.cpp:232: GrClip::kIRect_ClipType ==
clip.clipType();
> > On 2015/02/23 19:31:29, bsalomon wrote:
> > > This seems unsafe... can't we just make the CMM not install an IRect clip
> > rather
> > > than ignoring it here?
> > 
> > IRect code doesn't work now yet anyways because we use the origin in the
code
> > which follows.  CMM is the only guy who uses the IRect, and I'll fix this
all
> in
> > a follow on CL.  If its really important to fix this now, then I have to add
> 0,0
> > origin to IRects which I plan to do anyways, but I think its more dangerous
to
> > start using the IRect code path when it hasn't been tested.
> 
> Maybe the IRect code should be added in a follow on CL then?

I've posted the follow on CL.  Its very straightforward.  Is it okay with you if
we just get that CL reviewed and then land them both one after the other?

[bsalomon, 2015-02-23 22:04:19]

lgtm with comments on comments.

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

https://codereview.chromium.org/947443003/diff/120001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:19: * empty.  If a clipstack than fOrigin' is only non-zero
when saveLayer has been called
get rid of empty or mark as todo?

https://codereview.chromium.org/947443003/diff/120001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:20: * with an offset bounding box. The clips in
'fClipStack' are in
The origin thing is confusing...

Howabout...

"""
If the clip is a clipstack and origin is used to translate the stack with
respect to device coordinates. This allows us to use a clip stack that is
specified for a root device with a layer device that is restricted to a subset
of the original canvas. For other clip types the origin will always be (0,0).
"""

[joshualitt, 2015-02-23 22:26:34]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-02-23 22:26: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/947443003/#ps160001
(title: "feedback inc")

[commit-bot: I haz the power, 2015-02-23 22:26:58]

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

[commit-bot: I haz the power, 2015-02-23 22:45:02]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/44701df5ce572ac3cccec785cf52103d3d5d14a5