call drawRect to try GrAARectRenderer if the path is a rect

If the path is a rect, call drawRect to raster the geometry in SkCanvas::drawPath to get better performance.

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

R=bsalomon@google.com

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

[yunchao, 2013-08-29 09:40:00]

if the path is a rect, for miter-stroke rect or a hairline-stroke rect, call
drawRect in drawPath may use GrAARectRenderer, while previous implementation
will use GrSoftwarePathRender and then upload texture, that is very slow; for
fill rect, this will use GrAARectRenderer, it is not worse than
GrAAConvexPathRenderer in previous implementation. 

Some test data from bench(revise the Join Style to kMiter_Style in
bench/PathBench.cpp before testing):
bench name            w/o patch(ms) w/ patch(ms) improvement
path_stroke_big_rect        19.70     2.54       775.6%
path_stroke_small_rect      31.69     2.51       1262.5%

[bsalomon, 2013-08-29 12:49:58]

This makes sense for the GPU device. Adding Mike to make a call about whether we
want to vector out at the canvas level or push this decision down to
SkGpuDevice. I don't see any reason other devices types would get slower by
drawing rect-paths as rects, though.

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

https://codereview.chromium.org/23484007/diff/1/src/core/SkCanvas.cpp#newcode...
src/core/SkCanvas.cpp:1723: // if the path is a rect, we can call drawRect to
try GrAARectRenderer.
This comment is specific to SkGpuDevice, perhaps it should read:

// If the path is a rect call drawRect for devices that have optimized rects.

https://codereview.chromium.org/23484007/diff/1/src/core/SkCanvas.cpp#newcode...
src/core/SkCanvas.cpp:1726: drawRect(rect, paint);
style-nit:
this->drawRect(...);

[bsalomon, 2013-08-29 12:50:20]

This makes sense for the GPU device. Adding Mike to make a call about whether we
want to vector out at the canvas level or push this decision down to
SkGpuDevice. I don't see any reason other devices types would get slower by
drawing rect-paths as rects, though.

[yunchao, 2013-08-29 13:11:53]

On 2013/08/29 12:50:20, bsalomon wrote:
> This makes sense for the GPU device. Adding Mike to make a call about whether
we
> want to vector out at the canvas level or push this decision down to
> SkGpuDevice. I don't see any reason other devices types would get slower by
> drawing rect-paths as rects, though.

Yes, and I have tried to add this code into SkGpuDevice::drawPath at first.
However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath in some
cases(bevel-stroke rect and round-stroke rect those can't use GrAARectRenderer),
so if adding this code into SkGpuDevice(call drawRect in drawPath in
SkGpuDevice), there would be a loop. I simply add a flag into the parameter
lists in SkGpuDevice::drawPath, unfortunately, it would cause segmentation fault
for some bench cases. So I didn't try to add more code to fix this introduced
problem. It may be feasible to do this, but would be a little complex logically
to handle these introduced problem in SkGpuDevice, so I add the code into
SkCanvas::drawPath. It is simple, and easy to understand.

[yunchao, 2013-09-02 08:51:41]

On 2013/08/29 13:11:53, yunchao wrote:
> On 2013/08/29 12:50:20, bsalomon wrote:
> > This makes sense for the GPU device. Adding Mike to make a call about
whether
> we
> > want to vector out at the canvas level or push this decision down to
> > SkGpuDevice. I don't see any reason other devices types would get slower by
> > drawing rect-paths as rects, though.
> 
> Yes, and I have tried to add this code into SkGpuDevice::drawPath at first.
> However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath in some
> cases(bevel-stroke rect and round-stroke rect those can't use
GrAARectRenderer),
> so if adding this code into SkGpuDevice(call drawRect in drawPath in
> SkGpuDevice), there would be a loop. I simply add a flag into the parameter
> lists in SkGpuDevice::drawPath, unfortunately, it would cause segmentation
fault
> for some bench cases. So I didn't try to add more code to fix this introduced
> problem. It may be feasible to do this, but would be a little complex
logically
> to handle these introduced problem in SkGpuDevice, so I add the code into
> SkCanvas::drawPath. It is simple, and easy to understand.

Hi, bsalomon, robertphillips and reed, any suggestions about this patch?

[bsalomon, 2013-09-05 13:25:52]

On 2013/09/02 08:51:41, yunchao wrote:
> On 2013/08/29 13:11:53, yunchao wrote:
> > On 2013/08/29 12:50:20, bsalomon wrote:
> > > This makes sense for the GPU device. Adding Mike to make a call about
> whether
> > we
> > > want to vector out at the canvas level or push this decision down to
> > > SkGpuDevice. I don't see any reason other devices types would get slower
by
> > > drawing rect-paths as rects, though.
> > 
> > Yes, and I have tried to add this code into SkGpuDevice::drawPath at first.
> > However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath in some
> > cases(bevel-stroke rect and round-stroke rect those can't use
> GrAARectRenderer),
> > so if adding this code into SkGpuDevice(call drawRect in drawPath in
> > SkGpuDevice), there would be a loop. I simply add a flag into the parameter
> > lists in SkGpuDevice::drawPath, unfortunately, it would cause segmentation
> fault
> > for some bench cases. So I didn't try to add more code to fix this
introduced
> > problem. It may be feasible to do this, but would be a little complex
> logically
> > to handle these introduced problem in SkGpuDevice, so I add the code into
> > SkCanvas::drawPath. It is simple, and easy to understand.
> 
> Hi, bsalomon, robertphillips and reed, any suggestions about this patch?

Hi, We're a little hesitant about having one virtual at the SkCanvas level call
another.

What we could do is make drawPath() be non-virtual and add a protected virtual
function called onDrawPath that SkCanvas subclasses would override. Roughly
speaking:

public:
void drawPath(path, paint) {
  if (path->isRect()) { this->drawRect() } else { this->onDrawPath(path, paint);
}
}

protected:
virtual void onDrawPath(path, paint) {
 // does the stuff drawPath() does today
}

[yunchao, 2013-09-10 09:17:19]

On 2013/09/05 13:25:52, bsalomon wrote:
> On 2013/09/02 08:51:41, yunchao wrote:
> > On 2013/08/29 13:11:53, yunchao wrote:
> > > On 2013/08/29 12:50:20, bsalomon wrote:
> > > > This makes sense for the GPU device. Adding Mike to make a call about
> > whether
> > > we
> > > > want to vector out at the canvas level or push this decision down to
> > > > SkGpuDevice. I don't see any reason other devices types would get slower
> by
> > > > drawing rect-paths as rects, though.
> > > 
> > > Yes, and I have tried to add this code into SkGpuDevice::drawPath at
first.
> > > However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath in some
> > > cases(bevel-stroke rect and round-stroke rect those can't use
> > GrAARectRenderer),
> > > so if adding this code into SkGpuDevice(call drawRect in drawPath in
> > > SkGpuDevice), there would be a loop. I simply add a flag into the
parameter
> > > lists in SkGpuDevice::drawPath, unfortunately, it would cause segmentation
> > fault
> > > for some bench cases. So I didn't try to add more code to fix this
> introduced
> > > problem. It may be feasible to do this, but would be a little complex
> > logically
> > > to handle these introduced problem in SkGpuDevice, so I add the code into
> > > SkCanvas::drawPath. It is simple, and easy to understand.
> > 
> > Hi, bsalomon, robertphillips and reed, any suggestions about this patch?
> 
> Hi, We're a little hesitant about having one virtual at the SkCanvas level
call
> another.
> 
> What we could do is make drawPath() be non-virtual and add a protected virtual
> function called onDrawPath that SkCanvas subclasses would override. Roughly
> speaking:
> 
> public:
> void drawPath(path, paint) {
>   if (path->isRect()) { this->drawRect() } else { this->onDrawPath(path,
paint);
> }
> }
> 
> protected:
> virtual void onDrawPath(path, paint) {
>  // does the stuff drawPath() does today
> }

hi, any update about this patch?

[bsalomon, 2013-09-10 14:39:06]

On 2013/09/10 09:17:19, yunchao wrote:
> On 2013/09/05 13:25:52, bsalomon wrote:
> > On 2013/09/02 08:51:41, yunchao wrote:
> > > On 2013/08/29 13:11:53, yunchao wrote:
> > > > On 2013/08/29 12:50:20, bsalomon wrote:
> > > > > This makes sense for the GPU device. Adding Mike to make a call about
> > > whether
> > > > we
> > > > > want to vector out at the canvas level or push this decision down to
> > > > > SkGpuDevice. I don't see any reason other devices types would get
slower
> > by
> > > > > drawing rect-paths as rects, though.
> > > > 
> > > > Yes, and I have tried to add this code into SkGpuDevice::drawPath at
> first.
> > > > However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath in some
> > > > cases(bevel-stroke rect and round-stroke rect those can't use
> > > GrAARectRenderer),
> > > > so if adding this code into SkGpuDevice(call drawRect in drawPath in
> > > > SkGpuDevice), there would be a loop. I simply add a flag into the
> parameter
> > > > lists in SkGpuDevice::drawPath, unfortunately, it would cause
segmentation
> > > fault
> > > > for some bench cases. So I didn't try to add more code to fix this
> > introduced
> > > > problem. It may be feasible to do this, but would be a little complex
> > > logically
> > > > to handle these introduced problem in SkGpuDevice, so I add the code
into
> > > > SkCanvas::drawPath. It is simple, and easy to understand.
> > > 
> > > Hi, bsalomon, robertphillips and reed, any suggestions about this patch?
> > 
> > Hi, We're a little hesitant about having one virtual at the SkCanvas level
> call
> > another.
> > 
> > What we could do is make drawPath() be non-virtual and add a protected
virtual
> > function called onDrawPath that SkCanvas subclasses would override. Roughly
> > speaking:
> > 
> > public:
> > void drawPath(path, paint) {
> >   if (path->isRect()) { this->drawRect() } else { this->onDrawPath(path,
> paint);
> > }
> > }
> > 
> > protected:
> > virtual void onDrawPath(path, paint) {
> >  // does the stuff drawPath() does today
> > }
> 
> hi, any update about this patch?

For this to work we have to find a way to avoid having on SkCanvas virtual call
another. I suggested one possibility, but there may be others.

[yunchao, 2013-09-11 05:55:45]

On 2013/09/10 14:39:06, bsalomon wrote:
> On 2013/09/10 09:17:19, yunchao wrote:
> > On 2013/09/05 13:25:52, bsalomon wrote:
> > > On 2013/09/02 08:51:41, yunchao wrote:
> > > > On 2013/08/29 13:11:53, yunchao wrote:
> > > > > On 2013/08/29 12:50:20, bsalomon wrote:
> > > > > > This makes sense for the GPU device. Adding Mike to make a call
about
> > > > whether
> > > > > we
> > > > > > want to vector out at the canvas level or push this decision down to
> > > > > > SkGpuDevice. I don't see any reason other devices types would get
> slower
> > > by
> > > > > > drawing rect-paths as rects, though.
> > > > > 
> > > > > Yes, and I have tried to add this code into SkGpuDevice::drawPath at
> > first.
> > > > > However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath in some
> > > > > cases(bevel-stroke rect and round-stroke rect those can't use
> > > > GrAARectRenderer),
> > > > > so if adding this code into SkGpuDevice(call drawRect in drawPath in
> > > > > SkGpuDevice), there would be a loop. I simply add a flag into the
> > parameter
> > > > > lists in SkGpuDevice::drawPath, unfortunately, it would cause
> segmentation
> > > > fault
> > > > > for some bench cases. So I didn't try to add more code to fix this
> > > introduced
> > > > > problem. It may be feasible to do this, but would be a little complex
> > > > logically
> > > > > to handle these introduced problem in SkGpuDevice, so I add the code
> into
> > > > > SkCanvas::drawPath. It is simple, and easy to understand.
> > > > 
> > > > Hi, bsalomon, robertphillips and reed, any suggestions about this patch?
> > > 
> > > Hi, We're a little hesitant about having one virtual at the SkCanvas level
> > call
> > > another.
> > > 
> > > What we could do is make drawPath() be non-virtual and add a protected
> virtual
> > > function called onDrawPath that SkCanvas subclasses would override.
Roughly
> > > speaking:
> > > 
> > > public:
> > > void drawPath(path, paint) {
> > >   if (path->isRect()) { this->drawRect() } else { this->onDrawPath(path,
> > paint);
> > > }
> > > }
> > > 
> > > protected:
> > > virtual void onDrawPath(path, paint) {
> > >  // does the stuff drawPath() does today
> > > }
> > 
> > hi, any update about this patch?
> 
> For this to work we have to find a way to avoid having on SkCanvas virtual
call
> another. I suggested one possibility, but there may be others.

Yeah, I got it. I revised the code to make sure it will call the non-virtual
version(base class) by this->SkCanvas::drawXxxx, just like drawRRect and
clipRRect did in SkCanvas.

[bsalomon, 2013-09-11 13:58:50]

On 2013/09/11 05:55:45, yunchao wrote:
> On 2013/09/10 14:39:06, bsalomon wrote:
> > On 2013/09/10 09:17:19, yunchao wrote:
> > > On 2013/09/05 13:25:52, bsalomon wrote:
> > > > On 2013/09/02 08:51:41, yunchao wrote:
> > > > > On 2013/08/29 13:11:53, yunchao wrote:
> > > > > > On 2013/08/29 12:50:20, bsalomon wrote:
> > > > > > > This makes sense for the GPU device. Adding Mike to make a call
> about
> > > > > whether
> > > > > > we
> > > > > > > want to vector out at the canvas level or push this decision down
to
> > > > > > > SkGpuDevice. I don't see any reason other devices types would get
> > slower
> > > > by
> > > > > > > drawing rect-paths as rects, though.
> > > > > > 
> > > > > > Yes, and I have tried to add this code into SkGpuDevice::drawPath at
> > > first.
> > > > > > However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath in
some
> > > > > > cases(bevel-stroke rect and round-stroke rect those can't use
> > > > > GrAARectRenderer),
> > > > > > so if adding this code into SkGpuDevice(call drawRect in drawPath in
> > > > > > SkGpuDevice), there would be a loop. I simply add a flag into the
> > > parameter
> > > > > > lists in SkGpuDevice::drawPath, unfortunately, it would cause
> > segmentation
> > > > > fault
> > > > > > for some bench cases. So I didn't try to add more code to fix this
> > > > introduced
> > > > > > problem. It may be feasible to do this, but would be a little
complex
> > > > > logically
> > > > > > to handle these introduced problem in SkGpuDevice, so I add the code
> > into
> > > > > > SkCanvas::drawPath. It is simple, and easy to understand.
> > > > > 
> > > > > Hi, bsalomon, robertphillips and reed, any suggestions about this
patch?
> > > > 
> > > > Hi, We're a little hesitant about having one virtual at the SkCanvas
level
> > > call
> > > > another.
> > > > 
> > > > What we could do is make drawPath() be non-virtual and add a protected
> > virtual
> > > > function called onDrawPath that SkCanvas subclasses would override.
> Roughly
> > > > speaking:
> > > > 
> > > > public:
> > > > void drawPath(path, paint) {
> > > >   if (path->isRect()) { this->drawRect() } else { this->onDrawPath(path,
> > > paint);
> > > > }
> > > > }
> > > > 
> > > > protected:
> > > > virtual void onDrawPath(path, paint) {
> > > >  // does the stuff drawPath() does today
> > > > }
> > > 
> > > hi, any update about this patch?
> > 
> > For this to work we have to find a way to avoid having on SkCanvas virtual
> call
> > another. I suggested one possibility, but there may be others.
> 
> Yeah, I got it. I revised the code to make sure it will call the non-virtual
> version(base class) by this->SkCanvas::drawXxxx, just like drawRRect and
> clipRRect did in SkCanvas.

I don't think this is correct. If a class overrode drawRect this may not behave
as expected.

I think we want to move more to a model where the public methods are thin
non-virtual shims that call proctected virtuals, perhaps with some
reinterpretation (e.g. converting rect drawPaths to drawRects, etc).

[yunchao, 2013-09-18 09:33:57]

On 2013/09/11 13:58:50, bsalomon wrote:
> On 2013/09/11 05:55:45, yunchao wrote:
> > On 2013/09/10 14:39:06, bsalomon wrote:
> > > On 2013/09/10 09:17:19, yunchao wrote:
> > > > On 2013/09/05 13:25:52, bsalomon wrote:
> > > > > On 2013/09/02 08:51:41, yunchao wrote:
> > > > > > On 2013/08/29 13:11:53, yunchao wrote:
> > > > > > > On 2013/08/29 12:50:20, bsalomon wrote:
> > > > > > > > This makes sense for the GPU device. Adding Mike to make a call
> > about
> > > > > > whether
> > > > > > > we
> > > > > > > > want to vector out at the canvas level or push this decision
down
> to
> > > > > > > > SkGpuDevice. I don't see any reason other devices types would
get
> > > slower
> > > > > by
> > > > > > > > drawing rect-paths as rects, though.
> > > > > > > 
> > > > > > > Yes, and I have tried to add this code into SkGpuDevice::drawPath
at
> > > > first.
> > > > > > > However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath in
> some
> > > > > > > cases(bevel-stroke rect and round-stroke rect those can't use
> > > > > > GrAARectRenderer),
> > > > > > > so if adding this code into SkGpuDevice(call drawRect in drawPath
in
> > > > > > > SkGpuDevice), there would be a loop. I simply add a flag into the
> > > > parameter
> > > > > > > lists in SkGpuDevice::drawPath, unfortunately, it would cause
> > > segmentation
> > > > > > fault
> > > > > > > for some bench cases. So I didn't try to add more code to fix this
> > > > > introduced
> > > > > > > problem. It may be feasible to do this, but would be a little
> complex
> > > > > > logically
> > > > > > > to handle these introduced problem in SkGpuDevice, so I add the
code
> > > into
> > > > > > > SkCanvas::drawPath. It is simple, and easy to understand.
> > > > > > 
> > > > > > Hi, bsalomon, robertphillips and reed, any suggestions about this
> patch?
> > > > > 
> > > > > Hi, We're a little hesitant about having one virtual at the SkCanvas
> level
> > > > call
> > > > > another.
> > > > > 
> > > > > What we could do is make drawPath() be non-virtual and add a protected
> > > virtual
> > > > > function called onDrawPath that SkCanvas subclasses would override.
> > Roughly
> > > > > speaking:
> > > > > 
> > > > > public:
> > > > > void drawPath(path, paint) {
> > > > >   if (path->isRect()) { this->drawRect() } else {
this->onDrawPath(path,
> > > > paint);
> > > > > }
> > > > > }
> > > > > 
> > > > > protected:
> > > > > virtual void onDrawPath(path, paint) {
> > > > >  // does the stuff drawPath() does today
> > > > > }
> > > > 
> > > > hi, any update about this patch?
> > > 
> > > For this to work we have to find a way to avoid having on SkCanvas virtual
> > call
> > > another. I suggested one possibility, but there may be others.
> > 
> > Yeah, I got it. I revised the code to make sure it will call the non-virtual
> > version(base class) by this->SkCanvas::drawXxxx, just like drawRRect and
> > clipRRect did in SkCanvas.
> 
> I don't think this is correct. If a class overrode drawRect this may not
behave
> as expected.
> 
> I think we want to move more to a model where the public methods are thin
> non-virtual shims that call proctected virtuals, perhaps with some
> reinterpretation (e.g. converting rect drawPaths to drawRects, etc).

Hi, Brian, Is this follow the model? Non-virtual public shim named drawPath call

virtual drawRect or drawNonRectPath. drawRect and drawNonRectPath will do the 
real drawing. 

Or I need to move drawNonRectPath to protected methods?

[bsalomon, 2013-09-23 17:55:04]

On 2013/09/18 09:33:57, yunchao wrote:
> On 2013/09/11 13:58:50, bsalomon wrote:
> > On 2013/09/11 05:55:45, yunchao wrote:
> > > On 2013/09/10 14:39:06, bsalomon wrote:
> > > > On 2013/09/10 09:17:19, yunchao wrote:
> > > > > On 2013/09/05 13:25:52, bsalomon wrote:
> > > > > > On 2013/09/02 08:51:41, yunchao wrote:
> > > > > > > On 2013/08/29 13:11:53, yunchao wrote:
> > > > > > > > On 2013/08/29 12:50:20, bsalomon wrote:
> > > > > > > > > This makes sense for the GPU device. Adding Mike to make a
call
> > > about
> > > > > > > whether
> > > > > > > > we
> > > > > > > > > want to vector out at the canvas level or push this decision
> down
> > to
> > > > > > > > > SkGpuDevice. I don't see any reason other devices types would
> get
> > > > slower
> > > > > > by
> > > > > > > > > drawing rect-paths as rects, though.
> > > > > > > > 
> > > > > > > > Yes, and I have tried to add this code into
SkGpuDevice::drawPath
> at
> > > > > first.
> > > > > > > > However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath
in
> > some
> > > > > > > > cases(bevel-stroke rect and round-stroke rect those can't use
> > > > > > > GrAARectRenderer),
> > > > > > > > so if adding this code into SkGpuDevice(call drawRect in
drawPath
> in
> > > > > > > > SkGpuDevice), there would be a loop. I simply add a flag into
the
> > > > > parameter
> > > > > > > > lists in SkGpuDevice::drawPath, unfortunately, it would cause
> > > > segmentation
> > > > > > > fault
> > > > > > > > for some bench cases. So I didn't try to add more code to fix
this
> > > > > > introduced
> > > > > > > > problem. It may be feasible to do this, but would be a little
> > complex
> > > > > > > logically
> > > > > > > > to handle these introduced problem in SkGpuDevice, so I add the
> code
> > > > into
> > > > > > > > SkCanvas::drawPath. It is simple, and easy to understand.
> > > > > > > 
> > > > > > > Hi, bsalomon, robertphillips and reed, any suggestions about this
> > patch?
> > > > > > 
> > > > > > Hi, We're a little hesitant about having one virtual at the SkCanvas
> > level
> > > > > call
> > > > > > another.
> > > > > > 
> > > > > > What we could do is make drawPath() be non-virtual and add a
protected
> > > > virtual
> > > > > > function called onDrawPath that SkCanvas subclasses would override.
> > > Roughly
> > > > > > speaking:
> > > > > > 
> > > > > > public:
> > > > > > void drawPath(path, paint) {
> > > > > >   if (path->isRect()) { this->drawRect() } else {
> this->onDrawPath(path,
> > > > > paint);
> > > > > > }
> > > > > > }
> > > > > > 
> > > > > > protected:
> > > > > > virtual void onDrawPath(path, paint) {
> > > > > >  // does the stuff drawPath() does today
> > > > > > }
> > > > > 
> > > > > hi, any update about this patch?
> > > > 
> > > > For this to work we have to find a way to avoid having on SkCanvas
virtual
> > > call
> > > > another. I suggested one possibility, but there may be others.
> > > 
> > > Yeah, I got it. I revised the code to make sure it will call the
non-virtual
> > > version(base class) by this->SkCanvas::drawXxxx, just like drawRRect and
> > > clipRRect did in SkCanvas.
> > 
> > I don't think this is correct. If a class overrode drawRect this may not
> behave
> > as expected.
> > 
> > I think we want to move more to a model where the public methods are thin
> > non-virtual shims that call proctected virtuals, perhaps with some
> > reinterpretation (e.g. converting rect drawPaths to drawRects, etc).
> 
> Hi, Brian, Is this follow the model? Non-virtual public shim named drawPath
call
> 
> virtual drawRect or drawNonRectPath. drawRect and drawNonRectPath will do the 
> real drawing. 
> 
> Or I need to move drawNonRectPath to protected methods?

I think what we want is this (in a *very* simplified form):

class SkCanvas {
public:
  void drawPath(path, ...) {
     SkRect rect;
     if (path.isRect(&rect) {
        this->onDrawRect(rect, ...);
     } else {
        this->onDrawPath(path, ...);
     }
  }
  void drawRect(rect, ...) {
     this->onDrawRect(rect, ...)
  }

protected:

  virtual void onDrawPath(path, ...) {
     device->drawPath(path, ...)
  }

  virtual void onDrawRect(rect, ...) {
     device->drawRect(rect, ...)
  }
};


This would mean modifying all subclasses of SkCanvas to override onDrawRect and
onDrawPath instead of drawRect and drawPath.

[yunchao, 2013-10-15 09:11:34]

On 2013/09/23 17:55:04, bsalomon wrote:
> On 2013/09/18 09:33:57, yunchao wrote:
> > On 2013/09/11 13:58:50, bsalomon wrote:
> > > On 2013/09/11 05:55:45, yunchao wrote:
> > > > On 2013/09/10 14:39:06, bsalomon wrote:
> > > > > On 2013/09/10 09:17:19, yunchao wrote:
> > > > > > On 2013/09/05 13:25:52, bsalomon wrote:
> > > > > > > On 2013/09/02 08:51:41, yunchao wrote:
> > > > > > > > On 2013/08/29 13:11:53, yunchao wrote:
> > > > > > > > > On 2013/08/29 12:50:20, bsalomon wrote:
> > > > > > > > > > This makes sense for the GPU device. Adding Mike to make a
> call
> > > > about
> > > > > > > > whether
> > > > > > > > > we
> > > > > > > > > > want to vector out at the canvas level or push this decision
> > down
> > > to
> > > > > > > > > > SkGpuDevice. I don't see any reason other devices types
would
> > get
> > > > > slower
> > > > > > > by
> > > > > > > > > > drawing rect-paths as rects, though.
> > > > > > > > > 
> > > > > > > > > Yes, and I have tried to add this code into
> SkGpuDevice::drawPath
> > at
> > > > > > first.
> > > > > > > > > However, SkGpuDevice::drawRect will call SkGpuDevice::drawPath
> in
> > > some
> > > > > > > > > cases(bevel-stroke rect and round-stroke rect those can't use
> > > > > > > > GrAARectRenderer),
> > > > > > > > > so if adding this code into SkGpuDevice(call drawRect in
> drawPath
> > in
> > > > > > > > > SkGpuDevice), there would be a loop. I simply add a flag into
> the
> > > > > > parameter
> > > > > > > > > lists in SkGpuDevice::drawPath, unfortunately, it would cause
> > > > > segmentation
> > > > > > > > fault
> > > > > > > > > for some bench cases. So I didn't try to add more code to fix
> this
> > > > > > > introduced
> > > > > > > > > problem. It may be feasible to do this, but would be a little
> > > complex
> > > > > > > > logically
> > > > > > > > > to handle these introduced problem in SkGpuDevice, so I add
the
> > code
> > > > > into
> > > > > > > > > SkCanvas::drawPath. It is simple, and easy to understand.
> > > > > > > > 
> > > > > > > > Hi, bsalomon, robertphillips and reed, any suggestions about
this
> > > patch?
> > > > > > > 
> > > > > > > Hi, We're a little hesitant about having one virtual at the
SkCanvas
> > > level
> > > > > > call
> > > > > > > another.
> > > > > > > 
> > > > > > > What we could do is make drawPath() be non-virtual and add a
> protected
> > > > > virtual
> > > > > > > function called onDrawPath that SkCanvas subclasses would
override.
> > > > Roughly
> > > > > > > speaking:
> > > > > > > 
> > > > > > > public:
> > > > > > > void drawPath(path, paint) {
> > > > > > >   if (path->isRect()) { this->drawRect() } else {
> > this->onDrawPath(path,
> > > > > > paint);
> > > > > > > }
> > > > > > > }
> > > > > > > 
> > > > > > > protected:
> > > > > > > virtual void onDrawPath(path, paint) {
> > > > > > >  // does the stuff drawPath() does today
> > > > > > > }
> > > > > > 
> > > > > > hi, any update about this patch?
> > > > > 
> > > > > For this to work we have to find a way to avoid having on SkCanvas
> virtual
> > > > call
> > > > > another. I suggested one possibility, but there may be others.
> > > > 
> > > > Yeah, I got it. I revised the code to make sure it will call the
> non-virtual
> > > > version(base class) by this->SkCanvas::drawXxxx, just like drawRRect and
> > > > clipRRect did in SkCanvas.
> > > 
> > > I don't think this is correct. If a class overrode drawRect this may not
> > behave
> > > as expected.
> > > 
> > > I think we want to move more to a model where the public methods are thin
> > > non-virtual shims that call proctected virtuals, perhaps with some
> > > reinterpretation (e.g. converting rect drawPaths to drawRects, etc).
> > 
> > Hi, Brian, Is this follow the model? Non-virtual public shim named drawPath
> call
> > 
> > virtual drawRect or drawNonRectPath. drawRect and drawNonRectPath will do
the 
> > real drawing. 
> > 
> > Or I need to move drawNonRectPath to protected methods?
> 
> I think what we want is this (in a *very* simplified form):
> 
> class SkCanvas {
> public:
>   void drawPath(path, ...) {
>      SkRect rect;
>      if (path.isRect(&rect) {
>         this->onDrawRect(rect, ...);
>      } else {
>         this->onDrawPath(path, ...);
>      }
>   }
>   void drawRect(rect, ...) {
>      this->onDrawRect(rect, ...)
>   }
> 
> protected:
> 
>   virtual void onDrawPath(path, ...) {
>      device->drawPath(path, ...)
>   }
> 
>   virtual void onDrawRect(rect, ...) {
>      device->drawRect(rect, ...)
>   }
> };
> 
> 
> This would mean modifying all subclasses of SkCanvas to override onDrawRect
and
> onDrawPath instead of drawRect and drawPath.

Hi, Brian, I updated this CL to follow your suggestion: Public APIs(drawRect and
drawPath here) are non-virtual, they are simple wrappers, who call protected
virtual API onDrawXXXX. Accordingly, all subclasses of SkCanvas have been
revised to define/impl onDrawRect and onDrawPath instead of drawRect and
drawPath respectively. please help to review it.

[bsalomon, 2013-10-15 13:25:51]

Hi Yunchao, I noted two small issues inline below. I'll check if Chromium has
its own subclasses of SkCanvas. If it does we will have to stage this change to
not get build errors in Chromium.

https://codereview.chromium.org/23484007/diff/35001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/23484007/diff/35001/include/core/SkCanvas.h#n...
include/core/SkCanvas.h:580: {
style nit: brace on prev line.

https://codereview.chromium.org/23484007/diff/35001/include/core/SkCanvas.h#n...
include/core/SkCanvas.h:1047: virtual void onDrawRect(const SkRect& rect, const
SkPaint& paint);
Since these are protected the subclasses should not make them public, they
should be in protected sections of subclass definitions.

[yunchao, 2013-10-16 07:22:51]

On 2013/10/15 13:25:51, bsalomon wrote:
> Hi Yunchao, I noted two small issues inline below. I'll check if Chromium has
> its own subclasses of SkCanvas. If it does we will have to stage this change
to
> not get build errors in Chromium.
> 
> https://codereview.chromium.org/23484007/diff/35001/include/core/SkCanvas.h
> File include/core/SkCanvas.h (right):
> 
>
https://codereview.chromium.org/23484007/diff/35001/include/core/SkCanvas.h#n...
> include/core/SkCanvas.h:580: {
> style nit: brace on prev line.
> 
>
https://codereview.chromium.org/23484007/diff/35001/include/core/SkCanvas.h#n...
> include/core/SkCanvas.h:1047: virtual void onDrawRect(const SkRect& rect,
const
> SkPaint& paint);
> Since these are protected the subclasses should not make them public, they
> should be in protected sections of subclass definitions.

Hi, Brian, updated this CL accordingly, please help to review it.

[bsalomon, 2013-10-16 19:03:14]

Yunchao, It basically looks good to me. I went ahead and made drawRect and
drawPath virtual again. Unfortunately, Chromium subclasses SkCanvas. We will
have to roll the change as it is into Chromium. Then make Chromium override the
new on* functions. Then remove virtual from the public functions on the Skia
side.

Mike, want to take a look? I don't think this needs to wait on the rect/close
changes. I verified that all subclasses within our code base are updated.

[yunchao, 2013-10-17 09:15:19]

On 2013/10/16 19:03:14, bsalomon wrote:
> Yunchao, It basically looks good to me. I went ahead and made drawRect and
> drawPath virtual again. Unfortunately, Chromium subclasses SkCanvas. We will
> have to roll the change as it is into Chromium. Then make Chromium override
the
> new on* functions. Then remove virtual from the public functions on the Skia
> side.
> 
> Mike, want to take a look? I don't think this needs to wait on the rect/close
> changes. I verified that all subclasses within our code base are updated.

Yeah, I also found subclass of SkCanvas with drawRect interface in
chromium(MockCanvas in src/cc/ for unittest), and hesitated to make drawRect
virtual or not temporarily in this CL. Thanks!

[bsalomon, 2013-10-17 14:48:52]

lgtm, and Mike gave a verbal lgtm.

[commit-bot: I haz the power, 2013-10-17 14:49:08]

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

[commit-bot: I haz the power, 2013-10-17 16:29:29]

Change committed as 11842

[robertphillips, 2013-10-17 17:35:50]

This was reverted in 11845 due to image changes to the following GMs:

inverse_paths
pathopsinverse

It appears that the new patch doesn't correctly support inverse ops?

[bsalomon, 2013-10-18 22:10:46]

On 2013/10/17 17:35:50, robertphillips wrote:
> This was reverted in 11845 due to image changes to the following GMs:
> 
> inverse_paths
> pathopsinverse
> 
> It appears that the new patch doesn't correctly support inverse ops?

The new version fixes the issue. I'll land it when I'll be around to handle
resbaselining the subtle gm changes.

[yunchao, 2013-10-21 05:51:26]

https://codereview.chromium.org/23484007/diff/79001/src/utils/debugger/SkDebu...
File src/utils/debugger/SkDebugCanvas.cpp (right):

https://codereview.chromium.org/23484007/diff/79001/src/utils/debugger/SkDebu...
src/utils/debugger/SkDebugCanvas.cpp:112: // The OverdrawFilter modifies every
paint to use an SkProcXfermode which
need to change to SkOverdrawFilter.

[yunchao, 2013-10-21 06:33:21]

On 2013/10/18 22:10:46, bsalomon wrote:
> On 2013/10/17 17:35:50, robertphillips wrote:
> > This was reverted in 11845 due to image changes to the following GMs:
> > 
> > inverse_paths
> > pathopsinverse
> > 
> > It appears that the new patch doesn't correctly support inverse ops?
> 
> The new version fixes the issue. I'll land it when I'll be around to handle
> resbaselining the subtle gm changes.

Yeah, drawRect and drawPath for a rect is a little different, we need to
rebaseline the gm output.

[bsalomon, 2013-10-21 13:14:43]

https://codereview.chromium.org/23484007/diff/79001/src/utils/debugger/SkDebu...
File src/utils/debugger/SkDebugCanvas.cpp (right):

https://codereview.chromium.org/23484007/diff/79001/src/utils/debugger/SkDebu...
src/utils/debugger/SkDebugCanvas.cpp:112: // The OverdrawFilter modifies every
paint to use an SkProcXfermode which
On 2013/10/21 05:51:26, yunchao wrote:
> need to change to SkOverdrawFilter.

Why would it work differently with drawRect vs drawPath()?

[yunchao, 2013-10-21 15:13:42]

On 2013/10/21 13:14:43, bsalomon wrote:

Hi, Brian, I just tried the gm case of rects as following:
run this gm case and write it into a file, then changed drawRect(rect, paint)
to: 
SkPath path;
path.addRect(rect);
canvas->drawPath(rect, paint). 
Run rects again and read from that file, found that they are slightly different.


AFAIK, drawPath and drawRect may be different for these cases in GPU path: 
1) axis-aligned SkIRect for AA:
drawRect: directly draw 2 triangle, don't need AA. 
drawPath: draw the edge one by one, and still draw AA?

2) different AA for the edge of a rect: 
drawRect: innerRect is 0.5 pixel smaller than the real rect with coverage=1,
outerRect is 0.5 pixel bigger than the rect with coverage=0
drawPath: inneredge is the real edge with coverage=1, outer edge is 1 pixel
bigger with coverage=0. 

3) different corner.
drawRect: draw a small rect for the corner for AA, the diagonal length is
sqrt(2). 
drawPath： draw 2 triangle for the corner for AA, the diagonal length is 1(see
the FIXME in src/gpu/GrAAConvexPathRenderer.cpp)

Well, I am not very sure about 1) ~ 3) I listed above. I will check it carefully
tomorrow. If drawRect vs drawPath is different, I will file a bug and fix it.

[bsalomon, 2013-10-21 16:33:50]

On 2013/10/21 15:13:42, yunchao wrote:
> On 2013/10/21 13:14:43, bsalomon wrote:
> 
> Hi, Brian, I just tried the gm case of rects as following:
> run this gm case and write it into a file, then changed drawRect(rect, paint)
> to: 
> SkPath path;
> path.addRect(rect);
> canvas->drawPath(rect, paint). 
> Run rects again and read from that file, found that they are slightly
different.
> 
> 
> AFAIK, drawPath and drawRect may be different for these cases in GPU path: 
> 1) axis-aligned SkIRect for AA:
> drawRect: directly draw 2 triangle, don't need AA. 
> drawPath: draw the edge one by one, and still draw AA?
> 
> 2) different AA for the edge of a rect: 
> drawRect: innerRect is 0.5 pixel smaller than the real rect with coverage=1,
> outerRect is 0.5 pixel bigger than the rect with coverage=0
> drawPath: inneredge is the real edge with coverage=1, outer edge is 1 pixel
> bigger with coverage=0. 
> 
> 3) different corner.
> drawRect: draw a small rect for the corner for AA, the diagonal length is
> sqrt(2). 
> drawPath： draw 2 triangle for the corner for AA, the diagonal length is 1(see
> the FIXME in src/gpu/GrAAConvexPathRenderer.cpp)
> 
> Well, I am not very sure about 1) ~ 3) I listed above. I will check it
carefully
> tomorrow. If drawRect vs drawPath is different, I will file a bug and fix it.

Those all should be ignorable. I diff'ed all the GMs and marked the ones that
subtly changed as ignored pending rebaselining.

[commit-bot: I haz the power, 2013-10-21 16:34:12]

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

[commit-bot: I haz the power, 2013-10-21 16:34:19]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file expectations/gm/ignored-tests.txt
  Hunk #1 FAILED at 34.
  1 out of 1 hunk FAILED -- saving rejects to file
expectations/gm/ignored-tests.txt.rej

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
5a5b4ec5e2d4ddc366024670bf3f68f87c847668..ab418fc1c69911d68032934546ad5eafddabc4cf
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -34,3 +34,11 @@ pdf-poppler
 # Added by senorblanco as part of https://codereview.chromium.org/27471002/.
 # New test cases added; will need rebaseline.
 matrixconvolution
+
+# Added by bsalomon as part of https://codereview.chromium.org/23484007/
+# to temporarily ignore slight changes between the way rectangle paths
+# and rectangles render.
+pathopsinverse
+convexpaths
+path-reverse
+inverse_paths
\ No newline at end of file

[commit-bot: I haz the power, 2013-10-21 17:36:35]

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

[commit-bot: I haz the power, 2013-10-21 17:36:41]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file expectations/gm/ignored-tests.txt
  Hunk #1 FAILED at 34.
  1 out of 1 hunk FAILED -- saving rejects to file
expectations/gm/ignored-tests.txt.rej

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
5a5b4ec5e2d4ddc366024670bf3f68f87c847668..dcc31aeb35c110e79d83b69f2c0b98a307db057b
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -34,3 +34,11 @@ pdf-poppler
 # Added by senorblanco as part of https://codereview.chromium.org/27471002/.
 # New test cases added; will need rebaseline.
 matrixconvolution
+
+# Added by bsalomon as part of https://codereview.chromium.org/23484007/
+# to temporarily ignore slight changes between the way rectangle paths
+# and rectangles render.
+pathopsinverse
+convexpaths
+path-reverse
+inverse_paths

[commit-bot: I haz the power, 2013-10-22 01:09:01]

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

[commit-bot: I haz the power, 2013-10-22 01:09:08]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file expectations/gm/ignored-tests.txt
  Hunk #1 FAILED at 34.
  1 out of 1 hunk FAILED -- saving rejects to file
expectations/gm/ignored-tests.txt.rej

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
5a5b4ec5e2d4ddc366024670bf3f68f87c847668..dcc31aeb35c110e79d83b69f2c0b98a307db057b
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -34,3 +34,11 @@ pdf-poppler
 # Added by senorblanco as part of https://codereview.chromium.org/27471002/.
 # New test cases added; will need rebaseline.
 matrixconvolution
+
+# Added by bsalomon as part of https://codereview.chromium.org/23484007/
+# to temporarily ignore slight changes between the way rectangle paths
+# and rectangles render.
+pathopsinverse
+convexpaths
+path-reverse
+inverse_paths

[bsalomon, 2013-10-22 13:13:47]

On 2013/10/22 01:09:08, I haz the power (commit-bot) wrote:
> Failed to apply patch for expectations/gm/ignored-tests.txt:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file expectations/gm/ignored-tests.txt
>   Hunk #1 FAILED at 34.
>   1 out of 1 hunk FAILED -- saving rejects to file
> expectations/gm/ignored-tests.txt.rej
> 
> Patch:       expectations/gm/ignored-tests.txt
> Index: expectations/gm/ignored-tests.txt
> diff --git a/expectations/gm/ignored-tests.txt
> b/expectations/gm/ignored-tests.txt
> index
>
5a5b4ec5e2d4ddc366024670bf3f68f87c847668..dcc31aeb35c110e79d83b69f2c0b98a307db057b
> 100644
> --- a/expectations/gm/ignored-tests.txt
> +++ b/expectations/gm/ignored-tests.txt
> @@ -34,3 +34,11 @@ pdf-poppler
>  # Added by senorblanco as part of https://codereview.chromium.org/27471002/.
>  # New test cases added; will need rebaseline.
>  matrixconvolution
> +
> +# Added by bsalomon as part of https://codereview.chromium.org/23484007/
> +# to temporarily ignore slight changes between the way rectangle paths
> +# and rectangles render.
> +pathopsinverse
> +convexpaths
> +path-reverse
> +inverse_paths

I'm going to land this manually.

[bsalomon, 2013-10-22 13:19:42]

Committed patchset #19 manually as r11904 (presubmit successful).

[yunchao, 2013-10-23 06:57:05]

On 2013/10/22 13:19:42, bsalomon wrote:
> Committed patchset #19 manually as r11904 (presubmit successful).

Hi, Brian, just wonder why revert r11904? 
Besides, do we need to fix the small difference between drawRect and drawPath?

[bsalomon, 2013-10-23 13:22:52]

On 2013/10/23 06:57:05, yunchao wrote:
> On 2013/10/22 13:19:42, bsalomon wrote:
> > Committed patchset #19 manually as r11904 (presubmit successful).
> 
> Hi, Brian, just wonder why revert r11904? 
> Besides, do we need to fix the small difference between drawRect and drawPath?

Hey Yunchao, when I tested blink layout tests there were a bunch of image
differences. Most were minor difference that would be OK to rebaseline but a
couple canvas2d tests failed. I'll investigate this. It's possible the tests or
baseline images are just wrong.

[yunchao, 2013-10-30 07:24:52]

On 2013/10/23 13:22:52, bsalomon wrote:
> On 2013/10/23 06:57:05, yunchao wrote:
> > On 2013/10/22 13:19:42, bsalomon wrote:
> > > Committed patchset #19 manually as r11904 (presubmit successful).
> > 
> > Hi, Brian, just wonder why revert r11904? 
> > Besides, do we need to fix the small difference between drawRect and
drawPath?
> 
> Hey Yunchao, when I tested blink layout tests there were a bunch of image
> differences. Most were minor difference that would be OK to rebaseline but a
> couple canvas2d tests failed. I'll investigate this. It's possible the tests
or
> baseline images are just wrong.

Hi, Brian, do you have some update about the layout tests differences? seems
that this CL has not landed yet. Maybe I can look into the blink layout test
failure.

[bsalomon, 2013-10-30 13:46:28]

On 2013/10/30 07:24:52, yunchao wrote:
> On 2013/10/23 13:22:52, bsalomon wrote:
> > On 2013/10/23 06:57:05, yunchao wrote:
> > > On 2013/10/22 13:19:42, bsalomon wrote:
> > > > Committed patchset #19 manually as r11904 (presubmit successful).
> > > 
> > > Hi, Brian, just wonder why revert r11904? 
> > > Besides, do we need to fix the small difference between drawRect and
> drawPath?
> > 
> > Hey Yunchao, when I tested blink layout tests there were a bunch of image
> > differences. Most were minor difference that would be OK to rebaseline but a
> > couple canvas2d tests failed. I'll investigate this. It's possible the tests
> or
> > baseline images are just wrong.
> 
> Hi, Brian, do you have some update about the layout tests differences? seems
> that this CL has not landed yet. Maybe I can look into the blink layout test
> failure.

I looked at this yesterday. The problem is that SkPath::isRect returns true for
a path like this:

moveTo(0,0)
lineTo(100,0)
lineTo(100,100)
lineTo(0, 100)
lineTo(0,0)

When that path is stroked there should be caps on the line from 0,0 to 100,0 and
from 0,100 to 0,0 but instead it gets a join.

We should create a GM in Skia that reproduces this and then fix the rect test.

[yunchao, 2013-10-31 08:45:36]

On 2013/10/30 13:46:28, bsalomon wrote:
> On 2013/10/30 07:24:52, yunchao wrote:
> > On 2013/10/23 13:22:52, bsalomon wrote:
> > > On 2013/10/23 06:57:05, yunchao wrote:
> > > > On 2013/10/22 13:19:42, bsalomon wrote:
> > > > > Committed patchset #19 manually as r11904 (presubmit successful).
> > > > 
> > > > Hi, Brian, just wonder why revert r11904? 
> > > > Besides, do we need to fix the small difference between drawRect and
> > drawPath?
> > > 
> > > Hey Yunchao, when I tested blink layout tests there were a bunch of image
> > > differences. Most were minor difference that would be OK to rebaseline but
a
> > > couple canvas2d tests failed. I'll investigate this. It's possible the
tests
> > or
> > > baseline images are just wrong.
> > 
> > Hi, Brian, do you have some update about the layout tests differences? seems
> > that this CL has not landed yet. Maybe I can look into the blink layout test
> > failure.
> 
> I looked at this yesterday. The problem is that SkPath::isRect returns true
for
> a path like this:
> 
> moveTo(0,0)
> lineTo(100,0)
> lineTo(100,100)
> lineTo(0, 100)
> lineTo(0,0)
> 
> When that path is stroked there should be caps on the line from 0,0 to 100,0
and
> from 0,100 to 0,0 but instead it gets a join.
> 
> We should create a GM in Skia that reproduces this and then fix the rect test.

I got the reason now, if it is not closed, and the path is not filled, then the
path will not recognized as a rect, it is still a non-closed lines with ends,
although the last point in the path coincide with the first point, so the path
has caps at each end. If it is recognized as a rect, then it will have joins,
instead of caps. Other browser also obey this rule. I will submit a new patch to
fix this small bug in SkPath::isRectContour.

[yunchao, 2014-03-19 06:54:56]

Hi Brian, I will rebase this CL to latest Skia code. I think the performance
improvement still exists, and the bug in LayoutTest has been fixed. What's your
opinion?