rebaseline_server: use new intermediate JSON format

rebaseline_server: use new intermediate JSON format as described in https://goto.google.com/ChangingRbsJson

There are still some places in the frontend (HTML+Javascript) code where
we assume we are handling expectations vs actuals, but there are just a few
and we should be able to remove them easily in a coming CL.
At that point, the frontend will work just as well for displaying any set
of image pairs.

BUG=skia:1919
NOTRY=True

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

[epoger, 2014-02-26 04:38:14]

Ready for review at patchset 3!

https://codereview.chromium.org/178253010/diff/20001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/178253010/diff/20001/gm/gm_json.py#newcode99
gm/gm_json.py:99: #          hash_digest)
As I recall, the Python style guide recommends against custom spacing like this.

If you have a better format to suggest, please let me know.  I would be happy
for this code to be both readable AND style-guide-compliant!

https://codereview.chromium.org/178253010/diff/20001/gm/rebaseline_server/res...
File gm/rebaseline_server/results.py (right):

https://codereview.chromium.org/178253010/diff/20001/gm/rebaseline_server/res...
gm/rebaseline_server/results.py:160: Results are returned in a dictionary as
output by ImagePairSet.as_dict().
This is one of the big changes in this CL: results.py used to assemble the
intermediate JSON from scratch.  Now it passes (expected, actual) pairs to
ImagePairSet, and lets ImagePairSet assemble the JSON.

Later on, when we want to create intermediate JSON describing different sets of
image pairs (e.g., actual-on-platform-A vs actual-on-platform-B), we will fork
results.py to send different sets of image pairs to ImagePairSet.  The rest of
the code shouldn't need any work.

https://codereview.chromium.org/178253010/diff/20001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/constants.js (right):

https://codereview.chromium.org/178253010/diff/20001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/constants.js:11: // Keep these in sync with
../column.py
This is another big part of this CL: use of defined constants for keys in the
intermediate JSON.

Previously, both the client-side code (HTML+Javascript) and server-side code
(Python) used string literals, without any mechanism in place to keep them in
sync.  If we had changed some of those literals, it could have easily broken
other code.

I'm sure we can still break things now, but it should be harder to do.

https://codereview.chromium.org/178253010/diff/20001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/view.html (right):

https://codereview.chromium.org/178253010/diff/20001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/view.html:66: <label ng-repeat="(resultType, count)
in
extraColumnHeaders[constants.KEY__EXTRACOLUMN__RESULT_TYPE][constants.KEY__VALUES_AND_COUNTS]
track by $index">
Here's an example of a remaining assumption that we are displaying expectations
vs actuals: a hard-coded reference to resultType (which is one of succeeded,
failed, no-comparison).

In a coming CL, such columns will be automatically displayed (or not) using
ng-repeat, depending on which extraColumns are present in the intermediate JSON.

https://codereview.chromium.org/178253010/diff/20001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/view.html:350:
title="{{imagePair[constants.KEY__DIFFERENCE_DATA][constants.KEY__DIFFERENCE_DATA__NUM_DIFF_PIXELS]
| number:0}} of {{(100 *
imagePair[constants.KEY__DIFFERENCE_DATA][constants.KEY__DIFFERENCE_DATA__NUM_DIFF_PIXELS]
/
imagePair[constants.KEY__DIFFERENCE_DATA][constants.KEY__DIFFERENCE_DATA__PERCENT_DIFF_PIXELS])
| number:0}} pixels
({{imagePair[constants.KEY__DIFFERENCE_DATA][constants.KEY__DIFFERENCE_DATA__PERCENT_DIFF_PIXELS].toFixed(4)}}%)
differ from expectation.">
Yikes, that's a long line.  I'm open to suggestion on how to break it up,
refactor, etc.

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/tes...
File
gm/rebaseline_server/testdata/outputs/actual/results_test.ResultsTest.test_gm/gm.json
(left):

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/tes...
gm/rebaseline_server/testdata/outputs/actual/results_test.ResultsTest.test_gm/gm.json:1:
{
Whoops, shouldn't have added this file.  (Should have only added the copy in the
"expected" dir.)  Deleted it.

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/tes...
File
gm/rebaseline_server/testdata/outputs/expected/results_test.ResultsTest.test_gm/gm.json
(right):

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/tes...
gm/rebaseline_server/testdata/outputs/expected/results_test.ResultsTest.test_gm/gm.json:2:
"extraColumnHeaders": {
The updated expectation for this unittest demonstrates nicely how the
intermediate JSON is now rendered as described in
https://goto.google.com/ChangingRbsJson .

[epoger, 2014-02-26 15:14:38]

Trying another run of the HouseKeeper bot at patchset 3.  The one I ran
overnight failed due to a network connection failure.

[rmistry, 2014-02-26 17:35:39]

Minor nits but other than that LGTM!

https://codereview.chromium.org/178253010/diff/40001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/178253010/diff/40001/gm/gm_json.py#newcode116
gm/gm_json.py:116: return '%s/%s' % (gm_actuals_root_url, CreateGmRelativeUrl(
[Optional]
I do not feel strongly about this (using '%s/%s/%s' in places) just wanted to
list some other options for this:
* posixpath.join('%s'...
* '/'.join(list)

Upto you.

https://codereview.chromium.org/178253010/diff/40001/gm/gm_json.py#newcode118
gm/gm_json.py:118: 
Nit: Two newlines between top level defs and everywhere below.

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/col...
File gm/rebaseline_server/column.py (right):

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/col...
gm/rebaseline_server/column.py:13: # Keep these in sync with static/constants.js
Can we make this (and other references to constants.js) slightly more
noticeable? Something like:

# NOTE: Keep these in sync with static/constants.js

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagepair.py (right):

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagepair.py:49: if imageA_relative_url ==
imageB_relative_url:
The following is more readable to me, WDYT?

if not imageA_relative_url or not imageB_relative_url:
  self.diff_record = None
elif imageA_relative_url == imageB_releative_url:
  self.diff_record = None
else
  ...

[epoger, 2014-02-26 17:56:19]

https://codereview.chromium.org/178253010/diff/40001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/178253010/diff/40001/gm/gm_json.py#newcode116
gm/gm_json.py:116: return '%s/%s' % (gm_actuals_root_url, CreateGmRelativeUrl(
On 2014/02/26 17:35:39, rmistry wrote:
> [Optional]
> I do not feel strongly about this (using '%s/%s/%s' in places) just wanted to
> list some other options for this:
> * posixpath.join('%s'...
> * '/'.join(list)
> 
> Upto you.

Good point; I changed it to use posixpath.join().  I hadn't done that in here
before, because I had previously been tacking on the ".png" too.  But that has
moved elsewhere now...

https://codereview.chromium.org/178253010/diff/40001/gm/gm_json.py#newcode118
gm/gm_json.py:118: 
On 2014/02/26 17:35:39, rmistry wrote:
> Nit: Two newlines between top level defs and everywhere below.

Done throughout the CL, as specified in
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Blank_Lines . 
Sorry I keep forgetting this!

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/col...
File gm/rebaseline_server/column.py (right):

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/col...
gm/rebaseline_server/column.py:13: # Keep these in sync with static/constants.js
On 2014/02/26 17:35:39, rmistry wrote:
> Can we make this (and other references to constants.js) slightly more
> noticeable? Something like:
> 
> # NOTE: Keep these in sync with static/constants.js

Done.

sed -i 's/Keep these in sync/NOTE: Keep these in sync/g'
gm/rebaseline_server/*.py gm/rebaseline_server/*/*.js

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagepair.py (right):

https://codereview.chromium.org/178253010/diff/40001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagepair.py:49: if imageA_relative_url ==
imageB_relative_url:
On 2014/02/26 17:35:39, rmistry wrote:
> The following is more readable to me, WDYT?
> 
> if not imageA_relative_url or not imageB_relative_url:
>   self.diff_record = None
> elif imageA_relative_url == imageB_releative_url:
>   self.diff_record = None
> else
>   ...

Agreed!  Done.

P.S. There was a typo in your suggestion: imageB_releative_url .  Thankfully,
the unittests caught it.  Python scares me.

[epoger, 2014-02-26 18:30:00]

Trying another run of the HouseKeeper bot at patchset 4.  More network
problems...

[epoger, 2014-02-26 18:52:03]

Trying a THIRD run of the HouseKeeper trybot at patchset 4.  Yet more network
problems...

[epoger, 2014-02-26 19:04:58]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-02-26 19:05:07]

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

[commit-bot: I haz the power, 2014-02-26 19:05:31]

Change committed as 13598