DM: some refactoring

DM: some refactoring
  - rename ComparisonTask to ChecksumTask
  - have ChecksumTask handle all the checksum-checking
  - turn on all extra modes by default
  - simplify progress output to a countdown

BUG=
R=bsalomon@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12398

[epoger, 2013-11-26 17:55:22]

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.cpp
File dm/DMChecksumTask.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.cpp#newcode12
dm/DMChecksumTask.cpp:12: , fBitmap(bitmap)
I take it that copying the SkBitmap struct is pretty cheap?  Otherwise, given
that you only use the digest anyway, it might be better to construct this using
the digest instead.

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.h
File dm/DMChecksumTask.h (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.h#newcode13
dm/DMChecksumTask.h:13: class ChecksumTask : public Task {
Not a big deal either way, but IMO "ComparisonTask" is still more descriptive
(based on the description above, and the arguments passed into the constructor)

https://codereview.chromium.org/88543002/diff/1/dm/DMPipeTask.cpp
File dm/DMPipeTask.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMPipeTask.cpp#newcode9
dm/DMPipeTask.cpp:9: DEFINE_bool(pipe, true, "If true, check several pipe
variants against the reference bitmap.");
If it's not too much trouble, I would recommend separate CLs for refactoring vs
changing behavior.

https://codereview.chromium.org/88543002/diff/1/dm/DMReplayTask.cpp
File dm/DMReplayTask.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMReplayTask.cpp#newcode8
dm/DMReplayTask.cpp:8: DEFINE_bool(replay, true, "If true, run picture replay
tests.");
Ditto

https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp
File dm/DMReporter.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp#newcode15
dm/DMReporter.cpp:15: status.printf("\r\033[K%d tasks left", this->started() -
this->finished());
This user prefers to see "completed X of Y", rather than just "(Y-X) remaining".

https://codereview.chromium.org/88543002/diff/1/dm/DMSerializeTask.cpp
File dm/DMSerializeTask.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMSerializeTask.cpp#newcode9
dm/DMSerializeTask.cpp:9: DEFINE_bool(serialize, true, "If true, run picture
serialization tests.");
better to separate

[bsalomon, 2013-11-26 18:21:52]

lgtm

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.cpp
File dm/DMChecksumTask.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.cpp#newcode12
dm/DMChecksumTask.cpp:12: , fBitmap(bitmap)
On 2013/11/26 17:55:22, epoger wrote:
> I take it that copying the SkBitmap struct is pretty cheap?  Otherwise, given
> that you only use the digest anyway, it might be better to construct this
using
> the digest instead.

bitmap is very shallow.

https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp
File dm/DMReporter.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp#newcode15
dm/DMReporter.cpp:15: status.printf("\r\033[K%d tasks left", this->started() -
this->finished());
On 2013/11/26 17:55:22, epoger wrote:
> This user prefers to see "completed X of Y", rather than just "(Y-X)
remaining".

That is potentially misleading as Y grows in DM.

[mtklein, 2013-11-26 18:25:30]

Thanks for looking guys.

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.cpp
File dm/DMChecksumTask.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.cpp#newcode12
dm/DMChecksumTask.cpp:12: , fBitmap(bitmap)
On 2013/11/26 17:55:22, epoger wrote:
> I take it that copying the SkBitmap struct is pretty cheap?  Otherwise, given
> that you only use the digest anyway, it might be better to construct this
using
> the digest instead.

Yes, copying SkBitmap is trivially cheap.  This class pretty much just exists to
delay calculating the digest until we're certain it's needed (line 20), as
that's the really expensive part.

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.h
File dm/DMChecksumTask.h (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMChecksumTask.h#newcode13
dm/DMChecksumTask.h:13: class ChecksumTask : public Task {
On 2013/11/26 17:55:22, epoger wrote:
> Not a big deal either way, but IMO "ComparisonTask" is still more descriptive
> (based on the description above, and the arguments passed into the
constructor)

Yeah, wanted something to make it distinct from the other bitmap-to-bitmap
comparison tasks (replay, pipe, etc.)

https://codereview.chromium.org/88543002/diff/1/dm/DMPipeTask.cpp
File dm/DMPipeTask.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMPipeTask.cpp#newcode9
dm/DMPipeTask.cpp:9: DEFINE_bool(pipe, true, "If true, check several pipe
variants against the reference bitmap.");
On 2013/11/26 17:55:22, epoger wrote:
> If it's not too much trouble, I would recommend separate CLs for refactoring
vs
> changing behavior.

Yeah, ordinarily I'm with you, but here this isn't really important.  I'm just
getting tired of typing out the flags.

https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp
File dm/DMReporter.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp#newcode15
dm/DMReporter.cpp:15: status.printf("\r\033[K%d tasks left", this->started() -
this->finished());
On 2013/11/26 17:55:22, epoger wrote:
> This user prefers to see "completed X of Y", rather than just "(Y-X)
remaining".

Have you run this or just talking abstractly?  I'd be surprised if you really
preferred X of Y here.  Y increases as the program goes along, sometimes as fast
as X.  Y-X is a much better indicator of time remaining.

[epoger, 2013-11-26 18:34:48]

https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp
File dm/DMReporter.cpp (right):

https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp#newcode15
dm/DMReporter.cpp:15: status.printf("\r\033[K%d tasks left", this->started() -
this->finished());
On 2013/11/26 18:25:30, mtklein wrote:
> On 2013/11/26 17:55:22, epoger wrote:
> > This user prefers to see "completed X of Y", rather than just "(Y-X)
> remaining".
> 
> Have you run this or just talking abstractly?  I'd be surprised if you really
> preferred X of Y here.  Y increases as the program goes along, sometimes as
fast
> as X.  Y-X is a much better indicator of time remaining.

I have not run DM, and it's not a big deal to me either way... but given that Y
grows, it seems *especially* useful to know both X and Y at any given point.

Most users, given X and Y, will be able to calculate Y-X. :-)

At any rate... the more info, the better.

[mtklein, 2013-11-26 18:40:35]

On 2013/11/26 18:34:48, epoger wrote:
> https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp
> File dm/DMReporter.cpp (right):
> 
> https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp#newcode15
> dm/DMReporter.cpp:15: status.printf("\r\033[K%d tasks left", this->started() -
> this->finished());
> On 2013/11/26 18:25:30, mtklein wrote:
> > On 2013/11/26 17:55:22, epoger wrote:
> > > This user prefers to see "completed X of Y", rather than just "(Y-X)
> > remaining".
> > 
> > Have you run this or just talking abstractly?  I'd be surprised if you
really
> > preferred X of Y here.  Y increases as the program goes along, sometimes as
> fast
> > as X.  Y-X is a much better indicator of time remaining.
> 
> I have not run DM, and it's not a big deal to me either way... but given that
Y
> grows, it seems *especially* useful to know both X and Y at any given point.
> 
> Most users, given X and Y, will be able to calculate Y-X. :-)
> 
> At any rate... the more info, the better.

You're going to have to trust us that X of Y is noise compared to Y-X.

[epoger, 2013-11-26 18:43:50]

On 2013/11/26 18:40:35, mtklein wrote:
> On 2013/11/26 18:34:48, epoger wrote:
> > https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp
> > File dm/DMReporter.cpp (right):
> > 
> > https://codereview.chromium.org/88543002/diff/1/dm/DMReporter.cpp#newcode15
> > dm/DMReporter.cpp:15: status.printf("\r\033[K%d tasks left", this->started()
-
> > this->finished());
> > On 2013/11/26 18:25:30, mtklein wrote:
> > > On 2013/11/26 17:55:22, epoger wrote:
> > > > This user prefers to see "completed X of Y", rather than just "(Y-X)
> > > remaining".
> > > 
> > > Have you run this or just talking abstractly?  I'd be surprised if you
> really
> > > preferred X of Y here.  Y increases as the program goes along, sometimes
as
> > fast
> > > as X.  Y-X is a much better indicator of time remaining.
> > 
> > I have not run DM, and it's not a big deal to me either way... but given
that
> Y
> > grows, it seems *especially* useful to know both X and Y at any given point.
> > 
> > Most users, given X and Y, will be able to calculate Y-X. :-)
> > 
> > At any rate... the more info, the better.
> 
> You're going to have to trust us that X of Y is noise compared to Y-X.

"have to" seems like a stretch, but sure, go ahead.

[mtklein, 2013-11-26 18:52:38]

Committed patchset #1 manually as r12398 (presubmit successful).