add explicit filepaths to render_pictures JSON summary

add explicit filepaths to render_pictures JSON summary

BUG=skia:2230, skia:1942
Committed: http://code.google.com/p/skia/source/detail?r=14133

[epoger, 2014-04-04 18:14:40]

notes from live discussion with Ravi

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
File tools/tests/render_pictures_test.py (right):

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
tools/tests/render_pictures_test.py:111: "filepath" :
"bitmap-64bitMD5/red_skp/11092453015575919668.png",
instead of 
bitmap-64bitMD5/red_skp/11092453015575919668.png

make this
red_skp/BM64-11092453015575919668.png

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
tools/tests/render_pictures_test.py:252: "red.skp-tile0" : {
add some version header too

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
tools/tests/render_pictures_test.py:254: "filepath" :
"bitmap-64bitMD5/red_skp-tile0/5815827069051002745.png",
instead of 
bitmap-64bitMD5/red_skp-tile0/5815827069051002745.png

make it
red_skp-tile0/BM64-5815827069051002745.png

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
tools/tests/render_pictures_test.py:256: "red.skp-tile1" : {
instead:
"algorithm": "bitmap-64bitMD5",
"checksum": 9323613075234140270,

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
tools/tests/render_pictures_test.py:261: "bitmap-64bitMD5" :
16670399404877552232,
Note: changing the format of this JSON will break Ravi's cluster telemetry
skia_try_server

[epoger, 2014-04-04 18:34:21]

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
File tools/tests/render_pictures_test.py (right):

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
tools/tests/render_pictures_test.py:111: "filepath" :
"bitmap-64bitMD5/red_skp/11092453015575919668.png",
On 2014/04/04 18:14:40, epoger wrote:
> instead of 
> bitmap-64bitMD5/red_skp/11092453015575919668.png
> 
> make this
> red_skp/BM64-11092453015575919668.png

Done.

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
tools/tests/render_pictures_test.py:252: "red.skp-tile0" : {
On 2014/04/04 18:14:40, epoger wrote:
> add some version header too

Done.

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
tools/tests/render_pictures_test.py:254: "filepath" :
"bitmap-64bitMD5/red_skp-tile0/5815827069051002745.png",
On 2014/04/04 18:14:40, epoger wrote:
> instead of 
> bitmap-64bitMD5/red_skp-tile0/5815827069051002745.png
> 
> make it
> red_skp-tile0/BM64-5815827069051002745.png

Done.

https://codereview.chromium.org/226293002/diff/1/tools/tests/render_pictures_...
tools/tests/render_pictures_test.py:256: "red.skp-tile1" : {
On 2014/04/04 18:14:40, epoger wrote:
> instead:
> "algorithm": "bitmap-64bitMD5",
> "checksum": 9323613075234140270,

Done.

[epoger, 2014-04-07 19:43:12]

At patchset 3, it now passes the updated unittests we created in the previous
patchsets.  Ready for review at patchset 3!

I have noted below a few places where I further adjusted the unittest
expectations, based on findings while I was working on the implementation.  Let
me know if you think any of those adjustments are problematic.

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
File tools/tests/render_pictures_test.py (right):

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
tools/tests/render_pictures_test.py:63: "filepath" : "red_skp.png",
I changed this for consistency with the tiled case (where we write out
red_skp-tile0.png, red_skp-tile1.png, etc.).

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
tools/tests/render_pictures_test.py:124: "filepath" :
"red_skp/bitmap-64bitMD5_11092453015575919668.png",
I changed this so we don't have to keep Yet Another Constant String representing
each hashing algorithm.  If you think it's worth doing so, let me know.

[rmistry, 2014-04-07 20:07:22]

LGTM

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
File tools/tests/render_pictures_test.py (right):

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
tools/tests/render_pictures_test.py:63: "filepath" : "red_skp.png",
On 2014/04/07 19:43:13, epoger wrote:
> I changed this for consistency with the tiled case (where we write out
> red_skp-tile0.png, red_skp-tile1.png, etc.).

Acknowledged.

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
tools/tests/render_pictures_test.py:124: "filepath" :
"red_skp/bitmap-64bitMD5_11092453015575919668.png",
On 2014/04/07 19:43:13, epoger wrote:
> I changed this so we don't have to keep Yet Another Constant String
representing
> each hashing algorithm.  If you think it's worth doing so, let me know.

Not sure I understand, Do you mean having a module level constant with the value
'bitmap-64bitMD5' ?

[epoger, 2014-04-07 20:11:52]

Thanks for the quick feedback!

I will create a matching CL for skia_try_server, to properly handle the new JSON
format, before I commit this one.

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
File tools/tests/render_pictures_test.py (right):

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
tools/tests/render_pictures_test.py:124: "filepath" :
"red_skp/bitmap-64bitMD5_11092453015575919668.png",
On 2014/04/07 20:07:22, rmistry wrote:
> On 2014/04/07 19:43:13, epoger wrote:
> > I changed this so we don't have to keep Yet Another Constant String
> representing
> > each hashing algorithm.  If you think it's worth doing so, let me know.
> 
> Not sure I understand, Do you mean having a module level constant with the
value
> 'bitmap-64bitMD5' ?

Sorry that was confusing.

We already have the kJsonValue_Image_ChecksumAlgorithm_Bitmap64bitMD5 constant
(in PictureRenderer.cpp) for "bitmap-64bitMD5"

I didn't really want to add another one
(kJsonValue_Image_ChecksumAlgorithmAbbr_Bitmap64bitMD5 ?) for "BM64".  Nor did I
want to use a magic "BM64" string within the code.

[rmistry, 2014-04-07 20:19:52]

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
File tools/tests/render_pictures_test.py (right):

https://codereview.chromium.org/226293002/diff/40001/tools/tests/render_pictu...
tools/tests/render_pictures_test.py:124: "filepath" :
"red_skp/bitmap-64bitMD5_11092453015575919668.png",
On 2014/04/07 20:11:52, epoger wrote:
> On 2014/04/07 20:07:22, rmistry wrote:
> > On 2014/04/07 19:43:13, epoger wrote:
> > > I changed this so we don't have to keep Yet Another Constant String
> > representing
> > > each hashing algorithm.  If you think it's worth doing so, let me know.
> > 
> > Not sure I understand, Do you mean having a module level constant with the
> value
> > 'bitmap-64bitMD5' ?
> 
> Sorry that was confusing.
> 
> We already have the kJsonValue_Image_ChecksumAlgorithm_Bitmap64bitMD5 constant
> (in PictureRenderer.cpp) for "bitmap-64bitMD5"
> 
> I didn't really want to add another one
> (kJsonValue_Image_ChecksumAlgorithmAbbr_Bitmap64bitMD5 ?) for "BM64".  Nor did
I
> want to use a magic "BM64" string within the code.

Makes sense. LGTM.

[epoger, 2014-04-08 21:34:59]

Ravi- PTAL at patchset 5.

render_pictures_diff.py diffs between patchsets 3-4 show the new format you and
I agreed on earlier today:
https://codereview.chromium.org/226293002/diff2/40001:90001/tools/tests/rende...

render_pictures_diff.py diffs between patchsets 4-5 show an additional change I
decided to make during implementation:
https://codereview.chromium.org/226293002/diff2/90001:110001/tools/tests/rend...

remaining diffs between patchsets 3-5 show the code changes needed to implement
the new format.

[rmistry, 2014-04-08 21:36:42]

LGTM

[epoger, 2014-04-10 15:02:37]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-04-10 15:02:41]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/epoger@google.com/226293002/130001

[commit-bot: I haz the power, 2014-04-10 15:39:10]

Change committed as 14133