Refactoring in preparation for adding the picture benchmarking program.

Refactoring in preparation for adding the picture benchmarking program.

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

[keyar, 2012-06-21 21:07:55]

Please review.

[reed1, 2012-06-21 21:19:56]

refactoring common tool code: good idea!

https://codereview.appspot.com/6339043/diff/3001/tools/picture_utils.h
File tools/picture_utils.h (right):

https://codereview.appspot.com/6339043/diff/3001/tools/picture_utils.h#newcode10
tools/picture_utils.h:10: 
#include "SkTypes.h"

https://codereview.appspot.com/6339043/diff/3001/tools/picture_utils.h#newcode15
tools/picture_utils.h:15: 
1. put these in a class as static functions or namespace or some prefix (e.g.
sk_tools_...)

2. need some documentation for each (e.g. what does setup_bitmap do to the
bitmap?)

[keyar, 2012-06-21 21:39:17]

Made changes as requested by reed.

[twiz1, 2012-06-21 21:46:31]

https://codereview.appspot.com/6339043/diff/3001/tools/picture_utils.cpp
File tools/picture_utils.cpp (right):

https://codereview.appspot.com/6339043/diff/3001/tools/picture_utils.cpp#newc...
tools/picture_utils.cpp:12: #include "picture_utils.h"
The general coding standard is that foo.cpp should include foo.h first, to
ensure that the header files compile independently of where they are included.

[reed1, 2012-06-22 12:12:12]

https://codereview.appspot.com/6339043/diff/9008/tools/picture_utils.h
File tools/picture_utils.h (right):

https://codereview.appspot.com/6339043/diff/9008/tools/picture_utils.h#newcode23
tools/picture_utils.h:23: // inputStream.
This one is sort of weird : it takes a dir and name, instead of a full path (see
helper above). It only calls setPath on the stream (nothing picture related at
all). Can we remove this guy, or have it do more?

[keyar, 2012-06-22 15:47:58]

https://codereview.appspot.com/6339043/diff/9008/tools/picture_utils.h
File tools/picture_utils.h (right):

https://codereview.appspot.com/6339043/diff/9008/tools/picture_utils.h#newcode23
tools/picture_utils.h:23: // inputStream.
On 2012/06/22 12:12:12, reed1 wrote:
> This one is sort of weird : it takes a dir and name, instead of a full path
(see
> helper above). It only calls setPath on the stream (nothing picture related at
> all). Can we remove this guy, or have it do more?

I am not sure if I can remove it without having some (minor) code duplication.
The render_pictures program uses it exactly as written and I am planning for the
soon-to-exist bench_pictures program to use it. Further, the helper function,
make_filepath, is also used separately in render_pictures.

I don't know how it could do more.

Would you prefer if the utility file was differently named?

[reed1, 2012-06-22 15:55:13]

The utility function seems like a 2-liner:

path = make_path(dir, name);
stream->setPath(path);

Seems insignificant enough to just inline at all of the callers (2) imho.

[keyar, 2012-06-22 16:58:09]

On 2012/06/22 15:55:13, reed1 wrote:
> The utility function seems like a 2-liner:
> 
> path = make_path(dir, name);
> stream->setPath(path);
> 
> Seems insignificant enough to just inline at all of the callers (2) imho.

It still needs the error handling code though.

[keyar, 2012-06-22 17:02:18]

On 2012/06/22 16:58:09, keyar wrote:
> On 2012/06/22 15:55:13, reed1 wrote:
> > The utility function seems like a 2-liner:
> > 
> > path = make_path(dir, name);
> > stream->setPath(path);
> > 
> > Seems insignificant enough to just inline at all of the callers (2) imho.
> 
> It still needs the error handling code though.

I take that back, I need the error handling code in the main function so that I
properly exit the running of the current test.

[keyar, 2012-06-22 17:12:19]

Removed the open_picture_stream function.

[reed1, 2012-06-22 17:39:20]

lgtm

[twiz1, 2012-06-22 18:26:03]

On 2012/06/22 17:39:20, reed1 wrote:
> lgtm

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