Switch DrawImage to sk_sp<>

Switch DrawImage to sk_sp<>

As suggested in https://codereview.chromium.org/1869753003/,
converts DrawImage to storing and returning sk_sp<const SkImage> instead of const SkImage*.

BUG=skia:5077
R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/836d81873a23b3f3ad1fae3c3a96f7722df07510
Cr-Commit-Position: refs/heads/master@{#389264}

[tomhudson, 2016-04-19 19:13:38]

Description was changed from

==========
WIP Switch DrawImage to sk_sp<>

As suggested in https://codereview.chromium.org/1869753003/, explores
converting DrawImage to returning (and thus storing) sk_sp<const SkImage>
instead of const SkImage*.

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

to

==========
WIP Switch DrawImage to sk_sp<>

As suggested in https://codereview.chromium.org/1869753003/, explores
converting DrawImage to returning (and thus storing) sk_sp<const SkImage>
instead of const SkImage*.

BUG=skia:5077
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[tomhudson, 2016-04-19 19:17:22]

tomhudson@google.com changed reviewers:
+ danakj@chromium.org

[tomhudson, 2016-04-19 19:17:23]

Dana, is this what you were asking for with your request to make image.image()
return sk_sp<>?

Can't land it yet because there are runtime issues - DrawImage holding raw
pointers before was apparently hiding some implicit management or a leak?

I separated this out from the big CL because it started getting messy. It's also
more heavyweight than I expected - for example,
ImageDecodeControllerKey::FromDrawImage() now has something like ten extra
atomic ops because it calls image.image()->width(), image.image()->height(),
image.image()->uniqueID()...

[tomhudson, 2016-04-20 12:14:52]

Notes from last night's debugging: the GetDecodedImageForDraw() unit test is
setting up at least 4 now-reference-counted pointers to the SkImage of interest
by the time it's done with
SoftwareImageDecodeController::GetTaskForImageAndRef().

image
draw_image.image_
task->image.draw_image.image_
task->image_ref_

Somewhere in task->RunOnWorkerThread() we unref.

At scope exit we try to unref four more times, and underflow the refcnt.

[tomhudson, 2016-04-20 14:48:55]

Description was changed from

==========
WIP Switch DrawImage to sk_sp<>

As suggested in https://codereview.chromium.org/1869753003/, explores
converting DrawImage to returning (and thus storing) sk_sp<const SkImage>
instead of const SkImage*.

BUG=skia:5077
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Switch DrawImage to sk_sp<>

As suggested in https://codereview.chromium.org/1869753003/, 
converts DrawImage to storing and returning sk_sp<const SkImage> instead of
const SkImage*.

BUG=skia:5077
R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[tomhudson, 2016-04-20 14:49:05]

The CQ bit was checked by tomhudson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-20 14:49:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900953004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900953004/80001

[commit-bot: I haz the power, 2016-04-20 14:51:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 14:51:32]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tomhudson, 2016-04-20 16:21:36]

The CQ bit was checked by tomhudson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-20 16:21:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900953004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900953004/100001

[commit-bot: I haz the power, 2016-04-20 16:29:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 16:29:55]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[tomhudson, 2016-04-20 17:29:02]

The CQ bit was checked by tomhudson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-20 17:29:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900953004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900953004/120001

[commit-bot: I haz the power, 2016-04-20 18:45:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 18:45:08]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tomhudson, 2016-04-20 18:45:34]

PTAL.

I think this would *also* let us get rid of ImageDecodeTaskImpl::image_ref_,
since DrawImage objects will be keeping a ref anyway.

[danakj, 2016-04-20 20:02:20]

Thanks! Looks good, we can go a bit further, and reduce some ref churn in a few
places.

https://codereview.chromium.org/1900953004/diff/120001/cc/layers/picture_laye...
File cc/layers/picture_layer_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer_unittest.cc:305:
client.add_draw_image(CreateDiscardableImage(layer_size).get(), gfx::Point(),
pass the sk_sp here, and to the ImageData constructor?

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.cc:13: : image_(image),
move

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.cc:21: : image_(image),
make this constructor delegate to the one above

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.cc:27: DecodedDrawImage::DecodedDrawImage(const
DecodedDrawImage& other)
= default?

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.cc:34: DecodedDrawImage::~DecodedDrawImage() {}
= default

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.h:29: sk_sp<const SkImage> image() const { return
image_; }
return const&?

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
File cc/playback/discardable_image_map.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map.cc:148: std::make_pair(DrawImage(image,
src_irect, filter_quality, matrix),
move image

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
File cc/playback/discardable_image_map_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:30: : image(image),
image_rect(image_rect) {}
move image

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:83: discardable_image[y][x].get(),
gfx::Point(x * 512 + 6, y * 512 + 6),
pass the sk_sp

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:160:
discardable_image[y][x].get(),
pass the sk_sp

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:304: discardable_image =
CreateDiscardableImage(gfx::Size(1 << 25, 1 << 25));
you could merge these 2 statements

sk_sp<SkImage> discardable_image = CreateDiscardableImage(...)

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:306:
content_layer_client.add_draw_image(discardable_image.get(), gfx::Point(0, 0),
pass the sk_sp

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:336: discardable_image =
CreateDiscardableImage(gfx::Size(10, 10));
ditto merge

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image.cc
File cc/playback/draw_image.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image...
cc/playback/draw_image.cc:37: : image_(image),
move

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image...
cc/playback/draw_image.cc:44: DrawImage::DrawImage(const DrawImage& other)
this constructor should be =default?

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image...
cc/playback/draw_image.cc:52: DrawImage::~DrawImage() {}
=default

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image.h
File cc/playback/draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image...
cc/playback/draw_image.h:29: sk_sp<const SkImage> image() const { return image_;
}
return const&

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/image_hija...
File cc/playback/image_hijack_canvas.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/image_hija...
cc/playback/image_hijack_canvas.cc:28: draw_image_(image,
move image

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/image_hija...
cc/playback/image_hijack_canvas.cc:34: DCHECK(image->isLazyGenerated());
and dcheck draw_image_.image()->isLaz...

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_controller.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:68: image_ref_(draw_image.image()),
ya we already have a ref in image_, so we can kill this

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:68: image_ref_(image.image()),
kill image_ref here too?

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:381: return
GetOriginalImageDecode(key, image);
move image

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:386: return
GetScaledImageDecode(key, image);
move

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:543: DrawImage
original_size_draw_image(image, gfx::RectToSkIRect(full_image_rect),
|image| not used after this right? move it

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.h:144: sk_sp<SkImage> image() const {
return const&

[tomhudson, 2016-04-21 14:17:00]

The CQ bit was checked by tomhudson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 14:17:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900953004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900953004/140001

[tomhudson, 2016-04-21 14:17:25]

(easy parts)

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.cc:21: : image_(image),
On 2016/04/20 20:02:18, danakj wrote:
> make this constructor delegate to the one above

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.cc:27: DecodedDrawImage::DecodedDrawImage(const
DecodedDrawImage& other)
On 2016/04/20 20:02:18, danakj wrote:
> = default?

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.cc:34: DecodedDrawImage::~DecodedDrawImage() {}
On 2016/04/20 20:02:18, danakj wrote:
> = default

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.h:29: sk_sp<const SkImage> image() const { return
image_; }
On 2016/04/20 20:02:19, danakj wrote:
> return const&?

Our current policy is *not* to do that because it makes the refcounting behavior
too subtle: if you copy the object returned you take a ref, if you just store it
in a reference you don't.

There's a "sleepy debate" about it on the team, you can chip in to influence
that in the longer term, but for now, no.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
File cc/playback/discardable_image_map.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map.cc:148: std::make_pair(DrawImage(image,
src_irect, filter_quality, matrix),
On 2016/04/20 20:02:19, danakj wrote:
> move image

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
File cc/playback/discardable_image_map_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:30: : image(image),
image_rect(image_rect) {}
On 2016/04/20 20:02:19, danakj wrote:
> move image

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:304: discardable_image =
CreateDiscardableImage(gfx::Size(1 << 25, 1 << 25));
On 2016/04/20 20:02:19, danakj wrote:
> you could merge these 2 statements
> 
> sk_sp<SkImage> discardable_image = CreateDiscardableImage(...)

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image.cc
File cc/playback/draw_image.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image...
cc/playback/draw_image.cc:37: : image_(image),
On 2016/04/20 20:02:19, danakj wrote:
> move

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image...
cc/playback/draw_image.cc:44: DrawImage::DrawImage(const DrawImage& other)
On 2016/04/20 20:02:19, danakj wrote:
> this constructor should be =default?

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image...
cc/playback/draw_image.cc:52: DrawImage::~DrawImage() {}
On 2016/04/20 20:02:19, danakj wrote:
> =default

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image.h
File cc/playback/draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image...
cc/playback/draw_image.h:29: sk_sp<const SkImage> image() const { return image_;
}
On 2016/04/20 20:02:19, danakj wrote:
> return const&

No, see above for excessive subtlety.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/image_hija...
File cc/playback/image_hijack_canvas.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/image_hija...
cc/playback/image_hijack_canvas.cc:28: draw_image_(image,
On 2016/04/20 20:02:20, danakj wrote:
> move image

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/image_hija...
cc/playback/image_hijack_canvas.cc:34: DCHECK(image->isLazyGenerated());
On 2016/04/20 20:02:20, danakj wrote:
> and dcheck draw_image_.image()->isLaz...

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_controller.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:68: image_ref_(draw_image.image()),
On 2016/04/20 20:02:20, danakj wrote:
> ya we already have a ref in image_, so we can kill this

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:68: image_ref_(image.image()),
On 2016/04/20 20:02:20, danakj wrote:
> kill image_ref here too?

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:381: return
GetOriginalImageDecode(key, image);
On 2016/04/20 20:02:20, danakj wrote:
> move image

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:386: return
GetScaledImageDecode(key, image);
On 2016/04/20 20:02:20, danakj wrote:
> move

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:543: DrawImage
original_size_draw_image(image, gfx::RectToSkIRect(full_image_rect),
On 2016/04/20 20:02:20, danakj wrote:
> |image| not used after this right? move it

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.h:144: sk_sp<SkImage> image() const {
On 2016/04/20 20:02:20, danakj wrote:
> return const&

No, excessively subtle - discussed above.

[commit-bot: I haz the power, 2016-04-21 14:27:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 14:27:59]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tomhudson, 2016-04-21 14:37:50]

https://codereview.chromium.org/1900953004/diff/120001/cc/layers/picture_laye...
File cc/layers/picture_layer_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/layers/picture_laye...
cc/layers/picture_layer_unittest.cc:305:
client.add_draw_image(CreateDiscardableImage(layer_size).get(), gfx::Point(),
On 2016/04/20 20:02:18, danakj wrote:
> pass the sk_sp here, and to the ImageData constructor?

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.cc:13: : image_(image),
On 2016/04/20 20:02:18, danakj wrote:
> move

Done. (in previous patchset)

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
File cc/playback/discardable_image_map_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:83: discardable_image[y][x].get(),
gfx::Point(x * 512 + 6, y * 512 + 6),
On 2016/04/20 20:02:19, danakj wrote:
> pass the sk_sp

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:160:
discardable_image[y][x].get(),
On 2016/04/20 20:02:19, danakj wrote:
> pass the sk_sp

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:306:
content_layer_client.add_draw_image(discardable_image.get(), gfx::Point(0, 0),
On 2016/04/20 20:02:19, danakj wrote:
> pass the sk_sp

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:336: discardable_image =
CreateDiscardableImage(gfx::Size(10, 10));
On 2016/04/20 20:02:19, danakj wrote:
> ditto merge

Done. (previous patchset)

[tomhudson, 2016-04-21 14:37:56]

The CQ bit was checked by tomhudson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 14:38:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900953004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900953004/160001

[commit-bot: I haz the power, 2016-04-21 16:55:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 16:55:23]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tomhudson, 2016-04-21 17:22:13]

tomhudson@google.com changed reviewers:
+ fmalita@chromium.org

[tomhudson, 2016-04-21 17:22:14]

linux_android_rel_ng looks like a bot infra issue; after the 2nd review patchset
I now think this has everything danakj@ asked for.

Adding fmalita@ since he's still commenting on the CL-this-is-mostly-replacing;
some of his concerns may be valid here.

[vmpstr, 2016-04-21 18:40:57]

vmpstr@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2016-04-21 18:40:58]

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_controller.cc (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:684: SkPixmap p(image_info,
image_data->decode.data->data(),
nit: pixmap instead of p

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller.cc (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:891: SkPixmap p(image_info_,
memory_->data(), image_info_.minRowBytes());
nit: pixmap instead of p

[tomhudson, 2016-04-21 19:03:27]

On 2016/04/21 18:40:58, vmpstr wrote:

>
https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
> cc/tiles/gpu_image_decode_controller.cc:684: SkPixmap p(image_info,
> image_data->decode.data->data(),
> nit: pixmap instead of p

Done.

However, although this is common in Chrome, I don't see this in either the
Chrome or Google style guides.
It seems *exactly* backwards to me. Am I missing a style discussion somewhere?

[f(malita), 2016-04-21 19:50:56]

https://codereview.chromium.org/1900953004/diff/160001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.h:44: sk_sp<const SkImage> image_;
I'm generally wary of sk_sp<const SkFoo>, because Skia factories produce
sk_sp<SkFoo> and Skia consumers ingest sk_sp<SkFoo> => we're likely introducing
friction at some level (hitting the covariant ctor to produce temp values?).

(SkImage/SkPicture/SkTextBlob are intrinsically immutable, so the const
qualifier is redundant)

That said, both Blink and CC are mostly wired with const types at this point, so
if we're going to tackle deconstification it should be done separately.

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_content_l...
File cc/test/fake_content_layer_client.h (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.h:69: ImageData data(image, point, paint);
std::move(image)

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.h:76: ImageData data(image, transform, paint);
ditto

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_recording...
File cc/test/fake_recording_source.h (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_recording...
cc/test/fake_recording_source.h:96: client_.add_draw_image(image, point,
default_paint_);
ditto

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_recording...
cc/test/fake_recording_source.h:101:
client_.add_draw_image_with_transform(image, transform, default_paint_);
ditto

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_recording...
cc/test/fake_recording_source.h:107: client_.add_draw_image(image, point,
paint);
ditto

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_controller.cc (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:298: DecodedDrawImage
decoded_draw_image(image, draw_image.filter_quality());
ditto

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_controller_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller_unittest.cc:18:
bitmap.allocPixels(SkImageInfo::MakeN32Premul(width, height));
Not new to this CL, but we better not do anything with these pixels, because
they're uninitialized.

I think a safer/more modern pattern would be

  sk_sp<SkSurface> surface = SkSurface::MakeRasterN32Premul(width, height);
  surface->getCanvas()->clear(SK_ColorTRANSPARENT);
  return surface->makeImageSnapshot();

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller_unittest.cc:18: return
SkImage::MakeFromBitmap(bitmap);
ditto

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/tile_manager_...
File cc/tiles/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/tile_manager_...
cc/tiles/tile_manager_unittest.cc:1769:
recording_source->add_draw_image(blue_image, gfx::Point());
move(blue_image)

(not critical for a test, but good for conditioning:)

[danakj, 2016-04-21 20:01:31]

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.h:29: sk_sp<const SkImage> image() const { return
image_; }
On 2016/04/21 14:17:24, tomhudson wrote:
> On 2016/04/20 20:02:19, danakj wrote:
> > return const&?
> 
> Our current policy is *not* to do that because it makes the refcounting
behavior
> too subtle: if you copy the object returned you take a ref, if you just store
it
> in a reference you don't.
> 
> There's a "sleepy debate" about it on the team, you can chip in to influence
> that in the longer term, but for now, no.

Well, I think you should do that in cc regardless. It's done with other things
that return a member, this isn't somehow that special than returning any other
copyable class type.

[vmpstr, 2016-04-21 20:15:48]

On 2016/04/21 19:03:27, tomhudson wrote:
> On 2016/04/21 18:40:58, vmpstr wrote:
> 
> >
>
https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
> > cc/tiles/gpu_image_decode_controller.cc:684: SkPixmap p(image_info,
> > image_data->decode.data->data(),
> > nit: pixmap instead of p
> 
> Done.
> 
> However, although this is common in Chrome, I don't see this in either the
> Chrome or Google style guides.
> It seems *exactly* backwards to me. Am I missing a style discussion somewhere?

IMO, this falls under the general google c++ style guide:
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

[f(malita), 2016-04-21 20:23:39]

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.h:29: sk_sp<const SkImage> image() const { return
image_; }
On 2016/04/21 20:01:31, danakj wrote:
> On 2016/04/21 14:17:24, tomhudson wrote:
> > On 2016/04/20 20:02:19, danakj wrote:
> > > return const&?
> > 
> > Our current policy is *not* to do that because it makes the refcounting
> behavior
> > too subtle: if you copy the object returned you take a ref, if you just
store
> it
> > in a reference you don't.
> > 
> > There's a "sleepy debate" about it on the team, you can chip in to influence
> > that in the longer term, but for now, no.
> 
> Well, I think you should do that in cc regardless. It's done with other things
> that return a member, this isn't somehow that special than returning any other
> copyable class type.

Yeah, I would go even further and claim that getters should never front-load the
churn:  if the client is not interested in taking a ref (and only wants to
null-check for example), that's just unnecessary overhead.

[tomhudson, 2016-04-21 20:30:24]

The CQ bit was checked by tomhudson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 20:31:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900953004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900953004/200001

[tomhudson, 2016-04-21 20:31:22]

OK, I guess I know which side of the sleepy debate Florin is on now. Done and
done, says the curmudgeon.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.h:29: sk_sp<const SkImage> image() const { return
image_; }
On 2016/04/20 20:02:19, danakj wrote:
> return const&?

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image.h
File cc/playback/draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/playback/draw_image...
cc/playback/draw_image.h:29: sk_sp<const SkImage> image() const { return image_;
}
On 2016/04/20 20:02:19, danakj wrote:
> return const&

Done.

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller.h (right):

https://codereview.chromium.org/1900953004/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.h:144: sk_sp<SkImage> image() const {
On 2016/04/20 20:02:20, danakj wrote:
> return const&

Done.

https://codereview.chromium.org/1900953004/diff/160001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.h (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.h:44: sk_sp<const SkImage> image_;
On 2016/04/21 19:50:56, f(malita) wrote:
> I'm generally wary of sk_sp<const SkFoo>, because Skia factories produce
> sk_sp<SkFoo> and Skia consumers ingest sk_sp<SkFoo> => we're likely
introducing
> friction at some level (hitting the covariant ctor to produce temp values?).
> 
> (SkImage/SkPicture/SkTextBlob are intrinsically immutable, so the const
> qualifier is redundant)
> 
> That said, both Blink and CC are mostly wired with const types at this point,
so
> if we're going to tackle deconstification it should be done separately.

Acknowledged.

I think we've identified deconstifying Chrome as a separate high-level task,
possibly even not planned until next quarter.

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_content_l...
File cc/test/fake_content_layer_client.h (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.h:69: ImageData data(image, point, paint);
On 2016/04/21 19:50:56, f(malita) wrote:
> std::move(image)

Done.

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.h:76: ImageData data(image, transform, paint);
On 2016/04/21 19:50:56, f(malita) wrote:
> ditto

Done.

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_recording...
File cc/test/fake_recording_source.h (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_recording...
cc/test/fake_recording_source.h:96: client_.add_draw_image(image, point,
default_paint_);
On 2016/04/21 19:50:56, f(malita) wrote:
> ditto

Done.

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_recording...
cc/test/fake_recording_source.h:101:
client_.add_draw_image_with_transform(image, transform, default_paint_);
On 2016/04/21 19:50:56, f(malita) wrote:
> ditto

Done.

https://codereview.chromium.org/1900953004/diff/160001/cc/test/fake_recording...
cc/test/fake_recording_source.h:107: client_.add_draw_image(image, point,
paint);
On 2016/04/21 19:50:56, f(malita) wrote:
> ditto

Done.

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_controller.cc (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:298: DecodedDrawImage
decoded_draw_image(image, draw_image.filter_quality());
On 2016/04/21 19:50:56, f(malita) wrote:
> ditto

Done.

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/tile_manager_...
File cc/tiles/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/160001/cc/tiles/tile_manager_...
cc/tiles/tile_manager_unittest.cc:1769:
recording_source->add_draw_image(blue_image, gfx::Point());
On 2016/04/21 19:50:56, f(malita) wrote:
> move(blue_image)
> 
> (not critical for a test, but good for conditioning:)

Done.

[f(malita), 2016-04-21 20:49:13]

non-owner lgtm

https://codereview.chromium.org/1900953004/diff/200001/cc/playback/discardabl...
File cc/playback/discardable_image_map_unittest.cc (right):

https://codereview.chromium.org/1900953004/diff/200001/cc/playback/discardabl...
cc/playback/discardable_image_map_unittest.cc:346:
generator.canvas()->drawImage(discardable_image.get(), 0, 0, nullptr);
nit: we can now use drawImage(const sk_sp<SkImage>&,...), no .get() needed.

[danakj, 2016-04-22 20:36:04]

LGTM

https://codereview.chromium.org/1900953004/diff/220001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller.h (right):

https://codereview.chromium.org/1900953004/diff/220001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.h:219: sk_sp<const SkImage> image);
I think florin was against saying const for these? or i misunderstood

[tomhudson, 2016-04-22 20:45:21]

The CQ bit was checked by tomhudson@google.com

[tomhudson, 2016-04-22 20:45:21]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1900953004/#ps220001
(title: "Florin's nit")

[commit-bot: I haz the power, 2016-04-22 20:46:15]

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

[f(malita), 2016-04-22 20:46:59]

On 2016/04/22 20:36:04, danakj wrote:
> LGTM
> 
>
https://codereview.chromium.org/1900953004/diff/220001/cc/tiles/software_imag...
> File cc/tiles/software_image_decode_controller.h (right):
> 
>
https://codereview.chromium.org/1900953004/diff/220001/cc/tiles/software_imag...
> cc/tiles/software_image_decode_controller.h:219: sk_sp<const SkImage> image);
> I think florin was against saying const for these? or i misunderstood

Yeah, but they're so ingrained in both Blink/Chromium it's going to take some
work to clean up right - I'm fine with tackling that separately.

[tomhudson, 2016-04-22 20:47:38]

Thanks.

>
https://codereview.chromium.org/1900953004/diff/220001/cc/tiles/software_imag...
> File cc/tiles/software_image_decode_controller.h (right):
> 
>
https://codereview.chromium.org/1900953004/diff/220001/cc/tiles/software_imag...
> cc/tiles/software_image_decode_controller.h:219: sk_sp<const SkImage> image);
> I think florin was against saying const for these? or i misunderstood

Florin also wrote:
> That said, both Blink and CC are mostly wired with const types at this point,
so
> if we're going to tackle deconstification it should be done separately.

[commit-bot: I haz the power, 2016-04-22 22:15:55]

Description was changed from

==========
Switch DrawImage to sk_sp<>

As suggested in https://codereview.chromium.org/1869753003/, 
converts DrawImage to storing and returning sk_sp<const SkImage> instead of
const SkImage*.

BUG=skia:5077
R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Switch DrawImage to sk_sp<>

As suggested in https://codereview.chromium.org/1869753003/, 
converts DrawImage to storing and returning sk_sp<const SkImage> instead of
const SkImage*.

BUG=skia:5077
R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-22 22:15:56]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-04-22 22:17:17]

Description was changed from

==========
Switch DrawImage to sk_sp<>

As suggested in https://codereview.chromium.org/1869753003/, 
converts DrawImage to storing and returning sk_sp<const SkImage> instead of
const SkImage*.

BUG=skia:5077
R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Switch DrawImage to sk_sp<>

As suggested in https://codereview.chromium.org/1869753003/,
converts DrawImage to storing and returning sk_sp<const SkImage> instead of
const SkImage*.

BUG=skia:5077
R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/836d81873a23b3f3ad1fae3c3a96f7722df07510
Cr-Commit-Position: refs/heads/master@{#389264}
==========

[commit-bot: I haz the power, 2016-04-22 22:17:18]

Patchset 12 (id:??) landed as
https://crrev.com/836d81873a23b3f3ad1fae3c3a96f7722df07510
Cr-Commit-Position: refs/heads/master@{#389264}