Allow SkAndroidCodec to use SkPngChunkReader

Allow SkAndroidCodec to use SkPngChunkReader

Also update documentation of SkPngChunkReader
in SkCodec.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/7d5105c4d2a9be7ef9ddba646471c35d4c966d97

[msarett, 2015-11-30 15:24:04]

msarett@google.com changed reviewers:
+ djsollen@google.com, scroggo@google.com

[scroggo, 2015-11-30 15:27:37]

lgtm

[djsollen, 2015-11-30 15:42:22]

https://codereview.chromium.org/1487583003/diff/1/include/codec/SkAndroidCodec.h
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1487583003/diff/1/include/codec/SkAndroidCode...
include/codec/SkAndroidCodec.h:27: *  the image is a png.
what is the expected behavior if you pass in a chunkReader if the image is not a
PNG or it is a PNG but not a 9patch?  I assume it succeeds but doesn't modify
the chunkReader

[msarett, 2015-11-30 15:44:33]

https://codereview.chromium.org/1487583003/diff/1/include/codec/SkAndroidCodec.h
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1487583003/diff/1/include/codec/SkAndroidCode...
include/codec/SkAndroidCodec.h:27: *  the image is a png.
On 2015/11/30 15:42:22, djsollen wrote:
> what is the expected behavior if you pass in a chunkReader if the image is not
a
> PNG or it is a PNG but not a 9patch?  I assume it succeeds but doesn't modify
> the chunkReader

Me too.  Leon, can you confirm?

[scroggo, 2015-11-30 15:55:09]

https://codereview.chromium.org/1487583003/diff/1/include/codec/SkAndroidCodec.h
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1487583003/diff/1/include/codec/SkAndroidCode...
include/codec/SkAndroidCodec.h:27: *  the image is a png.
On 2015/11/30 15:42:22, djsollen wrote:
> what is the expected behavior if you pass in a chunkReader if the image is not
a
> PNG or it is a PNG but not a 9patch?

If the image is not a PNG, no ref is taken.

If it *is* a PNG, we don't actually know whether the image is a ninepatch - it's
the SkPngChunkReader's job to determine that. So it gets reffed, and if the PNG
has unknown chunks, SkPngChunkReader's callback will get called.

These comments match the comments in SkCodec. If we update these, we should
update SkCodec as well.

> I assume it succeeds but doesn't modify the chunkReader

The only way chunkReader can affect whether NewFrom*** succeeds is if its
implementation of readChunk returns false.

And chunkReader is never modified (besides changing its refcount), unless
readChunk modifies it.

[djsollen, 2015-11-30 15:58:47]

https://codereview.chromium.org/1487583003/diff/1/include/codec/SkAndroidCodec.h
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1487583003/diff/1/include/codec/SkAndroidCode...
include/codec/SkAndroidCodec.h:27: *  the image is a png.
On 2015/11/30 15:55:09, scroggo wrote:
> On 2015/11/30 15:42:22, djsollen wrote:
> > what is the expected behavior if you pass in a chunkReader if the image is
not
> a
> > PNG or it is a PNG but not a 9patch?
> 
> If the image is not a PNG, no ref is taken.
> 
> If it *is* a PNG, we don't actually know whether the image is a ninepatch -
it's
> the SkPngChunkReader's job to determine that. So it gets reffed, and if the
PNG
> has unknown chunks, SkPngChunkReader's callback will get called.
> 
> These comments match the comments in SkCodec. If we update these, we should
> update SkCodec as well.
> 
> > I assume it succeeds but doesn't modify the chunkReader
> 
> The only way chunkReader can affect whether NewFrom*** succeeds is if its
> implementation of readChunk returns false.
> 
> And chunkReader is never modified (besides changing its refcount), unless
> readChunk modifies it.

Can you update the comment in SkCodec?  I'm fine if this comment is more spartan
and points the reader to the larger description in SkCodec.

[msarett, 2015-11-30 16:39:58]

Please let me know if you prefer Patch Set 2 or Patch Set 3.

[djsollen, 2015-11-30 17:52:52]

use patch set 2 style so that doxygen can properly create docs.

https://codereview.chromium.org/1487583003/diff/20001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1487583003/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:32: *  If the SkPngChunkReader is not NULL:
not NULL then:

https://codereview.chromium.org/1487583003/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:34: *      If the image is a PNG, the SkPngChunkReader
will be reffed and
don't say that is is passed to libpng

https://codereview.chromium.org/1487583003/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:37: *      to handle these chunks.  libpng may call the
SkPngChunkReader to
SkPngChunkReader will be called to read any unknown chunk at any point...

[msarett, 2015-11-30 18:05:40]

Description was changed from

==========
Allow SkAndroidCodec to use SkPngChunkReader

BUG=skia:
==========

to

==========
Allow SkAndroidCodec to use SkPngChunkReader

Also update documentation of SkPngChunkReader
in SkCodec.

BUG=skia:
==========

[msarett, 2015-11-30 18:05:49]

https://codereview.chromium.org/1487583003/diff/20001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1487583003/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:32: *  If the SkPngChunkReader is not NULL:
On 2015/11/30 17:52:52, djsollen wrote:
> not NULL then:

Done.

https://codereview.chromium.org/1487583003/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:34: *      If the image is a PNG, the SkPngChunkReader
will be reffed and
On 2015/11/30 17:52:52, djsollen wrote:
> don't say that is is passed to libpng

Done.

https://codereview.chromium.org/1487583003/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:37: *      to handle these chunks.  libpng may call the
SkPngChunkReader to
On 2015/11/30 17:52:52, djsollen wrote:
> SkPngChunkReader will be called to read any unknown chunk at any point...

Done.

[djsollen, 2015-12-02 14:45:05]

lgtm

[msarett, 2015-12-02 14:49:04]

The CQ bit was checked by msarett@google.com

[msarett, 2015-12-02 14:49:05]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1487583003/#ps60001
(title: "Response to comments on Patch Set 2")

[commit-bot: I haz the power, 2015-12-02 14:49:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1487583003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1487583003/60001

[commit-bot: I haz the power, 2015-12-02 15:02:45]

Description was changed from

==========
Allow SkAndroidCodec to use SkPngChunkReader

Also update documentation of SkPngChunkReader
in SkCodec.

BUG=skia:
==========

to

==========
Allow SkAndroidCodec to use SkPngChunkReader

Also update documentation of SkPngChunkReader
in SkCodec.

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/7d5105c4d2a9be7ef9ddba646471c35d4c966d97
==========

[commit-bot: I haz the power, 2015-12-02 15:02:46]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/7d5105c4d2a9be7ef9ddba646471c35d4c966d97