add GM case nonclosedpaths

add GM case nonclosedpaths. The 2 bugs below showed some non-closed paths which are easy to get wrong rendering results. So add a GM case for this kind paths to avoid potential bugs to be introduced.

BUG=skia:1782
BUG=skia:1811
Committed: http://code.google.com/p/skia/source/detail?r=12206

[yunchao, 2013-11-08 03:20:37]

Hi, Brian, Cary, Mike and Rob, 
Does this GM test need be added into expectations/gm/ignored-tests.txt?

[bsalomon, 2013-11-08 04:01:33]

On 2013/11/08 03:20:37, yunchao wrote:
> Hi, Brian, Cary, Mike and Rob, 
> Does this GM test need be added into expectations/gm/ignored-tests.txt?

Hi, new tests do not need to be added to ignored-tests.txt.

Perhaps the GM should be called nonclosedrects?

[yunchao, 2013-11-08 04:19:36]

On 2013/11/08 04:01:33, bsalomon wrote:
> On 2013/11/08 03:20:37, yunchao wrote:
> > Hi, Brian, Cary, Mike and Rob, 
> > Does this GM test need be added into expectations/gm/ignored-tests.txt?
> 
> Hi, new tests do not need to be added to ignored-tests.txt.

Got it.

> Perhaps the GM should be called nonclosedrects?
 
Hi, Brian, It is easier to see the visual difference of lineJoin and lineCap for
a right angle(through skpdiff tool together with viewer.html), So I used a
rect-like geometry to show the case. But this case indeed test a non-close path.

[robertphillips, 2013-11-08 14:18:50]

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp
File gm/nonclosepaths.cpp (right):

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:13: 
// This GM tests ...

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:17: 
Maybe make this ClosureType rather than NonCloseType?

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:21: 
coincide -> coincides

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:24: 
coincide -> coincides

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:25: FakeCloseMiddle,  // The last point coincide with the
first one in the middle of a line.
// The path looks closed, and the final rendering looks closed too. 
(i.e., remove the extra like)

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:31: protected:
SK_OVERRIDE

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:32: virtual SkString onShortName() {
nonclosedpaths (i.e., add 'd')

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:35: 
SK_OVERRIDE

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:36: virtual SkISize onISize() {
Does it need to be this big?

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:39: 
non-close path -> non-closed paths
for right angle is easier -> right angles make it easier

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:41: // show the visual difference of lineCap and lineJoin.
We only use const references in Skia. If the object is non-const you need to use
a pointer.

Seems like this method could be static.

Why is the rect offset to 30,30. Why not have it start at 0,0?

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:58: // set the location for the current test on the canvas
SetLocation for static member methods

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:64: 
SK_OVERRIDE

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:65: virtual void onDraw(SkCanvas* canvas) {
Maybe kStrokeWidths or gStrokeWidths?

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:66: static const int strokeWidth[] = {0, 10, 40, 50};
Maybe numWidths?

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:68: 
Same here w.r.t. naming. Maybe prefix with a 'k' or 'g' and make plural?

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:89: // For stroke style painter and fill-and-stroke style
painter
pre-increments for these please (e.g., ++type)

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:98: SkPath path;
this->makePath

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:103: paint.setStrokeJoin(testJoin[join]);
SkIntToScalar instead of SkScalar

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:120: SkPath path;
this->makePath

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:136: 
Please use DEF_GM macro instead here

[yunchao, 2013-11-08 15:56:21]

Thanks for you careful review, Rob. I have revised it accordingly. I alos
replied to your questions. Please help to review...

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp
File gm/nonclosepaths.cpp (right):

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:36: virtual SkISize onISize() {
On 2013/11/08 14:18:50, robertphillips wrote:
> Does it need to be this big?

Yeah, there are 12 * 18 + 3 cases, very one is 100 * 100 pixels. From
viewer.html for the output of skpdiff, the layout of nonclosedpaths is not bad.

https://codereview.chromium.org/64173009/diff/170001/gm/nonclosepaths.cpp#new...
gm/nonclosepaths.cpp:41: // show the visual difference of lineCap and lineJoin.
On 2013/11/08 14:18:50, robertphillips wrote:
> We only use const references in Skia. If the object is non-const you need to
use
> a pointer.
> 
> Seems like this method could be static.
> 
> Why is the rect offset to 30,30. Why not have it start at 0,0?

Because some cases are stroke-style, and the biggest strokeWidth is 50(half
would be 25). Set to (0, 0) may exceed the bounds.

[robertphillips, 2013-11-08 16:19:43]

What do you think about having a comment explaining the genesis of the width &
height?

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp
File gm/nonclosedpaths.cpp (right):

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:13: 
Could you add a comment here describing what the GM tests? Something like:

// This GM tests a grab-bag of closed and pseudo-closed paths. Depending on
// the stroke settings these slightly different paths can give widely different
// results

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:18: enum ClosureType {
TotalNonClose -> TotallyNonClosed

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:22: FakeCloseCorner,  // The last point coincides with the
first one at a corner.
Please remove the "like".

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:41: // easier to show the visual difference of lineCap and
lineJoin.
Please change makePath to MakePath.

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:58: // Set the location for the current test on the canvas
Please change to SetLocation.

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:98: SkPath path;
Don't need this-> any more.

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:107: canvas->restore();
Pre-increment here please.

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:120: SkPath path;
Don't need this-> here anymore

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:122: 
Don't think this needs to be inside the loop.

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:126: canvas->restore();
Pre-increment here too.

[yunchao, 2013-11-08 16:54:43]

Thanks again, Rob. Have revised accordingly. Please help to review it.

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp
File gm/nonclosedpaths.cpp (right):

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:13: 
On 2013/11/08 16:19:44, robertphillips wrote:
> Could you add a comment here describing what the GM tests? Something like:
> 
> // This GM tests a grab-bag of closed and pseudo-closed paths. Depending on
> // the stroke settings these slightly different paths can give widely
different
> // results

Agree.

https://codereview.chromium.org/64173009/diff/280001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:98: SkPath path;
On 2013/11/08 16:19:44, robertphillips wrote:
> Don't need this-> any more.

I don't know what I was thinking, I have made it a static function already...

[robertphillips, 2013-11-08 18:13:28]

lgtm + a nit

https://codereview.chromium.org/64173009/diff/340001/gm/nonclosedpaths.cpp
File gm/nonclosedpaths.cpp (right):

https://codereview.chromium.org/64173009/diff/340001/gm/nonclosedpaths.cpp#ne...
gm/nonclosedpaths.cpp:21: enum ClosureType {
Please line up the comment.

[yunchao, 2013-11-08 18:29:37]

On 2013/11/08 18:13:28, robertphillips wrote:
> lgtm + a nit
> 
> https://codereview.chromium.org/64173009/diff/340001/gm/nonclosedpaths.cpp
> File gm/nonclosedpaths.cpp (right):
> 
>
https://codereview.chromium.org/64173009/diff/340001/gm/nonclosedpaths.cpp#ne...
> gm/nonclosedpaths.cpp:21: enum ClosureType {
> Please line up the comment.

Done.

[robertphillips, 2013-11-08 18:38:25]

Please fix the trybot compiler errors before committing.

[yunchao, 2013-11-09 01:19:24]

On 2013/11/08 18:38:25, robertphillips wrote:
> Please fix the trybot compiler errors before committing.

I know that, Rob. I am at home yesterday night, no build env, so I just rewrite
this patch without building it for immediate review because it is your working
time, :)  I will build it successfully locally before landing.

[commit-bot: I haz the power, 2013-11-09 01:26:14]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/yunchao.he@intel.com/64173009/430001

[commit-bot: I haz the power, 2013-11-09 01:37:33]

Change committed as 12206