Alter SkCanvas::drawPicture (devirtualize, take const SkPicture, take pointer)

Alter SkCanvas::drawPicture (devirtualize, take const SkPicture, take pointer)

Committed: https://skia.googlesource.com/skia/+/9b14f26d0f3a974f3dd626c8354e1db1cfcd322f

[robertphillips, 2014-06-03 21:46:37]

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

https://codereview.chromium.org/313613004/diff/1/include/core/SkPicture.h#new...
include/core/SkPicture.h:71: void EXPERIMENTAL_addAccelData(const AccelData*
data) const {
This is a bit of a cheat here.

https://codereview.chromium.org/313613004/diff/1/include/core/SkPicture.h#new...
include/core/SkPicture.h:288: int                   fWidth, fHeight;
and here.

[reed1, 2014-06-03 22:20:17]

I love it...

Will you get away with this when it initially lands? Seems like any chrome
subclass that overrode drawPicture may generate a warning (non-virtual shadows a
virtual with the same name).

Anyway, great direction.

lgtm

[mtklein, 2014-06-03 22:23:24]

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

https://codereview.chromium.org/313613004/diff/20001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:986: void drawPicture(const SkPicture* picture);
How come these aren't all const SkPicture&?

[robertphillips, 2014-06-03 23:17:21]

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

https://codereview.chromium.org/313613004/diff/20001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:986: void drawPicture(const SkPicture* picture);
On 2014/06/03 22:23:23, mtklein wrote:
> How come these aren't all const SkPicture&?

It seems/seemed at bit odd to pass by reference something the callee could take
a ref on (and make live longer than the caller might expect)

[reed1, 2014-06-03 23:21:41]

On 2014/06/03 23:17:21, robertphillips wrote:
> https://codereview.chromium.org/313613004/diff/20001/include/core/SkCanvas.h
> File include/core/SkCanvas.h (right):
> 
>
https://codereview.chromium.org/313613004/diff/20001/include/core/SkCanvas.h#...
> include/core/SkCanvas.h:986: void drawPicture(const SkPicture* picture);
> On 2014/06/03 22:23:23, mtklein wrote:
> > How come these aren't all const SkPicture&?
> 
> It seems/seemed at bit odd to pass by reference something the callee could
take
> a ref on (and make live longer than the caller might expect)

Agreed. I know its just a style thing, but pointers feel mucho bettero for
something that I may want to also own.

[mtklein, 2014-06-03 23:30:40]

On 2014/06/03 23:21:41, reed1 wrote:
> On 2014/06/03 23:17:21, robertphillips wrote:
> > https://codereview.chromium.org/313613004/diff/20001/include/core/SkCanvas.h
> > File include/core/SkCanvas.h (right):
> > 
> >
>
https://codereview.chromium.org/313613004/diff/20001/include/core/SkCanvas.h#...
> > include/core/SkCanvas.h:986: void drawPicture(const SkPicture* picture);
> > On 2014/06/03 22:23:23, mtklein wrote:
> > > How come these aren't all const SkPicture&?
> > 
> > It seems/seemed at bit odd to pass by reference something the callee could
> take
> > a ref on (and make live longer than the caller might expect)
> 
> Agreed. I know its just a style thing, but pointers feel mucho bettero for
> something that I may want to also own.

OK by me.  I just keep reading const SkPicture* as somehow optional.

[reed1, 2014-06-03 23:36:04]

On 2014/06/03 23:30:40, mtklein wrote:
> On 2014/06/03 23:21:41, reed1 wrote:
> > On 2014/06/03 23:17:21, robertphillips wrote:
> > >
https://codereview.chromium.org/313613004/diff/20001/include/core/SkCanvas.h
> > > File include/core/SkCanvas.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/313613004/diff/20001/include/core/SkCanvas.h#...
> > > include/core/SkCanvas.h:986: void drawPicture(const SkPicture* picture);
> > > On 2014/06/03 22:23:23, mtklein wrote:
> > > > How come these aren't all const SkPicture&?
> > > 
> > > It seems/seemed at bit odd to pass by reference something the callee could
> > take
> > > a ref on (and make live longer than the caller might expect)
> > 
> > Agreed. I know its just a style thing, but pointers feel mucho bettero for
> > something that I may want to also own.
> 
> OK by me.  I just keep reading const SkPicture* as somehow optional.

Sure. You can pass null if you want to crash...

[robertphillips, 2014-06-04 12:15:59]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-06-04 12:17:03]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/313613004/2...

[commit-bot: I haz the power, 2014-06-04 12:40:49]

Change committed as 9b14f26d0f3a974f3dd626c8354e1db1cfcd322f