Write/compare against expectations in skimage tool.

Write/compare against expectations in skimage tool.

skimage:
Add two new modes: one to write expectations to a json file, and
another to compare results against expectations.
Use SkPATH_SEPARATOR instead of '/'.

gm_expectations:
Split into a static library so it can be used by skimage.
Make functions non static and move function definitions into source
file.
Capitalize static member functions to follow the coding style
guidelines.

BUG=https://code.google.com/p/skia/issues/detail?id=1241
R=epoger@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9069

[epoger, 2013-05-08 17:24:47]

LGTM, either with or without the suggestions below.

I'll be curious to see the JSON files this reads and writes, and see what other
opportunities for code sharing there are between skimage and gm...

https://codereview.chromium.org/14670021/diff/1/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.chromium.org/14670021/diff/1/gyp/tools.gyp#newcode83
gyp/tools.gyp:83: '../src/utils/',
we may wish to consider graduating SkBitmapHasher.h into include/utils, at some
point.

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:168: // Stored expectations to be written to a file
createExpectationsPath is specified.
"written to a file createExpectationsPath" -> "written to a file *if*
createExpectationsPath" ?

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:193: SkTArray<SkString, false>* failureArray) {
could you pass failureArray by reference instead, and then not have to worry
about it maybe being NULL?

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:202: if (SkBitmapHasher::ComputeDigest(bitmap,
&checksum)) {
I think it would be a bit easier to follow if you exit early when possible, like
you already do above...

if (!SkBitmapHasher::ComputeDigest(bitmap, &checksum)) {
  failureArray->push_back().printf("decoded %s, but could not create a
checksum.", filename);
  return false;
}

if (jsExpectation.match(checksum)) {
  return true;
}

failureArray->push_back().printf("decoded %s, but the result does not match "
  "expectations.", filename);
return false;

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:241: const char* filename = strrchr(srcPath,
SkPATH_SEPARATOR);
Maybe split this out into its own SkBasename() method, which we could eventually
move into SkOSFile.h ?

[scroggo, 2013-05-08 18:46:33]

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:168: // Stored expectations to be written to a file
createExpectationsPath is specified.
On 2013/05/08 17:24:48, epoger wrote:
> "written to a file createExpectationsPath" -> "written to a file *if*
> createExpectationsPath" ?

Done.

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:193: SkTArray<SkString, false>* failureArray) {
On 2013/05/08 17:24:48, epoger wrote:
> could you pass failureArray by reference instead, and then not have to worry
> about it maybe being NULL?

That makes sense to me, but according to our coding style guidelines, we pass
mutable parameters as pointers.

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:202: if (SkBitmapHasher::ComputeDigest(bitmap,
&checksum)) {
On 2013/05/08 17:24:48, epoger wrote:
> I think it would be a bit easier to follow if you exit early when possible,
like
> you already do above...
> 
> if (!SkBitmapHasher::ComputeDigest(bitmap, &checksum)) {
>   failureArray->push_back().printf("decoded %s, but could not create a
> checksum.", filename);
>   return false;
> }
> 
> if (jsExpectation.match(checksum)) {
>   return true;
> }
> 
> failureArray->push_back().printf("decoded %s, but the result does not match "
>   "expectations.", filename);
> return false;

Yes, that is much clearer! Done!

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:241: const char* filename = strrchr(srcPath,
SkPATH_SEPARATOR);
On 2013/05/08 17:24:48, epoger wrote:
> Maybe split this out into its own SkBasename() method, which we could
eventually
> move into SkOSFile.h ?

Moved into its own method. Sure enough, other files do something similar.
SkOSFile.h may be a good final place for it.

https://codereview.chromium.org/14670021/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:424: if (outDir.c_str()[outDir.size() - 1] !=
SkPATH_SEPARATOR) {
Also moved into its own function.

[epoger, 2013-05-08 18:48:36]

LGTM

[scroggo, 2013-05-08 19:14:27]

Committed patchset #4 manually as r9069 (presubmit successful).