make render_pictures properly handle empty expectations file

make render_pictures properly handle empty expectations file

note that this changes the parameters taken by ImageResultsAndExpectations::Parse()

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

[epoger, 2014-05-16 21:02:31]

Leon, PTAL at patchset 2.

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
File tools/image_expectations.cpp (right):

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
tools/image_expectations.cpp:123: if (strcmp(headerType.asCString(),
kJsonValue_Header_Type)) {
While I was in here, added validation of headerType and headerRevision, along
with other validation during the file-reading process.

I tested each of these cases with temporary code; I didn't think adding those
tests to the unittests was worth the additional code bloat, but if you disagree
I can add them to the unittests.

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations.h
File tools/image_expectations.h (right):

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
tools/image_expectations.h:89: * Reasoning:
Reason for the CL is given here.

[scroggo, 2014-05-19 13:28:32]

lgtm

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
File tools/image_expectations.cpp (right):

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
tools/image_expectations.cpp:123: if (strcmp(headerType.asCString(),
kJsonValue_Header_Type)) {
On 2014/05/16 21:02:32, epoger wrote:
> While I was in here, added validation of headerType and headerRevision, along
> with other validation during the file-reading process.
> 
> I tested each of these cases with temporary code; I didn't think adding those
> tests to the unittests was worth the additional code bloat, but if you
disagree
> I can add them to the unittests.

What would cause one of these to fail?

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
tools/image_expectations.cpp:217: SkAutoDataUnref
dataRef(SkData::NewFromFILE(filePtr));
What's the motivation for the change to make the caller open/close the file?

(Either way seems fine to me; just wondering what the benefit of the change is.
Could you add it to the change description?)

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

https://codereview.chromium.org/289063010/diff/20001/tools/tests/render_pictu...
tools/tests/render_pictures_test.py:272: def
test_untiled_empty_expectations_file(self):
It looks like this function could share code with the one above. (The only
difference is the creation of the expectations file, correct?)

[epoger, 2014-05-19 14:52:00]

Thanks, Leon.  PTAL.

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
File tools/image_expectations.cpp (right):

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
tools/image_expectations.cpp:123: if (strcmp(headerType.asCString(),
kJsonValue_Header_Type)) {
On 2014/05/19 13:28:32, scroggo wrote:
> On 2014/05/16 21:02:32, epoger wrote:
> > While I was in here, added validation of headerType and headerRevision,
along
> > with other validation during the file-reading process.
> > 
> > I tested each of these cases with temporary code; I didn't think adding
those
> > tests to the unittests was worth the additional code bloat, but if you
> disagree
> > I can add them to the unittests.
> 
> What would cause one of these to fail?

If you mean "what would cause one of these if-statements to trigger": that would
happen if someone ran render-pictures --readJsonSummaryPath
/path/to/expectations/with/incompatible/schema

If you mean "what would cause one of those unittests to fail": that would happen
if this version-checking code somehow stopped working as expected (which might
cause the tool to fail in production, in unexpected/hard-to-diagnose ways)

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
tools/image_expectations.cpp:217: SkAutoDataUnref
dataRef(SkData::NewFromFILE(filePtr));
On 2014/05/19 13:28:32, scroggo wrote:
> What's the motivation for the change to make the caller open/close the file?

I made this change so that the empty file case could be handled specially.  (In
this case readExpectationsFile() succeeds, even though there is no JSON data
that was parsed.)

I realize now I could have done this without changing the parameters of
Parse()-- Parse() could look for this special case, and if found, create
jsonRoot from scratch such that  fExpectedJsonRoot[kJsonKey_ExpectedResults]
would return an empty Json::Value object, pretending that it had successfully
parsed that from disk.

If you think that would be better, I'll change the implementation--I actually
think it's cleaner the new way.  Just let me know.

> (Either way seems fine to me; just wondering what the benefit of the change
is.
> Could you add it to the change description?)

Done.

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

https://codereview.chromium.org/289063010/diff/20001/tools/tests/render_pictu...
tools/tests/render_pictures_test.py:272: def
test_untiled_empty_expectations_file(self):
On 2014/05/19 13:28:32, scroggo wrote:
> It looks like this function could share code with the one above. (The only
> difference is the creation of the expectations file, correct?)

Good idea.  I took a stab at this; please let me know what you think.

[scroggo, 2014-05-19 15:07:37]

LGTM

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
File tools/image_expectations.cpp (right):

https://codereview.chromium.org/289063010/diff/20001/tools/image_expectations...
tools/image_expectations.cpp:217: SkAutoDataUnref
dataRef(SkData::NewFromFILE(filePtr));
On 2014/05/19 14:52:00, epoger wrote:
> On 2014/05/19 13:28:32, scroggo wrote:
> > What's the motivation for the change to make the caller open/close the file?
> 
> I made this change so that the empty file case could be handled specially. 
(In
> this case readExpectationsFile() succeeds, even though there is no JSON data
> that was parsed.)
> 
> I realize now I could have done this without changing the parameters of
> Parse()-- Parse() could look for this special case, and if found, create
> jsonRoot from scratch such that  fExpectedJsonRoot[kJsonKey_ExpectedResults]
> would return an empty Json::Value object, pretending that it had successfully
> parsed that from disk.
> 
> If you think that would be better, I'll change the implementation--I actually
> think it's cleaner the new way.  Just let me know.

The new way looks good to me.

> 
> > (Either way seems fine to me; just wondering what the benefit of the change
> is.
> > Could you add it to the change description?)
> 
> Done.
>

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

https://codereview.chromium.org/289063010/diff/20001/tools/tests/render_pictu...
tools/tests/render_pictures_test.py:272: def
test_untiled_empty_expectations_file(self):
On 2014/05/19 14:52:00, epoger wrote:
> On 2014/05/19 13:28:32, scroggo wrote:
> > It looks like this function could share code with the one above. (The only
> > difference is the creation of the expectations file, correct?)
> 
> Good idea.  I took a stab at this; please let me know what you think.

Looks good.

[epoger, 2014-05-19 15:17:38]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-05-19 15:17:46]

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

[commit-bot: I haz the power, 2014-05-19 15:25:12]

Change committed as 14779