Test decoders when hasLength returns false.

Test decoders when hasLength returns false.

Compare the result of a decode of a stream without a length to the
result of the same stream with a length. Ensures that our decoders
work regardless of whether hasLength returns true (since the decode
potentially follows two paths).

R=epoger@google.com

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

[scroggo, 2013-08-27 22:13:11]

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

https://codereview.chromium.org/23598004/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:470: if
(compare_to_expectations_if_necessary(subsetWithDigest.fDigest,
I could have left this as bitmapFromDecodeSubset, since the constructor for
GmResultDigest was not marked explicit. Was that on purpose? Should I use the
implicit constructor?

[epoger, 2013-08-28 14:47:56]

LGTM with or without the following suggestions...

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

https://codereview.chromium.org/23598004/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:309: // getLength.
Can we leave the code in place, but only CALL it on Android/Linux (ifdef at line
435)?  I'd rather keep our ifdef's at a minimum.

Also, is there a bug filed to track this functionality on Windows/Mac?  If so,
please reference it here.

https://codereview.chromium.org/23598004/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:419: skiagm::BitmapAndDigest bandd(bitmap);
Minor suggestion: maybe "bitmapAndDigest" instead of "bandd"?  (It took me a few
seconds to realize what "bandd" meant)

https://codereview.chromium.org/23598004/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:470: if
(compare_to_expectations_if_necessary(subsetWithDigest.fDigest,
On 2013/08/27 22:13:11, scroggo wrote:
> I could have left this as bitmapFromDecodeSubset, since the constructor for
> GmResultDigest was not marked explicit. Was that on purpose? Should I use the
> implicit constructor?

I don't know what the guidelines are for using the implicit constructor or not. 
Personally, I like being explicit about it (as you were here), so it is as clear
as possible what's going on.

I don't understand why you create the BitmapAndDigest object, though, since you
never use it except to get its fDigest.  Maybe this would be simpler?

if (compare_to_expectations_if_necessary(
    GmResultDigest(bitmapFromDecodeSubset),
    subsetName.c_str(),
    ...

Or, using the implicit constructor, whichever you prefer.

[scroggo, 2013-08-28 17:54:38]

New patch.

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

https://codereview.chromium.org/23598004/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:309: // getLength.
On 2013/08/28 14:47:56, epoger wrote:
> Can we leave the code in place, but only CALL it on Android/Linux (ifdef at
line
> 435)?  I'd rather keep our ifdef's at a minimum.

Won't our warnings complain that there is a static function which is never used?
I find it kind of nice that functions we won't call are not compiled to begin
with.

> 
> Also, is there a bug filed to track this functionality on Windows/Mac?  If so,
> please reference it here.

Now there is: https://code.google.com/p/skia/issues/detail?id=1570.

https://codereview.chromium.org/23598004/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:419: skiagm::BitmapAndDigest bandd(bitmap);
On 2013/08/28 14:47:56, epoger wrote:
> Minor suggestion: maybe "bitmapAndDigest" instead of "bandd"?  (It took me a
few
> seconds to realize what "bandd" meant)

Done.

https://codereview.chromium.org/23598004/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:470: if
(compare_to_expectations_if_necessary(subsetWithDigest.fDigest,
On 2013/08/28 14:47:56, epoger wrote:
> On 2013/08/27 22:13:11, scroggo wrote:
> > I could have left this as bitmapFromDecodeSubset, since the constructor for
> > GmResultDigest was not marked explicit. Was that on purpose? Should I use
the
> > implicit constructor?
> 
> I don't know what the guidelines are for using the implicit constructor or
not. 
> Personally, I like being explicit about it (as you were here), so it is as
clear
> as possible what's going on.
> 
> I don't understand why you create the BitmapAndDigest object, though, since
you
> never use it except to get its fDigest.  Maybe this would be simpler?
> 
> if (compare_to_expectations_if_necessary(
>     GmResultDigest(bitmapFromDecodeSubset),
>     subsetName.c_str(),
>     ...
> 
> Or, using the implicit constructor, whichever you prefer.

Oh right, I do not need a BitmapAndDigest. (In fact I never need the class in
this file, so I'm just sticking to GmResultDigest.) I would have created the
GmResultDigest inline, like you suggested, but that makes my long line even
longer.

[epoger, 2013-08-28 18:24:04]

lgtm

[scroggo, 2013-08-28 18:33:39]

Committed patchset #2 manually as r10984 (presubmit successful).