track oval in SkPath

track oval in SkPath

This CL is separated from the big CL (circle drawing via GPU shader).
There are some changes:
 - use SkBool8 instead of bool.
 - add isOval() interface to check whether it's an oval. This interface returns the bounding rect.
 - add detailed comments for isOval() and field fIsOval.

BUG=
TEST=

[guanqun, 2012-04-12 06:53:56]

please review. thanks!

[bsalomon, 2012-04-16 14:16:15]

On 2012/04/12 06:53:56, guanqun wrote:
> please review. thanks!

It mostly looks good, I have some small comments. I'd like Mike to look at this
since it is a core Skia class.

[bsalomon, 2012-04-16 14:16:22]

http://codereview.appspot.com/6012047/diff/2001/include/core/SkPath.h
File include/core/SkPath.h (right):

http://codereview.appspot.com/6012047/diff/2001/include/core/SkPath.h#newcode164
include/core/SkPath.h:164: /** Returns true if the path is an oval (Empty path
is not an oval).
Maybe we should say that tracking whether a path is an oval is considered an
optimization for performance and so some paths that are in fact ovals can report
false. That gives greater flexibility to change the implementation without
breaking a contract with the caller (and we don't need to detail the exact logic
in the comment).

http://codereview.appspot.com/6012047/diff/2001/include/core/SkPath.h#newcode772
include/core/SkPath.h:772: /* This flag is used to track whether this path is an
oval.
Since this field is private I'm not sure we need such a thorough warning about
the difference between the public member function and the boolean.

http://codereview.appspot.com/6012047/diff/2001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/6012047/diff/2001/src/core/SkPath.cpp#newcode35
src/core/SkPath.cpp:35: class SkAutoDisableCircleCheck {
We should call this Oval now not Circle, right?

[guanqun.lu_gmail.com, 2012-04-16 15:57:53]

Comment updated and changed SkAutoDisableCircleCheck to
SkAutoDisableOvalCheck (nice catch!)

-- 
Guanqun

[reed1, 2012-04-16 19:19:29]

getting close.

Can we default to fIsOval being false? That feels better to me. What is the
thinking for defaulting to true?

http://codereview.appspot.com/6012047/diff/9004/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/6012047/diff/9004/src/core/SkPath.cpp#newcode761
src/core/SkPath.cpp:761: if (fIsOval) {
Can we write this has a private helper called

hasOnlyMoveTos() or something? Is that what it is determining?

[guanqun, 2012-04-17 00:38:40]

Making fIsOval default to true was a little optimization derived from my first
patch, otherwise, it had to track tristate (not determined, true, false).

In this new patch, it can be default to true without incurring much overhead. So
I've changed it to being false.

Please review again, thanks!

http://codereview.appspot.com/6012047/diff/9004/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/6012047/diff/9004/src/core/SkPath.cpp#newcode761
src/core/SkPath.cpp:761: if (fIsOval) {
On 2012/04/16 19:19:30, reed1 wrote:
> Can we write this has a private helper called
> 
> hasOnlyMoveTos() or something? Is that what it is determining?

Done.

[guanqun, 2012-04-17 00:41:28]

On 2012/04/17 00:38:40, guanqun wrote:
> In this new patch, it can be default to true without incurring much overhead.
So

typo. s/true/false

[reed1, 2012-04-17 12:26:56]

lgtm

[bsalomon, 2012-04-17 15:39:03]

On 2012/04/17 12:26:56, reed1 wrote:
> lgtm

I committed this as r3705 but then reverted in r3706 for fixed pt failures (all
platforms)

[guanqun, 2012-04-18 01:26:59]

So embarrassed. :(

It's due to the fact I'm not wrapping number 30 by SkIntToScalar(). Fixed the
tests. It won't cause the failure now. I've tested it on x86_64 Linux platform,
both float and fixed build.

Sorry for the trouble...

BTW: Do we have some try bots that I can run the patch on different platforms?
Something like WebKit does.

Thanks!

[bsalomon, 2012-04-18 13:30:13]

Relanded at r3716.

On 2012/04/18 01:26:59, guanqun wrote:
> So embarrassed. :(
> 
> It's due to the fact I'm not wrapping number 30 by SkIntToScalar(). Fixed the
> tests. It won't cause the failure now. I've tested it on x86_64 Linux
platform,
> both float and fixed build.

That mistake has been made many times before :)

> 
> Sorry for the trouble...
> 
> BTW: Do we have some try bots that I can run the patch on different platforms?
> Something like WebKit does.

We don't currently have try bots but we hope to have them come online sometime
in June.

> 
> Thanks!