DM: Add --skps.

DM: Add --skps.

This does render_pictures, plus checks SkRecord optimizations.

Disable an SkRecord optimization that draws several bot SKPs wrong.  (To be investigated.)

BUG=skia:2378
Committed: http://code.google.com/p/skia/source/detail?r=14739

[mtklein, 2014-05-14 00:33:49]

I figure I'd better force you to review one of these DM CLs eventually.

[reed1, 2014-05-14 12:40:10]

seems like a great addition. lgtm. lots of optional nits

https://codereview.chromium.org/270543004/diff/20001/dm/DMRecordTask.cpp
File dm/DMRecordTask.cpp (right):

https://codereview.chromium.org/270543004/diff/20001/dm/DMRecordTask.cpp#newc...
dm/DMRecordTask.cpp:22: RecordTask::RecordTask(const Task& parent, SkPicture*
pic, SkBitmap reference, bool optimize)
nit: the call-sites are slightly harder to understand with that bool parameter.
e.g. an enum { kOptimize, kRaw } might be clearer.

https://codereview.chromium.org/270543004/diff/20001/dm/DMRecordTask.h
File dm/DMRecordTask.h (right):

https://codereview.chromium.org/270543004/diff/20001/dm/DMRecordTask.h#newcode18
dm/DMRecordTask.h:18: RecordTask(const Task& parent, skiagm::GM*, SkBitmap
reference, bool optimize);
nit: why isn't this parameter const SkBitmap& ?

https://codereview.chromium.org/270543004/diff/20001/dm/DMSKPTask.h
File dm/DMSKPTask.h (right):

https://codereview.chromium.org/270543004/diff/20001/dm/DMSKPTask.h#newcode20
dm/DMSKPTask.h:20: bool shouldSkip() const SK_OVERRIDE { return false; }
nit: skia's convention has been to also add 'virtual' to overrides

https://codereview.chromium.org/270543004/diff/20001/dm/DMUtil.h
File dm/DMUtil.h (right):

https://codereview.chromium.org/270543004/diff/20001/dm/DMUtil.h#newcode26
dm/DMUtil.h:26: void SetupBitmap(const SkColorType, SkBenchmark* bench,
SkBitmap* bitmap);
nit: using 'const' on POD parameters seems funny, as it (to me) speaks to the
impl but not to the caller.

[mtklein, 2014-05-14 17:47:18]

https://codereview.chromium.org/270543004/diff/20001/dm/DMRecordTask.cpp
File dm/DMRecordTask.cpp (right):

https://codereview.chromium.org/270543004/diff/20001/dm/DMRecordTask.cpp#newc...
dm/DMRecordTask.cpp:22: RecordTask::RecordTask(const Task& parent, SkPicture*
pic, SkBitmap reference, bool optimize)
On 2014/05/14 12:40:11, reed1 wrote:
> nit: the call-sites are slightly harder to understand with that bool
parameter.
> e.g. an enum { kOptimize, kRaw } might be clearer.

Done, for all the existing tasks too.

https://codereview.chromium.org/270543004/diff/20001/dm/DMRecordTask.h
File dm/DMRecordTask.h (right):

https://codereview.chromium.org/270543004/diff/20001/dm/DMRecordTask.h#newcode18
dm/DMRecordTask.h:18: RecordTask(const Task& parent, skiagm::GM*, SkBitmap
reference, bool optimize);
On 2014/05/14 12:40:11, reed1 wrote:
> nit: why isn't this parameter const SkBitmap& ?

It could be, but we've got to make the copy either way because we could be
crossing threads.  That is, it can't safely be const SkBitmap& fReference below.

It seemed clearer to remind the caller that the SkBitmap would be copied in the
method signature.  Because the constructor initializes fReference in the
intializer list, the copy only happens once, so it's the same cost as passing by
reference.

https://codereview.chromium.org/270543004/diff/20001/dm/DMSKPTask.h
File dm/DMSKPTask.h (right):

https://codereview.chromium.org/270543004/diff/20001/dm/DMSKPTask.h#newcode20
dm/DMSKPTask.h:20: bool shouldSkip() const SK_OVERRIDE { return false; }
On 2014/05/14 12:40:11, reed1 wrote:
> nit: skia's convention has been to also add 'virtual' to overrides

Done.

https://codereview.chromium.org/270543004/diff/20001/dm/DMUtil.h
File dm/DMUtil.h (right):

https://codereview.chromium.org/270543004/diff/20001/dm/DMUtil.h#newcode26
dm/DMUtil.h:26: void SetupBitmap(const SkColorType, SkBenchmark* bench,
SkBitmap* bitmap);
On 2014/05/14 12:40:11, reed1 wrote:
> nit: using 'const' on POD parameters seems funny, as it (to me) speaks to the
> impl but not to the caller.

Yeah, don't know how that got there in the first place.  Done.

[mtklein, 2014-05-14 17:47:24]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-05-14 17:47:33]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/270543004/40001

[commit-bot: I haz the power, 2014-05-14 17:56:10]

Change committed as 14739