GM: include filename extension (.png) of each output file in JSON summary

GM: include filename extension (.png) of each output file in JSON summary

Doing this so that, once we *do* start writing PDF checksums into the JSON
summary, we'll be able to distinguish those from the PNG checksums.
Otherwise, we could have naming collisions.

[epoger, 2013-05-08 19:39:48]

https://codereview.chromium.org/15014011/diff/1/gm/tests/run.sh
File gm/tests/run.sh (right):

https://codereview.chromium.org/15014011/diff/1/gm/tests/run.sh#newcode28
gm/tests/run.sh:28: WRITE_IMAGE_FILES=1
First patchset turns on WRITE_IMAGE_FILES in the gm self-tests, and adds all the
self-test output that follows from that change.

[epoger, 2013-05-08 19:45:08]

https://codereview.chromium.org/15014011/diff/4001/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/15014011/diff/4001/gm/gm_expectations.cpp#new...
gm/gm_expectations.cpp:169: testNameWithExtension.append(".png");
Patchset 2 adds ".png" extension to the lines in the JSON file, both when
reading and writing, and updates the self-test results accordingly.

The implementation isn't the cleanest thing in the world... more ".png"
constants floating around... but it gives us the desired output.

[epoger, 2013-05-08 19:57:16]

https://codereview.chromium.org/15014011/diff/5098/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/15014011/diff/5098/gm/gm_expectations.cpp#new...
gm/gm_expectations.cpp:169: testNameWithExtension.append(kPNG_FileExtension);
In patchset 3, we replace all appearances of ".png" and "png" with references to
a new kPNG_FileExtension constant.  It's bad that we make the png-only
assumption throughout, but at least there is now a term we can search for in the
code to fix these assumptions later.

[epoger, 2013-05-08 20:03:28]

https://codereview.chromium.org/15014011/diff/10006/gm/gm.h
File gm/gm.h (right):

https://codereview.chromium.org/15014011/diff/10006/gm/gm.h#newcode1
gm/gm.h:1: 
Patchset 4 reverts the setting of WRITE_IMAGE_FILES=1 from patchset 1, because
we don't want to commit that (it's only for local testing).

[epoger, 2013-05-08 20:06:07]

Ready for review at patchset 4.

If you look at the earlier patchsets, you'll see where I temporary turned on
WRITE_IMAGE_FILES for the gm self-tests, just to make sure that I didn't break
the image reading/writing part of GM with this change.

But in patchset 4, I turned that back off so it would be ok to commit.

[scroggo, 2013-05-08 20:39:45]

https://codereview.chromium.org/15014011/diff/10006/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/15014011/diff/10006/gm/gm_expectations.cpp#ne...
gm/gm_expectations.cpp:169: testNameWithExtension.append(kPNG_FileExtension);
This will break my use of this function in skimage_main.cpp (working on that
self test...).

I see that IndividualImageExpectationsSource::get(const char *testName) already
had similar behavior. I do think it makes sense to make the two consistent. In
order to make this function work for PDF, PNG, and skimage (where testName
already includes the extension), maybe the function should take two parameters,
testName and extension? A NULL extension can skip the append, allowing skimage
to work.

[epoger, 2013-05-09 14:20:07]

https://codereview.chromium.org/15014011/diff/10006/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/15014011/diff/10006/gm/gm_expectations.cpp#ne...
gm/gm_expectations.cpp:169: testNameWithExtension.append(kPNG_FileExtension);
On 2013/05/08 20:39:45, scroggo wrote:
> This will break my use of this function in skimage_main.cpp (working on that
> self test...).
> 
> I see that IndividualImageExpectationsSource::get(const char *testName)
already
> had similar behavior. I do think it makes sense to make the two consistent. In
> order to make this function work for PDF, PNG, and skimage (where testName
> already includes the extension), maybe the function should take two
parameters,
> testName and extension? A NULL extension can skip the append, allowing skimage
> to work.

Thanks for the early warning on that.  Actually, in the long run, I think we
want both this method and IndividualImageExpectationsSource::get(const char
*testName) to look up the result without appending any file extension.  That
way, it will work for any filetype in GM, and it will continue to work as-is for
skimage.

I made this change for now, because I figured it would be easier... but I didn't
bank on it breaking skimage.

So, would you be in favor of the long-term fix above?  If so, I'll figure out a
way to just do it now.

[scroggo, 2013-05-09 14:28:48]

https://codereview.chromium.org/15014011/diff/10006/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/15014011/diff/10006/gm/gm_expectations.cpp#ne...
gm/gm_expectations.cpp:169: testNameWithExtension.append(kPNG_FileExtension);
On 2013/05/09 14:20:07, epoger wrote:
> On 2013/05/08 20:39:45, scroggo wrote:
> > This will break my use of this function in skimage_main.cpp (working on that
> > self test...).
> > 
> > I see that IndividualImageExpectationsSource::get(const char *testName)
> already
> > had similar behavior. I do think it makes sense to make the two consistent.
In
> > order to make this function work for PDF, PNG, and skimage (where testName
> > already includes the extension), maybe the function should take two
> parameters,
> > testName and extension? A NULL extension can skip the append, allowing
skimage
> > to work.
> 
> Thanks for the early warning on that.  Actually, in the long run, I think we
> want both this method and IndividualImageExpectationsSource::get(const char
> *testName) to look up the result without appending any file extension.  That
> way, it will work for any filetype in GM, and it will continue to work as-is
for
> skimage.
> 
> I made this change for now, because I figured it would be easier... but I
didn't
> bank on it breaking skimage.
> 
> So, would you be in favor of the long-term fix above?  If so, I'll figure out
a
> way to just do it now.

Yes. Thanks!

[epoger, 2013-05-09 18:26:35]

Closing in favor of https://codereview.chromium.org/14890017/ .  I'll make the
changes there...