abstract name of clipping ops, to transtion to a more restricted set

abstract name of clipping ops, to transtion to a more restricted set

SkRegion::Op --> SkCanvas::ClipOp (alias) --> SkClipOp

pre-CL needed in chrome : https://codereview.chromium.org/2355583002/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2355483002

Committed: https://skia.googlesource.com/skia/+/73603f3c52ffd89fe9d035be827b566a0e7d3b79

[reed1, 2016-09-19 15:27:39]

Description was changed from

==========
abstract name of clipping ops, to transtion to a more restrict set

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

to

==========
abstract name of clipping ops, to transtion to a more restrict set

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2355483002
==========

[reed1, 2016-09-19 15:28:12]

reed@google.com changed reviewers:
+ bsalomon@google.com, fmalita@chromium.org

[reed1, 2016-09-19 15:28:13]

a more conservative first step...

[f(malita), 2016-09-19 15:39:26]

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:69: typedef SkRegion::Op ClipOp;
Any reason to use typedef + static consts instead of enum ClipOp?

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:71: static const ClipOp kDifference_Op         =
SkRegion::kDifference_Op;
constexpr?

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:544: #ifdef SK_LEGACY_SUPPORT_SETCLIPREGION
SK_SUPPORT_LEGACY_SETCLIPREGION?

[bsalomon, 2016-09-19 16:07:20]

Is there supposed to be a SkClipOp.h in the patchset?

[reed1, 2016-09-19 18:17:08]

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:69: typedef SkRegion::Op ClipOp;
On 2016/09/19 15:39:25, f(malita) wrote:
> Any reason to use typedef + static consts instead of enum ClipOp?

I want an alias for whatever ClipOp is going to be (either SkClipOp or
SkRegion::Op). This way functions can take SkCanvas::ClipOp as a type, and
callers can use SkCanvas::kIntersect_Op either way

Eventually I will remove this aliasing entirely, and just land on SkClipOp.

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:71: static const ClipOp kDifference_Op         =
SkRegion::kDifference_Op;
On 2016/09/19 15:39:25, f(malita) wrote:
> constexpr?

Turns out the linker wants to see these as actual global variables, so I need
const.

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:544: #ifdef SK_LEGACY_SUPPORT_SETCLIPREGION
On 2016/09/19 15:39:26, f(malita) wrote:
> SK_SUPPORT_LEGACY_SETCLIPREGION?

Done.

[reed1, 2016-09-19 18:21:31]

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

[commit-bot: I haz the power, 2016-09-19 18:21:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-09-19 18:43:11]

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

[commit-bot: I haz the power, 2016-09-19 18:43:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-09-19 18:43:49]

Description was changed from

==========
abstract name of clipping ops, to transtion to a more restrict set

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2355483002
==========

to

==========
abstract name of clipping ops, to transtion to a more restrict set

pre-CL needed in chrome : https://codereview.chromium.org/2355583002/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2355483002
==========

[reed1, 2016-09-19 18:55:13]

Description was changed from

==========
abstract name of clipping ops, to transtion to a more restrict set

pre-CL needed in chrome : https://codereview.chromium.org/2355583002/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2355483002
==========

to

==========
abstract name of clipping ops, to transtion to a more restricted set

SkRegion::Op --> SkCanvas::ClipOp (alias) --> SkClipOp

pre-CL needed in chrome : https://codereview.chromium.org/2355583002/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2355483002
==========

[commit-bot: I haz the power, 2016-09-19 19:31:10]

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

[commit-bot: I haz the power, 2016-09-19 19:31:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[f(malita), 2016-09-19 19:43:25]

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2355483002/diff/1/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:69: typedef SkRegion::Op ClipOp;
On 2016/09/19 18:17:08, reed1 wrote:
> On 2016/09/19 15:39:25, f(malita) wrote:
> > Any reason to use typedef + static consts instead of enum ClipOp?
> 
> I want an alias for whatever ClipOp is going to be (either SkClipOp or
> SkRegion::Op). This way functions can take SkCanvas::ClipOp as a type, and
> callers can use SkCanvas::kIntersect_Op either way
> 
> Eventually I will remove this aliasing entirely, and just land on SkClipOp.

I see.  Would it work to typedef unsigned ClipOp for the legacy case?  I think
that would allow passing/comparing both enum types thanks to implicit enum
conversion?

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:1553: if (SkRegion::kIntersect_Op == op && !doAA &&
fMCRec->fMatrix.isScaleTranslate()) {
kIntersect_Op

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:1576: if (SkRegion::kIntersect_Op == op) {
kIntersect_Op

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:1618: fMCRec->fRasterClip.op(devR,
this->getTopLayerBounds(), (SkRegion::Op)op, isAA);
Maybe add static asserts for all ClipOp values matching SkRegion::Op vals
someplace?

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:1696: if (SkRegion::kIntersect_Op == op &&
!path.isInverseFillType()) {
kIntersect_Op

[reed1, 2016-09-19 20:42:50]

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:1553: if (SkRegion::kIntersect_Op == op && !doAA &&
fMCRec->fMatrix.isScaleTranslate()) {
On 2016/09/19 19:43:25, f(malita) wrote:
> kIntersect_Op

deleted

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:1576: if (SkRegion::kIntersect_Op == op) {
On 2016/09/19 19:43:25, f(malita) wrote:
> kIntersect_Op

deleted

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:1618: fMCRec->fRasterClip.op(devR,
this->getTopLayerBounds(), (SkRegion::Op)op, isAA);
On 2016/09/19 19:43:25, f(malita) wrote:
> Maybe add static asserts for all ClipOp values matching SkRegion::Op vals
> someplace?

Done.

https://codereview.chromium.org/2355483002/diff/100001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:1696: if (SkRegion::kIntersect_Op == op &&
!path.isInverseFillType()) {
On 2016/09/19 19:43:25, f(malita) wrote:
> kIntersect_Op

deleted

[reed1, 2016-09-19 20:42:56]

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

[commit-bot: I haz the power, 2016-09-19 20:43:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 21:02:29]

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

[commit-bot: I haz the power, 2016-09-19 21:02:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[reed1, 2016-09-19 21:36:06]

ptal

[reed1, 2016-09-19 22:44:10]

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

[commit-bot: I haz the power, 2016-09-19 22:44:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 22:50:22]

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

[commit-bot: I haz the power, 2016-09-19 22:50:23]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-Clang-arm64-Debug-GN_Android_FrameworkDefs-Trybot on
master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...)

[reed1, 2016-09-20 10:13:29]

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

[commit-bot: I haz the power, 2016-09-20 10:13:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 10:14:22]

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

[commit-bot: I haz the power, 2016-09-20 10:14:23]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-Clang-arm64-Debug-GN_Android_FrameworkDefs-Trybot on
master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...)

[f(malita), 2016-09-20 12:56:11]

lgtm

[reed1, 2016-09-20 13:37:36]

The CQ bit was checked by reed@google.com

[reed1, 2016-09-20 13:37:37]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2355483002/#ps160001
(title: "no need for ifdef for globals")

[commit-bot: I haz the power, 2016-09-20 13:37:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 13:43:32]

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

[commit-bot: I haz the power, 2016-09-20 13:43:33]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-arm64-Debug-GN_Android_FrameworkDefs-Trybot on
master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...)

[mtklein_C, 2016-09-20 15:36:15]

The CQ bit was checked by mtklein@chromium.org

[commit-bot: I haz the power, 2016-09-20 15:36:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 15:42:41]

Description was changed from

==========
abstract name of clipping ops, to transtion to a more restricted set

SkRegion::Op --> SkCanvas::ClipOp (alias) --> SkClipOp

pre-CL needed in chrome : https://codereview.chromium.org/2355583002/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2355483002
==========

to

==========
abstract name of clipping ops, to transtion to a more restricted set

SkRegion::Op --> SkCanvas::ClipOp (alias) --> SkClipOp

pre-CL needed in chrome : https://codereview.chromium.org/2355583002/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2355483002

Committed:
https://skia.googlesource.com/skia/+/73603f3c52ffd89fe9d035be827b566a0e7d3b79
==========

[commit-bot: I haz the power, 2016-09-20 15:42:42]

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