cc/paint: Remove PaintCanvas::peekPixels.

cc/paint: Remove PaintCanvas::peekPixels.

This patch removes peekPixels and replaces callers with an explicit
SkBitmap construction and PaintCanvas (as opposed to PaintSurface).

R=enne@chromium.org
TBR=jochen@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2758413002
Cr-Commit-Position: refs/heads/master@{#458880}
Committed: https://chromium.googlesource.com/chromium/src/+/4f7674971344b7788b85caabba2a234aa3918e6b

[vmpstr, 2017-03-20 20:19:02]

Description was changed from

==========
cc/paint: Remove PaintCanvas::peekPixels.

This patch removes peekPixels and replaces callers with an explicit
SkBitmap construction and PaintCanvas (as opposed to PaintSurface).

R=enne@chromium.org
==========

to

==========
cc/paint: Remove PaintCanvas::peekPixels.

This patch removes peekPixels and replaces callers with an explicit
SkBitmap construction and PaintCanvas (as opposed to PaintSurface).

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-03-20 20:22:56]

vmpstr@chromium.org changed reviewers:
+ dalecurtis@chromium.org, danakj@chromium.org, emircan@chromium.org

[vmpstr, 2017-03-20 20:22:57]

+enne for cc and overall
+danakj for ui
+dalecurtis for media
+emircan for webrtc

Please take a look.

[vmpstr, 2017-03-20 21:29:09]

vmpstr@chromium.org changed reviewers:
+ msw@chromium.org
- danakj@chromium.org

[vmpstr, 2017-03-20 21:29:09]

-danakj,+msw since it's rendertext unittest change.

[enne (OOO), 2017-03-20 21:45:13]

enne@chromium.org changed reviewers:
+ danakj@chromium.org
- msw@chromium.org

[enne (OOO), 2017-03-20 21:45:14]

lgtm overall.  This looks like a straightforward transformation.

https://codereview.chromium.org/2758413002/diff/20001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/2758413002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_compositor.cc:65: video_renderer->Copy(
TIL: drawImage(gpu image) into a bitmap backed canvas just does a readback
implicitly.  Who knew.

[DaleCurtis, 2017-03-20 21:49:45]

lgtm

[emircan, 2017-03-20 22:00:09]

content/renderer/media/webrtc/* lgtm

[msw, 2017-03-20 22:03:13]

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

[msw, 2017-03-20 22:03:13]

render_text_unittest.cc lgtm

[vmpstr, 2017-03-20 22:19:12]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2017-03-20 22:19:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 22:54:24]

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

[commit-bot: I haz the power, 2017-03-20 22:54:26]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[vmpstr, 2017-03-20 23:50:33]

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

[vmpstr, 2017-03-20 23:50:33]

Had to touch gfx::Canvas as well, since the mac version uses peekPixels.
enne/danakj/ccameron, could you please take a look?

[enne (OOO), 2017-03-21 00:46:38]

https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h#newcode479
ui/gfx/canvas.h:479: SkBitmap& get_bitmap() {
Maybe ToBitmap() should return bitmap_ if it exists? and then you can just use
ToBitmap instead of get_bitmap?

[vmpstr, 2017-03-21 17:45:06]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 17:45:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-03-21 17:47:13]

https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h#newcode479
ui/gfx/canvas.h:479: SkBitmap& get_bitmap() {
On 2017/03/21 00:46:38, enne wrote:
> Maybe ToBitmap() should return bitmap_ if it exists? and then you can just use
> ToBitmap instead of get_bitmap?

Oh, I completely missed that there is ToBitmap here. It seems that it's only
used in unittests. I've updated it to just return the bitmap instead of doing a
readback and now am using it in canvas_paint_mac.mm. 

I could also change it to do a readback if the bitmap doesn't exist, but I don't
think there are usecases right now that do this. WDYT?

[enne (OOO), 2017-03-21 17:51:02]

https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h#newcode479
ui/gfx/canvas.h:479: SkBitmap& get_bitmap() {
On 2017/03/21 at 17:47:12, vmpstr wrote:
> On 2017/03/21 00:46:38, enne wrote:
> > Maybe ToBitmap() should return bitmap_ if it exists? and then you can just
use
> > ToBitmap instead of get_bitmap?
> 
> Oh, I completely missed that there is ToBitmap here. It seems that it's only
used in unittests. I've updated it to just return the bitmap instead of doing a
readback and now am using it in canvas_paint_mac.mm. 
> 
> I could also change it to do a readback if the bitmap doesn't exist, but I
don't think there are usecases right now that do this. WDYT?

Oh, even better! If that's always the case, then let's do that!

[commit-bot: I haz the power, 2017-03-21 18:00:09]

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

[commit-bot: I haz the power, 2017-03-21 18:00:11]

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

[vmpstr, 2017-03-21 18:30:51]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 18:31:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 18:58:01]

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

[commit-bot: I haz the power, 2017-03-21 18:58:02]

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

[vmpstr, 2017-03-21 19:30:18]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 19:30:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-03-21 19:31:13]

PTAL. I had to add a few includes, since I removed gfx::Canvas include for
SkSurface.

[enne (OOO), 2017-03-21 19:32:59]

lgtm

[vmpstr, 2017-03-21 19:37:16]

danakj/ccameron could you take a look at ui/ bits please?

[commit-bot: I haz the power, 2017-03-21 20:02:09]

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

[commit-bot: I haz the power, 2017-03-21 20:02:10]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2017-03-21 20:02:57]

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:42: bool success = bitmap.tryAllocPixels(info);
why not allocPixels? We should crash on OOM

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:620: bitmap_ = CreateBitmap(size, is_opaque);
This is the only caller of CreateBitmap, inline it here?

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h#newcod...
ui/gfx/canvas.h:475: SkBitmap ToBitmap();
make a note that writing to this bitmap will change the content of the
gfx::Canvas? or maybe it can return const in some way?

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:3828: bitmap.peekPixels(&pixmap);
this could just use the bitmap directly instead of SkPixmap now?

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:3917: bitmap.peekPixels(&pixmap);
same

[vmpstr, 2017-03-21 22:34:39]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[vmpstr, 2017-03-21 22:34:54]

PTAL

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:42: bool success = bitmap.tryAllocPixels(info);
On 2017/03/21 20:02:56, danakj wrote:
> why not allocPixels? We should crash on OOM

Done.

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:620: bitmap_ = CreateBitmap(size, is_opaque);
On 2017/03/21 20:02:56, danakj wrote:
> This is the only caller of CreateBitmap, inline it here?

Done.

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h#newcod...
ui/gfx/canvas.h:475: SkBitmap ToBitmap();
On 2017/03/21 20:02:56, danakj wrote:
> make a note that writing to this bitmap will change the content of the
> gfx::Canvas? or maybe it can return const in some way?

Done. I also renamed it to AsBitmap to (hopefully) make it clear that both refer
to the same pixels and the fact that the canvas has to be backed by its own
bitmap for this to work (as opposed to using an external canvas).

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:3828: bitmap.peekPixels(&pixmap);
On 2017/03/21 20:02:57, danakj wrote:
> this could just use the bitmap directly instead of SkPixmap now?

Done.

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:3917: bitmap.peekPixels(&pixmap);
On 2017/03/21 20:02:57, danakj wrote:
> same

Done.

[commit-bot: I haz the power, 2017-03-21 22:35:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-03-21 22:40:02]

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h#newcod...
ui/gfx/canvas.h:475: SkBitmap ToBitmap();
On 2017/03/21 22:34:54, vmpstr wrote:
> On 2017/03/21 20:02:56, danakj wrote:
> > make a note that writing to this bitmap will change the content of the
> > gfx::Canvas? or maybe it can return const in some way?
> 
> Done. I also renamed it to AsBitmap to (hopefully) make it clear that both
refer
> to the same pixels and the fact that the canvas has to be backed by its own
> bitmap for this to work (as opposed to using an external canvas).

GetBitmap() does an even better job of this I think. AsFoo() is used in other
gfx:: things and it returns a different object, not like an accessor.

[danakj, 2017-03-21 22:42:48]

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h#newcod...
ui/gfx/canvas.h:475: SkBitmap ToBitmap();
On 2017/03/21 22:40:01, danakj wrote:
> On 2017/03/21 22:34:54, vmpstr wrote:
> > On 2017/03/21 20:02:56, danakj wrote:
> > > make a note that writing to this bitmap will change the content of the
> > > gfx::Canvas? or maybe it can return const in some way?
> > 
> > Done. I also renamed it to AsBitmap to (hopefully) make it clear that both
> refer
> > to the same pixels and the fact that the canvas has to be backed by its own
> > bitmap for this to work (as opposed to using an external canvas).
> 
> GetBitmap() does an even better job of this I think. AsFoo() is used in other
> gfx:: things and it returns a different object, not like an accessor.

In fact, why not return const SkBitmap&?

https://codereview.chromium.org/2758413002/diff/120001/ash/common/accelerator...
File ash/common/accelerators/debug_commands.cc (right):

https://codereview.chromium.org/2758413002/diff/120001/ash/common/accelerator...
ash/common/accelerators/debug_commands.cc:24: #include
"third_party/skia/include/core/SkSurface.h"
Good news this won't be needed with https://codereview.chromium.org/2764163002/
<_<

https://codereview.chromium.org/2758413002/diff/120001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2758413002/diff/120001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:609: bitmap_->allocPixels(info);
don't you have to construct the bitmap_ first?

[vmpstr, 2017-03-21 22:59:39]

The CQ bit was unchecked by vmpstr@chromium.org

[vmpstr, 2017-03-21 23:48:25]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 23:48:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-03-21 23:50:33]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 23:50:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2017-03-22 00:20:17]

ui lgtm

[ccameron, 2017-03-22 00:38:04]

On 2017/03/22 00:20:17, ccameron wrote:
> ui lgtm

(% danakj's remaining comments)

[commit-bot: I haz the power, 2017-03-22 00:46:10]

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

[commit-bot: I haz the power, 2017-03-22 00:46:12]

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

[vmpstr, 2017-03-22 17:34:43]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 17:35:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 17:37:44]

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

[commit-bot: I haz the power, 2017-03-22 17:37:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vmpstr, 2017-03-22 17:45:46]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[vmpstr, 2017-03-22 17:46:55]

PTAL. Things to look out for:

- I removed the now unneeded includes
- GetBitmap locks pixels with a comment explaining
- CreateOwnedCanvas clears the bitmap to 0, as per test expectations

https://codereview.chromium.org/2758413002/diff/120001/ash/common/accelerator...
File ash/common/accelerators/debug_commands.cc (right):

https://codereview.chromium.org/2758413002/diff/120001/ash/common/accelerator...
ash/common/accelerators/debug_commands.cc:24: #include
"third_party/skia/include/core/SkSurface.h"
On 2017/03/21 22:42:48, danakj wrote:
> Good news this won't be needed with
https://codereview.chromium.org/2764163002/
> <_<

I'll wait until it lands and remove it.

https://codereview.chromium.org/2758413002/diff/120001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2758413002/diff/120001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:609: bitmap_->allocPixels(info);
On 2017/03/21 22:42:48, danakj wrote:
> don't you have to construct the bitmap_ first?

Whoops. Done.

[commit-bot: I haz the power, 2017-03-22 17:47:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-03-22 18:46:39]

ui/gfx LGTM

https://codereview.chromium.org/2758413002/diff/200001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2758413002/diff/200001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:546: // When the bitmap is copied, it shares the underlying
pixelref, but doesn't
amazing comment for amazing behaviour

[vmpstr, 2017-03-22 19:04:30]

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

[vmpstr, 2017-03-22 19:04:31]

+jochen for chrome/, components/ and content/. Please take a look, it's just a
function rename in those places.

[commit-bot: I haz the power, 2017-03-22 19:43:16]

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

[commit-bot: I haz the power, 2017-03-22 19:43:17]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-03-22 21:16:36]

Description was changed from

==========
cc/paint: Remove PaintCanvas::peekPixels.

This patch removes peekPixels and replaces callers with an explicit
SkBitmap construction and PaintCanvas (as opposed to PaintSurface).

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc/paint: Remove PaintCanvas::peekPixels.

This patch removes peekPixels and replaces callers with an explicit
SkBitmap construction and PaintCanvas (as opposed to PaintSurface).

R=enne@chromium.org
TBR=jochen@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-03-22 21:17:08]

TBRing jochen for function name changes

[vmpstr, 2017-03-22 21:17:14]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-03-22 21:17:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
emircan@chromium.org, dalecurtis@chromium.org, msw@chromium.org,
enne@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2758413002/#ps200001
(title: "update")

[commit-bot: I haz the power, 2017-03-22 21:18:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 21:24:12]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490217434104870,
"parent_rev": "da65ddd7e896738a9ccbeacf277105394d429291", "commit_rev":
"4f7674971344b7788b85caabba2a234aa3918e6b"}

[commit-bot: I haz the power, 2017-03-22 21:24:56]

Description was changed from

==========
cc/paint: Remove PaintCanvas::peekPixels.

This patch removes peekPixels and replaces callers with an explicit
SkBitmap construction and PaintCanvas (as opposed to PaintSurface).

R=enne@chromium.org
TBR=jochen@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc/paint: Remove PaintCanvas::peekPixels.

This patch removes peekPixels and replaces callers with an explicit
SkBitmap construction and PaintCanvas (as opposed to PaintSurface).

R=enne@chromium.org
TBR=jochen@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2758413002
Cr-Commit-Position: refs/heads/master@{#458880}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f7674971344b7788b85caabba2a...
==========

[commit-bot: I haz the power, 2017-03-22 21:24:58]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/4f7674971344b7788b85caabba2a...