DM: add --rtree.

DM: add --rtree.

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=12033

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

Several comments, all in the same vein.

https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp
File dm/DMReplayTask.cpp (right):

https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp#newco...
dm/DMReplayTask.cpp:20: default:                                                
  return "hey-give-me-a-name";
maybe SkASSERT failure in this case?

https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp#newco...
dm/DMReplayTask.cpp:53: if (FLAGS_rtree && flags_name(fRecordFlags) == kRTree) {
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).

https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp#newco...
dm/DMReplayTask.cpp:54: return false;
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;
  }
}

https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp#newco...
dm/DMReplayTask.cpp:56: if (FLAGS_replay && flags_name(fRecordFlags) == kReplay)
{
Here and above, please put in more parentheses.  No harm in being explicit...

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

https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.h
File dm/DMReplayTask.h (right):

https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.h#newcode20
dm/DMReplayTask.h:20: uint32_t recordFlags);  // Flags to pass to
SkPicture::beginRecording().
AFAICT, there are exactly two allowed values to pass in recordFlags: 0 and
SkPicture::kOptimizeForClippedPlayback_RecordingFlag

I think developers will be rightfully confused.  "So, I can pass in any single
value in the SkPicture::RecordingFlags enum?  Or a bitmask of those values?"

https://codereview.chromium.org/51243003/diff/30001/dm/DMUtil.h
File dm/DMUtil.h (right):

https://codereview.chromium.org/51243003/diff/30001/dm/DMUtil.h#newcode21
dm/DMUtil.h:21: // Draw gm to picture.
please document parameters.  How is the caller supposed to know what values are
valid to pass into recordFlags?

(I think THIS one allows any bitmask of values from the
SkPicture::RecordingFlags enum, unlike the one in DMReplayTask.h.  But I'm not
sure, and it's confusing.)

[mtklein, 2013-10-30 19:54:06]

On 2013/10/30 19:37:01, epoger wrote:
> Several comments, all in the same vein.
> 
> https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp
> File dm/DMReplayTask.cpp (right):
> 
>
https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp#newco...
> dm/DMReplayTask.cpp:20: default:                                              
 
>   return "hey-give-me-a-name";
> maybe SkASSERT failure in this case?
> 
>
https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp#newco...
> dm/DMReplayTask.cpp:53: if (FLAGS_rtree && flags_name(fRecordFlags) == kRTree)
{
> 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).
> 
>
https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp#newco...
> dm/DMReplayTask.cpp:54: return false;
> 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;
>   }
> }
> 
>
https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.cpp#newco...
> dm/DMReplayTask.cpp:56: if (FLAGS_replay && flags_name(fRecordFlags) ==
kReplay)
> {
> Here and above, please put in more parentheses.  No harm in being explicit...
> 
> if (FLAGS_rtree && (flags_name(fRecordFlags) == kRTree)) {
> 
> https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.h
> File dm/DMReplayTask.h (right):
> 
>
https://codereview.chromium.org/51243003/diff/30001/dm/DMReplayTask.h#newcode20
> dm/DMReplayTask.h:20: uint32_t recordFlags);  // Flags to pass to
> SkPicture::beginRecording().
> AFAICT, there are exactly two allowed values to pass in recordFlags: 0 and
> SkPicture::kOptimizeForClippedPlayback_RecordingFlag
> 
> I think developers will be rightfully confused.  "So, I can pass in any single
> value in the SkPicture::RecordingFlags enum?  Or a bitmask of those values?"
> 
> https://codereview.chromium.org/51243003/diff/30001/dm/DMUtil.h
> File dm/DMUtil.h (right):
> 
> https://codereview.chromium.org/51243003/diff/30001/dm/DMUtil.h#newcode21
> dm/DMUtil.h:21: // Draw gm to picture.
> please document parameters.  How is the caller supposed to know what values
are
> valid to pass into recordFlags?
> 
> (I think THIS one allows any bitmask of values from the
> SkPicture::RecordingFlags enum, unlike the one in DMReplayTask.h.  But I'm not
> sure, and it's confusing.)

Have another look?

[epoger, 2013-10-30 20:04:23]

lgtm

[commit-bot: I haz the power, 2013-10-30 20:04:45]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/51243003/90001

[commit-bot: I haz the power, 2013-10-30 20:45:31]

Change committed as 12033