Reland "Fix genID cloning bugs."

tests/BitmapCopyTest.cpp

Index: tests/BitmapCopyTest.cpp
diff --git a/tests/BitmapCopyTest.cpp b/tests/BitmapCopyTest.cpp
index 455fc691e3ec004a3c8433d6eb095b37b68559b9..7cad730460a2bd934de63efbd2849d574db04676 100644
--- a/tests/BitmapCopyTest.cpp
+++ b/tests/BitmapCopyTest.cpp
@@ -1,4 +1,3 @@
-
 /*
  * Copyright 2011 Google Inc.
  *
@@ -223,26 +222,78 @@ DEF_TEST(BitmapCopy, reporter) {
     const int H = 33;
 
     for (size_t i = 0; i < SK_ARRAY_COUNT(gPairs); i++) {
-        for (size_t j = 0; j < SK_ARRAY_COUNT(gPairs); j++) {
-            SkBitmap srcOpaque, srcPremul, dst;
+        SkBitmap srcOpaque, srcPremul;
+        {
+            SkColorTable* ctOpaque = NULL;
+            SkColorTable* ctPremul = NULL;
+
+            srcOpaque.setConfig(gPairs[i].fConfig, W, H, 0, kOpaque_SkAlphaType);
+            srcPremul.setConfig(gPairs[i].fConfig, W, H, 0, kPremul_SkAlphaType);
+            if (SkBitmap::kIndex8_Config == gPairs[i].fConfig) {
+                ctOpaque = init_ctable(kOpaque_SkAlphaType);
+                ctPremul = init_ctable(kPremul_SkAlphaType);
+            }
+            srcOpaque.allocPixels(ctOpaque);
+            srcPremul.allocPixels(ctPremul);
+            SkSafeUnref(ctOpaque);
+            SkSafeUnref(ctPremul);
+        }
+        init_src(srcOpaque);
+        init_src(srcPremul);
+
+        // test extractSubset

[mtklein, 2014/01/09 15:58:07]

A block like this makes me think you might want to extract a new
DEF_TEST(BitmapCopy_extractSubset, reporter) { ... }

[scroggo, 2014/01/09 20:45:23]

Why so, for clarity? Or is there an advantage I'm not seeing? To me it makes
sense here because it shares some of the same code (setting up the bitmaps,
iterating through the configs) and it is testing copying the extracted subset,
but I could go either way.

[mtklein, 2014/01/09 21:29:18]

Yeah, clarity.  It's nearly-independent from the code below and it's a feature
that could fail independently of the other things we test below it.  If nothing
else, it'd help readability of that code below, in that we wouldn't have to
think about anything in the extractSubset block setting up state or
preconditions the test depends on.  Same reason we break up code into separate
functions even if they're only called from one place.

That shared setup code itself could be pulled out as a function too:
  static void setup_src_bitmaps(SkBitmap* opaque, SkBitmap* premul) { ... }

My rule of thumb is that, nearly any time you write a freestanding {} block and
it doesn't involve some sort of RAII scope trick, it wants to be its own
function, especially if it's got a comment like // test extractSubset above it. 
It's a major  step up in readability if you can get your functions to fit
entirely within a reasonable screen height, and I find it helps me get a better
handle on data dependencies too when I'm forced to think about which functions
need what.

[scroggo, 2014/01/09 21:47:29]

Thanks for the justification - you have me convinced! I have moved extractSubset
to its own test.

+        {
+            SkBitmap bitmap(srcOpaque);
+            SkBitmap subset;
+            SkIRect r;
+            // Extract a subset which has the same width as the original. This
+            // catches a bug where we cloned the genID incorrectly.
+            r.set(0, 1, W, 3);
+            bitmap.setIsVolatile(true);
+            if (bitmap.extractSubset(&subset, r)) {
+                REPORTER_ASSERT(reporter, subset.width() == W);
+                REPORTER_ASSERT(reporter, subset.height() == 2);
+                REPORTER_ASSERT(reporter, subset.alphaType() == bitmap.alphaType());
+                REPORTER_ASSERT(reporter, subset.isVolatile() == true);
+
+                // Test copying an extracted subset.
+                for (size_t j = 0; j < SK_ARRAY_COUNT(gPairs); j++) {
+                    SkBitmap copy;
+                    bool success = subset.copyTo(&copy, gPairs[j].fConfig);
+                    if (!success) {
+                        // Skip checking that success matches fValid, which is redundant
+                        // with the code below.
+                        REPORTER_ASSERT(reporter, gPairs[i].fConfig != gPairs[j].fConfig);
+                        continue;
+                    }
+
+                    // When performing a copy of an extracted subset, the gen id should
+                    // change.
+                    REPORTER_ASSERT(reporter, copy.getGenerationID() != subset.getGenerationID());
 
-            {
-                SkColorTable* ctOpaque = NULL;
-                SkColorTable* ctPremul = NULL;
+                    REPORTER_ASSERT(reporter, copy.width() == W);
+                    REPORTER_ASSERT(reporter, copy.height() == 2);
 
-                srcOpaque.setConfig(gPairs[i].fConfig, W, H, 0, kOpaque_SkAlphaType);
-                srcPremul.setConfig(gPairs[i].fConfig, W, H, 0, kPremul_SkAlphaType);
-                if (SkBitmap::kIndex8_Config == gPairs[i].fConfig) {
-                    ctOpaque = init_ctable(kOpaque_SkAlphaType);
-                    ctPremul = init_ctable(kPremul_SkAlphaType);
+                    if (gPairs[i].fConfig == gPairs[j].fConfig) {
+                        SkAutoLockPixels alp0(subset);
+                        SkAutoLockPixels alp1(copy);
+                        // they should both have, or both not-have, a colortable
+                        bool hasCT = subset.getColorTable() != NULL;
+                        REPORTER_ASSERT(reporter, (copy.getColorTable() != NULL) == hasCT);
+                    }
                 }
-                srcOpaque.allocPixels(ctOpaque);
-                srcPremul.allocPixels(ctPremul);
-                SkSafeUnref(ctOpaque);
-                SkSafeUnref(ctPremul);
             }
-            init_src(srcOpaque);
-            init_src(srcPremul);
+
+            bitmap = srcPremul;
+            bitmap.setIsVolatile(false);
+            if (bitmap.extractSubset(&subset, r)) {
+                REPORTER_ASSERT(reporter, subset.alphaType() == bitmap.alphaType());
+                REPORTER_ASSERT(reporter, subset.isVolatile() == false);
+            }
+        }
+
+        for (size_t j = 0; j < SK_ARRAY_COUNT(gPairs); j++) {
+            SkBitmap dst;
 
             bool success = srcPremul.copyTo(&dst, gPairs[j].fConfig);
             bool expected = gPairs[i].fValid[j] != '0';
@@ -282,44 +333,6 @@ DEF_TEST(BitmapCopy, reporter) {
                 } else {
                     REPORTER_ASSERT(reporter, srcPremul.getGenerationID() != dst.getGenerationID());
                 }
-                // test extractSubset
-                {
-                    SkBitmap bitmap(srcOpaque);
-                    SkBitmap subset;
-                    SkIRect r;
-                    r.set(1, 1, 2, 2);
-                    bitmap.setIsVolatile(true);
-                    if (bitmap.extractSubset(&subset, r)) {
-                        REPORTER_ASSERT(reporter, subset.width() == 1);
-                        REPORTER_ASSERT(reporter, subset.height() == 1);
-                        REPORTER_ASSERT(reporter,
-                                        subset.alphaType() == bitmap.alphaType());
-                        REPORTER_ASSERT(reporter,
-                                        subset.isVolatile() == true);
-
-                        SkBitmap copy;
-                        REPORTER_ASSERT(reporter,
-                                        subset.copyTo(&copy, subset.config()));
-                        REPORTER_ASSERT(reporter, copy.width() == 1);
-                        REPORTER_ASSERT(reporter, copy.height() == 1);
-                        REPORTER_ASSERT(reporter, copy.rowBytes() <= 4);
-
-                        SkAutoLockPixels alp0(subset);
-                        SkAutoLockPixels alp1(copy);
-                        // they should both have, or both not-have, a colortable
-                        bool hasCT = subset.getColorTable() != NULL;
-                        REPORTER_ASSERT(reporter,
-                                    (copy.getColorTable() != NULL) == hasCT);
-                    }
-                    bitmap = srcPremul;
-                    bitmap.setIsVolatile(false);
-                    if (bitmap.extractSubset(&subset, r)) {
-                        REPORTER_ASSERT(reporter,
-                                        subset.alphaType() == bitmap.alphaType());
-                        REPORTER_ASSERT(reporter,
-                                        subset.isVolatile() == false);
-                    }
-                }
             } else {
                 // dst should be unchanged from its initial state
                 REPORTER_ASSERT(reporter, dst.config() == SkBitmap::kNo_Config);