Replace many skia::RefPtr with sk_sp<> in cc/

Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

R=mkwst@chromium.org,danakj@chromium.org,miguelg@chromium.org,fmalita@chromium.org
TBR=esprehn@chromium.org
BUG=skia:5077
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/992ec28bf1a2821dd704d5acb74a0d382d3c19c5
Cr-Commit-Position: refs/heads/master@{#390102}

[tomhudson, 2016-04-07 14:02:24]

Description was changed from

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

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

to

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

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

[tomhudson, 2016-04-07 14:21:01]

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

[commit-bot: I haz the power, 2016-04-07 14:21:16]

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

[commit-bot: I haz the power, 2016-04-07 14:42:14]

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

[commit-bot: I haz the power, 2016-04-07 14:42:15]

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

[tomhudson, 2016-04-07 15:28:37]

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

[commit-bot: I haz the power, 2016-04-07 15:29:02]

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

[commit-bot: I haz the power, 2016-04-07 15:54:23]

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

[commit-bot: I haz the power, 2016-04-07 15:54:24]

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

[tomhudson, 2016-04-07 16:43:53]

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

[commit-bot: I haz the power, 2016-04-07 16:44:08]

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

[commit-bot: I haz the power, 2016-04-07 17:03:24]

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

[commit-bot: I haz the power, 2016-04-07 17:03:26]

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

[tomhudson, 2016-04-07 17:09:31]

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

[commit-bot: I haz the power, 2016-04-07 17:09:45]

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

[commit-bot: I haz the power, 2016-04-07 17:35:50]

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

[commit-bot: I haz the power, 2016-04-07 17:35:51]

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

[tomhudson, 2016-04-08 21:50:03]

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

[commit-bot: I haz the power, 2016-04-08 21:50:31]

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

[commit-bot: I haz the power, 2016-04-08 21:54:25]

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

[commit-bot: I haz the power, 2016-04-08 21:54:26]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  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-12 20:29:35]

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

[commit-bot: I haz the power, 2016-04-12 20:30:00]

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

[commit-bot: I haz the power, 2016-04-12 23:24:54]

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

[commit-bot: I haz the power, 2016-04-12 23:24:55]

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

[tomhudson, 2016-04-13 15:56:21]

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

[commit-bot: I haz the power, 2016-04-13 15:57:03]

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

[commit-bot: I haz the power, 2016-04-13 16:14:35]

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

[commit-bot: I haz the power, 2016-04-13 16:14:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[tomhudson, 2016-04-13 17:05:09]

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

[commit-bot: I haz the power, 2016-04-13 17:05:40]

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

[commit-bot: I haz the power, 2016-04-13 18:48:40]

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

[commit-bot: I haz the power, 2016-04-13 18:48:41]

Dry run: This issue passed the CQ dry run.

[tomhudson, 2016-04-13 19:18:16]

tomhudson@google.com changed reviewers:
+ danakj@chromium.org, junov@chromium.org, miguelg@chromium.org,
mkwst@chromium.org

[tomhudson, 2016-04-13 19:18:17]

danakj@: please review cc/
miguelg@: please review chrome/browser/android/
mkwst@: please security review content/common/
junov@: please review third_party/WebKit/Source/platform/graphics/

[Mike West, 2016-04-13 20:07:15]

IPC LGTM.

[Miguel Garcia, 2016-04-13 20:13:18]

chrome/browser/android lgtm

[danakj, 2016-04-14 19:37:33]

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.cc:90: const sk_sp<SkImageFilter>& image_filter)
ditto

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.cc:166: return
FilterOperation::CreateReferenceFilter(sk_sp<SkImageFilter>());
nullptr

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.h:150: const sk_sp<SkImageFilter>& image_filter) {
while you're here, with c++11 I think you should write these to pass by value
instead of by reference, and use std::move() when you use it below.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.h:199: void set_image_filter(const
sk_sp<SkImageFilter>& image_filter) {
ditto

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.h:248: FilterOperation(FilterType type, const
sk_sp<SkImageFilter>& image_filter);
ditto

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
File cc/output/filter_operations_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:603:
FilterOperation::CreateReferenceFilter(sk_sp<SkImageFilter>());
nullptr

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:607: return sk_sp<SkImage>();
nullptr

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:610: return sk_sp<SkImage>();
nullptr

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:634: return sk_sp<SkImage>();
nullptr

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:645: return sk_sp<SkImage>();
nullptr

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:663: return sk_sp<SkImage>();
nullptr

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:1439:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
can move the |filter| here

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:149: const sk_sp<SkImageFilter>& input) {
by value

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:152: return sk_sp<SkImageFilter>(
call Make and you don't need to cast/wrap here

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:168: image_filter =
CreateMatrixImageFilter(matrix, image_filter);
should move image_filter on all these calls

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:203: image_filter =
sk_sp<SkImageFilter>(SkDropShadowImageFilter::Create(
call Make instead?

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:225: image_filter =
SkComposeImageFilter::Make(zoom_filter, image_filter);
move zoom_filter too

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:244: cf =
sk_sp<SkColorFilter>(colorfilter_rawptr);
cf.reset(

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:249: image_filter =
CreateMatrixImageFilter(matrix, image_filter);
move image_filter

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:252:
SkComposeImageFilter::Make(op.image_filter(), image_filter);
move image_filter

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:262: image_filter =
SkComposeImageFilter::Make(alpha_filter, image_filter);
move alpha_filter

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:264: image_filter = alpha_filter;
move

https://codereview.chromium.org/1869753003/diff/140001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1321:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
move filter

https://codereview.chromium.org/1869753003/diff/140001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1560:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
ditto

https://codereview.chromium.org/1869753003/diff/140001/cc/output/software_ren...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/software_ren...
cc/output/software_renderer.cc:157: sk_sp<SkCanvas>(new
SkCanvas(current_framebuffer_lock_->sk_bitmap()));
is there nothing like make_scoped_refptr, make_scoped_ptr, WrapUnique, etc for
sk_sp?

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

https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
cc/playback/discardable_image_map.cc:166: return sk_sp<SkCanvas>(new
DiscardableImagesMetadataCanvas(
no make_scoped_refptr equivalent?

oh i found sk_make_sp. use that?

https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
File cc/playback/discardable_image_map.h (left):

https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
cc/playback/discardable_image_map.h:14: #include "skia/ext/refptr.h"
we should technically be including SkRefCnt.h in every place that uses sk_sp.
might come back to bite you that you're not (it's been a huuuge pain for
scoped_ptr.h)

https://codereview.chromium.org/1869753003/diff/140001/cc/raster/tile_task_wo...
File cc/raster/tile_task_worker_pool_perftest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/raster/tile_task_wo...
cc/raster/tile_task_worker_pool_perftest.cc:109: scoped_ptr<PerfGLES2Interface>
context_gl_;
undo

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.cc:47: pixel_ref_ = pixel_ref;
move

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.cc:78: Create(pixel_ref, size,
UIResourceBitmap::ETC1);
move

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.h:44: UIResourceBitmap(const sk_sp<SkPixelRef>&
pixel_ref, const gfx::Size& size);
pass by value

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.h:51: void Create(const sk_sp<SkPixelRef>&
pixel_ref,
pass by value

https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
File cc/test/fake_content_layer_client.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.cc:20:
FakeContentLayerClient::ImageData::ImageData(const SkImage* img,
can you make this an sk_sp?

https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.cc:25:
FakeContentLayerClient::ImageData::ImageData(const SkImage* img,
and this?

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

https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.h:15: #include "skia/ext/refptr.h"
?

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

https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:66:
image_ref_(sk_ref_sp(draw_image.image())),
make image() return sk_sp

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

https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:66:
image_ref_(sk_ref_sp(image.image())),
make image() return sk_sp

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_blending.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_blending.cc:148:
layer->SetImage(skia::SharePtr(image));
move image

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_blending.cc:171:
mask->SetImage(skia::SharePtr(image));
move image

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_filters.cc:254:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
move filter

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_filters.cc:566:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
move filter

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_filters.cc:614:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
move filter

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_masks.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_masks.cc:114:
mask->SetImage(skia::SharePtr(image));
move image

https://codereview.chromium.org/1869753003/diff/140001/content/common/cc_mess...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/content/common/cc_mess...
content/common/cc_messages_unittest.cc:288:
cc::FilterOperation::CreateReferenceFilter(arbitrary_filter));
move filter

[tomhudson, 2016-04-15 22:57:58]

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

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

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

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

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

[commit-bot: I haz the power, 2016-04-15 23:02:57]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fmalita_google_do_not_use, 2016-04-16 15:48:41]

fmalita@google.com changed reviewers:
+ fmalita@google.com

[fmalita_google_do_not_use, 2016-04-16 15:48:42]

https://codereview.chromium.org/1869753003/diff/160001/cc/output/filter_opera...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/1869753003/diff/160001/cc/output/filter_opera...
cc/output/filter_operation.h:150: const sk_sp<SkImageFilter> image_filter) {
When pass-by-val, the param should be non-const - otherwise we're not going to
trigger the move ctor below.

(ditto for all other methods)

[tomhudson, 2016-04-18 19:30:44]

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

[commit-bot: I haz the power, 2016-04-18 19:31:11]

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

[tomhudson, 2016-04-18 19:43:38]

Description was changed from

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

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

to

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

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

[tomhudson, 2016-04-18 19:43:38]

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

[commit-bot: I haz the power, 2016-04-18 20:32:40]

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

[commit-bot: I haz the power, 2016-04-18 20:32:42]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[f(malita), 2016-04-21 15:45:47]

https://codereview.chromium.org/1869753003/diff/200001/cc/layers/picture_imag...
File cc/layers/picture_image_layer_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/layers/picture_imag...
cc/layers/picture_image_layer_unittest.cc:42:
layer->SetImage(skia::SharePtr(image));
I think the new version introduces extra churn: SharePtr takes a new sk_sp value
(while AdoptRef would just steal the rval ref).  Inlining (or prev AdoptRef +
move) should avoid that:

  layer->SetImage(skia::AdoptRef(image_surface->makeImageSnapshot()));

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operation.cc:96: image_filter_(image_filter),
std::move(image_filter)

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operation.h:201: image_filter_ = image_filter;
std::move(image_filter)

(now that the params are non-const values, we can use move more aggressively)

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
File cc/output/filter_operations_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:40: sk_sp<SkImageFilter>
filter(SkDropShadowImageFilter::Create(
SkDropShadowImageFilter::Make(...)

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:45:
ops.Append(FilterOperation::CreateReferenceFilter(filter));
(not particularly important for a test, but good for consistency/conditioning:)

move(filter)

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:57: sk_sp<SkImageFilter>
filter(SkDropShadowImageFilter::Create(
SkDropShadowImageFilter::Make(...)

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:62:
ops.Append(FilterOperation::CreateReferenceFilter(filter));
move(filter)

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:584: FilterOperation to =
FilterOperation::CreateReferenceFilter(to_filter);
move(from_filter/to_filter).

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:601: FilterOperation filter =
FilterOperation::CreateReferenceFilter(image_filter);
move(image_filter)

https://codereview.chromium.org/1869753003/diff/200001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:654: surface->getCanvas()->drawImage(srcImage.get(),
src_rect.x(), src_rect.y(),
nit: we can use the drawImage(const sk_sp<SkImage>&, ...) now, .get() no longer
needed.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/render_surfa...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:150:
SkColorFilter::MakeMatrixFilterRowMajor255(matrix), input);
move(input)

https://codereview.chromium.org/1869753003/diff/200001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:240: // XXXXXXXXXXXXXXXXXXXXXXXXXX
forgot marker?

https://codereview.chromium.org/1869753003/diff/200001/cc/playback/display_it...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/playback/display_it...
cc/playback/display_item_list.cc:92: canvas_ =
sk_ref_sp(recorder_->beginRecording(
(note to self, maybe add a TODO(fmalita)?)

Tracking the recorder canvas separately seems fragile - it is intrinsically tied
to the recorder state.  We should probably dump canvas_ and just use
recorder_->getCanvas() instead.

https://codereview.chromium.org/1869753003/diff/200001/cc/playback/filter_dis...
File cc/playback/filter_display_item.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/playback/filter_dis...
cc/playback/filter_display_item.cc:72: paint.setImageFilter(image_filter.get());
std::move(image_filter)

https://codereview.chromium.org/1869753003/diff/200001/cc/resources/ui_resour...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.cc:72: Create(pixel_ref, size,
UIResourceBitmap::RGBA8);
move(pixel_ref)

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

https://codereview.chromium.org/1869753003/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:109: image_ref_(draw_image.image()),
sk_ref_sp(draw_image.image())?

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

https://codereview.chromium.org/1869753003/diff/200001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:66: image_ref_(image.image()),
sk_ref_sp(image.image())?

https://codereview.chromium.org/1869753003/diff/200001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:372: sk_sp<const SkImage>
image(draw_image.image());
sk_ref_sp(draw_image.image())?

(or just leave as is/raw ptr - I don't think having a local sk_sp is helping
much here)

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/damage_tracke...
File cc/trees/damage_tracker_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/damage_tracke...
cc/trees/damage_tracker_unittest.cc:568:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
move(filter)

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:7759: UIResourceBitmap
bitmap(pixel_ref, size);
move(pixel_ref)

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_blending.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_blending.cc:147:
layer->SetImage(skia::SharePtr(backing_store->makeImageSnapshot()));
nit: in this particular case I think they're functionally equivalent (both steal
the ref value), but skia::AdoptRef is easier to grok for me.

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_blending.cc:169:
mask->SetImage(skia::SharePtr(surface->makeImageSnapshot()));
nit: ditto

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_masks.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_masks.cc:113:
mask->SetImage(skia::SharePtr(surface->makeImageSnapshot()));
nit: ditto

https://codereview.chromium.org/1869753003/diff/200001/chrome/browser/android...
File chrome/browser/android/thumbnail/thumbnail.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail.cc:81: bitmap_ =
cc::UIResourceBitmap(compressed_bitmap, data_size);
move(compressed_bitmap)

https://codereview.chromium.org/1869753003/diff/200001/content/common/cc_mess...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/content/common/cc_mess...
content/common/cc_messages.cc:131: r->set_image_filter(filter);
I can't quite tell what param type set_image_filter expects, but if possible we
should move(filter) here.

https://codereview.chromium.org/1869753003/diff/200001/skia/ext/platform_canv...
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/1869753003/diff/200001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:92: SK_API SkCanvas*
CreateCanvas(sk_sp<SkBaseDevice>& device,
const sk_sp<SkBaseDevice>&

(
We normally use either const sk_sp<>& or sk_sp<>.  In this case the former looks
like a better fit because all we use is its raw value.

If SkCanvas ever grows a sk_sp<SkDevice> ctor, we can switch to the latter.
)

https://codereview.chromium.org/1869753003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.cpp
(right):

https://codereview.chromium.org/1869753003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.cpp:91:
void CompositorFilterOperations::appendReferenceFilter(SkImageFilter*
imageFilter)
Can we update the signature to sk_sp<SkImageFilter>?

(if it's not trivial just add a TODO - this CL is already massive :)

[danakj, 2016-04-21 23:37:21]

https://codereview.chromium.org/1869753003/diff/200001/skia/ext/platform_canv...
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/1869753003/diff/200001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:92: SK_API SkCanvas*
CreateCanvas(sk_sp<SkBaseDevice>& device,
On 2016/04/21 15:45:47, f(malita) wrote:
> const sk_sp<SkBaseDevice>&
> 
> (
> We normally use either const sk_sp<>& or sk_sp<>.  In this case the former
looks
> like a better fit because all we use is its raw value.
> 
> If SkCanvas ever grows a sk_sp<SkDevice> ctor, we can switch to the latter.
> )

If you're not going to ref the thing, take a T* instead of a const sk_sp<T>&.

[tomhudson, 2016-04-25 16:34:13]

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

[commit-bot: I haz the power, 2016-04-25 16:34:37]

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

[commit-bot: I haz the power, 2016-04-25 19:50:54]

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

[commit-bot: I haz the power, 2016-04-25 19:50:55]

Dry run: This issue passed the CQ dry run.

[tomhudson, 2016-04-25 20:20:51]

Florin's recent review. I think the only question is AdoptRef vs SharePtr.

https://codereview.chromium.org/1869753003/diff/160001/cc/output/filter_opera...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/1869753003/diff/160001/cc/output/filter_opera...
cc/output/filter_operation.h:150: const sk_sp<SkImageFilter> image_filter) {
On 2016/04/16 15:48:42, fmalita_google_do_not_use wrote:
> When pass-by-val, the param should be non-const - otherwise we're not going to
> trigger the move ctor below.
> 
> (ditto for all other methods)

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/layers/picture_imag...
File cc/layers/picture_image_layer_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/layers/picture_imag...
cc/layers/picture_image_layer_unittest.cc:42:
layer->SetImage(skia::SharePtr(image));
On 2016/04/21 15:45:46, f(malita) wrote:
> I think the new version introduces extra churn: SharePtr takes a new sk_sp
value
> (while AdoptRef would just steal the rval ref).  Inlining (or prev AdoptRef +
> move) should avoid that:
> 
>   layer->SetImage(skia::AdoptRef(image_surface->makeImageSnapshot()));

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operation.cc:96: image_filter_(image_filter),
On 2016/04/21 15:45:46, f(malita) wrote:
> std::move(image_filter)

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operation.h:201: image_filter_ = image_filter;
On 2016/04/21 15:45:46, f(malita) wrote:
> std::move(image_filter)
> 
> (now that the params are non-const values, we can use move more aggressively)

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
File cc/output/filter_operations_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:40: sk_sp<SkImageFilter>
filter(SkDropShadowImageFilter::Create(
On 2016/04/21 15:45:46, f(malita) wrote:
> SkDropShadowImageFilter::Make(...)

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:45:
ops.Append(FilterOperation::CreateReferenceFilter(filter));
On 2016/04/21 15:45:46, f(malita) wrote:
> (not particularly important for a test, but good for
consistency/conditioning:)
> 
> move(filter)
How about I just inline the Make() call?

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:57: sk_sp<SkImageFilter>
filter(SkDropShadowImageFilter::Create(
On 2016/04/21 15:45:46, f(malita) wrote:
> SkDropShadowImageFilter::Make(...)

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:62:
ops.Append(FilterOperation::CreateReferenceFilter(filter));
On 2016/04/21 15:45:46, f(malita) wrote:
> move(filter)

inlined again?

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:584: FilterOperation to =
FilterOperation::CreateReferenceFilter(to_filter);
On 2016/04/21 15:45:46, f(malita) wrote:
> move(from_filter/to_filter).

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:601: FilterOperation filter =
FilterOperation::CreateReferenceFilter(image_filter);
On 2016/04/21 15:45:46, f(malita) wrote:
> move(image_filter)

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:654: surface->getCanvas()->drawImage(srcImage.get(),
src_rect.x(), src_rect.y(),
On 2016/04/21 15:45:47, f(malita) wrote:
> nit: we can use the drawImage(const sk_sp<SkImage>&, ...) now, .get() no
longer
> needed.

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/render_surfa...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:150:
SkColorFilter::MakeMatrixFilterRowMajor255(matrix), input);
On 2016/04/21 15:45:47, f(malita) wrote:
> move(input)

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:240: // XXXXXXXXXXXXXXXXXXXXXXXXXX
On 2016/04/21 15:45:47, f(malita) wrote:
> forgot marker?

Ugh, my bad. Removed.

https://codereview.chromium.org/1869753003/diff/200001/cc/playback/display_it...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/playback/display_it...
cc/playback/display_item_list.cc:92: canvas_ =
sk_ref_sp(recorder_->beginRecording(
On 2016/04/21 15:45:47, f(malita) wrote:
> (note to self, maybe add a TODO(fmalita)?)
> 
> Tracking the recorder canvas separately seems fragile - it is intrinsically
tied
> to the recorder state.  We should probably dump canvas_ and just use
> recorder_->getCanvas() instead.

Acknowledged.

https://codereview.chromium.org/1869753003/diff/200001/cc/playback/filter_dis...
File cc/playback/filter_display_item.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/playback/filter_dis...
cc/playback/filter_display_item.cc:72: paint.setImageFilter(image_filter.get());
On 2016/04/21 15:45:47, f(malita) wrote:
> std::move(image_filter)

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/resources/ui_resour...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.cc:72: Create(pixel_ref, size,
UIResourceBitmap::RGBA8);
On 2016/04/21 15:45:47, f(malita) wrote:
> move(pixel_ref)

Done.

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

https://codereview.chromium.org/1869753003/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:109: image_ref_(draw_image.image()),
On 2016/04/21 15:45:47, f(malita) wrote:
> sk_ref_sp(draw_image.image())?

Dana's suggestions made this member go away.

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

https://codereview.chromium.org/1869753003/diff/200001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:66: image_ref_(image.image()),
On 2016/04/21 15:45:47, f(malita) wrote:
> sk_ref_sp(image.image())?

Dana's suggestions made this member go away.

https://codereview.chromium.org/1869753003/diff/200001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:372: sk_sp<const SkImage>
image(draw_image.image());
On 2016/04/21 15:45:47, f(malita) wrote:
> sk_ref_sp(draw_image.image())?
> 
> (or just leave as is/raw ptr - I don't think having a local sk_sp is helping
> much here)

This is now returning sk_sp<>, and passing it on to clients - who need it in
that form because they may take a ref.

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/damage_tracke...
File cc/trees/damage_tracker_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/damage_tracke...
cc/trees/damage_tracker_unittest.cc:568:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
On 2016/04/21 15:45:47, f(malita) wrote:
> move(filter)

inlined instead.

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:7759: UIResourceBitmap
bitmap(pixel_ref, size);
On 2016/04/21 15:45:47, f(malita) wrote:
> move(pixel_ref)

Done.

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_blending.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_blending.cc:147:
layer->SetImage(skia::SharePtr(backing_store->makeImageSnapshot()));
On 2016/04/21 15:45:47, f(malita) wrote:
> nit: in this particular case I think they're functionally equivalent (both
steal
> the ref value), but skia::AdoptRef is easier to grok for me.

I don't understand - AdoptRef *does not* increment the reference count, but
SharePtr *does* increment the refcount, right?

https://codereview.chromium.org/1869753003/diff/200001/chrome/browser/android...
File chrome/browser/android/thumbnail/thumbnail.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail.cc:81: bitmap_ =
cc::UIResourceBitmap(compressed_bitmap, data_size);
On 2016/04/21 15:45:47, f(malita) wrote:
> move(compressed_bitmap)

Done.

https://codereview.chromium.org/1869753003/diff/200001/content/common/cc_mess...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/content/common/cc_mess...
content/common/cc_messages.cc:131: r->set_image_filter(filter);
On 2016/04/21 15:45:47, f(malita) wrote:
> I can't quite tell what param type set_image_filter expects, but if possible
we
> should move(filter) here.

Yes, cc::FilterOperation() takes sk_sp<>, so we can std::move() here. Done.

https://codereview.chromium.org/1869753003/diff/200001/skia/ext/platform_canv...
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/1869753003/diff/200001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:92: SK_API SkCanvas*
CreateCanvas(sk_sp<SkBaseDevice>& device,
On 2016/04/21 15:45:47, f(malita) wrote:
> const sk_sp<SkBaseDevice>&
> 
> (
> We normally use either const sk_sp<>& or sk_sp<>.  In this case the former
looks
> like a better fit because all we use is its raw value.
> 
> If SkCanvas ever grows a sk_sp<SkDevice> ctor, we can switch to the latter.
> )

Done.

https://codereview.chromium.org/1869753003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.cpp
(right):

https://codereview.chromium.org/1869753003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorFilterOperations.cpp:91:
void CompositorFilterOperations::appendReferenceFilter(SkImageFilter*
imageFilter)
On 2016/04/21 15:45:47, f(malita) wrote:
> Can we update the signature to sk_sp<SkImageFilter>?
> 
> (if it's not trivial just add a TODO - this CL is already massive :)

Done.

[tomhudson, 2016-04-25 20:48:16]

Dana: the one thing that seemed problematic was your proposals in
layer_tree_host_pixeltest_blending and layer_tree_host_pixeltest_masks.

If I make PictureImageLayer take sk_sp<>, that's also being written to by
WebImageLayerImpl::setImage() (among others), which only has a raw pointer. The
member function signature is inherited out of Blink's WebImageLayer, and AFAIK
per Florin we currently aren't exposing sk_sp<> in most WebKit APIs?

Florin, I know we've *started* relaxing that restriction, but need to know if
Blink is ready to let our smart pointers in more generally.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
File cc/output/filter_operation.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.cc:90: const sk_sp<SkImageFilter>& image_filter)
On 2016/04/14 19:37:31, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.cc:166: return
FilterOperation::CreateReferenceFilter(sk_sp<SkImageFilter>());
On 2016/04/14 19:37:31, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.h:150: const sk_sp<SkImageFilter>& image_filter) {
On 2016/04/14 19:37:31, danakj wrote:
> while you're here, with c++11 I think you should write these to pass by value
> instead of by reference, and use std::move() when you use it below.

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.h:199: void set_image_filter(const
sk_sp<SkImageFilter>& image_filter) {
On 2016/04/14 19:37:31, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operation.h:248: FilterOperation(FilterType type, const
sk_sp<SkImageFilter>& image_filter);
On 2016/04/14 19:37:31, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
File cc/output/filter_operations_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
cc/output/filter_operations_unittest.cc:603:
FilterOperation::CreateReferenceFilter(sk_sp<SkImageFilter>());
On 2016/04/14 19:37:31, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:607: return sk_sp<SkImage>();
On 2016/04/14 19:37:31, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:610: return sk_sp<SkImage>();
On 2016/04/14 19:37:31, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:634: return sk_sp<SkImage>();
On 2016/04/14 19:37:31, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:645: return sk_sp<SkImage>();
On 2016/04/14 19:37:31, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:663: return sk_sp<SkImage>();
On 2016/04/14 19:37:31, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:1439:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
On 2016/04/14 19:37:31, danakj wrote:
> can move the |filter| here

inlined instead.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:149: const sk_sp<SkImageFilter>& input) {
On 2016/04/14 19:37:32, danakj wrote:
> by value

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:152: return sk_sp<SkImageFilter>(
On 2016/04/14 19:37:32, danakj wrote:
> call Make and you don't need to cast/wrap here

Done.

(Some of those Make() calls on filters were only written after this CL started,
but they should all be finished. now.)

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:168: image_filter =
CreateMatrixImageFilter(matrix, image_filter);
On 2016/04/14 19:37:32, danakj wrote:
> should move image_filter on all these calls

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:203: image_filter =
sk_sp<SkImageFilter>(SkDropShadowImageFilter::Create(
On 2016/04/14 19:37:31, danakj wrote:
> call Make instead?

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:225: image_filter =
SkComposeImageFilter::Make(zoom_filter, image_filter);
On 2016/04/14 19:37:32, danakj wrote:
> move zoom_filter too

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:244: cf =
sk_sp<SkColorFilter>(colorfilter_rawptr);
On 2016/04/14 19:37:32, danakj wrote:
> cf.reset(

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:249: image_filter =
CreateMatrixImageFilter(matrix, image_filter);
On 2016/04/14 19:37:31, danakj wrote:
> move image_filter

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:252:
SkComposeImageFilter::Make(op.image_filter(), image_filter);
On 2016/04/14 19:37:32, danakj wrote:
> move image_filter

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:262: image_filter =
SkComposeImageFilter::Make(alpha_filter, image_filter);
On 2016/04/14 19:37:32, danakj wrote:
> move alpha_filter

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
cc/output/render_surface_filters.cc:264: image_filter = alpha_filter;
On 2016/04/14 19:37:32, danakj wrote:
> move

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1321:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
On 2016/04/14 19:37:32, danakj wrote:
> move filter

inlined.

https://codereview.chromium.org/1869753003/diff/140001/cc/output/software_ren...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/output/software_ren...
cc/output/software_renderer.cc:157: sk_sp<SkCanvas>(new
SkCanvas(current_framebuffer_lock_->sk_bitmap()));
On 2016/04/14 19:37:32, danakj wrote:
> is there nothing like make_scoped_refptr, make_scoped_ptr, WrapUnique, etc for
> sk_sp?

Done: sk_make_sp<SkCanvas>().

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

https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
cc/playback/discardable_image_map.cc:166: return sk_sp<SkCanvas>(new
DiscardableImagesMetadataCanvas(
On 2016/04/14 19:37:32, danakj wrote:
> no make_scoped_refptr equivalent?
> 
> oh i found sk_make_sp. use that?

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
File cc/playback/discardable_image_map.h (left):

https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
cc/playback/discardable_image_map.h:14: #include "skia/ext/refptr.h"
On 2016/04/14 19:37:32, danakj wrote:
> we should technically be including SkRefCnt.h in every place that uses sk_sp.
> might come back to bite you that you're not (it's been a huuuge pain for
> scoped_ptr.h)

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/raster/tile_task_wo...
File cc/raster/tile_task_worker_pool_perftest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/raster/tile_task_wo...
cc/raster/tile_task_worker_pool_perftest.cc:109: scoped_ptr<PerfGLES2Interface>
context_gl_;
On 2016/04/14 19:37:32, danakj wrote:
> undo

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.cc:47: pixel_ref_ = pixel_ref;
On 2016/04/14 19:37:32, danakj wrote:
> move

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.cc:78: Create(pixel_ref, size,
UIResourceBitmap::ETC1);
On 2016/04/14 19:37:32, danakj wrote:
> move

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.h:44: UIResourceBitmap(const sk_sp<SkPixelRef>&
pixel_ref, const gfx::Size& size);
On 2016/04/14 19:37:32, danakj wrote:
> pass by value

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.h:51: void Create(const sk_sp<SkPixelRef>&
pixel_ref,
On 2016/04/14 19:37:32, danakj wrote:
> pass by value

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
File cc/test/fake_content_layer_client.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.cc:20:
FakeContentLayerClient::ImageData::ImageData(const SkImage* img,
On 2016/04/14 19:37:32, danakj wrote:
> can you make this an sk_sp?

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.cc:25:
FakeContentLayerClient::ImageData::ImageData(const SkImage* img,
On 2016/04/14 19:37:32, danakj wrote:
> and this?

Done.

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

https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.h:15: #include "skia/ext/refptr.h"
On 2016/04/14 19:37:32, danakj wrote:
> ?

Done.

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

https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:66:
image_ref_(sk_ref_sp(draw_image.image())),
On 2016/04/14 19:37:33, danakj wrote:
> make image() return sk_sp

Done in another not-quite-epic-but-largish CL.

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

https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:66:
image_ref_(sk_ref_sp(image.image())),
On 2016/04/14 19:37:33, danakj wrote:
> make image() return sk_sp

Done.

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_filters.cc:254:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
On 2016/04/14 19:37:33, danakj wrote:
> move filter

inlined

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_filters.cc:566:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
On 2016/04/14 19:37:33, danakj wrote:
> move filter

inlined

https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_filters.cc:614:
filters.Append(FilterOperation::CreateReferenceFilter(filter));
On 2016/04/14 19:37:33, danakj wrote:
> move filter

inlined

https://codereview.chromium.org/1869753003/diff/140001/content/common/cc_mess...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/140001/content/common/cc_mess...
content/common/cc_messages_unittest.cc:288:
cc::FilterOperation::CreateReferenceFilter(arbitrary_filter));
On 2016/04/14 19:37:33, danakj wrote:
> move filter

inlined

[danakj, 2016-04-25 21:51:11]

On Mon, Apr 25, 2016 at 1:48 PM, <tomhudson@google.com> wrote:

> Dana: the one thing that seemed problematic was your proposals in
> layer_tree_host_pixeltest_blending and layer_tree_host_pixeltest_masks.
>
> If I make PictureImageLayer take sk_sp<>, that's also being written to by
> WebImageLayerImpl::setImage() (among others), which only has a raw
> pointer. The
> member function signature is inherited out of Blink's WebImageLayer, and
> AFAIK
> per Florin we currently aren't exposing sk_sp<> in most WebKit APIs?
>
> Florin, I know we've *started* relaxing that restriction, but need to know
> if
> Blink is ready to let our smart pointers in more generally.
>

If you need to do converstion to/from raw pointer and sk_sp do it in the
Web*LayerImpl bindings. The core cc api should work with smart pointers
when ownership is transferred always.


>
>
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> File cc/output/filter_operation.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.cc:90: const sk_sp<SkImageFilter>&
> image_filter)
> On 2016/04/14 19:37:31, danakj wrote:
> > ditto
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.cc:166: return
> FilterOperation::CreateReferenceFilter(sk_sp<SkImageFilter>());
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> File cc/output/filter_operation.h (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.h:150: const sk_sp<SkImageFilter>&
> image_filter) {
> On 2016/04/14 19:37:31, danakj wrote:
> > while you're here, with c++11 I think you should write these to pass
> by value
> > instead of by reference, and use std::move() when you use it below.
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.h:199: void set_image_filter(const
> sk_sp<SkImageFilter>& image_filter) {
> On 2016/04/14 19:37:31, danakj wrote:
> > ditto
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.h:248: FilterOperation(FilterType type, const
> sk_sp<SkImageFilter>& image_filter);
> On 2016/04/14 19:37:31, danakj wrote:
> > ditto
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> File cc/output/filter_operations_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operations_unittest.cc:603:
> FilterOperation::CreateReferenceFilter(sk_sp<SkImageFilter>());
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:607: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:610: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:634: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:645: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:663: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer_...
> File cc/output/gl_renderer_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer_...
> cc/output/gl_renderer_unittest.cc:1439:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:31, danakj wrote:
> > can move the |filter| here
>
> inlined instead.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> File cc/output/render_surface_filters.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:149: const sk_sp<SkImageFilter>&
> input) {
> On 2016/04/14 19:37:32, danakj wrote:
> > by value
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:152: return sk_sp<SkImageFilter>(
> On 2016/04/14 19:37:32, danakj wrote:
> > call Make and you don't need to cast/wrap here
>
> Done.
>
> (Some of those Make() calls on filters were only written after this CL
> started, but they should all be finished. now.)
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:168: image_filter =
> CreateMatrixImageFilter(matrix, image_filter);
> On 2016/04/14 19:37:32, danakj wrote:
> > should move image_filter on all these calls
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:203: image_filter =
> sk_sp<SkImageFilter>(SkDropShadowImageFilter::Create(
> On 2016/04/14 19:37:31, danakj wrote:
> > call Make instead?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:225: image_filter =
> SkComposeImageFilter::Make(zoom_filter, image_filter);
> On 2016/04/14 19:37:32, danakj wrote:
> > move zoom_filter too
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:244: cf =
> sk_sp<SkColorFilter>(colorfilter_rawptr);
> On 2016/04/14 19:37:32, danakj wrote:
> > cf.reset(
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:249: image_filter =
> CreateMatrixImageFilter(matrix, image_filter);
> On 2016/04/14 19:37:31, danakj wrote:
> > move image_filter
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:252:
> SkComposeImageFilter::Make(op.image_filter(), image_filter);
> On 2016/04/14 19:37:32, danakj wrote:
> > move image_filter
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:262: image_filter =
> SkComposeImageFilter::Make(alpha_filter, image_filter);
> On 2016/04/14 19:37:32, danakj wrote:
> > move alpha_filter
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:264: image_filter = alpha_filter;
> On 2016/04/14 19:37:32, danakj wrote:
> > move
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/renderer_pix...
> File cc/output/renderer_pixeltest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/renderer_pix...
> cc/output/renderer_pixeltest.cc:1321:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:32, danakj wrote:
> > move filter
>
> inlined.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/software_ren...
> File cc/output/software_renderer.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/software_ren...
> cc/output/software_renderer.cc:157: sk_sp<SkCanvas>(new
> SkCanvas(current_framebuffer_lock_->sk_bitmap()));
> On 2016/04/14 19:37:32, danakj wrote:
> > is there nothing like make_scoped_refptr, make_scoped_ptr, WrapUnique,
> etc for
> > sk_sp?
>
> Done: sk_make_sp<SkCanvas>().
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
> File cc/playback/discardable_image_map.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
> cc/playback/discardable_image_map.cc:166: return sk_sp<SkCanvas>(new
> DiscardableImagesMetadataCanvas(
> On 2016/04/14 19:37:32, danakj wrote:
> > no make_scoped_refptr equivalent?
> >
> > oh i found sk_make_sp. use that?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
> File cc/playback/discardable_image_map.h (left):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
> cc/playback/discardable_image_map.h:14: #include "skia/ext/refptr.h"
> On 2016/04/14 19:37:32, danakj wrote:
> > we should technically be including SkRefCnt.h in every place that uses
> sk_sp.
> > might come back to bite you that you're not (it's been a huuuge pain
> for
> > scoped_ptr.h)
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/raster/tile_task_wo...
> File cc/raster/tile_task_worker_pool_perftest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/raster/tile_task_wo...
> cc/raster/tile_task_worker_pool_perftest.cc:109:
> scoped_ptr<PerfGLES2Interface> context_gl_;
> On 2016/04/14 19:37:32, danakj wrote:
> > undo
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> File cc/resources/ui_resource_bitmap.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> cc/resources/ui_resource_bitmap.cc:47: pixel_ref_ = pixel_ref;
> On 2016/04/14 19:37:32, danakj wrote:
> > move
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> cc/resources/ui_resource_bitmap.cc:78: Create(pixel_ref, size,
> UIResourceBitmap::ETC1);
> On 2016/04/14 19:37:32, danakj wrote:
> > move
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> File cc/resources/ui_resource_bitmap.h (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> cc/resources/ui_resource_bitmap.h:44: UIResourceBitmap(const
> sk_sp<SkPixelRef>& pixel_ref, const gfx::Size& size);
> On 2016/04/14 19:37:32, danakj wrote:
> > pass by value
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> cc/resources/ui_resource_bitmap.h:51: void Create(const
> sk_sp<SkPixelRef>& pixel_ref,
> On 2016/04/14 19:37:32, danakj wrote:
> > pass by value
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> File cc/test/fake_content_layer_client.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> cc/test/fake_content_layer_client.cc:20:
> FakeContentLayerClient::ImageData::ImageData(const SkImage* img,
> On 2016/04/14 19:37:32, danakj wrote:
> > can you make this an sk_sp?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> cc/test/fake_content_layer_client.cc:25:
> FakeContentLayerClient::ImageData::ImageData(const SkImage* img,
> On 2016/04/14 19:37:32, danakj wrote:
> > and this?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> File cc/test/fake_content_layer_client.h (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> cc/test/fake_content_layer_client.h:15: #include "skia/ext/refptr.h"
> On 2016/04/14 19:37:32, danakj wrote:
> > ?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/gpu_image_dec...
> File cc/tiles/gpu_image_decode_controller.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/gpu_image_dec...
> cc/tiles/gpu_image_decode_controller.cc:66:
> image_ref_(sk_ref_sp(draw_image.image())),
> On 2016/04/14 19:37:33, danakj wrote:
> > make image() return sk_sp
>
> Done in another not-quite-epic-but-largish CL.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/software_imag...
> File cc/tiles/software_image_decode_controller.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/software_imag...
> cc/tiles/software_image_decode_controller.cc:66:
> image_ref_(sk_ref_sp(image.image())),
> On 2016/04/14 19:37:33, danakj wrote:
> > make image() return sk_sp
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
> File cc/trees/layer_tree_host_pixeltest_filters.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_pixeltest_filters.cc:254:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:33, danakj wrote:
> > move filter
>
> inlined
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_pixeltest_filters.cc:566:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:33, danakj wrote:
> > move filter
>
> inlined
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_pixeltest_filters.cc:614:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:33, danakj wrote:
> > move filter
>
> inlined
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/content/common/cc_mess...
> File content/common/cc_messages_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/content/common/cc_mess...
> content/common/cc_messages_unittest.cc:288:
> cc::FilterOperation::CreateReferenceFilter(arbitrary_filter));
> On 2016/04/14 19:37:33, danakj wrote:
> > move filter
>
> inlined
>
> https://codereview.chromium.org/1869753003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2016-04-25 21:53:08]

On Mon, Apr 25, 2016 at 1:48 PM, <tomhudson@google.com> wrote:

> Dana: the one thing that seemed problematic was your proposals in
> layer_tree_host_pixeltest_blending and layer_tree_host_pixeltest_masks.
>
> If I make PictureImageLayer take sk_sp<>, that's also being written to by
> WebImageLayerImpl::setImage() (among others), which only has a raw
> pointer. The
> member function signature is inherited out of Blink's WebImageLayer, and
> AFAIK
> per Florin we currently aren't exposing sk_sp<> in most WebKit APIs?
>
> Florin, I know we've *started* relaxing that restriction, but need to know
> if
> Blink is ready to let our smart pointers in more generally.
>

If you need to do converstion to/from raw pointer and sk_sp do it in the
Web*LayerImpl bindings. The core cc api should work with smart pointers
when ownership is transferred always.


>
>
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> File cc/output/filter_operation.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.cc:90: const sk_sp<SkImageFilter>&
> image_filter)
> On 2016/04/14 19:37:31, danakj wrote:
> > ditto
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.cc:166: return
> FilterOperation::CreateReferenceFilter(sk_sp<SkImageFilter>());
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> File cc/output/filter_operation.h (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.h:150: const sk_sp<SkImageFilter>&
> image_filter) {
> On 2016/04/14 19:37:31, danakj wrote:
> > while you're here, with c++11 I think you should write these to pass
> by value
> > instead of by reference, and use std::move() when you use it below.
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.h:199: void set_image_filter(const
> sk_sp<SkImageFilter>& image_filter) {
> On 2016/04/14 19:37:31, danakj wrote:
> > ditto
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operation.h:248: FilterOperation(FilterType type, const
> sk_sp<SkImageFilter>& image_filter);
> On 2016/04/14 19:37:31, danakj wrote:
> > ditto
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> File cc/output/filter_operations_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/filter_opera...
> cc/output/filter_operations_unittest.cc:603:
> FilterOperation::CreateReferenceFilter(sk_sp<SkImageFilter>());
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:607: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:610: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:634: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:645: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer....
> cc/output/gl_renderer.cc:663: return sk_sp<SkImage>();
> On 2016/04/14 19:37:31, danakj wrote:
> > nullptr
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer_...
> File cc/output/gl_renderer_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/gl_renderer_...
> cc/output/gl_renderer_unittest.cc:1439:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:31, danakj wrote:
> > can move the |filter| here
>
> inlined instead.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> File cc/output/render_surface_filters.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:149: const sk_sp<SkImageFilter>&
> input) {
> On 2016/04/14 19:37:32, danakj wrote:
> > by value
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:152: return sk_sp<SkImageFilter>(
> On 2016/04/14 19:37:32, danakj wrote:
> > call Make and you don't need to cast/wrap here
>
> Done.
>
> (Some of those Make() calls on filters were only written after this CL
> started, but they should all be finished. now.)
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:168: image_filter =
> CreateMatrixImageFilter(matrix, image_filter);
> On 2016/04/14 19:37:32, danakj wrote:
> > should move image_filter on all these calls
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:203: image_filter =
> sk_sp<SkImageFilter>(SkDropShadowImageFilter::Create(
> On 2016/04/14 19:37:31, danakj wrote:
> > call Make instead?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:225: image_filter =
> SkComposeImageFilter::Make(zoom_filter, image_filter);
> On 2016/04/14 19:37:32, danakj wrote:
> > move zoom_filter too
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:244: cf =
> sk_sp<SkColorFilter>(colorfilter_rawptr);
> On 2016/04/14 19:37:32, danakj wrote:
> > cf.reset(
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:249: image_filter =
> CreateMatrixImageFilter(matrix, image_filter);
> On 2016/04/14 19:37:31, danakj wrote:
> > move image_filter
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:252:
> SkComposeImageFilter::Make(op.image_filter(), image_filter);
> On 2016/04/14 19:37:32, danakj wrote:
> > move image_filter
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:262: image_filter =
> SkComposeImageFilter::Make(alpha_filter, image_filter);
> On 2016/04/14 19:37:32, danakj wrote:
> > move alpha_filter
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/render_surfa...
> cc/output/render_surface_filters.cc:264: image_filter = alpha_filter;
> On 2016/04/14 19:37:32, danakj wrote:
> > move
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/renderer_pix...
> File cc/output/renderer_pixeltest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/renderer_pix...
> cc/output/renderer_pixeltest.cc:1321:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:32, danakj wrote:
> > move filter
>
> inlined.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/software_ren...
> File cc/output/software_renderer.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/output/software_ren...
> cc/output/software_renderer.cc:157: sk_sp<SkCanvas>(new
> SkCanvas(current_framebuffer_lock_->sk_bitmap()));
> On 2016/04/14 19:37:32, danakj wrote:
> > is there nothing like make_scoped_refptr, make_scoped_ptr, WrapUnique,
> etc for
> > sk_sp?
>
> Done: sk_make_sp<SkCanvas>().
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
> File cc/playback/discardable_image_map.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
> cc/playback/discardable_image_map.cc:166: return sk_sp<SkCanvas>(new
> DiscardableImagesMetadataCanvas(
> On 2016/04/14 19:37:32, danakj wrote:
> > no make_scoped_refptr equivalent?
> >
> > oh i found sk_make_sp. use that?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
> File cc/playback/discardable_image_map.h (left):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/playback/discardabl...
> cc/playback/discardable_image_map.h:14: #include "skia/ext/refptr.h"
> On 2016/04/14 19:37:32, danakj wrote:
> > we should technically be including SkRefCnt.h in every place that uses
> sk_sp.
> > might come back to bite you that you're not (it's been a huuuge pain
> for
> > scoped_ptr.h)
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/raster/tile_task_wo...
> File cc/raster/tile_task_worker_pool_perftest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/raster/tile_task_wo...
> cc/raster/tile_task_worker_pool_perftest.cc:109:
> scoped_ptr<PerfGLES2Interface> context_gl_;
> On 2016/04/14 19:37:32, danakj wrote:
> > undo
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> File cc/resources/ui_resource_bitmap.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> cc/resources/ui_resource_bitmap.cc:47: pixel_ref_ = pixel_ref;
> On 2016/04/14 19:37:32, danakj wrote:
> > move
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> cc/resources/ui_resource_bitmap.cc:78: Create(pixel_ref, size,
> UIResourceBitmap::ETC1);
> On 2016/04/14 19:37:32, danakj wrote:
> > move
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> File cc/resources/ui_resource_bitmap.h (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> cc/resources/ui_resource_bitmap.h:44: UIResourceBitmap(const
> sk_sp<SkPixelRef>& pixel_ref, const gfx::Size& size);
> On 2016/04/14 19:37:32, danakj wrote:
> > pass by value
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/resources/ui_resour...
> cc/resources/ui_resource_bitmap.h:51: void Create(const
> sk_sp<SkPixelRef>& pixel_ref,
> On 2016/04/14 19:37:32, danakj wrote:
> > pass by value
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> File cc/test/fake_content_layer_client.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> cc/test/fake_content_layer_client.cc:20:
> FakeContentLayerClient::ImageData::ImageData(const SkImage* img,
> On 2016/04/14 19:37:32, danakj wrote:
> > can you make this an sk_sp?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> cc/test/fake_content_layer_client.cc:25:
> FakeContentLayerClient::ImageData::ImageData(const SkImage* img,
> On 2016/04/14 19:37:32, danakj wrote:
> > and this?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> File cc/test/fake_content_layer_client.h (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/test/fake_content_l...
> cc/test/fake_content_layer_client.h:15: #include "skia/ext/refptr.h"
> On 2016/04/14 19:37:32, danakj wrote:
> > ?
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/gpu_image_dec...
> File cc/tiles/gpu_image_decode_controller.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/gpu_image_dec...
> cc/tiles/gpu_image_decode_controller.cc:66:
> image_ref_(sk_ref_sp(draw_image.image())),
> On 2016/04/14 19:37:33, danakj wrote:
> > make image() return sk_sp
>
> Done in another not-quite-epic-but-largish CL.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/software_imag...
> File cc/tiles/software_image_decode_controller.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/tiles/software_imag...
> cc/tiles/software_image_decode_controller.cc:66:
> image_ref_(sk_ref_sp(image.image())),
> On 2016/04/14 19:37:33, danakj wrote:
> > make image() return sk_sp
>
> Done.
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
> File cc/trees/layer_tree_host_pixeltest_filters.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_pixeltest_filters.cc:254:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:33, danakj wrote:
> > move filter
>
> inlined
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_pixeltest_filters.cc:566:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:33, danakj wrote:
> > move filter
>
> inlined
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_pixeltest_filters.cc:614:
> filters.Append(FilterOperation::CreateReferenceFilter(filter));
> On 2016/04/14 19:37:33, danakj wrote:
> > move filter
>
> inlined
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/content/common/cc_mess...
> File content/common/cc_messages_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1869753003/diff/140001/content/common/cc_mess...
> content/common/cc_messages_unittest.cc:288:
> cc::FilterOperation::CreateReferenceFilter(arbitrary_filter));
> On 2016/04/14 19:37:33, danakj wrote:
> > move filter
>
> inlined
>
> https://codereview.chromium.org/1869753003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-04-25 22:06:24]

LGTM few more comments/nits. This is a tricky change, touching a lot of stuff.
Thanks for keeping at it .

https://codereview.chromium.org/1869753003/diff/240001/cc/animation/animation...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/animation/animation...
cc/animation/animation_player.cc:7: #include <algorithm>
rebase error? this is the only change in this file

https://codereview.chromium.org/1869753003/diff/240001/cc/output/filter_opera...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/filter_opera...
cc/output/filter_operation.h:79: sk_sp<SkImageFilter> image_filter() const {
return a const& here too

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:627: sk_sp<SkImage> srcImage =
SkImage::MakeFromTexture(
nit: while you're here.. src_image?

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:651:
paint.setImageFilter(filter->makeWithLocalMatrix(local_matrix));
The SkPaint is taking ownership of this filter right? Then it shouldn't be a raw
pointer as an argument to ApplyImageFilter

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:653: surface->getCanvas()->drawImage(srcImage,
src_rect.x(), src_rect.y(), &paint);
I found it a bit surprising that drawImage takes ownership of the SkImage.  I
guess that is intentional? If so, you can move the src_image then.

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:861: quad->filters_scale, filter.get(),
background_texture);
and we can move the filter here

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:1439:
SkColorFilterImageFilter::Make(color_filter, NULL)));
nit: nullptr

https://codereview.chromium.org/1869753003/diff/240001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1396:
SkColorFilter::MakeMatrixFilterRowMajor255(matrix), NULL)));
nit: nullptr

https://codereview.chromium.org/1869753003/diff/240001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1633:
SkColorFilter::MakeMatrixFilterRowMajor255(matrix), NULL)));
nit: nullptr

https://codereview.chromium.org/1869753003/diff/240001/cc/test/test_in_proces...
File cc/test/test_in_process_context_provider.h (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/test/test_in_proces...
cc/test/test_in_process_context_provider.h:16: #include
"third_party/skia/include/core/SkRefCnt.h"
unused?

https://codereview.chromium.org/1869753003/diff/240001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_filters.cc:253:
SkColorFilterImageFilter::Make(color_filter, NULL, &crop_rect)));
nit: nullptr

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
File chrome/browser/android/thumbnail/thumbnail_cache.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:590: compressed_data =
etc1_pixel_ref;
move

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:598:
base::Bind(post_compression_task, compressed_data, content_size));
move data

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:620:
WriteThumbnailIfNecessary(tab_id, compressed_data, scale, content_size);
move data

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:758:
base::Bind(post_read_task, compressed_data, scale, content_size),
move data

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:764:
base::Bind(post_read_task, compressed_data, scale, content_size));
move data

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:793:
thumbnail->SetCompressedBitmap(compressed_data,
move data

[tomhudson, 2016-04-26 15:28:02]

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

[commit-bot: I haz the power, 2016-04-26 15:28:23]

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

[commit-bot: I haz the power, 2016-04-26 17:09:59]

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

[commit-bot: I haz the power, 2016-04-26 17:10:01]

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-27 12:06:48]

junov@: just one file in WebKit needs signoff...

https://codereview.chromium.org/1869753003/diff/240001/cc/animation/animation...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/animation/animation...
cc/animation/animation_player.cc:7: #include <algorithm>
On 2016/04/25 22:06:23, danakj wrote:
> rebase error? this is the only change in this file

I'm assuming it was getting this header via some magic, because without it my
build fails like so:

../../cc/animation/animation_player.cc:145:38: error: no member named
'remove_if' in namespace 'std'
    auto animations_to_remove = std::remove_if(
                                ~~~~~^
../../cc/animation/animation_player.cc:166:38: error: no member named
'remove_if' in namespace 'std'
    auto animations_to_remove = std::remove_if(

https://codereview.chromium.org/1869753003/diff/240001/cc/output/filter_opera...
File cc/output/filter_operation.h (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/filter_opera...
cc/output/filter_operation.h:79: sk_sp<SkImageFilter> image_filter() const {
On 2016/04/25 22:06:23, danakj wrote:
> return a const& here too

Done.

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:627: sk_sp<SkImage> srcImage =
SkImage::MakeFromTexture(
On 2016/04/25 22:06:23, danakj wrote:
> nit: while you're here.. src_image?

Done.

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:651:
paint.setImageFilter(filter->makeWithLocalMatrix(local_matrix));
On 2016/04/25 22:06:23, danakj wrote:
> The SkPaint is taking ownership of this filter right? Then it shouldn't be a
raw
> pointer as an argument to ApplyImageFilter

Done.

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:653: surface->getCanvas()->drawImage(srcImage,
src_rect.x(), src_rect.y(), &paint);
On 2016/04/25 22:06:23, danakj wrote:
> I found it a bit surprising that drawImage takes ownership of the SkImage.  I
> guess that is intentional? If so, you can move the src_image then.

As I understand it there are times when parts of the draw could be deferred, so
we need to keep a ref to the image. (e.g. what if the image is backed by
something that is lazily decoded?)

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:861: quad->filters_scale, filter.get(),
background_texture);
On 2016/04/25 22:06:23, danakj wrote:
> and we can move the filter here

Done.

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:1439:
SkColorFilterImageFilter::Make(color_filter, NULL)));
On 2016/04/25 22:06:23, danakj wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1869753003/diff/240001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1396:
SkColorFilter::MakeMatrixFilterRowMajor255(matrix), NULL)));
On 2016/04/25 22:06:23, danakj wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1869753003/diff/240001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1633:
SkColorFilter::MakeMatrixFilterRowMajor255(matrix), NULL)));
On 2016/04/25 22:06:23, danakj wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1869753003/diff/240001/cc/test/test_in_proces...
File cc/test/test_in_process_context_provider.h (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/test/test_in_proces...
cc/test/test_in_process_context_provider.h:16: #include
"third_party/skia/include/core/SkRefCnt.h"
On 2016/04/25 22:06:23, danakj wrote:
> unused?

Done.

https://codereview.chromium.org/1869753003/diff/240001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_filters.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_filters.cc:253:
SkColorFilterImageFilter::Make(color_filter, NULL, &crop_rect)));
On 2016/04/25 22:06:23, danakj wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
File chrome/browser/android/thumbnail/thumbnail_cache.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:590: compressed_data =
etc1_pixel_ref;
On 2016/04/25 22:06:24, danakj wrote:
> move

Done.

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:598:
base::Bind(post_compression_task, compressed_data, content_size));
On 2016/04/25 22:06:23, danakj wrote:
> move data

Done.

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:620:
WriteThumbnailIfNecessary(tab_id, compressed_data, scale, content_size);
On 2016/04/25 22:06:23, danakj wrote:
> move data

Done.

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:758:
base::Bind(post_read_task, compressed_data, scale, content_size),
On 2016/04/25 22:06:24, danakj wrote:
> move data

Done.

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:764:
base::Bind(post_read_task, compressed_data, scale, content_size));
On 2016/04/25 22:06:23, danakj wrote:
> move data

Done.

https://codereview.chromium.org/1869753003/diff/240001/chrome/browser/android...
chrome/browser/android/thumbnail/thumbnail_cache.cc:793:
thumbnail->SetCompressedBitmap(compressed_data,
On 2016/04/25 22:06:23, danakj wrote:
> move data

Done.

[f(malita), 2016-04-27 14:00:41]

lgtm

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_blending.cc (right):

https://codereview.chromium.org/1869753003/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_blending.cc:147:
layer->SetImage(skia::SharePtr(backing_store->makeImageSnapshot()));
On 2016/04/25 20:20:51, tomhudson wrote:
> On 2016/04/21 15:45:47, f(malita) wrote:
> > nit: in this particular case I think they're functionally equivalent (both
> steal
> > the ref value), but skia::AdoptRef is easier to grok for me.
> 
> I don't understand - AdoptRef *does not* increment the reference count, but
> SharePtr *does* increment the refcount, right?

Yes (functionally), but the increment is front-loaded in the arg value
instantiation (refptr.h):

  RefPtr<T> AdoptRef(sk_sp<T>&& sp) { return RefPtr<T>(sp.release()); }
  RefPtr<T> SharePtr(sk_sp<T> sp) { return RefPtr<T>(sp.release()); }

When the argument is an rvalue (as it was the case here) they're equivalent.

All moot now, the new version is much nicer - thanks!

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:653: surface->getCanvas()->drawImage(srcImage,
src_rect.x(), src_rect.y(), &paint);
On 2016/04/27 12:06:47, tomhudson wrote:
> On 2016/04/25 22:06:23, danakj wrote:
> > I found it a bit surprising that drawImage takes ownership of the SkImage. 
I
> > guess that is intentional? If so, you can move the src_image then.
>
> As I understand it there are times when parts of the draw could be deferred,
so
> we need to keep a ref to the image. (e.g. what if the image is backed by
> something that is lazily decoded?)

The most common case I believe is a recording canvas - but note that the
ownership transfer is not required/implied:

  drawImage(const sk_sp<SkImage>&,...) rather than drawImage(sk_sp<SkImage>,...)

so moving the argument is not buying us much here.

https://codereview.chromium.org/1869753003/diff/260001/cc/resources/ui_resour...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/1869753003/diff/260001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.cc:59: Create(pixel_ref, gfx::Size(info.width(),
info.height()),
std::move(pixel_ref)?

[tomhudson, 2016-04-27 14:36:41]

tomhudson@google.com changed reviewers:
- junov@chromium.org

[tomhudson, 2016-04-27 14:40:35]

Description was changed from

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

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

to

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

R=mkwst@chromium.org,danakj@chromium.org,miguelg@chromium.org,fmalita@chromiu...
TBR=esprehn@chromium.org
BUG=skia:5077
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[tomhudson, 2016-04-27 14:40:35]

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

[tomhudson, 2016-04-27 14:43:13]

Realized Florin's LG is good enough for Blink; don't need junov@ (sorry for the
noise, Justin!): content/renderer/ change is mechanical enough and similar
enough to others I just TBR'd it and will go ahead with the CQ.

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1869753003/diff/240001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:653: surface->getCanvas()->drawImage(srcImage,
src_rect.x(), src_rect.y(), &paint);
On 2016/04/27 14:00:41, f(malita) wrote:
> On 2016/04/27 12:06:47, tomhudson wrote:
> > On 2016/04/25 22:06:23, danakj wrote:
> > > I found it a bit surprising that drawImage takes ownership of the SkImage.

> I
> > > guess that is intentional? If so, you can move the src_image then.
> >
> > As I understand it there are times when parts of the draw could be deferred,
> so
> > we need to keep a ref to the image. (e.g. what if the image is backed by
> > something that is lazily decoded?)
> 
> The most common case I believe is a recording canvas - but note that the
> ownership transfer is not required/implied:
> 
>   drawImage(const sk_sp<SkImage>&,...) rather than
drawImage(sk_sp<SkImage>,...)
> 
> so moving the argument is not buying us much here.

OK, slightly better if still not completely understood, and so undone?

https://codereview.chromium.org/1869753003/diff/260001/cc/resources/ui_resour...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/1869753003/diff/260001/cc/resources/ui_resour...
cc/resources/ui_resource_bitmap.cc:59: Create(pixel_ref, gfx::Size(info.width(),
info.height()),
On 2016/04/27 14:00:41, f(malita) wrote:
> std::move(pixel_ref)?

Done.

[tomhudson, 2016-04-27 14:43:38]

The CQ bit was checked by tomhudson@google.com

[tomhudson, 2016-04-27 14:43:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
miguelg@chromium.org, mkwst@chromium.org, danakj@chromium.org,
fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1869753003/#ps280001
(title: "Florin's nits")

[commit-bot: I haz the power, 2016-04-27 14:43:57]

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

[commit-bot: I haz the power, 2016-04-27 16:45:49]

Description was changed from

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

R=mkwst@chromium.org,danakj@chromium.org,miguelg@chromium.org,fmalita@chromiu...
TBR=esprehn@chromium.org
BUG=skia:5077
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

R=mkwst@chromium.org,danakj@chromium.org,miguelg@chromium.org,fmalita@chromiu...
TBR=esprehn@chromium.org
BUG=skia:5077
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-27 16:45:51]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-04-30 17:10:36]

Patchset 15 (id:??) landed as
https://crrev.com/992ec28bf1a2821dd704d5acb74a0d382d3c19c5
Cr-Commit-Position: refs/heads/master@{#390102}

[commit-bot: I haz the power, 2016-04-30 17:50:25]

Description was changed from

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

R=mkwst@chromium.org,danakj@chromium.org,miguelg@chromium.org,fmalita@chromiu...
TBR=esprehn@chromium.org
BUG=skia:5077
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Replace many skia::RefPtr with sk_sp<> in cc/

Incremental progress towards getting rid of the skia::RefPtr
adapter class.

R=mkwst@chromium.org,danakj@chromium.org,miguelg@chromium.org,fmalita@chromiu...
TBR=esprehn@chromium.org
BUG=skia:5077
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/992ec28bf1a2821dd704d5acb74a0d382d3c19c5
Cr-Commit-Position: refs/heads/master@{#390102}
==========