Use SkPathRef gen id for SkPath::getGenerationID

Use SkPathRef gen id for SkPath::getGenerationID

Committed: http://code.google.com/p/skia/source/detail?r=12029

[mtklein, 2013-10-29 20:38:36]

https://codereview.chromium.org/49693002/diff/30001/include/core/SkPathRef.h
File include/core/SkPathRef.h (right):

https://codereview.chromium.org/49693002/diff/30001/include/core/SkPathRef.h#...
include/core/SkPathRef.h:233: * contents but different genIDs. Zero is reserved
and means an ID has not yet been determined
We don't ever return 0 here do we?  Do we need to tell the reader that 0 is
special here?  Or just a simple "Will never return 0."?  I hate to expose why.

https://codereview.chromium.org/49693002/diff/30001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/49693002/diff/30001/src/core/SkPath.cpp#newco...
src/core/SkPath.cpp:150: , fSourcePath(NULL)
Nice catch.

https://codereview.chromium.org/49693002/diff/30001/src/core/SkPath.cpp#newco...
src/core/SkPath.cpp:326: SkASSERT((unsigned)fFillType < (32 -
kPathRefGenIDBitCnt));
Comparing values vs bitcounts?  I think we need 1<<(32 - kPathRefGenIDBitCnt) on
the right?

[bsalomon, 2013-10-29 20:45:03]

https://codereview.chromium.org/49693002/diff/30001/include/core/SkPathRef.h
File include/core/SkPathRef.h (right):

https://codereview.chromium.org/49693002/diff/30001/include/core/SkPathRef.h#...
include/core/SkPathRef.h:233: * contents but different genIDs. Zero is reserved
and means an ID has not yet been determined
On 2013/10/29 20:38:36, mtklein wrote:
> We don't ever return 0 here do we?  Do we need to tell the reader that 0 is
> special here?  Or just a simple "Will never return 0."?  I hate to expose why.

good point, done.

https://codereview.chromium.org/49693002/diff/30001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/49693002/diff/30001/src/core/SkPath.cpp#newco...
src/core/SkPath.cpp:326: SkASSERT((unsigned)fFillType < (32 -
kPathRefGenIDBitCnt));
On 2013/10/29 20:38:36, mtklein wrote:
> Comparing values vs bitcounts?  I think we need 1<<(32 - kPathRefGenIDBitCnt)
on
> the right?

d'oh, yes.

[mtklein, 2013-10-29 20:47:07]

On 2013/10/29 20:45:03, bsalomon wrote:
> https://codereview.chromium.org/49693002/diff/30001/include/core/SkPathRef.h
> File include/core/SkPathRef.h (right):
> 
>
https://codereview.chromium.org/49693002/diff/30001/include/core/SkPathRef.h#...
> include/core/SkPathRef.h:233: * contents but different genIDs. Zero is
reserved
> and means an ID has not yet been determined
> On 2013/10/29 20:38:36, mtklein wrote:
> > We don't ever return 0 here do we?  Do we need to tell the reader that 0 is
> > special here?  Or just a simple "Will never return 0."?  I hate to expose
why.
> 
> good point, done.
> 
> https://codereview.chromium.org/49693002/diff/30001/src/core/SkPath.cpp
> File src/core/SkPath.cpp (right):
> 
>
https://codereview.chromium.org/49693002/diff/30001/src/core/SkPath.cpp#newco...
> src/core/SkPath.cpp:326: SkASSERT((unsigned)fFillType < (32 -
> kPathRefGenIDBitCnt));
> On 2013/10/29 20:38:36, mtklein wrote:
> > Comparing values vs bitcounts?  I think we need 1<<(32 -
kPathRefGenIDBitCnt)
> on
> > the right?
> 
> d'oh, yes.

lgtm

[commit-bot: I haz the power, 2013-10-29 20:49:55]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/bsalomon@google.com/49693002/130001

[commit-bot: I haz the power, 2013-10-29 21:34:58]

Change committed as 12010

[robertphillips, 2013-10-29 22:47:39]

reverted in r12012 due to Android failures on PathTest:

 	FAILED: ../../../tests/PathTest.cpp:150: copy.getGenerationID() > copyID
 	FAILED: ../../../tests/PathTest.cpp:156: assign.getGenerationID() > assignID
 	FAILED: ../../../tests/PathTest.cpp:164: copy.getGenerationID() > copyID
 	FAILED: ../../../tests/PathTest.cpp:165: assign.getGenerationID() > assignID

[bsalomon, 2013-10-30 13:06:45]

On 2013/10/29 22:47:39, robertphillips wrote:
> reverted in r12012 due to Android failures on PathTest:
> 
>  	FAILED: ../../../tests/PathTest.cpp:150: copy.getGenerationID() > copyID
>  	FAILED: ../../../tests/PathTest.cpp:156: assign.getGenerationID() > assignID
>  	FAILED: ../../../tests/PathTest.cpp:164: copy.getGenerationID() > copyID
>  	FAILED: ../../../tests/PathTest.cpp:165: assign.getGenerationID() > assignID

The test is now incorrect. The successive generation IDs associated with a path
do not have to be monotonic now that they are globally unique. I will update and
resubmit.

[bsalomon, 2013-10-30 15:49:53]

mtklein@, can you have a look at PathTest.cpp?

[mtklein, 2013-10-30 15:59:09]

lgtm with suggestions

https://codereview.chromium.org/49693002/diff/260001/tests/PathTest.cpp
File tests/PathTest.cpp (left):

https://codereview.chromium.org/49693002/diff/260001/tests/PathTest.cpp#oldco...
tests/PathTest.cpp:153: // Test reset.  Increment generation ID, don't touch
source path.
These little comments may need an update.  Maybe just s/Increment/Change ?

https://codereview.chromium.org/49693002/diff/260001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/49693002/diff/260001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:178: uint32_t z = a.getGenerationID();
const on v,w,x,y,z would help reading?

[bsalomon, 2013-10-30 17:39:07]

https://codereview.chromium.org/49693002/diff/260001/tests/PathTest.cpp
File tests/PathTest.cpp (left):

https://codereview.chromium.org/49693002/diff/260001/tests/PathTest.cpp#oldco...
tests/PathTest.cpp:153: // Test reset.  Increment generation ID, don't touch
source path.
On 2013/10/30 15:59:10, mtklein wrote:
> These little comments may need an update.  Maybe just s/Increment/Change ?

Done.

https://codereview.chromium.org/49693002/diff/260001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/49693002/diff/260001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:178: uint32_t z = a.getGenerationID();
On 2013/10/30 15:59:10, mtklein wrote:
> const on v,w,x,y,z would help reading?

Done.

[commit-bot: I haz the power, 2013-10-30 17:39:23]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/bsalomon@google.com/49693002/320001

[commit-bot: I haz the power, 2013-10-30 18:57:59]

Change committed as 12029