increase coverage of SkPath.cpp, remove unused code

increase coverage of SkPath.cpp, remove unused code

Using Mike Klein's excellent coverage tool, increase the
unit testing of SkPath.cpp from 70% to 95%.

Along the way, determined that these functions were not
maintained or used:

SkPath::pathTo
SkPath::contains

as well as a large block of SkPath::cheapGetDirection().

Changed SkPath::validate() to permit infinities in
the path data points.

Fixed errors in preserving direction.
Fixed error setting direction when convexity is unknown.

Added missing conic to moveTo only detector.

BUG=

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

[reed1, 2013-11-13 18:37:52]

https://codereview.chromium.org/65493004/diff/40001/include/core/SkPath.h
File include/core/SkPath.h (left):

https://codereview.chromium.org/65493004/diff/40001/include/core/SkPath.h#old...
include/core/SkPath.h:896: bool contains(SkScalar x, SkScalar y) const;
This guy was written partly to optimize the code in webkit's SkiaUtils.cpp
SkPathContainsPoint. Obviously we haven't make that connection yet, but I think
we should.

https://codereview.chromium.org/65493004/diff/40001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/65493004/diff/40001/tests/PathTest.cpp#newcode26
tests/PathTest.cpp:26: path->moveTo(SkFloatToScalar(146.939f),
SkFloatToScalar(1012.84f));
are these macros required?

https://codereview.chromium.org/65493004/diff/40001/tests/PathTest.cpp#newcod...
tests/PathTest.cpp:147: assign = assign;
this looks odd enough, perhaps we could add a comment for these 3 lines of
code... something like (I'm just guessing, as I don't really know why)

// test that self-assignment doesn't change our id

We don't, in general, test for self-assignment. Was this motivated by a bug
somewhere? I ask, because it seems possible that a future reviewer may delete
this code w/o knowing why its here.

[caryclark, 2013-11-13 18:52:37]

https://codereview.chromium.org/65493004/diff/40001/include/core/SkPath.h
File include/core/SkPath.h (left):

https://codereview.chromium.org/65493004/diff/40001/include/core/SkPath.h#old...
include/core/SkPath.h:896: bool contains(SkScalar x, SkScalar y) const;
On 2013/11/13 18:37:52, reed1 wrote:
> This guy was written partly to optimize the code in webkit's SkiaUtils.cpp
> SkPathContainsPoint. Obviously we haven't make that connection yet, but I
think
> we should.

I can put it back. It isn't called today within Skia or Chrome; there are no
tests, and when I wrote a test, I discovered the code didn't work.

On the other hand, it could either be in, but commented out, or be retrieved in
the future from the repository if the current code is a good starting point.
Just let me know what you'd prefer.

https://codereview.chromium.org/65493004/diff/40001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/65493004/diff/40001/tests/PathTest.cpp#newcode26
tests/PathTest.cpp:26: path->moveTo(SkFloatToScalar(146.939f),
SkFloatToScalar(1012.84f));
On 2013/11/13 18:37:52, reed1 wrote:
> are these macros required?

Only if one day, we wanted to allow optionally SkScalar == double.

SkFloatToScalar is used irregularly throughout. 

What is your desired use case for SkFloatToScalar?

https://codereview.chromium.org/65493004/diff/40001/tests/PathTest.cpp#newcod...
tests/PathTest.cpp:147: assign = assign;
On 2013/11/13 18:37:52, reed1 wrote:
> this looks odd enough, perhaps we could add a comment for these 3 lines of
> code... something like (I'm just guessing, as I don't really know why)
> 
> // test that self-assignment doesn't change our id
> 
> We don't, in general, test for self-assignment. Was this motivated by a bug
> somewhere? I ask, because it seems possible that a future reviewer may delete
> this code w/o knowing why its here.

I'll add a comment.

I wouldn't be surprised if I was misreading the code, but I thought that

SkPath& SkPath::operator=(const SkPath& that) {
    SkDEBUGCODE(that.validate();)

    if (this != &that) {

was a test for self-assignment.

[reed1, 2013-11-13 18:58:58]

https://codereview.chromium.org/65493004/diff/40001/include/core/SkPath.h
File include/core/SkPath.h (left):

https://codereview.chromium.org/65493004/diff/40001/include/core/SkPath.h#old...
include/core/SkPath.h:896: bool contains(SkScalar x, SkScalar y) const;
On 2013/11/13 18:52:37, caryclark wrote:
> On 2013/11/13 18:37:52, reed1 wrote:
> > This guy was written partly to optimize the code in webkit's SkiaUtils.cpp
> > SkPathContainsPoint. Obviously we haven't make that connection yet, but I
> think
> > we should.
> 
> I can put it back. It isn't called today within Skia or Chrome; there are no
> tests, and when I wrote a test, I discovered the code didn't work.
> 
> On the other hand, it could either be in, but commented out, or be retrieved
in
> the future from the repository if the current code is a good starting point.
> Just let me know what you'd prefer.

Well, it was called, until you removed the GM that called it.

If you have a test that fails, lets add that to a bug, and I'll try to fix it.

https://codereview.chromium.org/65493004/diff/40001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/65493004/diff/40001/tests/PathTest.cpp#newcode26
tests/PathTest.cpp:26: path->moveTo(SkFloatToScalar(146.939f),
SkFloatToScalar(1012.84f));
On 2013/11/13 18:52:37, caryclark wrote:
> On 2013/11/13 18:37:52, reed1 wrote:
> > are these macros required?
> 
> Only if one day, we wanted to allow optionally SkScalar == double.
> 
> SkFloatToScalar is used irregularly throughout. 
> 
> What is your desired use case for SkFloatToScalar?
> 

Good question. It began life when scalars could be fixed, in which case a macro
was definitely needed.

I wonder if we need any cast/macro when scalar is double. would we ever get a
warning if we said

double x;
x = 15.3f;

I think my macro comment is that much of the change in this file is from adding
that macro, which I'm not sure makes an impact, but is a lot more to read.

https://codereview.chromium.org/65493004/diff/40001/tests/PathTest.cpp#newcod...
tests/PathTest.cpp:147: assign = assign;
On 2013/11/13 18:52:37, caryclark wrote:
> On 2013/11/13 18:37:52, reed1 wrote:
> > this looks odd enough, perhaps we could add a comment for these 3 lines of
> > code... something like (I'm just guessing, as I don't really know why)
> > 
> > // test that self-assignment doesn't change our id
> > 
> > We don't, in general, test for self-assignment. Was this motivated by a bug
> > somewhere? I ask, because it seems possible that a future reviewer may
delete
> > this code w/o knowing why its here.
> 
> I'll add a comment.
> 
> I wouldn't be surprised if I was misreading the code, but I thought that
> 
> SkPath& SkPath::operator=(const SkPath& that) {
>     SkDEBUGCODE(that.validate();)
> 
>     if (this != &that) {
> 
> was a test for self-assignment.

It definitely is. I'm just noting that we don't *test* for that anywhere else,
but perhaps we should (e.g. SkRegion, etc. all of our other complex class that
have assignment).

[caryclark, 2013-11-13 21:47:26]

contains() restored; SkFloatToScalar() removed; please review again

[bsalomon, 2013-11-14 01:28:54]

lgtm, the new cheap dir computer is easier to read.

https://codereview.chromium.org/65493004/diff/170001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/65493004/diff/170001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:147: assignID = assign.getGenerationID();
alignment?

[caryclark, 2013-11-14 15:29:35]

This incremental CL gets the lines + functions coverage for SkPath.cpp above
95%. The coverage for branches is only at 77%, which will improve in a future CL
if SkASSERTs in the coverage build remove their internal branch.

https://codereview.chromium.org/65493004/diff/170001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/65493004/diff/170001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:147: assignID = assign.getGenerationID();
On 2013/11/14 01:28:54, bsalomon wrote:
> alignment?

I failed to realize that this entire block is an Android-only test. I moved the
operator= self-assignment test to the general area and verified that the
coverage picks up when this == that. I couldn't think of a way for PathTest.cpp
to verify that the self-assignment is there or not, since it is strictly a
performance optimization without any side effects.

(Fixed)

[reed1, 2013-11-14 16:21:57]

lgtm

[commit-bot: I haz the power, 2013-11-14 16:24:28]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/caryclark@google.com/65493004/270001

[commit-bot: I haz the power, 2013-11-14 16:53:30]

Change committed as 12291