add drawPicture variant that takes a matrix and paint

add drawPicture variant that takes a matrix and paint

will need some staging strategy, since chrome and blink have overrides of onDrawPicture

TBR=

Committed: https://skia.googlesource.com/skia/+/d5fa1a455aad61f3e99081fe7a9b065cb3b115c6

[bsalomon, 2014-08-06 21:16:27]

On 2014/08/06 21:13:37, reed1 wrote:

I like

[f(malita), 2014-08-07 00:48:26]

On 2014/08/06 21:16:27, bsalomon wrote:
> On 2014/08/06 21:13:37, reed1 wrote:
> 
> I like

+1

[robertphillips, 2014-08-07 12:35:55]

Looks good.

[reed1, 2014-08-07 21:05:08]

now implemented

[reed1, 2014-08-07 21:05:14]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-07 21:05:55]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/448793004/40001

[reed1, 2014-08-07 21:08:30]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-08-07 21:24:20]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-07 21:25:18]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/448793004/60001

[mtklein, 2014-08-07 21:25:20]

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

https://codereview.chromium.org/448793004/diff/40001/src/core/SkBBoxRecord.cp...
src/core/SkBBoxRecord.cpp:305: if (this->transformBounds(bounds, NULL)) {
NULL -> paint?

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

https://codereview.chromium.org/448793004/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:2406: if (matrix && matrix->isIdentity()) {
Sort of weird?  Can't we just not do this?

https://codereview.chromium.org/448793004/diff/40001/src/core/SkRecords.h
File src/core/SkRecords.h (right):

https://codereview.chromium.org/448793004/diff/40001/src/core/SkRecords.h#new...
src/core/SkRecords.h:17: SkPictureBox(const SkPictureBox& src) :
fObj(SkRef(src.fObj)) {}
Try removing this too?  I don't think we need it.

https://codereview.chromium.org/448793004/diff/40001/src/gpu/GrPictureUtils.cpp
File src/gpu/GrPictureUtils.cpp (right):

https://codereview.chromium.org/448793004/diff/40001/src/gpu/GrPictureUtils.c...
src/gpu/GrPictureUtils.cpp:245: playback.draw(this, NULL);
Seems like we should pass matrix and paint through here...

https://codereview.chromium.org/448793004/diff/40001/src/gpu/GrPictureUtils.c...
src/gpu/GrPictureUtils.cpp:249: picture->draw(this);
... and here?

[reed1, 2014-08-07 21:29:06]

The CQ bit was unchecked by reed@google.com

[bsalomon, 2014-08-08 13:49:38]

Seems mostly good to me. Rob should have a look to make sure it can fit into the
layer atlasing framework.

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

https://codereview.chromium.org/448793004/diff/60001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:2406: if (matrix && matrix->isIdentity()) {
What is this saving? Don't most of our matrix methods have shortcuts for I?
Invert and concat do.

[reed1, 2014-08-08 13:57:30]

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

https://codereview.chromium.org/448793004/diff/40001/src/core/SkBBoxRecord.cp...
src/core/SkBBoxRecord.cpp:305: if (this->transformBounds(bounds, NULL)) {
On 2014/08/07 21:25:19, mtklein wrote:
> NULL -> paint?

Done.

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

https://codereview.chromium.org/448793004/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:2406: if (matrix && matrix->isIdentity()) {
On 2014/08/07 21:25:20, mtklein wrote:
> Sort of weird?  Can't we just not do this?

Possibly, though null saves us having to do a save/restore. I figured any
canonicalization tweaks should happen in the non-virtual method (here). That way
all the subclasses need have no checks.

https://codereview.chromium.org/448793004/diff/40001/src/core/SkRecords.h
File src/core/SkRecords.h (right):

https://codereview.chromium.org/448793004/diff/40001/src/core/SkRecords.h#new...
src/core/SkRecords.h:17: SkPictureBox(const SkPictureBox& src) :
fObj(SkRef(src.fObj)) {}
On 2014/08/07 21:25:20, mtklein wrote:
> Try removing this too?  I don't think we need it.

Done.

https://codereview.chromium.org/448793004/diff/40001/src/gpu/GrPictureUtils.cpp
File src/gpu/GrPictureUtils.cpp (right):

https://codereview.chromium.org/448793004/diff/40001/src/gpu/GrPictureUtils.c...
src/gpu/GrPictureUtils.cpp:245: playback.draw(this, NULL);
On 2014/08/07 21:25:20, mtklein wrote:
> Seems like we should pass matrix and paint through here...

Done.

https://codereview.chromium.org/448793004/diff/40001/src/gpu/GrPictureUtils.c...
src/gpu/GrPictureUtils.cpp:249: picture->draw(this);
On 2014/08/07 21:25:20, mtklein wrote:
> ... and here?

Done.

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

https://codereview.chromium.org/448793004/diff/60001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:2406: if (matrix && matrix->isIdentity()) {
On 2014/08/08 13:49:38, bsalomon wrote:
> What is this saving? Don't most of our matrix methods have shortcuts for I?
> Invert and concat do.

save()/restore()

[bsalomon, 2014-08-08 13:59:42]

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

https://codereview.chromium.org/448793004/diff/60001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:2406: if (matrix && matrix->isIdentity()) {
On 2014/08/08 13:57:29, reed1 wrote:
> On 2014/08/08 13:49:38, bsalomon wrote:
> > What is this saving? Don't most of our matrix methods have shortcuts for I?
> > Invert and concat do.
> 
> save()/restore()

Ah, that makes sense!

[robertphillips, 2014-08-08 14:59:54]

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

https://codereview.chromium.org/448793004/diff/60001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:2414: SkASSERT(NULL != pic);
I think the save/saveLayer needs to be moved to after the device's attempt since
in that case the backend will take care of it itself.

https://codereview.chromium.org/448793004/diff/60001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:2415: 
Move bounds computation into "if (paint) {" block ?

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

https://codereview.chromium.org/448793004/diff/60001/src/core/SkPictureRecord...
src/core/SkPictureRecord.cpp:1423: this->addPaintPtr(paint);
Don't need this next line.

[robertphillips, 2014-08-08 15:01:18]

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

https://codereview.chromium.org/448793004/diff/60001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:2436: // the picture itself.
Can we add matrix & paint to this too?

[robertphillips, 2014-08-08 15:19:47]

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

https://codereview.chromium.org/448793004/diff/60001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:939: //
I second the motion.

https://codereview.chromium.org/448793004/diff/60001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:942: //
I don't think this will work right. Won't the layer calls not be propagated?

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

https://codereview.chromium.org/448793004/diff/60001/src/utils/SkDumpCanvas.c...
src/utils/SkDumpCanvas.cpp:426: const SkPaint* paint) {
dump new params here?

https://codereview.chromium.org/448793004/diff/60001/src/utils/SkLuaCanvas.cpp
File src/utils/SkLuaCanvas.cpp (right):

https://codereview.chromium.org/448793004/diff/60001/src/utils/SkLuaCanvas.cp...
src/utils/SkLuaCanvas.cpp:274: // call through so we can see the nested picture
ops
This will under count layers

[reed1, 2014-08-08 18:22:34]

The CQ bit was checked by reed@google.com

[reed1, 2014-08-08 18:22:42]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-08-08 18:23:00]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-08 18:23:31]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/448793004/120001

[commit-bot: I haz the power, 2014-08-08 18:23:32]

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 a valid reviewer till 2014-08-09 00:23 UTC

[reed1, 2014-08-08 18:24:37]

The CQ bit was unchecked by reed@google.com

[robertphillips, 2014-08-08 18:37:37]

https://codereview.chromium.org/448793004/diff/120001/gm/picture.cpp
File gm/picture.cpp (right):

https://codereview.chromium.org/448793004/diff/120001/gm/picture.cpp#newcode37
gm/picture.cpp:37: 
// Exercise the SkCanvas::drawPicture interface with different matrix/paint
combinations.

?

https://codereview.chromium.org/448793004/diff/120001/gm/picture.cpp#newcode48
gm/picture.cpp:48: virtual SkISize onISize() SK_OVERRIDE {
Does this need to be this large?

https://codereview.chromium.org/448793004/diff/120001/gm/picture.cpp#newcode69
gm/picture.cpp:69: }
private:
    typedef skiagm::GM INHERITED;

?

[reed1, 2014-08-09 17:42:31]

https://codereview.chromium.org/448793004/diff/120001/gm/picture.cpp
File gm/picture.cpp (right):

https://codereview.chromium.org/448793004/diff/120001/gm/picture.cpp#newcode37
gm/picture.cpp:37: 
On 2014/08/08 18:37:37, robertphillips wrote:
> // Exercise the SkCanvas::drawPicture interface with different matrix/paint
> combinations.
> 
> ?

Done.

https://codereview.chromium.org/448793004/diff/120001/gm/picture.cpp#newcode48
gm/picture.cpp:48: virtual SkISize onISize() SK_OVERRIDE {
On 2014/08/08 18:37:37, robertphillips wrote:
> Does this need to be this large?

Done.

https://codereview.chromium.org/448793004/diff/120001/gm/picture.cpp#newcode69
gm/picture.cpp:69: }
On 2014/08/08 18:37:37, robertphillips wrote:
> private:
>     typedef skiagm::<span id="abbrex-spnAbbr_18638" class="abbrex-Abbr
abbrex-Abbr_GM abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">GM</span>
INHERITED;
> 
> ?

Done.

[reed1, 2014-08-09 17:42:43]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-09 17:43:31]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/448793004/160001

[reed1, 2014-08-09 17:46:13]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-08-09 17:59:49]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-09 18:00:32]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/448793004/180001

[commit-bot: I haz the power, 2014-08-09 18:08:12]

Change committed as d5fa1a455aad61f3e99081fe7a9b065cb3b115c6