PictureBenchmark JSON logging

Add JSON logging support to bench_pictures by adding a PictureResultsWriter class (in tools/PictureResultsWriter.h) to process logging information, using a very similar style as bench/ResultsWriter.h

JSON format described in code, above PictureJSONResultsWriter class

BUG=skia:

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

[kelvinly, 2014-05-23 20:32:14]

Main changes are in tools/PictureResultsWriter.h

[jcgregorio, 2014-05-27 15:12:50]

On 2014/05/23 20:32:14, kelvinly wrote:
> Main changes are in tools/PictureResultsWriter.h

CL description needs more details.

[kelvinly, 2014-05-27 15:32:56]

Added JSON description, improved CL description too

[jcgregorio, 2014-05-27 15:34:25]

https://codereview.chromium.org/286903025/diff/30001/bench/ResultsWriter.h
File bench/ResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/30001/bench/ResultsWriter.h#ne...
bench/ResultsWriter.h:150: Json::Value* find_named_node(Json::Value* root, const
char name[]) {
Should be moved to a CPP file. Also I believe the naming convention for externed
static functions is camel caps:


https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...

https://codereview.chromium.org/286903025/diff/30001/tools/PictureResultsWrit...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/30001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:23: Json::Value* find_named_node(Json::Value* root,
const char name[]) {
Why is find_named_node duplicated here?

[kelvinly, 2014-05-27 16:41:06]

https://codereview.chromium.org/286903025/diff/30001/bench/ResultsWriter.h
File bench/ResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/30001/bench/ResultsWriter.h#ne...
bench/ResultsWriter.h:150: Json::Value* find_named_node(Json::Value* root, const
char name[]) {
On 2014/05/27 15:34:25, jcgregorio wrote:
> Should be moved to a CPP file. Also I believe the naming convention for
externed
> static functions is camel caps:
> 
> 
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...

Ah, sorry. Any advice on what to name the CPP file?

https://codereview.chromium.org/286903025/diff/30001/tools/PictureResultsWrit...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/30001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:23: Json::Value* find_named_node(Json::Value* root,
const char name[]) {
On 2014/05/27 15:34:25, jcgregorio wrote:
> Why is find_named_node duplicated here?

I guess I copy and pasted from ResultsWriter, and then forgot that I moved the
one in ResultsWriter out of the class. I'll fix that

[jcgregorio, 2014-05-27 17:14:05]

https://codereview.chromium.org/286903025/diff/30001/bench/ResultsWriter.h
File bench/ResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/30001/bench/ResultsWriter.h#ne...
bench/ResultsWriter.h:150: Json::Value* find_named_node(Json::Value* root, const
char name[]) {
ResultsWriter.cpp

On 2014/05/27 16:41:06, kelvinly wrote:
> On 2014/05/27 15:34:25, jcgregorio wrote:
> > Should be moved to a CPP file. Also I believe the naming convention for
> externed
> > static functions is camel caps:
> > 
> > 
> >
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
> 
> Ah, sorry. Any advice on what to name the CPP file?

[kelvinly, 2014-05-27 18:55:28]

Moved a function into ResultsWriter.cpp, changed the gyp files to depend on
that, built everything successfully

https://codereview.chromium.org/286903025/diff/30001/bench/ResultsWriter.h
File bench/ResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/30001/bench/ResultsWriter.h#ne...
bench/ResultsWriter.h:150: Json::Value* find_named_node(Json::Value* root, const
char name[]) {
On 2014/05/27 17:14:05, jcgregorio wrote:
> ResultsWriter.cpp
> 
> On 2014/05/27 16:41:06, kelvinly wrote:
> > On 2014/05/27 15:34:25, jcgregorio wrote:
> > > Should be moved to a CPP file. Also I believe the naming convention for
> > externed
> > > static functions is camel caps:
> > > 
> > > 
> > >
> >
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
> > 
> > Ah, sorry. Any advice on what to name the CPP file?
> 

Done.

[jcgregorio, 2014-05-27 19:57:16]

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:26: PictureResultsWriter() {;}
You don't need the ;

Fix here and below.

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:42: class PictureResultsMultiWriter : public
PictureResultsWriter {
What's the ownership model, does PictureResultsMultiWriter own the writers
passed to it in add()? Or is the caller required to clean them up? Add comments
to make the ownership clear.

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:139: * This ResultsWriter collects data in a JSON
node
PictureJSONResultsWriter

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:212: virtual void end() {
4 spaces

https://codereview.chromium.org/286903025/diff/60001/tools/bench_pictures_mai...
File tools/bench_pictures_main.cpp (right):

https://codereview.chromium.org/286903025/diff/60001/tools/bench_pictures_mai...
tools/bench_pictures_main.cpp:431: gWriter.add(&gLogWriter);
Shouldn't this be outside the #ifdef?

[kelvinly, 2014-05-27 20:27:30]

Problems fixed I think

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:26: PictureResultsWriter() {;}
On 2014/05/27 19:57:16, jcgregorio wrote:
> You don't need the ;
> 
> Fix here and below.

Done.

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:42: class PictureResultsMultiWriter : public
PictureResultsWriter {
On 2014/05/27 19:57:16, jcgregorio wrote:
> What's the ownership model, does PictureResultsMultiWriter own the writers
> passed to it in add()? Or is the caller required to clean them up? Add
comments
> to make the ownership clear.

Done.

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:139: * This ResultsWriter collects data in a JSON
node
On 2014/05/27 19:57:16, jcgregorio wrote:
> PictureJSONResultsWriter

Done.

https://codereview.chromium.org/286903025/diff/60001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:212: virtual void end() {
On 2014/05/27 19:57:16, jcgregorio wrote:
> 4 spaces

Done.

https://codereview.chromium.org/286903025/diff/60001/tools/bench_pictures_mai...
File tools/bench_pictures_main.cpp (right):

https://codereview.chromium.org/286903025/diff/60001/tools/bench_pictures_mai...
tools/bench_pictures_main.cpp:431: gWriter.add(&gLogWriter);
On 2014/05/27 19:57:16, jcgregorio wrote:
> Shouldn't this be outside the #ifdef?
Good catch, fixed

[jcgregorio, 2014-05-27 20:36:06]

https://codereview.chromium.org/286903025/diff/80001/tools/PictureResultsWrit...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/80001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:49: PictureResultsMultiWriter() : writers() {;}
More stray semicolons here and below.

Also see the coding style guidelines on constructor initializers, the
 : writers() should be on the next line.

https://codereview.chromium.org/286903025/diff/80001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:91: SkTArray<PictureResultsWriter*> writers;
fWriters

[kelvinly, 2014-05-27 20:41:17]

Fixed more style issues, I think that should be all of them. Sorry for not
fixing them earlier

https://codereview.chromium.org/286903025/diff/80001/tools/PictureResultsWrit...
File tools/PictureResultsWriter.h (right):

https://codereview.chromium.org/286903025/diff/80001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:49: PictureResultsMultiWriter() : writers() {;}
On 2014/05/27 20:36:07, jcgregorio wrote:
> More stray semicolons here and below.
> 
> Also see the coding style guidelines on constructor initializers, the
>  : writers() should be on the next line.

Sorry, think I got all of them now

https://codereview.chromium.org/286903025/diff/80001/tools/PictureResultsWrit...
tools/PictureResultsWriter.h:91: SkTArray<PictureResultsWriter*> writers;
On 2014/05/27 20:36:07, jcgregorio wrote:
> fWriters

Done.

[jcgregorio, 2014-05-27 20:49:47]

On 2014/05/27 20:41:17, kelvinly wrote:
> Fixed more style issues, I think that should be all of them. Sorry for not
> fixing them earlier
> 
>
https://codereview.chromium.org/286903025/diff/80001/tools/PictureResultsWrit...
> File tools/PictureResultsWriter.h (right):
> 
>
https://codereview.chromium.org/286903025/diff/80001/tools/PictureResultsWrit...
> tools/PictureResultsWriter.h:49: PictureResultsMultiWriter() : writers() {;}
> On 2014/05/27 20:36:07, jcgregorio wrote:
> > More stray semicolons here and below.
> > 
> > Also see the coding style guidelines on constructor initializers, the
> >  : writers() should be on the next line.
> 
> Sorry, think I got all of them now
> 
>
https://codereview.chromium.org/286903025/diff/80001/tools/PictureResultsWrit...
> tools/PictureResultsWriter.h:91: SkTArray<PictureResultsWriter*> writers;
> On 2014/05/27 20:36:07, jcgregorio wrote:
> > fWriters
> 
> Done.

LGTM

[kelvinly, 2014-05-27 20:52:17]

The CQ bit was checked by kelvinly@google.com

[commit-bot: I haz the power, 2014-05-27 20:52:47]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kelvinly@google.com/286903025/100001

[commit-bot: I haz the power, 2014-05-27 20:57:51]

Change committed as 14906

[epoger, 2014-05-27 21:50:55]

A revert of this CL has been created in
https://codereview.chromium.org/306483010/ by epoger@google.com.

The reason for reverting is: Broke some Windows builds; see
http://skbug.com/2609 ('certain Windows Build-* bots failing since r14905').

Before re-landing with a fix, please send to some of the trybots that failed the
first time..