SkPath::rewind needs to have same reset as SkPath::reset.

SkPath::rewind needs to have same reset as SkPath::reset.

R=caryclark@google.com, reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9718

[bungeman-skia, 2013-06-18 22:33:42]

While working on other things, noticed that SkPath::rewind didn't seem to reset
the same as SkPath::reset.

[reed1, 2013-06-19 12:29:28]

lgtm

[reed1, 2013-06-19 12:30:10]

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

https://codereview.chromium.org/17432003/diff/1/src/core/SkPath.cpp#newcode428
src/core/SkPath.cpp:428: fBoundsIsDirty = true;
Would be nice if we have a shared resetFields() helper method to encapsulate
this, maybe.

[bungeman-skia, 2013-06-19 17:15:46]

Patch set 2 is a bit more invasive, adding a SkPath::resetFields() which
(mostly) mirrors the default constructor. The copy constructor and operator= are
also brought into line.

It seemed that the existing handling of (the Android specific) fSourcePath in
particular was a bit spotty. This change attempts to normalize handling of
fSourcePath, but I'm unsure as to how it is used. A quick check of the Android
source suggests that it isn't used any more?

[reed1, 2013-06-19 17:36:49]

lets add a unittest to verify the (newly clarified) behavior of
constructor/reset/rewind.

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

https://codereview.chromium.org/17432003/diff/6001/src/core/SkPath.cpp#newcod...
src/core/SkPath.cpp:222: , fLastMoveToIndex(INITIAL_LASTMOVETOINDEX_VALUE)
why not have the constructor also call resetFields()? There is no allocation or
other complication, so no actual value in performing this construct-like syntax
over just an assignment.

[caryclark, 2013-06-19 17:38:21]

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

https://codereview.chromium.org/17432003/diff/6001/src/core/SkPath.cpp#newcod...
src/core/SkPath.cpp:235: 
could this be
#ifdef SK_BUILD_FOR_ANDROID
    , fGenerationID(-1)
#endif
{
    this->resetFields();
}

?

[bungeman-skia, 2013-06-19 19:59:48]

Clarified comments and consolidated code. Also have a slightly better idea of
what fGenerationID might be used for, so corrected (or at least made consistent)
use of GEN_ID_INC.

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

https://codereview.chromium.org/17432003/diff/6001/src/core/SkPath.cpp#newcod...
src/core/SkPath.cpp:222: , fLastMoveToIndex(INITIAL_LASTMOVETOINDEX_VALUE)
On 2013/06/19 17:36:49, reed1 wrote:
> why not have the constructor also call resetFields()? There is no allocation
or
> other complication, so no actual value in performing this construct-like
syntax
> over just an assignment.

Done.

https://codereview.chromium.org/17432003/diff/6001/src/core/SkPath.cpp#newcod...
src/core/SkPath.cpp:235: 
On 2013/06/19 17:38:21, caryclark wrote:
> could this be
> #ifdef SK_BUILD_FOR_ANDROID
>     , fGenerationID(-1)
> #endif
> {
>     this->resetFields();
> }
> 
> ?

Done.

[caryclark, 2013-06-19 20:02:56]

lgtm

[reed1, 2013-06-19 20:50:04]

unit test please :)

[bungeman-skia, 2013-06-20 17:46:51]

Patch Set 7 adds testing for reset/rewind/default constructor equivalence.

[bungeman-skia, 2013-06-21 14:51:58]

On 2013/06/19 20:50:04, reed1 wrote:
> unit test please :)

Patch Set 7 modifies the PathTest to test the reset/rewind/default-contructor
equivalence (since it already came rather close to doing so). reed1, ptal.

[reed1, 2013-06-21 15:08:20]

lgtm

[bungeman-skia, 2013-06-21 15:13:37]

Committed patchset #7 manually as r9718.