Defer saves() until they're needed

patch from issue 759443006 at patchset 40001 (http://crrev.com/759443006#ps40001)

BUG=skia:

Review URL: https://codereview.chromium.org/767333002

6 files changed, 136 insertions, 124 deletions

diff --git a/tests/PictureBBHTest.cpp b/tests/PictureBBHTest.cpp
index bc712f575e..562d9b5991 100644
--- a/tests/PictureBBHTest.cpp
+++ b/tests/PictureBBHTest.cpp
@@ -119,7 +119,8 @@ static void test_clear(skiatest::Reporter* r, SkBBHFactory* factory) {
 
     // Should be Clip - Save - Clear - Restore.
     // Buggy implementations might return 1 (just Clip) or 3 (Clip - Save - Restore).
-    REPORTER_ASSERT(r, dstPic->approximateOpCount() == 4);
+    // TODO: can we just search that it contains "clear"? <reed>
+    REPORTER_ASSERT(r, dstPic->approximateOpCount() == 4 || dstPic->approximateOpCount() == 2);
 }
 
 DEF_TEST(PictureBBH_Clear, r) {
diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp
index 2d9e90d48b..e830af4b43 100644
--- a/tests/RecordDrawTest.cpp
+++ b/tests/RecordDrawTest.cpp
@@ -30,6 +30,33 @@ private:
     int fCalls;
 };
 
+DEF_TEST(RecordDraw_LazySaves, r) {
+    // Record two commands.
+    SkRecord record;
+    SkRecorder recorder(&record, W, H);
+
+    REPORTER_ASSERT(r, 0 == record.count());
+    recorder.save();
+    REPORTER_ASSERT(r, 0 == record.count());    // the save was not recorded (yet)
+    recorder.drawColor(SK_ColorRED);
+    REPORTER_ASSERT(r, 1 == record.count());
+    recorder.scale(2, 2);
+    REPORTER_ASSERT(r, 3 == record.count());    // now we see the save
+    recorder.restore();
+    REPORTER_ASSERT(r, 4 == record.count());
+
+    assert_type<SkRecords::DrawPaint>(r, record, 0);
+    assert_type<SkRecords::Save>     (r, record, 1);
+    assert_type<SkRecords::SetMatrix>(r, record, 2);
+    assert_type<SkRecords::Restore>  (r, record, 3);
+
+    recorder.save();
+    recorder.save();
+    recorder.restore();
+    recorder.restore();
+    REPORTER_ASSERT(r, 4 == record.count());
+}
+
 DEF_TEST(RecordDraw_Abort, r) {
     // Record two commands.
     SkRecord record;
@@ -43,26 +70,23 @@ DEF_TEST(RecordDraw_Abort, r) {
     JustOneDraw callback;
     SkRecordDraw(record, &canvas, NULL, NULL, 0, NULL/*bbh*/, &callback);
 
-    REPORTER_ASSERT(r, 3 == rerecord.count());
-    assert_type<SkRecords::Save>    (r, rerecord, 0);
-    assert_type<SkRecords::DrawRect>(r, rerecord, 1);
-    assert_type<SkRecords::Restore> (r, rerecord, 2);
+    REPORTER_ASSERT(r, 1 == count_instances_of_type<SkRecords::DrawRect>(rerecord));
+    REPORTER_ASSERT(r, 0 == count_instances_of_type<SkRecords::ClipRect>(rerecord));
 }
 
 DEF_TEST(RecordDraw_Unbalanced, r) {
     SkRecord record;
     SkRecorder recorder(&record, W, H);
     recorder.save();  // We won't balance this, but SkRecordDraw will for us.
+    recorder.scale(2, 2);
 
     SkRecord rerecord;
     SkRecorder canvas(&rerecord, W, H);
     SkRecordDraw(record, &canvas, NULL, NULL, 0, NULL/*bbh*/, NULL/*callback*/);
 
-    REPORTER_ASSERT(r, 4 == rerecord.count());
-    assert_type<SkRecords::Save>    (r, rerecord, 0);
-    assert_type<SkRecords::Save>    (r, rerecord, 1);
-    assert_type<SkRecords::Restore> (r, rerecord, 2);
-    assert_type<SkRecords::Restore> (r, rerecord, 3);
+    int save_count = count_instances_of_type<SkRecords::Save>(rerecord);
+    int restore_count = count_instances_of_type<SkRecords::Save>(rerecord);
+    REPORTER_ASSERT(r, save_count == restore_count);
 }
 
 DEF_TEST(RecordDraw_SetMatrixClobber, r) {
@@ -193,12 +217,9 @@ DEF_TEST(RecordDraw_PartialStartStop, r) {
     SkRecorder canvas(&rerecord, kWidth, kHeight);
     SkRecordPartialDraw(record, &canvas, NULL, 0, 1, 2, SkMatrix::I()); // replay just drawRect of r2
 
-    REPORTER_ASSERT(r, 3 == rerecord.count());
-    assert_type<SkRecords::Save>     (r, rerecord, 0);
-    assert_type<SkRecords::DrawRect> (r, rerecord, 1);
-    assert_type<SkRecords::Restore>  (r, rerecord, 2);
-
-    const SkRecords::DrawRect* drawRect = assert_type<SkRecords::DrawRect>(r, rerecord, 1);
+    REPORTER_ASSERT(r, 1 == count_instances_of_type<SkRecords::DrawRect>(rerecord));
+    int index = find_first_instances_of_type<SkRecords::DrawRect>(rerecord);
+    const SkRecords::DrawRect* drawRect = assert_type<SkRecords::DrawRect>(r, rerecord, index);
     REPORTER_ASSERT(r, drawRect->rect == r2);
 }
 
diff --git a/tests/RecordOptsTest.cpp b/tests/RecordOptsTest.cpp
index c5c4471d67..cdc0350e53 100644
--- a/tests/RecordOptsTest.cpp
+++ b/tests/RecordOptsTest.cpp
@@ -16,26 +16,19 @@
 
 static const int W = 1920, H = 1080;
 
-DEF_TEST(RecordOpts_NoopDrawSaveRestore, r) {
+DEF_TEST(RecordOpts_NoopDraw, r) {
     SkRecord record;
     SkRecorder recorder(&record, W, H);
 
-    // The save and restore are pointless if there's only draw commands in the middle.
-    recorder.save();
-        recorder.drawRect(SkRect::MakeWH(200, 200), SkPaint());
-        recorder.drawRect(SkRect::MakeWH(300, 300), SkPaint());
-        recorder.drawRect(SkRect::MakeWH(100, 100), SkPaint());
-    recorder.restore();
+    recorder.drawRect(SkRect::MakeWH(200, 200), SkPaint());
+    recorder.drawRect(SkRect::MakeWH(300, 300), SkPaint());
+    recorder.drawRect(SkRect::MakeWH(100, 100), SkPaint());
 
-    record.replace<SkRecords::NoOp>(2);  // NoOps should be allowed.
+    record.replace<SkRecords::NoOp>(1);  // NoOps should be allowed.
 
     SkRecordNoopSaveRestores(&record);
 
-    assert_type<SkRecords::NoOp>(r, record, 0);
-    assert_type<SkRecords::DrawRect>(r, record, 1);
-    assert_type<SkRecords::NoOp>(r, record, 2);
-    assert_type<SkRecords::DrawRect>(r, record, 3);
-    assert_type<SkRecords::NoOp>(r, record, 4);
+    REPORTER_ASSERT(r, 2 == count_instances_of_type<SkRecords::DrawRect>(record));
 }
 
 DEF_TEST(RecordOpts_SingleNoopSaveRestore, r) {
@@ -70,7 +63,7 @@ DEF_TEST(RecordOpts_NoopSaveRestores, r) {
     recorder.restore();
 
     SkRecordNoopSaveRestores(&record);
-    for (unsigned index = 0; index < 8; index++) {
+    for (unsigned index = 0; index < record.count(); index++) {
         assert_type<SkRecords::NoOp>(r, record, index);
     }
 }
@@ -86,10 +79,22 @@ DEF_TEST(RecordOpts_SaveSaveLayerRestoreRestore, r) {
     recorder.restore();
 
     SkRecordNoopSaveRestores(&record);
-    assert_type<SkRecords::Save>     (r, record, 0);
-    assert_type<SkRecords::SaveLayer>(r, record, 1);
-    assert_type<SkRecords::Restore>  (r, record, 2);
-    assert_type<SkRecords::Restore>  (r, record, 3);
+    switch (record.count()) {
+        case 4:
+            assert_type<SkRecords::Save>     (r, record, 0);
+            assert_type<SkRecords::SaveLayer>(r, record, 1);
+            assert_type<SkRecords::Restore>  (r, record, 2);
+            assert_type<SkRecords::Restore>  (r, record, 3);
+            break;
+        case 2:
+            assert_type<SkRecords::SaveLayer>(r, record, 0);
+            assert_type<SkRecords::Restore>  (r, record, 1);
+            break;
+        case 0:
+            break;
+        default:
+            REPORTER_ASSERT(r, false);
+    }
 }
 
 static void assert_savelayer_restore(skiatest::Reporter* r,
diff --git a/tests/RecordPatternTest.cpp b/tests/RecordPatternTest.cpp
index 5f4d006683..1f5ce2c046 100644
--- a/tests/RecordPatternTest.cpp
+++ b/tests/RecordPatternTest.cpp
@@ -75,56 +75,19 @@ DEF_TEST(RecordPattern_Star, r) {
 
     SkRecord record;
     SkRecorder recorder(&record, 1920, 1200);
-
-    recorder.save();
-    recorder.restore();
-    REPORTER_ASSERT(r, pattern.match(&record, 0));
+    int index = 0;
 
     recorder.save();
         recorder.clipRect(SkRect::MakeWH(300, 200));
     recorder.restore();
-    REPORTER_ASSERT(r, pattern.match(&record, 2));
+    REPORTER_ASSERT(r, pattern.match(&record, index));
+    index += 3;
 
     recorder.save();
         recorder.clipRect(SkRect::MakeWH(300, 200));
         recorder.clipRect(SkRect::MakeWH(100, 100));
     recorder.restore();
-    REPORTER_ASSERT(r, pattern.match(&record, 5));
-}
-
-DEF_TEST(RecordPattern_IsDraw, r) {
-    Pattern3<Is<Save>, IsDraw, Is<Restore> > pattern;
-
-    SkRecord record;
-    SkRecorder recorder(&record, 1920, 1200);
-
-    recorder.save();
-        recorder.clipRect(SkRect::MakeWH(300, 200));
-    recorder.restore();
-
-    REPORTER_ASSERT(r, !pattern.match(&record, 0));
-
-    SkPaint paint;
-
-    recorder.save();
-        paint.setColor(0xEEAA8822);
-        recorder.drawRect(SkRect::MakeWH(300, 200), paint);
-    recorder.restore();
-
-    recorder.save();
-        paint.setColor(0xFACEFACE);
-        recorder.drawPaint(paint);
-    recorder.restore();
-
-    REPORTER_ASSERT(r, pattern.match(&record, 3));
-    REPORTER_ASSERT(r, pattern.first<Save>()    != NULL);
-    REPORTER_ASSERT(r, pattern.second<SkPaint>()->getColor() == 0xEEAA8822);
-    REPORTER_ASSERT(r, pattern.third<Restore>() != NULL);
-
-    REPORTER_ASSERT(r, pattern.match(&record, 6));
-    REPORTER_ASSERT(r, pattern.first<Save>()    != NULL);
-    REPORTER_ASSERT(r, pattern.second<SkPaint>()->getColor() == 0xFACEFACE);
-    REPORTER_ASSERT(r, pattern.third<Restore>() != NULL);
+    REPORTER_ASSERT(r, pattern.match(&record, index));
 }
 
 DEF_TEST(RecordPattern_Complex, r) {
@@ -136,54 +99,39 @@ DEF_TEST(RecordPattern_Complex, r) {
 
     SkRecord record;
     SkRecorder recorder(&record, 1920, 1200);
+    unsigned start, begin, end;
 
-    recorder.save();
-    recorder.restore();
-    REPORTER_ASSERT(r, pattern.match(&record, 0) == 2);
-
-    recorder.save();
-        recorder.save();
-        recorder.restore();
-    recorder.restore();
-    REPORTER_ASSERT(r, !pattern.match(&record, 2));
-    REPORTER_ASSERT(r, pattern.match(&record, 3) == 5);
-
+    start = record.count();
     recorder.save();
         recorder.clipRect(SkRect::MakeWH(300, 200));
     recorder.restore();
-    REPORTER_ASSERT(r, pattern.match(&record, 6) == 9);
+    REPORTER_ASSERT(r, pattern.match(&record, 0) == record.count());
+    end = start;
+    REPORTER_ASSERT(r, pattern.search(&record, &begin, &end));
+    REPORTER_ASSERT(r, begin == start);
+    REPORTER_ASSERT(r, end == record.count());
 
+    start = record.count();
     recorder.save();
         recorder.clipRect(SkRect::MakeWH(300, 200));
         recorder.drawRect(SkRect::MakeWH(100, 3000), SkPaint());
     recorder.restore();
-    REPORTER_ASSERT(r, !pattern.match(&record, 9));
+    REPORTER_ASSERT(r, !pattern.match(&record, start));
+    end = start;
+    REPORTER_ASSERT(r, !pattern.search(&record, &begin, &end));
 
+    start = record.count();
     recorder.save();
         recorder.pushCull(SkRect::MakeWH(300, 200));
         recorder.clipRect(SkRect::MakeWH(300, 200));
         recorder.clipRect(SkRect::MakeWH(100, 400));
         recorder.popCull();
     recorder.restore();
-    REPORTER_ASSERT(r, pattern.match(&record, 13) == 19);
-
-    // Same as above, but using pattern.search to step through matches.
-    unsigned begin, end = 0;
-    REPORTER_ASSERT(r, pattern.search(&record, &begin, &end));
-    REPORTER_ASSERT(r, begin == 0);
-    REPORTER_ASSERT(r, end == 2);
-
-    REPORTER_ASSERT(r, pattern.search(&record, &begin, &end));
-    REPORTER_ASSERT(r, begin == 3);
-    REPORTER_ASSERT(r, end == 5);
-
-    REPORTER_ASSERT(r, pattern.search(&record, &begin, &end));
-    REPORTER_ASSERT(r, begin == 6);
-    REPORTER_ASSERT(r, end == 9);
-
+    REPORTER_ASSERT(r, pattern.match(&record, start) == record.count());
+    end = start;
     REPORTER_ASSERT(r, pattern.search(&record, &begin, &end));
-    REPORTER_ASSERT(r, begin == 13);
-    REPORTER_ASSERT(r, end == 19);
+    REPORTER_ASSERT(r, begin == start);
+    REPORTER_ASSERT(r, end == record.count());
 
     REPORTER_ASSERT(r, !pattern.search(&record, &begin, &end));
 }
diff --git a/tests/RecordReplaceDrawTest.cpp b/tests/RecordReplaceDrawTest.cpp
index f1ebf82de0..e0e9466a2e 100644
--- a/tests/RecordReplaceDrawTest.cpp
+++ b/tests/RecordReplaceDrawTest.cpp
@@ -52,10 +52,18 @@ DEF_TEST(RecordReplaceDraw_Abort, r) {
     JustOneDraw callback;
     GrRecordReplaceDraw(pic, &canvas, NULL, SkMatrix::I(), &callback);
 
-    REPORTER_ASSERT(r, 3 == rerecord.count());
-    assert_type<SkRecords::Save>(r, rerecord, 0);
-    assert_type<SkRecords::DrawRect>(r, rerecord, 1);
-    assert_type<SkRecords::Restore>(r, rerecord, 2);
+    switch (rerecord.count()) {
+        case 3:
+            assert_type<SkRecords::Save>(r, rerecord, 0);
+            assert_type<SkRecords::DrawRect>(r, rerecord, 1);
+            assert_type<SkRecords::Restore>(r, rerecord, 2);
+            break;
+        case 1:
+            assert_type<SkRecords::DrawRect>(r, rerecord, 0);
+            break;
+        default:
+            REPORTER_ASSERT(r, false);
+    }
 }
 
 // Make sure GrRecordReplaceDraw balances unbalanced saves
@@ -68,7 +76,7 @@ DEF_TEST(RecordReplaceDraw_Unbalanced, r) {
 
         // We won't balance this, but GrRecordReplaceDraw will for us.
         canvas->save();
-
+        canvas->scale(2, 2);
         pic.reset(recorder.endRecording());
     }
 
@@ -77,11 +85,8 @@ DEF_TEST(RecordReplaceDraw_Unbalanced, r) {
 
     GrRecordReplaceDraw(pic, &canvas, NULL, SkMatrix::I(), NULL/*callback*/);
 
-    REPORTER_ASSERT(r, 4 == rerecord.count());
-    assert_type<SkRecords::Save>(r, rerecord, 0);
-    assert_type<SkRecords::Save>(r, rerecord, 1);
-    assert_type<SkRecords::Restore>(r, rerecord, 2);
-    assert_type<SkRecords::Restore>(r, rerecord, 3);
+    // ensure rerecord is balanced (in this case by checking that the count is even)
+    REPORTER_ASSERT(r, (rerecord.count() & 1) == 0);
 }
 
 // Test out the layer replacement functionality with and w/o a BBH
@@ -127,14 +132,22 @@ void test_replacements(skiatest::Reporter* r, GrContext* context, bool useBBH) {
     SkRecorder canvas(&rerecord, kWidth, kHeight);
     GrRecordReplaceDraw(pic, &canvas, layerCache, SkMatrix::I(), NULL/*callback*/);
 
-    REPORTER_ASSERT(r, 7 == rerecord.count());
-    assert_type<SkRecords::Save>(r, rerecord, 0);
-    assert_type<SkRecords::Save>(r, rerecord, 1);
-    assert_type<SkRecords::SetMatrix>(r, rerecord, 2);
-    assert_type<SkRecords::DrawBitmapRectToRect>(r, rerecord, 3);
-    assert_type<SkRecords::Restore>(r, rerecord, 4);
-    assert_type<SkRecords::DrawRect>(r, rerecord, 5);
-    assert_type<SkRecords::Restore>(r, rerecord, 6);
+    int recount = rerecord.count();
+    REPORTER_ASSERT(r, 5 == recount || 7 == recount);
+
+    int index = 0;
+    if (7 == recount) {
+        assert_type<SkRecords::Save>(r, rerecord, 0);
+        index += 1;
+    }
+    assert_type<SkRecords::Save>(r, rerecord, index + 0);
+    assert_type<SkRecords::SetMatrix>(r, rerecord, index + 1);
+    assert_type<SkRecords::DrawBitmapRectToRect>(r, rerecord, index + 2);
+    assert_type<SkRecords::Restore>(r, rerecord, index + 3);
+    assert_type<SkRecords::DrawRect>(r, rerecord, index + 4);
+    if (7 == recount) {
+        assert_type<SkRecords::Restore>(r, rerecord, 6);
+    }
 }
 
 DEF_GPUTEST(RecordReplaceDraw, r, factory) { 
diff --git a/tests/RecordTestUtils.h b/tests/RecordTestUtils.h
index 0575b83cdd..4bab8e4a39 100644
--- a/tests/RecordTestUtils.h
+++ b/tests/RecordTestUtils.h
@@ -28,4 +28,28 @@ static const T* assert_type(skiatest::Reporter* r, const SkRecord& record, unsig
     return reader.ptr;
 }
 
+template <typename DrawT> struct MatchType {
+    template <typename T> int operator()(const T&) { return 0; }
+    int operator()(const DrawT&) { return 1; }
+};
+
+template <typename DrawT> int count_instances_of_type(const SkRecord& record) {
+    MatchType<DrawT> matcher;
+    int counter = 0;
+    for (unsigned i = 0; i < record.count(); i++) {
+        counter += record.visit<int>(i, matcher);
+    }
+    return counter;
+}
+
+template <typename DrawT> int find_first_instances_of_type(const SkRecord& record) {
+    MatchType<DrawT> matcher;
+    for (unsigned i = 0; i < record.count(); i++) {
+        if (record.visit<int>(i, matcher)) {
+            return i;
+        }
+    }
+    return -1;
+}
+
 #endif//RecordTestUtils_DEFINED