DM: add --rtree.

dm/DMReplayTask.cpp

Index: dm/DMReplayTask.cpp
diff --git a/dm/DMReplayTask.cpp b/dm/DMReplayTask.cpp
index a0ecf5fadf7812e282b1a8a7b745a698204fda77..177d8760cd6c66eb4cebd695477b31e51cb9736f 100644
--- a/dm/DMReplayTask.cpp
+++ b/dm/DMReplayTask.cpp
@@ -6,21 +6,35 @@
 #include "SkPicture.h"
 
 DEFINE_bool(replay, false, "If true, run picture replay tests.");
+DEFINE_bool(rtree,  false, "If true, run picture replay tests with an rtree.");
 
 namespace DM {
 
+static const char* kReplay = "replay";
+static const char* kRTree  = "rtree";
+
+static const char* flags_name(uint32_t flags) {
+    switch (flags) {
+        case 0:                                                    return kReplay;
+        case SkPicture::kOptimizeForClippedPlayback_RecordingFlag: return kRTree;
+        default:                                                   return "hey-give-me-a-name";

[epoger, 2013/10/30 19:37:01]

maybe SkASSERT failure in this case?

+    }
+}
+
 ReplayTask::ReplayTask(const Task& parent,
                        skiagm::GM* gm,
-                       SkBitmap reference)
+                       SkBitmap reference,
+                       uint32_t recordFlags)
     : Task(parent)
-    , fName(UnderJoin(parent.name().c_str(), "replay"))
+    , fName(UnderJoin(parent.name().c_str(), flags_name(recordFlags)))
     , fGM(gm)
     , fReference(reference)
+    , fRecordFlags(recordFlags)
     {}
 
 void ReplayTask::draw() {
     SkPicture recorded;
-    RecordPicture(fGM.get(), &recorded);
+    RecordPicture(fGM.get(), &recorded, fRecordFlags);
 
     SkBitmap bitmap;
     SetupBitmap(fReference.config(), fGM.get(), &bitmap);
@@ -32,7 +46,17 @@ void ReplayTask::draw() {
 }
 
 bool ReplayTask::shouldSkip() const {
-    return !FLAGS_replay || fGM->getFlags() & skiagm::GM::kSkipPicture_Flag;
+    if (fGM->getFlags() & skiagm::GM::kSkipPicture_Flag) {
+        return true;
+    }
+
+    if (FLAGS_rtree && flags_name(fRecordFlags) == kRTree) {

[epoger, 2013/10/30 19:37:01]

It seems very strange to me that we change code paths based on the
*human-readable name* of something.  I understand that the performance is fine,
but IMO it's counterintuitive in C++ (would be more at home in Python).

I think using an enum or boolean would be cleaner, for this and other reasons
(see my other comments).

+        return false;

[epoger, 2013/10/30 19:37:01]

Maybe it's just me, but I would find it easier to follow if we computed
shouldRun() instead of this inverted shouldSkip():

bool shouldRun() {
  if (fGM->getFlags() & skiagm::GM::kSkipPicture_Flag) {
    return false;
  }

  // The following would have to use an enum rather than string descriptions
  switch (flags_name(fRecordFlags)) {
    case kRTree:
      return FLAGS_rtree;
    case kReplay:
      return FLAGS_replay;
    default:
      // SkASSERT failure about unknown flag
      return false;
  }
}

+    }
+    if (FLAGS_replay && flags_name(fRecordFlags) == kReplay) {

[epoger, 2013/10/30 19:37:01]

Here and above, please put in more parentheses.  No harm in being explicit...

if (FLAGS_rtree && (flags_name(fRecordFlags) == kRTree)) {

+        return false;
+    }
+    return true;
 }
 
 }  // namespace DM