Replace uses of legacy SkBitmap::copyTo() API

Replace uses of legacy SkBitmap::copyTo() API

BUG=skia:6464
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2855503003
Cr-Commit-Position: refs/heads/master@{#468945}
Committed: https://chromium.googlesource.com/chromium/src/+/95ea1df0596b4ee4ac745405b321e20b68f05d42

[msarett, 2017-05-01 16:21:01]

Description was changed from

==========
Replace uses of legacy SkBitmap::copyTo() API

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

to

==========
Replace uses of legacy SkBitmap::copyTo() API

BUG=skia:6464
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[msarett1, 2017-05-01 17:01:25]

msarett@chromium.org changed reviewers:
+ fmalita@chromium.org, msarett@chromium.org, reed@google.com

[f(malita), 2017-05-01 18:00:07]

https://codereview.chromium.org/2855503003/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc
(right):

https://codereview.chromium.org/2855503003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc:84:
image_.asLegacyBitmap(&before_bmp_, SkImage::kRW_LegacyBitmapMode);
asLegacyBitmap() doesn't copy the pixels for raster SkImages, so these are not
equivalent, no?

Depending on the test semantics, this may break.

https://codereview.chromium.org/2855503003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc:89:
image_.asLegacyBitmap(&after_bmp_, SkImage::kRW_LegacyBitmapMode);
Ditto.

[msarett1, 2017-05-01 18:04:55]

https://codereview.chromium.org/2855503003/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc
(right):

https://codereview.chromium.org/2855503003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc:84:
image_.asLegacyBitmap(&before_bmp_, SkImage::kRW_LegacyBitmapMode);
On 2017/05/01 18:00:06, f(malita) wrote:
> asLegacyBitmap() doesn't copy the pixels for raster SkImages, so these are not
> equivalent, no?
> 
> Depending on the test semantics, this may break.

I think that using kRW_LegacyBitmapMode will force a copy.  Regardless, this
might be confusing.  What do you think?

[f(malita), 2017-05-01 18:36:01]

https://codereview.chromium.org/2855503003/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc
(right):

https://codereview.chromium.org/2855503003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc:84:
image_.asLegacyBitmap(&before_bmp_, SkImage::kRW_LegacyBitmapMode);
On 2017/05/01 18:04:55, msarett1 wrote:
> On 2017/05/01 18:00:06, f(malita) wrote:
> > asLegacyBitmap() doesn't copy the pixels for raster SkImages, so these are
not
> > equivalent, no?
> > 
> > Depending on the test semantics, this may break.
> 
> I think that using kRW_LegacyBitmapMode will force a copy.  Regardless, this
> might be confusing.  What do you think?

Ah yeah, indeed.  It does feel a bit fragile/incidental, but LGTM for a test.

Maybe add a comment to explain that we're forcing a deep copy.

[msarett1, 2017-05-01 19:53:50]

https://codereview.chromium.org/2855503003/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc
(right):

https://codereview.chromium.org/2855503003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager_interactive_uitest.cc:84:
image_.asLegacyBitmap(&before_bmp_, SkImage::kRW_LegacyBitmapMode);
On 2017/05/01 18:36:01, f(malita) wrote:
> On 2017/05/01 18:04:55, msarett1 wrote:
> > On 2017/05/01 18:00:06, f(malita) wrote:
> > > asLegacyBitmap() doesn't copy the pixels for raster SkImages, so these are
> not
> > > equivalent, no?
> > > 
> > > Depending on the test semantics, this may break.
> > 
> > I think that using kRW_LegacyBitmapMode will force a copy.  Regardless, this
> > might be confusing.  What do you think?
> 
> Ah yeah, indeed.  It does feel a bit fragile/incidental, but LGTM for a test.
> 
> Maybe add a comment to explain that we're forcing a deep copy.

Agreed, yeah it is.  I've changed the impl to alloc() and readPixels()

[msarett1, 2017-05-01 20:44:02]

Adding OWNERs for review.  Please take a look!

dcheng@
cc/ipc/
ui/base/
components/arc/bitmap/

dfalcantara@
chrome/browser/bitmap_fetcher/

dmazzoni@
chrome/browser/chromeos/accessibility/

achuith@
chrome/browser/chromeos

jochen@
chrome/renderer
content/

skyostil@
headless/

[msarett1, 2017-05-01 20:45:15]

msarett@chromium.org changed reviewers:
+ achuith@chromium.org, dcheng@chromium.org, dfalcantara@chromium.org,
dmazzoni@chromium.org, jochen@chromium.org, skyostil@chromium.org

[msarett1, 2017-05-01 20:45:15]

Adding OWNERs for review.  Please take a look!

dcheng@
cc/ipc/
ui/base/
components/arc/bitmap/

dfalcantara@
chrome/browser/bitmap_fetcher/

dmazzoni@
chrome/browser/chromeos/accessibility/

achuith@
chrome/browser/chromeos

jochen@
chrome/renderer
content/

skyostil@
headless/

[gone, 2017-05-02 01:10:24]

bitmap_fetcher lgtm

[jochen (gone - plz use gerrit), 2017-05-02 09:07:59]

lgtm

[msarett1, 2017-05-02 14:07:14]

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

[commit-bot: I haz the power, 2017-05-02 14:07:28]

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-05-02 14:18:07]

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

[commit-bot: I haz the power, 2017-05-02 14:18:09]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[msarett1, 2017-05-02 14:35:01]

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

[commit-bot: I haz the power, 2017-05-02 14:35:18]

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-05-02 14:46:40]

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

[commit-bot: I haz the power, 2017-05-02 14:46:41]

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...)

[msarett1, 2017-05-02 15:05:11]

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

[commit-bot: I haz the power, 2017-05-02 15:05:48]

Dry run: CQ is trying da patch.

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

[msarett1, 2017-05-02 15:46:09]

msarett@chromium.org changed reviewers:
+ rohitrao@chromium.org

[msarett1, 2017-05-02 15:46:10]

Adding ios OWNER.  Updated patch to change additional callsite in ios.

[commit-bot: I haz the power, 2017-05-02 15:52:50]

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

[commit-bot: I haz the power, 2017-05-02 15:52:51]

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

[rohitrao (ping after 24h), 2017-05-02 16:39:17]

ios/ lgtm

[msarett1, 2017-05-02 17:14:24]

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

[commit-bot: I haz the power, 2017-05-02 17:14:42]

Dry run: CQ is trying da patch.

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

[Sami, 2017-05-02 17:57:28]

headless/ lgtm

[commit-bot: I haz the power, 2017-05-02 18:00:38]

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

[commit-bot: I haz the power, 2017-05-02 18:00:39]

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

[msarett1, 2017-05-02 18:03:31]

Patchset #6 (id:100001) has been deleted

[msarett1, 2017-05-02 18:11:54]

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

[commit-bot: I haz the power, 2017-05-02 18:12:20]

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-05-02 18:50:37]

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

[commit-bot: I haz the power, 2017-05-02 18:50:38]

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_...)

[msarett1, 2017-05-02 18:57:00]

Patchset #6 (id:120001) has been deleted

[msarett1, 2017-05-02 18:57:10]

Patchset #5 (id:80001) has been deleted

[msarett1, 2017-05-02 18:57:23]

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

[commit-bot: I haz the power, 2017-05-02 18:58:13]

Dry run: CQ is trying da patch.

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

[dcheng, 2017-05-02 19:16:13]

FWIW, it's not clear to me that readPixels() is clearly superior to copyTo() as
used here: there's a lot of boilerplate arguments that need to be specified with
readPixels() that could be omitted with copyTo()...

https://codereview.chromium.org/2855503003/diff/40001/components/arc/bitmap/b...
File components/arc/bitmap/bitmap_struct_traits.cc (right):

https://codereview.chromium.org/2855503003/diff/40001/components/arc/bitmap/b...
components/arc/bitmap/bitmap_struct_traits.cc:33: SkImageInfo infoN32 =
info.makeColorType(kN32_SkColorType);
Nit: image_info, to follow chrome naming conventions

[msarett1, 2017-05-02 19:28:37]

> FWIW, it's not clear to me that readPixels() is clearly superior to copyTo()
as
> used here: there's a lot of boilerplate arguments that need to be specified
with
> readPixels() that could be omitted with copyTo()...

Understood.  copyTo() is a convenience, the problem is that the existing
versions are poorly specified.

For the one that takes an SkColorType:
(1) What should we do if the existing SkColorSpace is not supported with the new
SkColorType?
(2) What should do if the caller requests an inherently opaque color type (565)
from a bitmap with alpha?
(3) Should we support other strange conversions?  Ex: Alpha8 to a colortype with
color?

For the more basic version:
(1) Does this only fail if the allocation fails?  In that case, should we follow
the normal Skia pattern?  Ex: "bool tryCopyTo()" returns false on OOM, "void
copyTo()" throws on OOM.

I think it makes sense to have some version of a copyTo() API.  Right now, I'm
starting by removing the bad versions.

https://codereview.chromium.org/2855503003/diff/40001/components/arc/bitmap/b...
File components/arc/bitmap/bitmap_struct_traits.cc (right):

https://codereview.chromium.org/2855503003/diff/40001/components/arc/bitmap/b...
components/arc/bitmap/bitmap_struct_traits.cc:33: SkImageInfo infoN32 =
info.makeColorType(kN32_SkColorType);
On 2017/05/02 19:16:13, dcheng (OOO through May 2) wrote:
> Nit: image_info, to follow chrome naming conventions

Done.

[msarett1, 2017-05-02 19:29:35]

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

[commit-bot: I haz the power, 2017-05-02 19:30:24]

Dry run: CQ is trying da patch.

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

[dcheng, 2017-05-02 20:13:12]

On 2017/05/02 19:28:37, msarett1 wrote:
> > FWIW, it's not clear to me that readPixels() is clearly superior to copyTo()
> as
> > used here: there's a lot of boilerplate arguments that need to be specified
> with
> > readPixels() that could be omitted with copyTo()...
> 
> Understood.  copyTo() is a convenience, the problem is that the existing
> versions are poorly specified.
> 
> For the one that takes an SkColorType:
> (1) What should we do if the existing SkColorSpace is not supported with the
new
> SkColorType?
> (2) What should do if the caller requests an inherently opaque color type
(565)
> from a bitmap with alpha?
> (3) Should we support other strange conversions?  Ex: Alpha8 to a colortype
with
> color?
> 
> For the more basic version:
> (1) Does this only fail if the allocation fails?  In that case, should we
follow
> the normal Skia pattern?  Ex: "bool tryCopyTo()" returns false on OOM, "void
> copyTo()" throws on OOM.
> 
> I think it makes sense to have some version of a copyTo() API.  Right now, I'm
> starting by removing the bad versions.

Hmm. I guess I would have liked to see the better version first, but I guess
this is fine. LGTM.

> 
>
https://codereview.chromium.org/2855503003/diff/40001/components/arc/bitmap/b...
> File components/arc/bitmap/bitmap_struct_traits.cc (right):
> 
>
https://codereview.chromium.org/2855503003/diff/40001/components/arc/bitmap/b...
> components/arc/bitmap/bitmap_struct_traits.cc:33: SkImageInfo infoN32 =
> info.makeColorType(kN32_SkColorType);
> On 2017/05/02 19:16:13, dcheng (OOO through May 2) wrote:
> > Nit: image_info, to follow chrome naming conventions
> 
> Done.

[commit-bot: I haz the power, 2017-05-02 20:24:24]

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

[commit-bot: I haz the power, 2017-05-02 20:24:26]

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

[msarett1, 2017-05-02 20:30:02]

Patchset #6 (id:160001) has been deleted

[msarett1, 2017-05-02 20:30:10]

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

[commit-bot: I haz the power, 2017-05-02 20:30:33]

Dry run: CQ is trying da patch.

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

[msarett1, 2017-05-02 21:26:25]

msarett@chromium.org changed reviewers:
+ estade@chromium.org

[msarett1, 2017-05-02 21:26:27]

Just needs one more OWNER.  estade@, would you mind looking at ui/native_theme?

[commit-bot: I haz the power, 2017-05-02 22:38:28]

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

[commit-bot: I haz the power, 2017-05-02 22:38:29]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-05-02 23:25:42]

rubber stamp lgtm

https://codereview.chromium.org/2855503003/diff/180001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2855503003/diff/180001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:80: }
nit: no curlies

[msarett1, 2017-05-03 02:40:03]

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

[msarett1, 2017-05-03 02:40:18]

https://codereview.chromium.org/2855503003/diff/180001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2855503003/diff/180001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:80: }
On 2017/05/02 23:25:42, Evan Stade wrote:
> nit: no curlies

Done.

[commit-bot: I haz the power, 2017-05-03 02:40:22]

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-05-03 04:45:18]

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

[commit-bot: I haz the power, 2017-05-03 04:45:19]

Dry run: This issue passed the CQ dry run.

[msarett1, 2017-05-03 12:17:45]

The CQ bit was checked by msarett@chromium.org

[msarett1, 2017-05-03 12:17:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, jochen@chromium.org, dfalcantara@chromium.org,
skyostil@chromium.org, rohitrao@chromium.org, dcheng@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2855503003/#ps200001
(title: "Remove braces")

[commit-bot: I haz the power, 2017-05-03 12:18:08]

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-05-03 12:22:57]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1493813865741100,
"parent_rev": "67e8cb2a0da35fe657eb91028b49f4340492b935", "commit_rev":
"95ea1df0596b4ee4ac745405b321e20b68f05d42"}

[commit-bot: I haz the power, 2017-05-03 12:23:06]

Description was changed from

==========
Replace uses of legacy SkBitmap::copyTo() API

BUG=skia:6464
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Replace uses of legacy SkBitmap::copyTo() API

BUG=skia:6464
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2855503003
Cr-Commit-Position: refs/heads/master@{#468945}
Committed:
https://chromium.googlesource.com/chromium/src/+/95ea1df0596b4ee4ac745405b321...
==========

[commit-bot: I haz the power, 2017-05-03 12:23:08]

Committed patchset #7 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/95ea1df0596b4ee4ac745405b321...