remove drawSprite from canvas

remove drawSprite from canvas

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/4657ce2324ea197507c4ba728d81138f56da13b1

[reed1, 2015-12-16 14:01:22]

reed@google.com changed reviewers:
+ fmalita@chromium.org, robertphillips@google.com, senorblanco@chromium.org

[reed1, 2015-12-16 14:01:22]

ptal

[reed1, 2015-12-16 14:02:35]

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

[commit-bot: I haz the power, 2015-12-16 14:02:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534443003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534443003/20001

[mtklein, 2015-12-16 14:06:11]

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

[mtklein, 2015-12-16 14:06:11]

https://codereview.chromium.org/1534443003/diff/20001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (left):

https://codereview.chromium.org/1534443003/diff/20001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:370: case DRAW_SPRITE: {
Does this not break .skp compatibility?  What happens if we read an .skp that
contains a DRAW_SPRITE?

Perhaps we should just comment out the canvas->drawSprite(...) call here, so
that everything keeps parsing?

[commit-bot: I haz the power, 2015-12-16 14:16:54]

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

[commit-bot: I haz the power, 2015-12-16 14:16:54]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-12-16 14:52:36]

https://codereview.chromium.org/1534443003/diff/20001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (left):

https://codereview.chromium.org/1534443003/diff/20001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:370: case DRAW_SPRITE: {
On 2015/12/16 14:06:11, mtklein wrote:
> Does this not break .skp compatibility?  What happens if we read an .skp that
> contains a DRAW_SPRITE?
> 
> Perhaps we should just comment out the canvas->drawSprite(...) call here, so
> that everything keeps parsing?

Done.

[reed1, 2015-12-16 14:52:42]

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

[commit-bot: I haz the power, 2015-12-16 14:52:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534443003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534443003/60001

[commit-bot: I haz the power, 2015-12-16 15:06:29]

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

[commit-bot: I haz the power, 2015-12-16 15:06:30]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-12-16 15:34:17]

https://codereview.chromium.org/1534443003/diff/60001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/1534443003/diff/60001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:375: // drawSprite removed dec-2015
Could we emulate drawSprite using setMatrix + drawBitmap instead?

[reed1, 2015-12-16 15:48:07]

On 2015/12/16 15:34:17, f(malita) wrote:
>
https://codereview.chromium.org/1534443003/diff/60001/src/core/SkPicturePlayb...
> File src/core/SkPicturePlayback.cpp (right):
> 
>
https://codereview.chromium.org/1534443003/diff/60001/src/core/SkPicturePlayb...
> src/core/SkPicturePlayback.cpp:375: // drawSprite removed dec-2015
> Could we emulate drawSprite using setMatrix + drawBitmap instead?

We could -- but I posit there there are no drawsprite-in-pictures out there.
Blink never used it, android doesn't use it. I think the only sprites-in-picts
are our GMs.

[robertphillips, 2015-12-16 15:49:14]

lgtm modulo Florin's request

[f(malita), 2015-12-16 15:51:32]

On 2015/12/16 15:48:07, reed1 wrote:
> On 2015/12/16 15:34:17, f(malita) wrote:
> >
>
https://codereview.chromium.org/1534443003/diff/60001/src/core/SkPicturePlayb...
> > File src/core/SkPicturePlayback.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1534443003/diff/60001/src/core/SkPicturePlayb...
> > src/core/SkPicturePlayback.cpp:375: // drawSprite removed dec-2015
> > Could we emulate drawSprite using setMatrix + drawBitmap instead?
> 
> We could -- but I posit there there are no drawsprite-in-pictures out there.
> Blink never used it, android doesn't use it. I think the only sprites-in-picts
> are our GMs.

Sounds good, LGTM.

[reed1, 2015-12-16 15:51:55]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-12-16 15:52:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534443003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534443003/60001

[reed1, 2015-12-16 15:52:20]

Happy to add emulation if/when we find the need. For now, I'm favoring
simplicity

[commit-bot: I haz the power, 2015-12-16 15:52:49]

Description was changed from

==========
remove drawSprite from canvas

BUG=skia:
==========

to

==========
remove drawSprite from canvas

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/4657ce2324ea197507c4ba728d81138f56da13b1
==========

[commit-bot: I haz the power, 2015-12-16 15:52:50]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/4657ce2324ea197507c4ba728d81138f56da13b1

[reed1, 2015-12-16 16:26:54]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1529803004/ by reed@google.com.

The reason for reverting is: need to remove an override in chrome.

[scroggo, 2015-12-16 16:34:08]

scroggo@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2015-12-16 16:34:08]

On 2015/12/16 16:26:54, reed1 wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/1529803004/ by mailto:reed@google.com.
> 
> The reason for reverting is: need to remove an override in chrome.


You also need to update a caller in Android:

frameworks/base/core/jni/../../libs/hwui/SkiaCanvasProxy.h:69:18: error:
'virtual void android::uirenderer::SkiaCanvasProxy::onDrawSprite(const
SkBitmap&, int, int, const SkPaint*)' marked override, but does not override
     virtual void onDrawSprite(const SkBitmap&, int left, int top,

https://android-build.storage.googleapis.com/builds/git_master-skia-linux-vol...

[scroggo, 2015-12-16 16:34:46]

On 2015/12/16 16:34:08, scroggo wrote:
> You also need to update a caller in Android:

s/You/We