make render_pictures properly handle empty expectations file

tools/image_expectations.cpp

Index: tools/image_expectations.cpp
diff --git a/tools/image_expectations.cpp b/tools/image_expectations.cpp
index 24c91755415d62e4e292cbd72d0af0b76f4045cc..ac232e9f30daf4eba7f8f04c59db01e3f3b80998 100644
--- a/tools/image_expectations.cpp
+++ b/tools/image_expectations.cpp
@@ -95,12 +95,43 @@ namespace sk_tools {
     // ImageResultsAndExpectations class...
 
     bool ImageResultsAndExpectations::readExpectationsFile(const char *jsonPath) {
-        if (Parse(jsonPath, &fExpectedJsonRoot)) {
-            fExpectedResults = fExpectedJsonRoot[kJsonKey_ExpectedResults];
+        if (NULL == jsonPath) {
+            SkDebugf("JSON expectations filename not specified\n");
+            return false;
+        }
+        SkFILE* filePtr = sk_fopen(jsonPath, kRead_SkFILE_Flag);
+        if (NULL == filePtr) {
+            SkDebugf("JSON expectations file '%s' does not exist\n", jsonPath);
+            return false;
+        }
+        size_t size = sk_fgetsize(filePtr);
+        if (0 == size) {
+            SkDebugf("JSON expectations file '%s' is empty, so no expectations\n", jsonPath);
+            sk_fclose(filePtr);
+            fExpectedResults.clear();
             return true;
-        } else {
+        }
+        bool parsedJson = Parse(filePtr, &fExpectedJsonRoot);
+        sk_fclose(filePtr);
+        if (!parsedJson) {
+            SkDebugf("Failed to parse JSON expectations file '%s'\n", jsonPath);
             return false;
         }
+        Json::Value header = fExpectedJsonRoot[kJsonKey_Header];
+        Json::Value headerType = header[kJsonKey_Header_Type];
+        Json::Value headerRevision = header[kJsonKey_Header_Revision];
+        if (strcmp(headerType.asCString(), kJsonValue_Header_Type)) {

[epoger, 2014/05/16 21:02:32]

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.

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

What would cause one of these to fail?

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

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)

+            SkDebugf("JSON expectations file '%s': expected headerType '%s', found '%s'\n",
+                     jsonPath, kJsonValue_Header_Type, headerType.asCString());
+            return false;
+        }
+        if (headerRevision.asInt() != kJsonValue_Header_Revision) {
+            SkDebugf("JSON expectations file '%s': expected headerRevision %d, found %d\n",
+                     jsonPath, kJsonValue_Header_Revision, headerRevision.asInt());
+            return false;
+        }
+        fExpectedResults = fExpectedJsonRoot[kJsonKey_ExpectedResults];
+        return true;
     }
 
     void ImageResultsAndExpectations::add(const char *sourceName, const char *fileName,
@@ -181,11 +212,10 @@ namespace sk_tools {
         stream.write(jsonStdString.c_str(), jsonStdString.length());
     }
 
-    /*static*/ bool ImageResultsAndExpectations::Parse(const char *jsonPath,
+    /*static*/ bool ImageResultsAndExpectations::Parse(SkFILE *filePtr,
                                                        Json::Value *jsonRoot) {
-        SkAutoDataUnref dataRef(SkData::NewFromFileName(jsonPath));
+        SkAutoDataUnref dataRef(SkData::NewFromFILE(filePtr));

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

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?)

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

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.
Done.

[scroggo, 2014/05/19 15:07:38]

The new way looks good to me.

         if (NULL == dataRef.get()) {
-            SkDebugf("error reading JSON file %s\n", jsonPath);
             return false;
         }
 
@@ -193,7 +223,6 @@ namespace sk_tools {
         size_t size = dataRef.get()->size();
         Json::Reader reader;
         if (!reader.parse(bytes, bytes+size, *jsonRoot)) {
-            SkDebugf("error parsing JSON file %s\n", jsonPath);
             return false;
         }