Convert GrClip to an abstract base class

Convert GrClip to an abstract base class

Converts GrClip to an abstract base class and adds a "GrFixedClip"
implementation. GrFixedClip denotes a clip implemented with fixed-
function hardware. GrFixedClip allows us to remove the stateful
"fClipMode" member from GrClipMaskManager, and in the future will
be able to nicely encapsulate window rectangles.

After this change GrClipMaskManager is just a wrapper around
GrDrawTarget. We may want to consider removing it altogether.

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

Committed: https://skia.googlesource.com/skia/+/846c051a4800b3cea341a0195db24297d6d9047f

[Chris Dalton, 2016-05-12 20:56:52]

Description was changed from

==========
Convert GrClip to an abstract base class

Converts GrClip to an abstract base class and adds a "GrFixedClip"
implementation. GrFixedClip denotes a clip implemented with fixed-
function hardware. GrFixedClip allows us to remove the stateful
"fClipMode" member from GrClipMaskManager, and in the future will
be able to nicely encapsulate window rectangles.

After this change GrClipMaskManager is just a wrapper around
GrDrawTarget. We may want to consider removing it altogether.

BUG=skia:
==========

to

==========
Convert GrClip to an abstract base class

Converts GrClip to an abstract base class and adds a "GrFixedClip"
implementation. GrFixedClip denotes a clip implemented with fixed-
function hardware. GrFixedClip allows us to remove the stateful
"fClipMode" member from GrClipMaskManager, and in the future will
be able to nicely encapsulate window rectangles.

After this change GrClipMaskManager is just a wrapper around
GrDrawTarget. We may want to consider removing it altogether.

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

[Chris Dalton, 2016-05-12 20:59:10]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com, robertphillips@google.com

[bsalomon, 2016-05-13 02:34:12]

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

https://codereview.chromium.org/1971343002/diff/40001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:58: class GrWideOpenClip final : public GrClip {
I've never been crazy about "WideOpen", what if we just called this GrUnclipped
or GrNoClip?

https://codereview.chromium.org/1971343002/diff/40001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:60: bool quickContains(const SkRect& rect) const override
final { return true; }
Can we have just one of final/override? Either seems sufficient in this case
since the class is final.

https://codereview.chromium.org/1971343002/diff/40001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/1971343002/diff/40001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:290: const SkIRect& dstIRect =
SkIRect::MakeWH(bounds.width(), bounds.height());
Does the change to references here add any value?

[Chris Dalton, 2016-05-13 03:53:46]

Addressed the comments so far. No rush, window rectangles aren't going to be
finished by the end of the day tomorrow.

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

https://codereview.chromium.org/1971343002/diff/40001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:58: class GrWideOpenClip final : public GrClip {
On 2016/05/13 02:34:12, bsalomon wrote:
> I've never been crazy about "WideOpen", what if we just called this
GrUnclipped
> or GrNoClip?

Done. I like GrNoClip.

https://codereview.chromium.org/1971343002/diff/40001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:60: bool quickContains(const SkRect& rect) const override
final { return true; }
On 2016/05/13 02:34:11, bsalomon wrote:
> Can we have just one of final/override? Either seems sufficient in this case
> since the class is final.

Done. Good point.

https://codereview.chromium.org/1971343002/diff/40001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/1971343002/diff/40001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:290: const SkIRect& dstIRect =
SkIRect::MakeWH(bounds.width(), bounds.height());
On 2016/05/13 02:34:12, bsalomon wrote:
> Does the change to references here add any value?

Semantically it avoids a copy constructor. But since these just use the default
copy constructor I would hope a good compiler would optimize that out anyway.

I only changed the two below to bind to the temporaries because I wanted to do
it for devIBounds, and it would have felt inconsistent if those two didn't as
well.

Either way I don't have a strong preference.

[bsalomon, 2016-05-13 13:44:56]

lgtm

https://codereview.chromium.org/1971343002/diff/40001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/1971343002/diff/40001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:290: const SkIRect& dstIRect =
SkIRect::MakeWH(bounds.width(), bounds.height());
On 2016/05/13 03:53:46, Chris Dalton wrote:
> On 2016/05/13 02:34:12, bsalomon wrote:
> > Does the change to references here add any value?
> 
> Semantically it avoids a copy constructor. But since these just use the
default
> copy constructor I would hope a good compiler would optimize that out anyway.
> 
> I only changed the two below to bind to the temporaries because I wanted to do
> it for devIBounds, and it would have felt inconsistent if those two didn't as
> well.
> 
> Either way I don't have a strong preference.

Ok, given that the Make functions are short NVRO-compatible inline functions and
that SkRect has extremely trivial default constructors I had assumed there was
no copying happening. I don't have a strong opinion either, this just stands out
from other uses of these rect factories that assign to local non-refs.

[Chris Dalton, 2016-05-13 14:25:29]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-05-13 14:25:29]

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/1971343002/#ps80001
(title: "undo binds to temporaries")

[commit-bot: I haz the power, 2016-05-13 14:25:38]

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

[commit-bot: I haz the power, 2016-05-13 14:27:09]

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

[commit-bot: I haz the power, 2016-05-13 14:27:10]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[Chris Dalton, 2016-05-13 15:28:34]

After the rebase we don't need "resetNonAA" anymore -- very nice!

[Chris Dalton, 2016-05-13 15:28:39]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-05-13 15:28:39]

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/1971343002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-13 15:28:44]

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

[commit-bot: I haz the power, 2016-05-13 15:31:13]

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

[commit-bot: I haz the power, 2016-05-13 15:31:14]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[Chris Dalton, 2016-05-13 15:47:23]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-05-13 15:47:23]

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/1971343002/#ps120001
(title: "compiler warnings")

[commit-bot: I haz the power, 2016-05-13 15:47:30]

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

[commit-bot: I haz the power, 2016-05-13 15:59:40]

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

[commit-bot: I haz the power, 2016-05-13 15:59:41]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[Chris Dalton, 2016-05-13 16:57:42]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-05-13 16:57:42]

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/1971343002/#ps140001
(title: "fix crash")

[commit-bot: I haz the power, 2016-05-13 16:57:51]

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

[commit-bot: I haz the power, 2016-05-13 17:01:02]

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

[commit-bot: I haz the power, 2016-05-13 17:01:03]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[Chris Dalton, 2016-05-13 17:20:38]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2016-05-13 17:20:43]

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

[commit-bot: I haz the power, 2016-05-13 17:25:03]

Description was changed from

==========
Convert GrClip to an abstract base class

Converts GrClip to an abstract base class and adds a "GrFixedClip"
implementation. GrFixedClip denotes a clip implemented with fixed-
function hardware. GrFixedClip allows us to remove the stateful
"fClipMode" member from GrClipMaskManager, and in the future will
be able to nicely encapsulate window rectangles.

After this change GrClipMaskManager is just a wrapper around
GrDrawTarget. We may want to consider removing it altogether.

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

to

==========
Convert GrClip to an abstract base class

Converts GrClip to an abstract base class and adds a "GrFixedClip"
implementation. GrFixedClip denotes a clip implemented with fixed-
function hardware. GrFixedClip allows us to remove the stateful
"fClipMode" member from GrClipMaskManager, and in the future will
be able to nicely encapsulate window rectangles.

After this change GrClipMaskManager is just a wrapper around
GrDrawTarget. We may want to consider removing it altogether.

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

Committed:
https://skia.googlesource.com/skia/+/846c051a4800b3cea341a0195db24297d6d9047f
==========

[commit-bot: I haz the power, 2016-05-13 17:25:04]

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