Change SkCanvasState to use inheritance.

Change SkCanvasState to use inheritance.

The base class, SkCanvasState, now holds the version, width, and
height. These fields will always be a necessary part of the class.
(Also add in some padding.)
The other fields, which may change, have been moved into the
subclass, SkCanvasState_v1. If/when the version changes, it will
correspond to a new subclass.
In SkCanvasStateUtils::CreateFromCanvasState, check the version on
the base class, then do a static_cast to the version corresponding
to SkCanvasState::version.

Remove CANVAS_STATE_VERSION, which is redundant with the version
specified by the subclass.

Use unambiguous type for rowBytes.

Build Android with SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG. This allows us
to run the full suite of CanvasState tests. It is also representative
of what will be used on Android by WebView.

Fix CanvasStateTest where it was broken inside ifdef'ed out code.

Use SkCanvas::getBaseLayerSize() instead of the deprecated
SkCanvas::getDeviceSize().

Update the comments in the header to be more clear. In particular,
an SkCanvasState can only be used to pass an SkCanvas' state to a
future version of Skia (or the same); not an older version.

NOTREECHECKS=true

BUG=b/15693384

Committed: https://skia.googlesource.com/skia/+/352c2181d15ed053c3b759f08ff1f51d50e2d3bb

[djsollen, 2014-07-07 20:02:16]

lgtm

[mtklein, 2014-07-07 20:02:28]

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
File src/utils/SkCanvasStateUtils.cpp (right):

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
src/utils/SkCanvasStateUtils.cpp:82: virtual ~SkCanvasState() {}
Drop virtual?

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
src/utils/SkCanvasStateUtils.cpp:346: SkDebugf("CreateFromCanvasState version is
not supported!");
"... version %d ...", state->version); ?

[reed1, 2014-07-07 20:05:12]

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
File src/utils/SkCanvasStateUtils.cpp (right):

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
src/utils/SkCanvasStateUtils.cpp:82: virtual ~SkCanvasState() {}
On 2014/07/07 20:02:28, mtklein wrote:
> Drop virtual?

Drop destructor?

[mtklein, 2014-07-07 20:05:53]

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
File src/utils/SkCanvasStateUtils.cpp (right):

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
src/utils/SkCanvasStateUtils.cpp:82: virtual ~SkCanvasState() {}
On 2014/07/07 20:05:12, reed1 wrote:
> On 2014/07/07 20:02:28, mtklein wrote:
> > Drop virtual?
> 
> Drop destructor?

Duh, yes, even better.

[scroggo, 2014-07-07 20:30:17]

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
File src/utils/SkCanvasStateUtils.cpp (right):

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
src/utils/SkCanvasStateUtils.cpp:82: virtual ~SkCanvasState() {}
On 2014/07/07 20:05:53, mtklein wrote:
> On 2014/07/07 20:05:12, reed1 wrote:
> > On 2014/07/07 20:02:28, mtklein wrote:
> > > Drop virtual?
> > 
> > Drop destructor?
> 
> Duh, yes, even better.

I don't think so. We call delete on a pointer to SkCanvasState (see
ReleaseCanvasState). If I remove the virtual, this will not call the destructor
for SkCanvasState_v1.

https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
src/utils/SkCanvasStateUtils.cpp:346: SkDebugf("CreateFromCanvasState version is
not supported!");
On 2014/07/07 20:02:28, mtklein wrote:
> "... version %d ...", state->version); ?

Done.

[mtklein, 2014-07-07 20:32:41]

https://codereview.chromium.org/372003002/diff/40001/src/utils/SkCanvasStateU...
File src/utils/SkCanvasStateUtils.cpp (right):

https://codereview.chromium.org/372003002/diff/40001/src/utils/SkCanvasStateU...
src/utils/SkCanvasStateUtils.cpp:354: SkDELETE(state);
Can't we do the same check-version-and-static-cast trick?

[reed1, 2014-07-07 20:35:13]

On 2014/07/07 20:30:17, scroggo wrote:
>
https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
> File src/utils/SkCanvasStateUtils.cpp (right):
> 
>
https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
> src/utils/SkCanvasStateUtils.cpp:82: virtual ~SkCanvasState() {}
> On 2014/07/07 20:05:53, mtklein wrote:
> > On 2014/07/07 20:05:12, reed1 wrote:
> > > On 2014/07/07 20:02:28, mtklein wrote:
> > > > Drop virtual?
> > > 
> > > Drop destructor?
> > 
> > Duh, yes, even better.
> 
> I don't think so. We call delete on a pointer to SkCanvasState (see
> ReleaseCanvasState). If I remove the virtual, this will not call the
destructor
> for SkCanvasState_v1.
>

Agreed. If we're going to use inheritance, than a virtual destructor in the base
is right. I retract my previous suggestion.

>
https://codereview.chromium.org/372003002/diff/20001/src/utils/SkCanvasStateU...
> src/utils/SkCanvasStateUtils.cpp:346: SkDebugf("CreateFromCanvasState version
is
> not supported!");
> On 2014/07/07 20:02:28, mtklein wrote:
> > "... version %d ...", state->version); ?
> 
> Done.

[scroggo, 2014-07-07 21:44:02]

https://codereview.chromium.org/372003002/diff/40001/src/utils/SkCanvasStateU...
File src/utils/SkCanvasStateUtils.cpp (right):

https://codereview.chromium.org/372003002/diff/40001/src/utils/SkCanvasStateU...
src/utils/SkCanvasStateUtils.cpp:354: SkDELETE(state);
On 2014/07/07 20:32:40, mtklein wrote:
> Can't we do the same check-version-and-static-cast trick?

To (try to) sum up our conversation live, you want to do this for two reasons:
1. The current implementation now has two kinds of virtual-ness: A virtual
destructor and the version field.
2. It is odd to have a class with no virtual functions except for its
destructor.

Done.

https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
File src/utils/SkCanvasStateUtils.cpp (right):

https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
src/utils/SkCanvasStateUtils.cpp:318: SkASSERT(CANVAS_STATE_VERSION ==
state->version);
Since CreateFromCanvasState will always be called from the updated version of
Skia, we don't need it to be compatible with other versions yet. If/when we
update the version, CreateFromCanvasState will need to handle older versions,
but never future versions.

Does that seem reasonable? Should we update the documentation explaining that we
can only recreate SkCanvas in one direction?

[mtklein, 2014-07-07 22:09:16]

Some suggestions, but LGTM as it is.

https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
File src/utils/SkCanvasStateUtils.cpp (right):

https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
src/utils/SkCanvasStateUtils.cpp:76: version = CANVAS_STATE_VERSION;
Might want to have the derived class pass in version so that it can be part of
that class?  E.g.

class SkCanvasState_v1 : public SkCanvasState {
  static const int32 kVersion = 1;

  SkCanvasState_v1(SkCanvas* canvas)
    : INHERITED(kVersion, canvas) ...  { ... }
  ...
};

This could make the asserts in ReleaseCanvasState a little clearer:
  SkASSERT(SkCanvasState_v1::kVersion == state->version);
  SkDELETE(static_cast<SkCanvasSatate_v1*>(state));
}

https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
src/utils/SkCanvasStateUtils.cpp:318: SkASSERT(CANVAS_STATE_VERSION ==
state->version);
It can't hurt to get this function in the state it'll eventually be in, e.g.

switch (state->version) {
  case 1: return create_from_v1(static_cast<const SkCanvasState_v1*>(state));
  default:
    SkASSERTF(false, "Unknown SkCanvasState version %d", state->version); 
    return NULL;
}
?

That said, leaving things as you've got them here certainly seems fine until we
add v2.

[mtklein, 2014-07-07 22:09:16]

Some suggestions, but LGTM as it is.

[scroggo, 2014-07-08 18:45:13]

I've made several changes since the last review. PTAL.

https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
File src/utils/SkCanvasStateUtils.cpp (right):

https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
src/utils/SkCanvasStateUtils.cpp:76: version = CANVAS_STATE_VERSION;
On 2014/07/07 22:09:16, mtklein wrote:
> Might want to have the derived class pass in version so that it can be part of
> that class?  E.g.
> 
> class SkCanvasState_v1 : public SkCanvasState {
>   static const int32 kVersion = 1;
> 
>   SkCanvasState_v1(SkCanvas* canvas)
>     : INHERITED(kVersion, canvas) ...  { ... }
>   ...
> };
> 
> This could make the asserts in ReleaseCanvasState a little clearer:
>   SkASSERT(SkCanvasState_v1::kVersion == state->version);
>   SkDELETE(static_cast<SkCanvasSatate_v1*>(state));
> }

Done.

[mtklein, 2014-07-08 19:17:24]

On 2014/07/08 18:45:13, scroggo wrote:
> I've made several changes since the last review. PTAL.
> 
>
https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
> File src/utils/SkCanvasStateUtils.cpp (right):
> 
>
https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
> src/utils/SkCanvasStateUtils.cpp:76: version = CANVAS_STATE_VERSION;
> On 2014/07/07 22:09:16, mtklein wrote:
> > Might want to have the derived class pass in version so that it can be part
of
> > that class?  E.g.
> > 
> > class SkCanvasState_v1 : public SkCanvasState {
> >   static const int32 kVersion = 1;
> > 
> >   SkCanvasState_v1(SkCanvas* canvas)
> >     : INHERITED(kVersion, canvas) ...  { ... }
> >   ...
> > };
> > 
> > This could make the asserts in ReleaseCanvasState a little clearer:
> >   SkASSERT(SkCanvasState_v1::kVersion == state->version);
> >   SkDELETE(static_cast<SkCanvasSatate_v1*>(state));
> > }
> 
> Done.

lgtm

[scroggo, 2014-07-08 19:25:53]

On 2014/07/08 19:17:24, mtklein wrote:
> On 2014/07/08 18:45:13, scroggo wrote:
> > I've made several changes since the last review. PTAL.
> > 
> >
>
https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
> > File src/utils/SkCanvasStateUtils.cpp (right):
> > 
> >
>
https://codereview.chromium.org/372003002/diff/100001/src/utils/SkCanvasState...
> > src/utils/SkCanvasStateUtils.cpp:76: version = CANVAS_STATE_VERSION;
> > On 2014/07/07 22:09:16, mtklein wrote:
> > > Might want to have the derived class pass in version so that it can be
part
> of
> > > that class?  E.g.
> > > 
> > > class SkCanvasState_v1 : public SkCanvasState {
> > >   static const int32 kVersion = 1;
> > > 
> > >   SkCanvasState_v1(SkCanvas* canvas)
> > >     : INHERITED(kVersion, canvas) ...  { ... }
> > >   ...
> > > };
> > > 
> > > This could make the asserts in ReleaseCanvasState a little clearer:
> > >   SkASSERT(SkCanvasState_v1::kVersion == state->version);
> > >   SkDELETE(static_cast<SkCanvasSatate_v1*>(state));
> > > }
> > 
> > Done.
> 
> lgtm

Derek, how do you advise landing this? It needs to go into Android's
external/skia at the same time as it goes into WebView.

[djsollen, 2014-07-08 19:39:54]

we should be ok if we land simultaneously.  It looks like their automerger is
current so the time when they will be out of phase should be small.

[scroggo, 2014-07-15 17:57:57]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-07-15 17:58:36]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/scroggo@google.com/372003002/220001

[commit-bot: I haz the power, 2014-07-15 17:58:38]

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

[commit-bot: I haz the power, 2014-07-15 17:58:39]

Presubmit check for 372003002-220001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

[scroggo, 2014-07-15 17:59:55]

On 2014/07/15 17:58:39, I haz the power (commit-bot) wrote:
> Presubmit check for 372003002-220001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Since the CL is editing public API, you must have an LGTM from one of:
> (mailto:, mailto:, mailto:,
> mailto:)

reed@, I think I need approval from you.

[reed1, 2014-07-15 19:01:22]

lgtm

[commit-bot: I haz the power, 2014-07-15 19:16:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[scroggo, 2014-07-15 19:20:22]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-07-15 19:20:26]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/scroggo@google.com/372003002/240001

[commit-bot: I haz the power, 2014-07-15 19:34:36]

Change committed as 352c2181d15ed053c3b759f08ff1f51d50e2d3bb