guarded change to SkImageGenerator to make getInfo() const

guarded change to SkImageGenerator to make getInfo() const

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/3ef71e343bf075888fb50892350390b4dd47de24

[reed1, 2015-03-18 21:17:31]

reed@google.com changed reviewers:
+ scroggo@google.com

[reed1, 2015-03-18 21:17:32]

ptal

[scroggo, 2015-03-19 13:27:47]

https://codereview.chromium.org/1017293002/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/dm/DMSrcSink.cpp#newcode58
dm/DMSrcSink.cpp:58: SkImageInfo info = codec->getInfo();
Yay! FYI: this (and the code in ImageSrc::size()) has moved to CodecSrc.

https://codereview.chromium.org/1017293002/diff/1/gyp/skia_for_android_framew...
File gyp/skia_for_android_framework_defines.gypi (right):

https://codereview.chromium.org/1017293002/diff/1/gyp/skia_for_android_framew...
gyp/skia_for_android_framework_defines.gypi:17:
'SK_SUPPORT_LEGACY_BOOL_ONGETINFO',
Not needed.

https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
File include/core/SkImageGenerator.h (left):

https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
include/core/SkImageGenerator.h:71: *  Repeated calls to this function should
give the same results,
Do you still want this comment (minus the part about the pixelref)?

https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
include/core/SkImageGenerator.h:79: SkImageInfo getInfo() const { return fInfo;
}
Why not return a const ref?

https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
include/core/SkImageGenerator.h:234: SkImageInfo fInfo;
Can we not already make this const?

https://codereview.chromium.org/1017293002/diff/1/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/codec/SkCodec.cpp#newco...
src/codec/SkCodec.cpp:48: SkCodec::SkCodec(const SkImageInfo& info, SkStream*
stream) : INHERITED(info)
nit: Shouldn't this go on the next line?

https://codereview.chromium.org/1017293002/diff/1/src/codec/SkCodec.cpp#newco...
src/codec/SkCodec.cpp:49: , fInfo(info)
fInfo is no longer needed by SkCodec. We can instead call getInfo().

Taking it a step further, we no longer need
getOriginalInfo()
SkCodec::onGetInfo

https://codereview.chromium.org/1017293002/diff/1/src/core/SkImageGenerator.cpp
File src/core/SkImageGenerator.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/core/SkImageGenerator.c...
src/core/SkImageGenerator.cpp:14: info = SkImageInfo::MakeUnknown(0, 0);
Why not return fInfo here? (Then SkCodec, which sets fInfo in its constructor,
doesn't need to override onGetInfo anymore.)

https://codereview.chromium.org/1017293002/diff/1/src/images/SkDecodingImageG...
File src/images/SkDecodingImageGenerator.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/images/SkDecodingImageG...
src/images/SkDecodingImageGenerator.cpp:42: bool onGetInfo(SkImageInfo* info)
SK_OVERRIDE {
Again, I think you can remove this, if you make SkImageGenerator::getInfo return
fInfo when onGetInfo returns false (the default).

https://codereview.chromium.org/1017293002/diff/1/src/images/SkDecodingImageG...
src/images/SkDecodingImageGenerator.cpp:89: const SkImageInfo fInfo;
Can query getInfo when needed.

https://codereview.chromium.org/1017293002/diff/1/src/lazy/SkDiscardablePixel...
File src/lazy/SkDiscardablePixelRef.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/lazy/SkDiscardablePixel...
src/lazy/SkDiscardablePixelRef.cpp:117: if (info.isEmpty() ||
!dst->setInfo(info)) {
Interesting that calling SkBitmap::setInfo with an empty info would return true,
but SkInstallDiscardablePixelRef returns false...

https://codereview.chromium.org/1017293002/diff/1/src/ports/SkImageGenerator_...
File src/ports/SkImageGenerator_skia.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/ports/SkImageGenerator_...
src/ports/SkImageGenerator_skia.cpp:51: virtual bool onGetInfo(SkImageInfo*
info) SK_OVERRIDE {
No longer needed.

[reed1, 2015-03-19 15:20:46]

https://codereview.chromium.org/1017293002/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/dm/DMSrcSink.cpp#newcode58
dm/DMSrcSink.cpp:58: SkImageInfo info = codec->getInfo();
On 2015/03/19 13:27:46, scroggo wrote:
> Yay! FYI: this (and the code in ImageSrc::size()) has moved to CodecSrc.

Acknowledged.

https://codereview.chromium.org/1017293002/diff/1/gyp/skia_for_android_framew...
File gyp/skia_for_android_framework_defines.gypi (right):

https://codereview.chromium.org/1017293002/diff/1/gyp/skia_for_android_framew...
gyp/skia_for_android_framework_defines.gypi:17:
'SK_SUPPORT_LEGACY_BOOL_ONGETINFO',
On 2015/03/19 13:27:46, scroggo wrote:
> Not needed.

Acknowledged.

https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
File include/core/SkImageGenerator.h (left):

https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
include/core/SkImageGenerator.h:71: *  Repeated calls to this function should
give the same results,
On 2015/03/19 13:27:47, scroggo wrote:
> Do you still want this comment (minus the part about the pixelref)?

Don't think so, as it is now baked in during the constructor. I think the
inspiration for that comment was as a warning to subclasses, since they were
responsible for it.

https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
include/core/SkImageGenerator.h:79: SkImageInfo getInfo() const { return fInfo;
}
On 2015/03/19 13:27:47, scroggo wrote:
> Why not return a const ref?

Done.

https://codereview.chromium.org/1017293002/diff/1/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/codec/SkCodec.cpp#newco...
src/codec/SkCodec.cpp:48: SkCodec::SkCodec(const SkImageInfo& info, SkStream*
stream) : INHERITED(info)
On 2015/03/19 13:27:47, scroggo wrote:
> nit: Shouldn't this go on the next line?

Done.

https://codereview.chromium.org/1017293002/diff/1/src/codec/SkCodec.cpp#newco...
src/codec/SkCodec.cpp:49: , fInfo(info)
On 2015/03/19 13:27:47, scroggo wrote:
> fInfo is no longer needed by SkCodec. We can instead call getInfo().
> 
> Taking it a step further, we no longer need
> getOriginalInfo()
> SkCodec::onGetInfo

I thought I would remove the (many) duplicates in subclasses *after* I'm able to
remove the compile-guard (since they are needed in the legacy case).

https://codereview.chromium.org/1017293002/diff/1/src/core/SkImageGenerator.cpp
File src/core/SkImageGenerator.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/core/SkImageGenerator.c...
src/core/SkImageGenerator.cpp:14: info = SkImageInfo::MakeUnknown(0, 0);
On 2015/03/19 13:27:47, scroggo wrote:
> Why not return fInfo here? (Then SkCodec, which sets fInfo in its constructor,
> doesn't need to override onGetInfo anymore.)

I think for compatibility (for a couple days), this is clearer/safer, since
fInfo *may* have been initialized by the subclass to non-empty.

https://codereview.chromium.org/1017293002/diff/1/src/images/SkDecodingImageG...
File src/images/SkDecodingImageGenerator.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/images/SkDecodingImageG...
src/images/SkDecodingImageGenerator.cpp:42: bool onGetInfo(SkImageInfo* info)
SK_OVERRIDE {
On 2015/03/19 13:27:47, scroggo wrote:
> Again, I think you can remove this, if you make SkImageGenerator::getInfo
return
> fInfo when onGetInfo returns false (the default).

deferring until I can remove the guard

https://codereview.chromium.org/1017293002/diff/1/src/images/SkDecodingImageG...
src/images/SkDecodingImageGenerator.cpp:89: const SkImageInfo fInfo;
On 2015/03/19 13:27:47, scroggo wrote:
> Can query getInfo when needed.

?

https://codereview.chromium.org/1017293002/diff/1/src/lazy/SkDiscardablePixel...
File src/lazy/SkDiscardablePixelRef.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/lazy/SkDiscardablePixel...
src/lazy/SkDiscardablePixelRef.cpp:117: if (info.isEmpty() ||
!dst->setInfo(info)) {
On 2015/03/19 13:27:47, scroggo wrote:
> Interesting that calling SkBitmap::setInfo with an empty info would return
true,
> but SkInstallDiscardablePixelRef returns false...

I think setInfo would succeed, but allocPixels would produce a null pixelref,
the same as this factory.

https://codereview.chromium.org/1017293002/diff/1/src/ports/SkImageGenerator_...
File src/ports/SkImageGenerator_skia.cpp (right):

https://codereview.chromium.org/1017293002/diff/1/src/ports/SkImageGenerator_...
src/ports/SkImageGenerator_skia.cpp:51: virtual bool onGetInfo(SkImageInfo*
info) SK_OVERRIDE {
On 2015/03/19 13:27:47, scroggo wrote:
> No longer needed.

?

[reed1, 2015-03-19 15:22:19]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-03-19 15:22:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1017293002/40001

[commit-bot: I haz the power, 2015-03-19 15:22:32]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-03-19 21:22 UTC

[scroggo, 2015-03-19 15:29:16]

On 2015/03/19 15:20:46, reed1 wrote:
> https://codereview.chromium.org/1017293002/diff/1/dm/DMSrcSink.cpp
> File dm/DMSrcSink.cpp (right):
> 
> https://codereview.chromium.org/1017293002/diff/1/dm/DMSrcSink.cpp#newcode58
> dm/DMSrcSink.cpp:58: SkImageInfo info = codec->getInfo();
> On 2015/03/19 13:27:46, scroggo wrote:
> > Yay! FYI: this (and the code in ImageSrc::size()) has moved to CodecSrc.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1017293002/diff/1/gyp/skia_for_android_framew...
> File gyp/skia_for_android_framework_defines.gypi (right):
> 
>
https://codereview.chromium.org/1017293002/diff/1/gyp/skia_for_android_framew...
> gyp/skia_for_android_framework_defines.gypi:17:
> 'SK_SUPPORT_LEGACY_BOOL_ONGETINFO',
> On 2015/03/19 13:27:46, scroggo wrote:
> > Not needed.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
> File include/core/SkImageGenerator.h (left):
> 
>
https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
> include/core/SkImageGenerator.h:71: *  Repeated calls to this function should
> give the same results,
> On 2015/03/19 13:27:47, scroggo wrote:
> > Do you still want this comment (minus the part about the pixelref)?
> 
> Don't think so, as it is now baked in during the constructor. I think the
> inspiration for that comment was as a warning to subclasses, since they were
> responsible for it.
> 
>
https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
> File include/core/SkImageGenerator.h (right):
> 
>
https://codereview.chromium.org/1017293002/diff/1/include/core/SkImageGenerat...
> include/core/SkImageGenerator.h:79: SkImageInfo getInfo() const { return
fInfo;
> }
> On 2015/03/19 13:27:47, scroggo wrote:
> > Why not return a const ref?
> 
> Done.
> 
> https://codereview.chromium.org/1017293002/diff/1/src/codec/SkCodec.cpp
> File src/codec/SkCodec.cpp (right):
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/codec/SkCodec.cpp#newco...
> src/codec/SkCodec.cpp:48: SkCodec::SkCodec(const SkImageInfo& info, SkStream*
> stream) : INHERITED(info)
> On 2015/03/19 13:27:47, scroggo wrote:
> > nit: Shouldn't this go on the next line?
> 
> Done.
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/codec/SkCodec.cpp#newco...
> src/codec/SkCodec.cpp:49: , fInfo(info)
> On 2015/03/19 13:27:47, scroggo wrote:
> > fInfo is no longer needed by SkCodec. We can instead call getInfo().
> > 
> > Taking it a step further, we no longer need
> > getOriginalInfo()
> > SkCodec::onGetInfo
> 
> I thought I would remove the (many) duplicates in subclasses *after* I'm able
to
> remove the compile-guard (since they are needed in the legacy case).
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/core/SkImageGenerator.cpp
> File src/core/SkImageGenerator.cpp (right):
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/core/SkImageGenerator.c...
> src/core/SkImageGenerator.cpp:14: info = SkImageInfo::MakeUnknown(0, 0);
> On 2015/03/19 13:27:47, scroggo wrote:
> > Why not return fInfo here? (Then SkCodec, which sets fInfo in its
constructor,
> > doesn't need to override onGetInfo anymore.)
> 
> I think for compatibility (for a couple days), this is clearer/safer, since
> fInfo *may* have been initialized by the subclass to non-empty.
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/images/SkDecodingImageG...
> File src/images/SkDecodingImageGenerator.cpp (right):
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/images/SkDecodingImageG...
> src/images/SkDecodingImageGenerator.cpp:42: bool onGetInfo(SkImageInfo* info)
> SK_OVERRIDE {
> On 2015/03/19 13:27:47, scroggo wrote:
> > Again, I think you can remove this, if you make SkImageGenerator::getInfo
> return
> > fInfo when onGetInfo returns false (the default).
> 
> deferring until I can remove the guard
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/images/SkDecodingImageG...
> src/images/SkDecodingImageGenerator.cpp:89: const SkImageInfo fInfo;
> On 2015/03/19 13:27:47, scroggo wrote:
> > Can query getInfo when needed.
> 
> ?
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/lazy/SkDiscardablePixel...
> File src/lazy/SkDiscardablePixelRef.cpp (right):
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/lazy/SkDiscardablePixel...
> src/lazy/SkDiscardablePixelRef.cpp:117: if (info.isEmpty() ||
> !dst->setInfo(info)) {
> On 2015/03/19 13:27:47, scroggo wrote:
> > Interesting that calling SkBitmap::setInfo with an empty info would return
> true,
> > but SkInstallDiscardablePixelRef returns false...
> 
> I think setInfo would succeed, but allocPixels would produce a null pixelref,
> the same as this factory.
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/ports/SkImageGenerator_...
> File src/ports/SkImageGenerator_skia.cpp (right):
> 
>
https://codereview.chromium.org/1017293002/diff/1/src/ports/SkImageGenerator_...
> src/ports/SkImageGenerator_skia.cpp:51: virtual bool onGetInfo(SkImageInfo*
> info) SK_OVERRIDE {
> On 2015/03/19 13:27:47, scroggo wrote:
> > No longer needed.
> 
> ?

lgtm

For both of your "?"s, I was assuming you'd made the other changes I suggested:
make legacy getInfo return fInfo if onGetInfo returned false. We're waiting to
do that for the next CL (after chrome is updated), so you can't yet make these
changes.

[commit-bot: I haz the power, 2015-03-19 15:31:17]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/3ef71e343bf075888fb50892350390b4dd47de24