Change SkCanvasState to use inheritance.

src/utils/SkCanvasStateUtils.cpp

Index: src/utils/SkCanvasStateUtils.cpp
diff --git a/src/utils/SkCanvasStateUtils.cpp b/src/utils/SkCanvasStateUtils.cpp
index eaee61b5e829a6534c87f0180b7f4c1856e7a7b0..5115199d975bd2bb0f3bf8227cc374dd5941e5e6 100644
--- a/src/utils/SkCanvasStateUtils.cpp
+++ b/src/utils/SkCanvasStateUtils.cpp
@@ -74,17 +74,37 @@ public:
     SkCanvasState(SkCanvas* canvas) {
         SkASSERT(canvas);
         version = CANVAS_STATE_VERSION;

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

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));
}

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

Done.

-        width = canvas->getDeviceSize().width();
-        height = canvas->getDeviceSize().height();
+        width = canvas->getBaseLayerSize().width();
+        height = canvas->getBaseLayerSize().height();
+
+    }
+
+    /**
+     * The version this struct was built with.  This field must always appear
+     * first in the struct so that when the versions don't match (and the
+     * remaining contents and size are potentially different) we can still
+     * compare the version numbers.
+     */
+    int32_t version;
+    int32_t width;
+    int32_t height;
+    int32_t alignmentPadding;
+};
+
+class SkCanvasState_v1 : public SkCanvasState {
+public:
+    SkCanvasState_v1(SkCanvas* canvas)
+    : INHERITED(canvas)
+    {
+        SkASSERT(this->version == 1);
         layerCount = 0;
         layers = NULL;
-        originalCanvas = SkRef(canvas);
-
         mcState.clipRectCount = 0;
         mcState.clipRects = NULL;
+        originalCanvas = SkRef(canvas);
     }
 
-    ~SkCanvasState() {
+    ~SkCanvasState_v1() {
         // loop through the layers and free the data allocated to the clipRects
         for (int i = 0; i < layerCount; ++i) {
             sk_free(layers[i].mcState.clipRects);
@@ -98,24 +118,13 @@ public:
         originalCanvas->unref();
     }
 
-    /**
-     * The version this struct was built with.  This field must always appear
-     * first in the struct so that when the versions don't match (and the
-     * remaining contents and size are potentially different) we can still
-     * compare the version numbers.
-     */
-    int32_t version;
-
-    int32_t width;
-    int32_t height;
-
     SkMCState mcState;
 
     int32_t layerCount;
     SkCanvasLayerState* layers;
-
 private:
     SkCanvas* originalCanvas;
+    typedef SkCanvasState INHERITED;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -191,7 +200,7 @@ SkCanvasState* SkCanvasStateUtils::CaptureCanvasState(SkCanvas* canvas) {
         return NULL;
     }
 
-    SkAutoTDelete<SkCanvasState> canvasState(SkNEW_ARGS(SkCanvasState, (canvas)));
+    SkAutoTDelete<SkCanvasState_v1> canvasState(SkNEW_ARGS(SkCanvasState_v1, (canvas)));
 
     // decompose the total matrix and clip
     setup_MC_state(&canvasState->mcState, canvas->getTotalMatrix(),
@@ -247,7 +256,7 @@ SkCanvasState* SkCanvasStateUtils::CaptureCanvasState(SkCanvas* canvas) {
 
     // for now, just ignore any client supplied DrawFilter.
     if (canvas->getDrawFilter()) {
-//        SkDEBUGF(("CaptureCanvasState will ignore the canvases draw filter.\n"));
+//        SkDEBUGF(("CaptureCanvasState will ignore the canvas's draw filter.\n"));
     }
 
     return canvasState.detach();
@@ -306,30 +315,27 @@ static SkCanvas* create_canvas_from_canvas_layer(const SkCanvasLayerState& layer
 
 SkCanvas* SkCanvasStateUtils::CreateFromCanvasState(const SkCanvasState* state) {
     SkASSERT(state);
+    SkASSERT(CANVAS_STATE_VERSION == state->version);

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

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]

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.

 
-    // check that the versions match
-    if (CANVAS_STATE_VERSION != state->version) {
-        SkDebugf("CreateFromCanvasState version does not match the one use to create the input");
-        return NULL;
-    }
+    const SkCanvasState_v1* state_v1 = static_cast<const SkCanvasState_v1*>(state);
 
-    if (state->layerCount < 1) {
+    if (state_v1->layerCount < 1) {
         return NULL;
     }
 
     SkAutoTUnref<SkCanvasStack> canvas(SkNEW_ARGS(SkCanvasStack, (state->width, state->height)));
 
     // setup the matrix and clip on the n-way canvas
-    setup_canvas_from_MC_state(state->mcState, canvas);
+    setup_canvas_from_MC_state(state_v1->mcState, canvas);
 
     // Iterate over the layers and add them to the n-way canvas
-    for (int i = state->layerCount - 1; i >= 0; --i) {
-        SkAutoTUnref<SkCanvas> canvasLayer(create_canvas_from_canvas_layer(state->layers[i]));
+    for (int i = state_v1->layerCount - 1; i >= 0; --i) {
+        SkAutoTUnref<SkCanvas> canvasLayer(create_canvas_from_canvas_layer(state_v1->layers[i]));
         if (!canvasLayer.get()) {
             return NULL;
         }
-        canvas->pushCanvas(canvasLayer.get(), SkIPoint::Make(state->layers[i].x,
-                                                             state->layers[i].y));
+        canvas->pushCanvas(canvasLayer.get(), SkIPoint::Make(state_v1->layers[i].x,
+                                                             state_v1->layers[i].y));
     }
 
     return canvas.detach();
@@ -338,5 +344,9 @@ SkCanvas* SkCanvasStateUtils::CreateFromCanvasState(const SkCanvasState* state)
 ////////////////////////////////////////////////////////////////////////////////
 
 void SkCanvasStateUtils::ReleaseCanvasState(SkCanvasState* state) {
-    SkDELETE(state);
+    SkASSERT(CANVAS_STATE_VERSION == state->version);
+    // Upcast to the correct version of SkCanvasState. This avoids having a virtual destructor on
+    // SkCanvasState. That would be strange since SkCanvasState has no other virtual functions, and
+    // instead uses the field "version" to determine how to behave.
+    SkDELETE(static_cast<SkCanvasState_v1*>(state));
 }