add SkCanvas::drawAtlas

add SkCanvas::drawAtlas


BUG=skia:

Committed: https://skia.googlesource.com/skia/+/71c3c760a83123ee0b3127b8c65c6394ce541c50

[reed1, 2015-06-15 15:22:53]

reed@google.com changed reviewers:
+ bsalomon@google.com, jvanverth@google.com, robertphillips@google.com,
scroggo@google.com

[reed1, 2015-06-15 15:22:54]

WIP -- mostly api functionality for now

[bsalomon, 2015-06-15 15:44:15]

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

https://codereview.chromium.org/1181913003/diff/1/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:1030: void drawAtlas(const SkImage* atlas, const
SkRSXform[], const SkRect tex[], const SkColor[],
What does SkRSXform apply to? A standard rectangle?

[jvanverth1, 2015-06-15 15:49:28]

jvanverth@google.com changed reviewers:
+ viktorl@google.com

[jvanverth1, 2015-06-15 15:51:19]

Adding viktorl@. (Viktor, this is a preliminary pass at a sprite interface in
Skia.)

[robertphillips, 2015-06-15 16:12:55]

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp
File samplecode/SampleAtlas.cpp (right):

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:18: static SkImage* make_atlas() {
defined kAtlasSize as 512 somewhere and pass it in ?

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:27: 
const int kCellSize = 32;
const SkScalar kHalf = kCellSize/2.0f;

?

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:73: fScale += fDScale;
SK_ScalarHalf ?

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:122: int i = 0;
512 -> kAtlasSize ?
32 -> kCellSize ?

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:124: for (int x = 0; x < 512; x += 32) {
SkASSERT(i < N); ?

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:138: }
Rm this debugf ?

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:157: SkRect onGetBounds() override {
Why not just fBounds ?

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:159: }
private:
    typedef  SkDrawable INHERITED;

?

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:187: 
Shouldn't we be using this ?

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:201: 
We don't have a macro for this ?

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

https://codereview.chromium.org/1181913003/diff/1/src/core/SkMatrix.cpp#newco...
src/core/SkMatrix.cpp:455: 
Seems like we could do better than this.

[reed1, 2015-06-15 18:52:33]

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

https://codereview.chromium.org/1181913003/diff/1/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:1030: void drawAtlas(const SkImage* atlas, const
SkRSXform[], const SkRect tex[], const SkColor[],
On 2015/06/15 15:44:15, bsalomon wrote:
> What does SkRSXform apply to? A standard rectangle?

added to dox:

xform maps [0, 0, tex.width, tex.height] -> quad

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp
File samplecode/SampleAtlas.cpp (right):

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:18: static SkImage* make_atlas() {
On 2015/06/15 16:12:55, robertphillips wrote:
> defined kAtlasSize as 512 somewhere and pass it in ?

Acknowledged.

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:27: 
On 2015/06/15 16:12:55, robertphillips wrote:
> const int kCellSize = 32;
> const SkScalar kHalf = kCellSize/2.0f;
> 
> ?

Acknowledged.

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:73: fScale += fDScale;
On 2015/06/15 16:12:54, robertphillips wrote:
> SK_ScalarHalf ?

Not sure. I was playing around with the 2 & 1/2

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:122: int i = 0;
On 2015/06/15 16:12:54, robertphillips wrote:
> 512 -> kAtlasSize ?
> 32 -> kCellSize ?

Acknowledged.

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:124: for (int x = 0; x < 512; x += 32) {
On 2015/06/15 16:12:55, robertphillips wrote:
> SkASSERT(i < N); ?

Acknowledged.

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:157: SkRect onGetBounds() override {
On 2015/06/15 16:12:54, robertphillips wrote:
> Why not just fBounds ?

bounds outset by max scale * max cell radius

https://codereview.chromium.org/1181913003/diff/1/samplecode/SampleAtlas.cpp#...
samplecode/SampleAtlas.cpp:159: }
On 2015/06/15 16:12:55, robertphillips wrote:
> private:
>     typedef  SkDrawable INHERITED;
> 
> ?

Done.

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

https://codereview.chromium.org/1181913003/diff/1/src/core/SkMatrix.cpp#newco...
src/core/SkMatrix.cpp:455: 
On 2015/06/15 16:12:55, robertphillips wrote:
> Seems like we could do better than this.

Acknowledged.

[reed1, 2015-06-15 19:56:04]

reed@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-06-15 20:45:40]

Picture stuff LG, just a couple small suggestions.

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

https://codereview.chromium.org/1181913003/diff/40001/include/core/SkRSXform....
include/core/SkRSXform.h:35: void toQuad(SkScalar width, SkScalar height,
SkPoint quad[4]) const;
// In SkMatrix.cpp ?

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkRecordDraw.c...
src/core/SkRecordDraw.cpp:467: return fCurrentClipBounds;
Add // We could brute force the bounds here.  ?

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkRecorder.cpp
File src/core/SkRecorder.cpp (right):

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkRecorder.cpp...
src/core/SkRecorder.cpp:309: colors ? this->copy(colors, count) : NULL,
This is fine, but copy does this check internally too.  Just this->copy(colors,
count) should be safe.

[mtklein, 2015-06-15 20:55:18]

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

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2485: void SkCanvas::onDrawAtlas(const SkImage* atlas,
const SkRSXform xform[], const SkRect tex[],
Am I crazy or is this never called?

[reed1, 2015-06-15 20:59:18]

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

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2485: void SkCanvas::onDrawAtlas(const SkImage* atlas,
const SkRSXform xform[], const SkRect tex[],
On 2015/06/15 20:55:18, mtklein wrote:
> Am I crazy or is this never called?

I'm crazy. Will fix.

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkRecordDraw.c...
src/core/SkRecordDraw.cpp:467: return fCurrentClipBounds;
On 2015/06/15 20:45:40, mtklein wrote:
> Add // We could brute force the bounds here.  ?

Agreed. Will write that. (not as cheap as just bounding some points, but
shouldn't be terrible). Perhaps I should do this when we record?

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkRecorder.cpp
File src/core/SkRecorder.cpp (right):

https://codereview.chromium.org/1181913003/diff/40001/src/core/SkRecorder.cpp...
src/core/SkRecorder.cpp:309: colors ? this->copy(colors, count) : NULL,
On 2015/06/15 20:45:40, mtklein wrote:
> This is fine, but copy does this check internally too.  Just
this->copy(colors,
> count) should be safe.

Ah, ok. I was trying to copy the pattern above on line 297.

[reed1, 2015-06-24 02:34:09]

ptal -- have done some optimization passes for gpu, but will need to land and
have the gpu-team take a 2nd pass.

[reed1, 2015-06-24 02:34:17]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 02:34:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181913003/140001

[reed1, 2015-06-24 02:34:31]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-06-24 02:50:12]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 02:50:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181913003/160001

[reed1, 2015-06-24 02:52:24]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-06-24 02:53:19]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-06-24 02:53:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181913003/160001

[commit-bot: I haz the power, 2015-06-24 02:53:35]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-06-24 08:53 UTC

[reed1, 2015-06-24 02:54:18]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-06-24 03:31:25]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 03:31:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181913003/200001

[reed1, 2015-06-24 03:34:28]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-06-24 03:39:05]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-06-24 03:39:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181913003/200001

[commit-bot: I haz the power, 2015-06-24 08:53:04]

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

[commit-bot: I haz the power, 2015-06-24 08:53:05]

No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer from
https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS

[robertphillips, 2015-06-24 15:11:02]

lgtm + nits & dox requests

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp
File gm/spritebitmap.cpp (right):

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp#ne...
gm/spritebitmap.cpp:101: 
should this go in its own file ?
// This GM ...

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp#ne...
gm/spritebitmap.cpp:102: class DrawAtlasGM : public skiagm::GM {
So we're not really using the entire atlas ?

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp#ne...
gm/spritebitmap.cpp:140: const SkRect target = { 50, 50, 80, 90 };
If we're going to use this pattern more widely (creating resources to match the
backend type) should we pass the canvas to onOnceBeforeDraw ?

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp#ne...
gm/spritebitmap.cpp:172: tex[i] = target;
Do we need to 565/8888 safety these colors ?

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

https://codereview.chromium.org/1181913003/diff/200001/include/core/SkCanvas....
include/core/SkCanvas.h:1034: *  The color array is optional. When specified,
each color modulates the pixels in its
kDst_Mode ?

https://codereview.chromium.org/1181913003/diff/200001/include/core/SkCanvas....
include/core/SkCanvas.h:1036: *  but with a non-opaque alpha will result in
fading the quad.
* If an explicit xfer mode is provided it ...

https://codereview.chromium.org/1181913003/diff/200001/include/core/SkRSXform.h
File include/core/SkRSXform.h (right):

https://codereview.chromium.org/1181913003/diff/200001/include/core/SkRSXform...
include/core/SkRSXform.h:12: 
// A compressed transformation representation. It only supports ...
?

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
File samplecode/SampleAtlas.cpp (right):

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:125: const SkScalar half = 8;
speed -> kMaxSpeed ?

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:127: int i = 0;
Have 512 be kAtlasWH (and use in make_atlas too) ?
Maybe with 32 being kAtlasCellWH ?

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:143: }
rm this ?

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:148: SkRSXform xform[N];
Why not have a keystroke to enable/disable animating colors ?

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:164: SkRect onGetBounds() override {
border ?

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:175: class DrawAtlasView : public SampleView {
Why not just have this be a member ?

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:182: 
rm virtual ?

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:200: 
Do we still need this code ?

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

https://codereview.chromium.org/1181913003/diff/200001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:2466: const SkRect* cull, const SkPaint* paint) {
Why not quickReject in the non-virtual method ?

[reed1, 2015-06-24 16:17:49]

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp
File gm/spritebitmap.cpp (right):

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp#ne...
gm/spritebitmap.cpp:101: 
On 2015/06/24 15:11:02, robertphillips wrote:
> should this go in its own file ?
> // This GM ...

Done.

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp#ne...
gm/spritebitmap.cpp:102: class DrawAtlasGM : public skiagm::GM {
On 2015/06/24 15:11:01, robertphillips wrote:
> So we're not really using the entire atlas ?

I'll add dox. I wanted explicitly to use a subset to ensure that I didn't see
any RED bleeding through.

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp#ne...
gm/spritebitmap.cpp:140: const SkRect target = { 50, 50, 80, 90 };
On 2015/06/24 15:11:01, robertphillips wrote:
> If we're going to use this pattern more widely (creating resources to match
the
> backend type) should we pass the canvas to onOnceBeforeDraw ?

Perhaps so.

https://codereview.chromium.org/1181913003/diff/200001/gm/spritebitmap.cpp#ne...
gm/spritebitmap.cpp:172: tex[i] = target;
On 2015/06/24 15:11:02, robertphillips wrote:
> Do we need to 565/8888 safety these colors ?

Perhaps, though I'm antialiasing and blending, so I don't think I can guarantee
565 perfection.

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

https://codereview.chromium.org/1181913003/diff/200001/include/core/SkCanvas....
include/core/SkCanvas.h:1034: *  The color array is optional. When specified,
each color modulates the pixels in its
On 2015/06/24 15:11:02, robertphillips wrote:
> kDst_Mode ?

Done.

https://codereview.chromium.org/1181913003/diff/200001/include/core/SkCanvas....
include/core/SkCanvas.h:1036: *  but with a non-opaque alpha will result in
fading the quad.
On 2015/06/24 15:11:02, robertphillips wrote:
> * If an explicit xfer mode is provided it ...

Done.

https://codereview.chromium.org/1181913003/diff/200001/include/core/SkRSXform.h
File include/core/SkRSXform.h (right):

https://codereview.chromium.org/1181913003/diff/200001/include/core/SkRSXform...
include/core/SkRSXform.h:12: 
On 2015/06/24 15:11:02, robertphillips wrote:
> // A compressed transformation representation. It only supports ...
> ?

Done.

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

https://codereview.chromium.org/1181913003/diff/200001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:2466: const SkRect* cull, const SkPaint* paint) {
On 2015/06/24 15:11:02, robertphillips wrote:
> Why not quickReject in the non-virtual method ?

We don't seem to do that for any calls (afaik). Thus pictures always see
everything.

[robertphillips, 2015-06-24 16:26:26]

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

https://codereview.chromium.org/1181913003/diff/200001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:2466: const SkRect* cull, const SkPaint* paint) {
On 2015/06/24 16:17:48, reed1 wrote:
> On 2015/06/24 15:11:02, robertphillips wrote:
> > Why not quickReject in the non-virtual method ?
> 
> We don't seem to do that for any calls (afaik). Thus pictures always see
> everything.

Don't we do it for drawDrawable & drawPath (just looking above in the file) ?

[reed1, 2015-06-24 16:30:53]

On 2015/06/24 16:26:26, robertphillips wrote:
> https://codereview.chromium.org/1181913003/diff/200001/src/core/SkCanvas.cpp
> File src/core/SkCanvas.cpp (right):
> 
>
https://codereview.chromium.org/1181913003/diff/200001/src/core/SkCanvas.cpp#...
> src/core/SkCanvas.cpp:2466: const SkRect* cull, const SkPaint* paint) {
> On 2015/06/24 16:17:48, reed1 wrote:
> > On 2015/06/24 15:11:02, robertphillips wrote:
> > > Why not quickReject in the non-virtual method ?
> > 
> > We don't seem to do that for any calls (afaik). Thus pictures always see
> > everything.
> 
> Don't we do it for drawDrawable & drawPath (just looking above in the file) ?

True. Most others don't, so I was going with the mob. If we want to change this,
lets have some concensus w/ mtklein etc.

[scroggo, 2015-06-24 17:00:21]

pipe code lgtm

https://codereview.chromium.org/1181913003/diff/200001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/1181913003/diff/200001/src/pipe/SkGPipeWrite....
src/pipe/SkGPipeWrite.cpp:1103: const SkImage*, const SkRSXform[], const
SkRect[], const SkColor[],
Is this comment necessary?

[reed1, 2015-06-24 17:02:42]

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
File samplecode/SampleAtlas.cpp (right):

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:125: const SkScalar half = 8;
On 2015/06/24 15:11:02, robertphillips wrote:
> speed -> kMaxSpeed ?

Done.

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:127: int i = 0;
On 2015/06/24 15:11:02, robertphillips wrote:
> Have 512 be kAtlasWH (and use in make_atlas too) ?
> Maybe with 32 being kAtlasCellWH ?

Done.

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:143: }
On 2015/06/24 15:11:02, robertphillips wrote:
> rm this ?

Done.

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:148: SkRSXform xform[N];
On 2015/06/24 15:11:02, robertphillips wrote:
> Why not have a keystroke to enable/disable animating colors ?

Done.

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:164: SkRect onGetBounds() override {
On 2015/06/24 15:11:02, robertphillips wrote:
> border ?

Done.

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:175: class DrawAtlasView : public SampleView {
On 2015/06/24 15:11:02, robertphillips wrote:
> Why not just have this be a member ?

canvas->drawDrawable(fDrawable)

this guy may need to "ref" its argument, if its a picture-recording-canvas, so
we have to allow for that.

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:182: 
On 2015/06/24 15:11:02, robertphillips wrote:
> rm virtual ?

Done.

https://codereview.chromium.org/1181913003/diff/200001/samplecode/SampleAtlas...
samplecode/SampleAtlas.cpp:200: 
On 2015/06/24 15:11:02, robertphillips wrote:
> Do we still need this code ?

Leaving it as a reminder/todo

[reed1, 2015-06-24 17:04:30]

https://codereview.chromium.org/1181913003/diff/200001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/1181913003/diff/200001/src/pipe/SkGPipeWrite....
src/pipe/SkGPipeWrite.cpp:1103: const SkImage*, const SkRSXform[], const
SkRect[], const SkColor[],
On 2015/06/24 17:00:21, scroggo wrote:
> Is this comment necessary?

Done.

[reed1, 2015-06-24 17:04:42]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2015-06-24 17:04:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com, scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1181913003/#ps260001
(title: " ")

[commit-bot: I haz the power, 2015-06-24 17:05:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181913003/260001

[reed1, 2015-06-24 17:06:01]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-06-24 17:11:48]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2015-06-24 17:11:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com, scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1181913003/#ps280001
(title: "fix warnings")

[commit-bot: I haz the power, 2015-06-24 17:12:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181913003/280001

[reed1, 2015-06-24 17:12:20]

The CQ bit was unchecked by reed@google.com

[mtklein, 2015-06-24 17:22:28]

On 2015/06/24 16:30:53, reed1 wrote:
> On 2015/06/24 16:26:26, robertphillips wrote:
> > https://codereview.chromium.org/1181913003/diff/200001/src/core/SkCanvas.cpp
> > File src/core/SkCanvas.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1181913003/diff/200001/src/core/SkCanvas.cpp#...
> > src/core/SkCanvas.cpp:2466: const SkRect* cull, const SkPaint* paint) {
> > On 2015/06/24 16:17:48, reed1 wrote:
> > > On 2015/06/24 15:11:02, robertphillips wrote:
> > > > Why not quickReject in the non-virtual method ?
> > > 
> > > We don't seem to do that for any calls (afaik). Thus pictures always see
> > > everything.
> > 
> > Don't we do it for drawDrawable & drawPath (just looking above in the file)
?
> 
> True. Most others don't, so I was going with the mob. If we want to change
this,
> lets have some concensus w/ mtklein etc.

Generally an onDrawFoo() quick-reject needs to be quick relative to drawing a
Foo, and a drawFoo() quick-reject must also quick relative to recording a Foo,
which is usually much cheaper than drawing it.  If we put it in drawFoo() and it
should have been in onDrawFoo(), we'll notice on a profile at some point.  To
date, I've never noticed any quick-reject on any recording profile.  So I'd just
put this one wherever seems most natural to you both for now.

[reed1, 2015-06-24 17:28:04]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-06-24 17:28:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181913003/280001

[commit-bot: I haz the power, 2015-06-24 17:29:24]

Committed patchset #15 (id:280001) as
https://skia.googlesource.com/skia/+/71c3c760a83123ee0b3127b8c65c6394ce541c50