Stopped skipping tests in dm of SkPatch

Stopped skipping tests in dm of SkPatch by implementing the
corresponding drawPath calls on classes that derive from SkCanvas.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/963137b75c0a1fe91f35e9826742f36309f5e65d

[dandov, 2014-08-04 18:06:26]

This the fix for the gm to draw with all the flags except for kSkipTiled_Flag.
For the pipe, picture and bounding box canvas I don't know who to add to review
them, maybe Rob?

[bsalomon, 2014-08-04 18:11:01]

Adding Mike' and Rob for skp stuff. Do we normally bump the skp version when
strictly adding new features? Seems like we prolly should.

[mtklein, 2014-08-04 18:23:03]

https://codereview.chromium.org/429343004/diff/1/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/429343004/diff/1/gm/patch.cpp#newcode97
gm/patch.cpp:97: return kSkipTiled_Flag;
Just checking, does this not draw correctly when tiled?  Can we remove this flag
too?

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkPatch.cpp#newcode267
src/core/SkPatch.cpp:267: fCtrlPoints[i] = pts[i];
Is this redundant with this->reset(pts, colors) below?

https://codereview.chromium.org/429343004/diff/1/src/core/SkPatch.cpp#newcode276
src/core/SkPatch.cpp:276: fCtrlPoints[i] = pts[i];
Did you mean to copy the colors here?

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:311: const SkPoint* pts = (const
SkPoint*)reader->skip(sizeof(SkPoint)* 12);
Not keen on expressing the format twice.  Can't we just use reader.readPatch()
here?

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1485: //TODO: drawPatch
?

https://codereview.chromium.org/429343004/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1491: // op + paint index + path index
please update comment

https://codereview.chromium.org/429343004/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1633: void SkPictureRecord::addPatch(const SkPatch&
patch) {
Isn't this just fWriter.writePatch(patch); ?

[mtklein, 2014-08-04 18:24:35]

On 2014/08/04 18:11:01, bsalomon wrote:
> Adding Mike' and Rob for skp stuff. Do we normally bump the skp version when
> strictly adding new features? Seems like we prolly should.

Don't think so.  The only time we really need to do that is when we change how
something existing is encoded.  New data is a perfect time to continue to append
changes to the existing picture version.

[robertphillips, 2014-08-04 18:44:19]

https://codereview.chromium.org/429343004/diff/1/include/core/SkPatch.h
File include/core/SkPatch.h (right):

https://codereview.chromium.org/429343004/diff/1/include/core/SkPatch.h#newco...
include/core/SkPatch.h:142: void setPoints(const SkPoint points[12]) {
Just memcpy these ?

https://codereview.chromium.org/429343004/diff/1/include/core/SkPatch.h#newco...
include/core/SkPatch.h:148: void setColors(const SkColor colors[4]) {
These too ?

https://codereview.chromium.org/429343004/diff/1/include/core/SkReader32.h
File include/core/SkReader32.h (right):

https://codereview.chromium.org/429343004/diff/1/include/core/SkReader32.h#ne...
include/core/SkReader32.h:110: bool readPatch(SkPatch* patch) {
this-> ?

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkBBoxRecord.cpp#ne...
src/core/SkBBoxRecord.cpp:289: SkRect bbox;
12 -> SkPatch::kNumControlPts ?

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkPatch.cpp#newcode241
src/core/SkPatch.cpp:241: size_t SkPatch::writeToMemory(void* storage) const {
12 -> kNumControlPts ?
4 -> kNumCornerColors ?

https://codereview.chromium.org/429343004/diff/1/src/core/SkPatch.cpp#newcode261
src/core/SkPatch.cpp:261: 
Can we not just read into fCtrlPoints & fCornerColors ?

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:310: const SkPaint& paint =
*fPictureData->getPaint(reader);
12 -> kNum... etc.

https://codereview.chromium.org/429343004/diff/1/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/429343004/diff/1/src/utils/SkDumpCanvas.cpp#n...
src/utils/SkDumpCanvas.cpp:446: const SkPoint* points =
patch.getControlPoints();
dump corner colors too?

[dandov, 2014-08-04 19:59:27]

Fixed issues from comments. Added kNumCtrlPts and kNumColors to avoid having
magic numbers, use memcpy instead for loops and modified some functions to avoid
repeated code. 

For the flag kSkipTiled this gm draws slightly different when it is tiled so I
need to keep it.

https://codereview.chromium.org/429343004/diff/1/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/429343004/diff/1/gm/patch.cpp#newcode97
gm/patch.cpp:97: return kSkipTiled_Flag;
On 2014/08/04 18:23:03, mtklein wrote:
> Just checking, does this not draw correctly when tiled?  Can we remove this
flag
> too?

Yes this doesn't draw correctly with tiles. I tried looking at the images it
generated and the references and they look pretty much the same but BitmapsEqual
says they aren't. 

After that I ran skdiff with the reference image and the generated one and they
differ in less than 0.02% so I think I will leave this flag.

https://codereview.chromium.org/429343004/diff/1/include/core/SkPatch.h
File include/core/SkPatch.h (right):

https://codereview.chromium.org/429343004/diff/1/include/core/SkPatch.h#newco...
include/core/SkPatch.h:142: void setPoints(const SkPoint points[12]) {
On 2014/08/04 18:44:18, robertphillips wrote:
> Just memcpy these ?

Done.

https://codereview.chromium.org/429343004/diff/1/include/core/SkPatch.h#newco...
include/core/SkPatch.h:148: void setColors(const SkColor colors[4]) {
On 2014/08/04 18:44:18, robertphillips wrote:
> These too ?

Done.

https://codereview.chromium.org/429343004/diff/1/include/core/SkReader32.h
File include/core/SkReader32.h (right):

https://codereview.chromium.org/429343004/diff/1/include/core/SkReader32.h#ne...
include/core/SkReader32.h:110: bool readPatch(SkPatch* patch) {
On 2014/08/04 18:44:18, robertphillips wrote:
> this-> ?

Done and also added (this->) to similar calls.

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkBBoxRecord.cpp#ne...
src/core/SkBBoxRecord.cpp:289: SkRect bbox;
On 2014/08/04 18:44:18, robertphillips wrote:
> 12 -> SkPatch::kNumControlPts ?

Done. Use constant SkPatch::kNumCtrlPts.

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkPatch.cpp#newcode241
src/core/SkPatch.cpp:241: size_t SkPatch::writeToMemory(void* storage) const {
On 2014/08/04 18:44:18, robertphillips wrote:
> 12 -> kNumControlPts ?
> 4 -> kNumCornerColors ?

Added constants and use them where they are needed.

https://codereview.chromium.org/429343004/diff/1/src/core/SkPatch.cpp#newcode261
src/core/SkPatch.cpp:261: 
On 2014/08/04 18:44:18, robertphillips wrote:
> Can we not just read into fCtrlPoints & fCornerColors ?

Done.

https://codereview.chromium.org/429343004/diff/1/src/core/SkPatch.cpp#newcode267
src/core/SkPatch.cpp:267: fCtrlPoints[i] = pts[i];
On 2014/08/04 18:23:03, mtklein wrote:
> Is this redundant with this->reset(pts, colors) below?

Done. Changed it to read directly into fCtrlPoints and fCornerColors.

https://codereview.chromium.org/429343004/diff/1/src/core/SkPatch.cpp#newcode276
src/core/SkPatch.cpp:276: fCtrlPoints[i] = pts[i];
On 2014/08/04 18:23:03, mtklein wrote:
> Did you mean to copy the colors here?

Also redundant because of this->reset, removed it and adjusted byteCount +=
4*sizeof(SkColor)

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:310: const SkPaint& paint =
*fPictureData->getPaint(reader);
On 2014/08/04 18:44:18, robertphillips wrote:
> 12 -> kNum... etc.

Done.

https://codereview.chromium.org/429343004/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:311: const SkPoint* pts = (const
SkPoint*)reader->skip(sizeof(SkPoint)* 12);
On 2014/08/04 18:23:03, mtklein wrote:
> Not keen on expressing the format twice.  Can't we just use reader.readPatch()
> here?

Done. Used reader.readPatch().

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

https://codereview.chromium.org/429343004/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1485: //TODO: drawPatch
On 2014/08/04 18:23:03, mtklein wrote:
> ?

Done.

https://codereview.chromium.org/429343004/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1491: // op + paint index + path index
On 2014/08/04 18:23:03, mtklein wrote:
> please update comment

Done.

https://codereview.chromium.org/429343004/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1633: void SkPictureRecord::addPatch(const SkPatch&
patch) {
On 2014/08/04 18:23:03, mtklein wrote:
> Isn't this just fWriter.writePatch(patch); ?

Done.

https://codereview.chromium.org/429343004/diff/1/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/429343004/diff/1/src/utils/SkDumpCanvas.cpp#n...
src/utils/SkDumpCanvas.cpp:446: const SkPoint* points =
patch.getControlPoints();
On 2014/08/04 18:44:19, robertphillips wrote:
> dump corner colors too?

Done. 
Is 4 points out of the 12 enough? or should it dump the 12 points. 
Also, is there an specific format I should follow?

[mtklein, 2014-08-04 20:40:47]

https://codereview.chromium.org/429343004/diff/1/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/429343004/diff/1/gm/patch.cpp#newcode97
gm/patch.cpp:97: return kSkipTiled_Flag;
On 2014/08/04 19:59:27, dandov wrote:
> On 2014/08/04 18:23:03, mtklein wrote:
> > Just checking, does this not draw correctly when tiled?  Can we remove this
> flag
> > too?
> 
> Yes this doesn't draw correctly with tiles. I tried looking at the images it
> generated and the references and they look pretty much the same but
BitmapsEqual
> says they aren't. 
> 
> After that I ran skdiff with the reference image and the generated one and
they
> differ in less than 0.02% so I think I will leave this flag. 

SGTM.  Thanks for checking.

https://codereview.chromium.org/429343004/diff/20001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/429343004/diff/20001/gm/patch.cpp#newcode64
gm/patch.cpp:64: SkPoint ptsCpy[12];
Seems like we still have many 12's and 4's.  It's pretty hard to understand code
that says kNumCtrlPts some places and 12 others.

Personally I don't mind which way it ends up --- all 12's or all kNumCtrlPts ---
but let's make it consistent everywhere in the codebase.

[egdaniel, 2014-08-04 20:45:38]

https://codereview.chromium.org/429343004/diff/20001/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/429343004/diff/20001/src/utils/SkDumpCanvas.c...
src/utils/SkDumpCanvas.cpp:448: this->dump(kDrawPatch_Verb, &paint,
"drawPatch(Vertices{[%f, %f], [%f, %f], [%f, %f], [%f, %f]}\
I would say you should be fine with just the corners and colors. Taking
drawVertices as a ref, we don't always print out every bit of information
possible.

[dandov, 2014-08-04 21:47:54]

https://codereview.chromium.org/429343004/diff/20001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/429343004/diff/20001/gm/patch.cpp#newcode64
gm/patch.cpp:64: SkPoint ptsCpy[12];
On 2014/08/04 20:40:47, mtklein wrote:
> Seems like we still have many 12's and 4's.  It's pretty hard to understand
code
> that says kNumCtrlPts some places and 12 others.
> 
> Personally I don't mind which way it ends up --- all 12's or all kNumCtrlPts
---
> but let's make it consistent everywhere in the codebase.

I think I fixed all of the 12's and 4's that are related with the number of
points or colors in a patch.

https://codereview.chromium.org/429343004/diff/20001/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/429343004/diff/20001/src/utils/SkDumpCanvas.c...
src/utils/SkDumpCanvas.cpp:448: this->dump(kDrawPatch_Verb, &paint,
"drawPatch(Vertices{[%f, %f], [%f, %f], [%f, %f], [%f, %f]}\
On 2014/08/04 20:45:38, egdaniel wrote:
> I would say you should be fine with just the corners and colors. Taking
> drawVertices as a ref, we don't always print out every bit of information
> possible.

Done.

[robertphillips, 2014-08-05 12:44:45]

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode18
gm/patch.cpp:18: 
const SkPatch& patch, const SkPaint& paint ?
If you want to modify a method parameter you need to pass it as a pointer (at
least in Skia's coding standard).

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode21
gm/patch.cpp:21: SkPaint copy(paint);
This is a bit odd. I would expect this to be kNumPtsCubic rather then
kNumColors.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode61
gm/patch.cpp:61: 
// Randomly alter 5 of the control points of the provided set and then draw the
result ?
Make points, colors and paint all const parameters ?

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode68
gm/patch.cpp:68: int index = rnd->nextRangeU(0, 11);
Move dy to its own line ?

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode106
gm/patch.cpp:106: };
// Order is clockwise starting at the UL corner

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode107
gm/patch.cpp:107: SkPoint points[SkPatch::kNumCtrlPts] = {
Label this with "// top" on RHS ?

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode109
gm/patch.cpp:109: {120,75},{180,125},{150,150},
Move the "{150,150}" down to the next line and label with "// bottom" ?

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode110
gm/patch.cpp:110: {125,120},{75,180},{50,150},
Label this with "// left" on RHS ?

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode114
gm/patch.cpp:114: SkRandom rnd;
static const SkScalar kScale = 0.5f; ?

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode124
gm/patch.cpp:124: canvas->translate(0, 270);
static const SkScalar kSkew = 0.1f; ?

https://codereview.chromium.org/429343004/diff/40001/include/core/SkPatch.h
File include/core/SkPatch.h (right):

https://codereview.chromium.org/429343004/diff/40001/include/core/SkPatch.h#n...
include/core/SkPatch.h:132: 
Not sure about this use of kNumPtsCubic. This could probably just be 4.

https://codereview.chromium.org/429343004/diff/40001/include/core/SkPatch.h#n...
include/core/SkPatch.h:172: * length Amount of memory available in the buffer
Add a "Returns" before "number" ?

https://codereview.chromium.org/429343004/diff/40001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/429343004/diff/40001/src/pipe/SkGPipeRead.cpp...
src/pipe/SkGPipeRead.cpp:407: static void drawPatch_rp(SkCanvas* canvas,
SkReader32* reader, uint32_t op32,
line this up with others ?

https://codereview.chromium.org/429343004/diff/40001/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/429343004/diff/40001/src/utils/SkDumpCanvas.c...
src/utils/SkDumpCanvas.cpp:452: points[SkPatch::kTopP3_CubicCtrlPts].fX,
points[SkPatch::kTopP3_CubicCtrlPts].fY,
Are these indices correct ?

[dandov, 2014-08-05 14:27:11]

Some small fixes and removed random parts of the gm.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode18
gm/patch.cpp:18: 
On 2014/08/05 12:44:45, robertphillips wrote:
> const SkPatch& patch, const SkPaint& paint ?
> If you want to modify a method parameter you need to pass it as a pointer (at
> least in Skia's coding standard).

Done.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode21
gm/patch.cpp:21: SkPaint copy(paint);
On 2014/08/05 12:44:45, robertphillips wrote:
> This is a bit odd. I would expect this to be kNumPtsCubic rather then
> kNumColors.

Done.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode61
gm/patch.cpp:61: 
On 2014/08/05 12:44:45, robertphillips wrote:
> // Randomly alter 5 of the control points of the provided set and then draw
the
> result ?
> Make points, colors and paint all const parameters ?

Made them all constant and removed function draw_random_patch.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode68
gm/patch.cpp:68: int index = rnd->nextRangeU(0, 11);
On 2014/08/05 12:44:44, robertphillips wrote:
> Move dy to its own line ?

Done.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode106
gm/patch.cpp:106: };
On 2014/08/05 12:44:45, robertphillips wrote:
> // Order is clockwise starting at the UL corner

Done.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode107
gm/patch.cpp:107: SkPoint points[SkPatch::kNumCtrlPts] = {
On 2014/08/05 12:44:45, robertphillips wrote:
> Label this with "// top" on RHS ?

Done.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode109
gm/patch.cpp:109: {120,75},{180,125},{150,150},
On 2014/08/05 12:44:45, robertphillips wrote:
> Move the "{150,150}" down to the next line and label with "// bottom" ?

Done.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode110
gm/patch.cpp:110: {125,120},{75,180},{50,150},
On 2014/08/05 12:44:45, robertphillips wrote:
> Label this with "// left" on RHS ?

Done.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode114
gm/patch.cpp:114: SkRandom rnd;
On 2014/08/05 12:44:45, robertphillips wrote:
> static const SkScalar kScale = 0.5f; ?

Done.

https://codereview.chromium.org/429343004/diff/40001/gm/patch.cpp#newcode124
gm/patch.cpp:124: canvas->translate(0, 270);
On 2014/08/05 12:44:45, robertphillips wrote:
> static const SkScalar kSkew = 0.1f; ?

Done.

https://codereview.chromium.org/429343004/diff/40001/include/core/SkPatch.h
File include/core/SkPatch.h (right):

https://codereview.chromium.org/429343004/diff/40001/include/core/SkPatch.h#n...
include/core/SkPatch.h:132: 
On 2014/08/05 12:44:45, robertphillips wrote:
> Not sure about this use of kNumPtsCubic. This could probably just be 4.

Done.

https://codereview.chromium.org/429343004/diff/40001/include/core/SkPatch.h#n...
include/core/SkPatch.h:172: * length Amount of memory available in the buffer
On 2014/08/05 12:44:45, robertphillips wrote:
> Add a "Returns" before "number" ?

Done.

https://codereview.chromium.org/429343004/diff/40001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/429343004/diff/40001/src/pipe/SkGPipeRead.cpp...
src/pipe/SkGPipeRead.cpp:407: static void drawPatch_rp(SkCanvas* canvas,
SkReader32* reader, uint32_t op32,
On 2014/08/05 12:44:45, robertphillips wrote:
> line this up with others ?

Done.

https://codereview.chromium.org/429343004/diff/40001/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/429343004/diff/40001/src/utils/SkDumpCanvas.c...
src/utils/SkDumpCanvas.cpp:452: points[SkPatch::kTopP3_CubicCtrlPts].fX,
points[SkPatch::kTopP3_CubicCtrlPts].fY,
On 2014/08/05 12:44:45, robertphillips wrote:
> Are these indices correct ?

On of them was not correct and switched them to keep the clockwise order.

[robertphillips, 2014-08-05 15:42:00]

https://codereview.chromium.org/429343004/diff/60001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/429343004/diff/60001/gm/patch.cpp#newcode18
gm/patch.cpp:18: 
I don't think we should pass in the paint. It doesn't look like we're actually
using any information from it from onDraw.

https://codereview.chromium.org/429343004/diff/60001/gm/patch.cpp#newcode105
gm/patch.cpp:105: SkPatch patch(points, colors);
kScale ?

https://codereview.chromium.org/429343004/diff/60001/gm/patch.cpp#newcode119
gm/patch.cpp:119: 
kSkew ?

https://codereview.chromium.org/429343004/diff/60001/include/core/SkPatch.h
File include/core/SkPatch.h (right):

https://codereview.chromium.org/429343004/diff/60001/include/core/SkPatch.h#n...
include/core/SkPatch.h:149: memcpy(fCtrlPoints, points, kNumCtrlPts *
sizeof(SkPoint));
extra line here ?

https://codereview.chromium.org/429343004/diff/60001/include/core/SkPatch.h#n...
include/core/SkPatch.h:172: * length Amount of memory available in the buffer
Should the rest of this comment not be:

* Returns the number of bytes read (must be a multiple of 4) or
*             0 if there was not enough memory available

?

https://codereview.chromium.org/429343004/diff/60001/src/core/SkPatch.cpp
File src/core/SkPatch.cpp (right):

https://codereview.chromium.org/429343004/diff/60001/src/core/SkPatch.cpp#new...
src/core/SkPatch.cpp:255: if (!buffer.read(fCtrlPoints, kNumCtrlPts *
sizeof(SkPoint))) {
By the header comment - aren't we supposed to return 0 here ?

https://codereview.chromium.org/429343004/diff/60001/src/core/SkPatch.cpp#new...
src/core/SkPatch.cpp:260: if (!buffer.read(fCornerColors, kNumColors *
sizeof(SkColor))) {
And here ?

[dandov, 2014-08-05 17:34:06]

https://codereview.chromium.org/429343004/diff/60001/gm/patch.cpp
File gm/patch.cpp (right):

https://codereview.chromium.org/429343004/diff/60001/gm/patch.cpp#newcode18
gm/patch.cpp:18: 
On 2014/08/05 15:42:00, robertphillips wrote:
> I don't think we should pass in the paint. It doesn't look like we're actually
> using any information from it from onDraw.

Done.

https://codereview.chromium.org/429343004/diff/60001/gm/patch.cpp#newcode105
gm/patch.cpp:105: SkPatch patch(points, colors);
On 2014/08/05 15:42:00, robertphillips wrote:
> kScale ?

Done.

https://codereview.chromium.org/429343004/diff/60001/gm/patch.cpp#newcode119
gm/patch.cpp:119: 
On 2014/08/05 15:42:00, robertphillips wrote:
> kSkew ?

Done.

https://codereview.chromium.org/429343004/diff/60001/include/core/SkPatch.h
File include/core/SkPatch.h (right):

https://codereview.chromium.org/429343004/diff/60001/include/core/SkPatch.h#n...
include/core/SkPatch.h:172: * length Amount of memory available in the buffer
On 2014/08/05 15:42:00, robertphillips wrote:
> Should the rest of this comment not be:
> 
> * Returns the number of bytes read (must be a multiple of 4) or
> *             0 if there was not enough memory available
> 
> ?

Done. Fixed function to follow the comment.

https://codereview.chromium.org/429343004/diff/60001/src/core/SkPatch.cpp
File src/core/SkPatch.cpp (right):

https://codereview.chromium.org/429343004/diff/60001/src/core/SkPatch.cpp#new...
src/core/SkPatch.cpp:255: if (!buffer.read(fCtrlPoints, kNumCtrlPts *
sizeof(SkPoint))) {
On 2014/08/05 15:42:00, robertphillips wrote:
> By the header comment - aren't we supposed to return 0 here ?

Done.

https://codereview.chromium.org/429343004/diff/60001/src/core/SkPatch.cpp#new...
src/core/SkPatch.cpp:260: if (!buffer.read(fCornerColors, kNumColors *
sizeof(SkColor))) {
On 2014/08/05 15:42:00, robertphillips wrote:
> And here ?

Done.

[robertphillips, 2014-08-05 20:10:23]

lgtm + some nits

https://codereview.chromium.org/429343004/diff/80001/src/core/SkPatch.cpp
File src/core/SkPatch.cpp (right):

https://codereview.chromium.org/429343004/diff/80001/src/core/SkPatch.cpp#new...
src/core/SkPatch.cpp:125: static uint8_t bilerp(SkScalar tx, SkScalar ty,
SkScalar c00, SkScalar c10, SkScalar c01,
line this guy up with the rest ?

https://codereview.chromium.org/429343004/diff/80001/src/core/SkPatch.cpp#new...
src/core/SkPatch.cpp:253: 
Might as well just compute byteCount at the end (right before returning it). I
don't think there is any reason to incrementally compute it anymore.

[dandov, 2014-08-06 18:45:44]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-08-06 18:45:59]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/429343004/100001

[commit-bot: I haz the power, 2014-08-06 18:46:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 18:46:37]

Presubmit check for 429343004-100001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')

[bsalomon, 2014-08-06 20:00:40]

lgtm

[dandov, 2014-08-06 20:01:21]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-08-06 20:01:40]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/429343004/100001

[commit-bot: I haz the power, 2014-08-06 20:08:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[commit-bot: I haz the power, 2014-08-06 20:11:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 20:11:44]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[dandov, 2014-08-07 14:13:46]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-08-07 14:14:22]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/429343004/140001

[commit-bot: I haz the power, 2014-08-07 14:22:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[commit-bot: I haz the power, 2014-08-07 14:25:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-07 14:25:44]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[dandov, 2014-08-07 14:49:29]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-08-07 14:49:34]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/429343004/140001

[commit-bot: I haz the power, 2014-08-07 14:49:59]

Change committed as 963137b75c0a1fe91f35e9826742f36309f5e65d