make render_pictures properly handle empty expectations file

tools/image_expectations.cpp

 /*
  * Copyright 2014 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBitmap.h"
 #include "SkBitmapHasher.h"
 #include "SkData.h"
@@ skipping 77 matching lines @@
         return fImageDigestRef.get();
     }
 
     const SkBitmap *BitmapAndDigest::getBitmapPtr() const {
         return &fBitmap;
     }
 
     // ImageResultsAndExpectations class...
 
     bool ImageResultsAndExpectations::readExpectationsFile(const char *jsonPath) {
-        if (Parse(jsonPath, &fExpectedJsonRoot)) {
-            fExpectedResults = fExpectedJsonRoot[kJsonKey_ExpectedResults];
-            return true;
-        } else {
+        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;
+        }
+        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,
                                           const ImageDigest &digest, const int *tileNumber) {
         // Get expectation, if any.
         Json::Value expectedImage;
         if (!fExpectedResults.isNull()) {
             if (NULL == tileNumber) {
                 expectedImage = fExpectedResults[sourceName][kJsonKey_Source_WholeImage];
             } else {
@@ skipping 60 matching lines @@
         header[kJsonKey_Header_Type] = kJsonValue_Header_Type;
         header[kJsonKey_Header_Revision] = kJsonValue_Header_Revision;
         Json::Value root;
         root[kJsonKey_Header] = header;
         root[kJsonKey_ActualResults] = fActualResults;
         std::string jsonStdString = root.toStyledString();
         SkFILEWStream stream(filename);
         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;
         }
 
         const char *bytes = reinterpret_cast<const char *>(dataRef.get()->data());
         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;
         }
 
         return true;
     }
 
 } // namespace sk_tools