De-virtualize SkCanvas save/restore.

De-virtualize SkCanvas save/restore.

This moves the state management logic into non-virtual SkCanvas methods,
and turns the virtuals into protected notifiers.

R=robertphillips@google.com, reed@google.com

Committed: http://code.google.com/p/skia/source/detail?r=13776

[f(malita), 2014-03-11 15:16:31]

Trying without guards.

[reed1, 2014-03-11 15:49:39]

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

https://codereview.chromium.org/194713008/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1185: virtual void onSave(SaveFlags) { }
nit: lets always put the impl of virtuals in the .cpp, even if they're empty.

https://codereview.chromium.org/194713008/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1288: SaveFlags, bool justForImageFilter, bool
skipLayer);
Can the caller(s) cleanly do whatever internalSaveLayer does, if the caller
knows it is going to pass false?

[robertphillips, 2014-03-11 15:49:43]

lgtm

https://codereview.chromium.org/194713008/diff/60001/experimental/PdfViewer/S...
File experimental/PdfViewer/SkNulCanvas.h (right):

https://codereview.chromium.org/194713008/diff/60001/experimental/PdfViewer/S...
experimental/PdfViewer/SkNulCanvas.h:101: 
Do we even need this onSave here?

https://codereview.chromium.org/194713008/diff/60001/experimental/PdfViewer/S...
experimental/PdfViewer/SkNulCanvas.h:107: }
same here

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

https://codereview.chromium.org/194713008/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:376: int saveLayer(const SkRect* bounds, const SkPaint*
paint,
line up?

[f(malita), 2014-03-11 16:38:07]

https://codereview.chromium.org/194713008/diff/60001/experimental/PdfViewer/S...
File experimental/PdfViewer/SkNulCanvas.h (right):

https://codereview.chromium.org/194713008/diff/60001/experimental/PdfViewer/S...
experimental/PdfViewer/SkNulCanvas.h:101: 
On 2014/03/11 15:49:44, robertphillips wrote:
> Do we even need this onSave here?

Gah, good point. Removed.

https://codereview.chromium.org/194713008/diff/60001/experimental/PdfViewer/S...
experimental/PdfViewer/SkNulCanvas.h:107: }
On 2014/03/11 15:49:44, robertphillips wrote:
> same here

Done.

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

https://codereview.chromium.org/194713008/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:376: int saveLayer(const SkRect* bounds, const SkPaint*
paint,
On 2014/03/11 15:49:44, robertphillips wrote:
> line up?

Done.

https://codereview.chromium.org/194713008/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1185: virtual void onSave(SaveFlags) { }
On 2014/03/11 15:49:39, reed1 wrote:
> nit: lets always put the impl of virtuals in the .cpp, even if they're empty.

Since we're always up-calling INHERITED::on* from derived classes (mostly for
consistency), I wanted to make sure these stay inlined (that's assuming they get
inlined in the first place, which I think they do when the call is
class-qualified). I'll gladly switch if we don't care about that.

https://codereview.chromium.org/194713008/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1288: SaveFlags, bool justForImageFilter, bool
skipLayer);
On 2014/03/11 15:49:39, reed1 wrote:
> Can the caller(s) cleanly do whatever internalSaveLayer does, if the caller
> knows it is going to pass false?

They probably could, but they still have to signal the common impl that it
should change behavior (and don't allocate a layer). Are you thinking of not
calling internalSaveLayer at all in that case? That would save us a flag, but
would also introduce some duplication in the handful of derived classes that
actually care about it.

[reed1, 2014-03-11 16:53:37]

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

https://codereview.chromium.org/194713008/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1185: virtual void onSave(SaveFlags) { }
On 2014/03/11 16:38:08, Florin Malita wrote:
> On 2014/03/11 15:49:39, reed1 wrote:
> > nit: lets always put the impl of virtuals in the .cpp, even if they're
empty.
> 
> Since we're always up-calling INHERITED::on* from derived classes (mostly for
> consistency), I wanted to make sure these stay inlined (that's assuming they
get
> inlined in the first place, which I think they do when the call is
> class-qualified). I'll gladly switch if we don't care about that.

I see your point, and I'm trying to recall all of the reasons I've heard against
inlining a virtual.

https://codereview.chromium.org/194713008/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1288: SaveFlags, bool justForImageFilter, bool
skipLayer);
On 2014/03/11 16:38:08, Florin Malita wrote:
> On 2014/03/11 15:49:39, reed1 wrote:
> > Can the caller(s) cleanly do whatever internalSaveLayer does, if the caller
> > knows it is going to pass false?
> 
> They probably could, but they still have to signal the common impl that it
> should change behavior (and don't allocate a layer). Are you thinking of not
> calling internalSaveLayer at all in that case? That would save us a flag, but
> would also introduce some duplication in the handful of derived classes that
> actually care about it.

Are derived classes allowed to call internalSaveLayer?

[f(malita), 2014-03-11 17:12:08]

On 2014/03/11 16:53:37, reed1 wrote:
>
https://codereview.chromium.org/194713008/diff/60001/include/core/SkCanvas.h#...
> include/core/SkCanvas.h:1288: SaveFlags, bool justForImageFilter, bool
> skipLayer);
> On 2014/03/11 16:38:08, Florin Malita wrote:
> > On 2014/03/11 15:49:39, reed1 wrote:
> > > Can the caller(s) cleanly do whatever internalSaveLayer does, if the
caller
> > > knows it is going to pass false?
> > 
> > They probably could, but they still have to signal the common impl that it
> > should change behavior (and don't allocate a layer). Are you thinking of not
> > calling internalSaveLayer at all in that case? That would save us a flag,
but
> > would also introduce some duplication in the handful of derived classes that
> > actually care about it.
> 
> Are derived classes allowed to call internalSaveLayer?

Currently not, it's private.

[f(malita), 2014-03-11 21:04:55]

Committed patchset #6 manually as r13747.

[f(malita), 2014-03-11 21:39:15]

A revert of this CL has been created in
https://codereview.chromium.org/195193005/ by fmalita@chromium.org.

The reason for reverting is: To allow a roll fix into the tree..

[hal.canary, 2014-03-11 21:45:06]

A revert of this CL has been created in
https://codereview.chromium.org/195843002/ by halcanary@google.com.

The reason for reverting is: roll fix.

[f(malita), 2014-03-12 19:12:17]

Re-opened: updated virtual names + enum willSaveLayer() ret value.

PTAL.

[reed1, 2014-03-12 19:13:30]

lgtm

[f(malita), 2014-03-12 20:13:24]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-03-12 20:13:26]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/194713008/140001

[commit-bot: I haz the power, 2014-03-12 20:21:56]

Change committed as 13776

[tomhudson, 2014-03-13 11:37:43]

There's no BUG=; have we recorded the motivation anywhere?

[f(malita), 2014-03-13 18:52:08]

On 2014/03/13 11:37:43, tomhudson wrote:
> There's no BUG=; have we recorded the motivation anywhere?

I just created https://code.google.com/p/skia/issues/detail?id=2297