accessBitmap is no longer called/needed

Subclasses of SkDevice that overrode onAccessBitmap can remove that code -- it is not being called.

One caller on platform_skia was calling it (incidentally), but they can call accessPixels() instead for the same result.

BUG=647756
Committed: https://crrev.com/ff7e85cb9b649940e25ef66cbf84910867551f75
Cr-Commit-Position: refs/heads/master@{#419446}

[reed1, 2016-09-16 18:45:09]

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

[reed1, 2016-09-16 18:45:22]

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

[commit-bot: I haz the power, 2016-09-16 18:46:12]

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, 2016-09-16 19:28:46]

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

[commit-bot: I haz the power, 2016-09-16 19:28:47]

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

[tomhudson, 2016-09-16 19:37:31]

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

[tomhudson, 2016-09-16 19:37:31]

Need to do something about bitmap_platform_device_win.cc's onAccessBitmap()

https://codereview.chromium.org/2340813006/diff/20001/skia/ext/bitmap_platfor...
File skia/ext/bitmap_platform_device_skia.cc (right):

https://codereview.chromium.org/2340813006/diff/20001/skia/ext/bitmap_platfor...
skia/ext/bitmap_platform_device_skia.cc:57: return accessPixels(&pixmap) ?
pixmap.writable_addr() : nullptr;
This caches a void* to the SkBitmapDevice's SkBitmap::getPixels() in the pixmap,
and then returns it - so I guess no lifetime issues, slightly roundabout but
that's how the API leans these days.

[reed1, 2016-09-16 20:17:39]

Description was changed from

==========
switch from (deprecated) accessBitmap to accessPixels

BUG=
==========

to

==========
Subclasses of SkDevice that overrode onAccessBitmap can remove that code -- it
is not being called.

One caller on platform_skia was calling it (incidentally), but they can call
accessPixels() instead for the same result.

BUG=
==========

[reed1, 2016-09-16 20:17:59]

Description was changed from

==========
Subclasses of SkDevice that overrode onAccessBitmap can remove that code -- it
is not being called.

One caller on platform_skia was calling it (incidentally), but they can call
accessPixels() instead for the same result.

BUG=
==========

to

==========
Subclasses of SkDevice that overrode onAccessBitmap can remove that code -- it
is not being called.

One caller on platform_skia was calling it (incidentally), but they can call
accessPixels() instead for the same result.

BUG=647756
==========

[reed1, 2016-09-16 20:18:04]

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

[commit-bot: I haz the power, 2016-09-16 20:18:43]

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

[reed1, 2016-09-16 20:24:13]

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

[commit-bot: I haz the power, 2016-09-16 20:24:27]

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

[reed1, 2016-09-16 20:29:03]

https://codereview.chromium.org/2340813006/diff/20001/skia/ext/bitmap_platfor...
File skia/ext/bitmap_platform_device_skia.cc (right):

https://codereview.chromium.org/2340813006/diff/20001/skia/ext/bitmap_platfor...
skia/ext/bitmap_platform_device_skia.cc:57: return accessPixels(&pixmap) ?
pixmap.writable_addr() : nullptr;
On 2016/09/16 19:37:31, tomhudson wrote:
> This caches a void* to the SkBitmapDevice's SkBitmap::getPixels() in the
pixmap,
> and then returns it - so I guess no lifetime issues, slightly roundabout but
> that's how the API leans these days.

Acknowledged.

[commit-bot: I haz the power, 2016-09-16 22:13:34]

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

[commit-bot: I haz the power, 2016-09-16 22:13:35]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-09-17 11:40:25]

ptal

[f(malita), 2016-09-17 14:21:23]

https://codereview.chromium.org/2340813006/diff/60001/skia/ext/platform_canva...
File skia/ext/platform_canvas_unittest.cc (right):

https://codereview.chromium.org/2340813006/diff/60001/skia/ext/platform_canva...
skia/ext/platform_canvas_unittest.cc:107: if (!canvas.readPixels(&bitmap, x, y))
{
nit: can readPixels ever fail for this unit test?  If not, maybe
ASSERT_TRUE(canvas.readPixels(...))?

https://codereview.chromium.org/2340813006/diff/60001/skia/ext/platform_canva...
skia/ext/platform_canvas_unittest.cc:112: if (!IsOfColor(bitmap, x, y,
canvas_color)) return false;
Previously we were reading the full backing store, but now we're only extracting
[x y w h].  I think the coords below need to be adjusted relative to the new
origin (x,y).  Might be easier to just read the full canvas, like before.

(kinda scary that it passes as-is; looks like there's only one client which uses
smallish insets so that may explain it)

[reed1, 2016-09-19 00:11:52]

https://codereview.chromium.org/2340813006/diff/60001/skia/ext/platform_canva...
File skia/ext/platform_canvas_unittest.cc (right):

https://codereview.chromium.org/2340813006/diff/60001/skia/ext/platform_canva...
skia/ext/platform_canvas_unittest.cc:107: if (!canvas.readPixels(&bitmap, x, y))
{
On 2016/09/17 14:21:22, f(malita) wrote:
> nit: can readPixels ever fail for this unit test?  If not, maybe
> ASSERT_TRUE(canvas.readPixels(...))?

This is a general pixel checker. I'm not sure that all callers ensure that read
should always work.

https://codereview.chromium.org/2340813006/diff/60001/skia/ext/platform_canva...
skia/ext/platform_canvas_unittest.cc:112: if (!IsOfColor(bitmap, x, y,
canvas_color)) return false;
On 2016/09/17 14:21:22, f(malita) wrote:
> Previously we were reading the full backing store, but now we're only
extracting
> [x y w h].  I think the coords below need to be adjusted relative to the new
> origin (x,y).  Might be easier to just read the full canvas, like before.
> 
> (kinda scary that it passes as-is; looks like there's only one client which
uses
> smallish insets so that may explain it)

Good point about needing the entire canvas read. Will fix.

[reed1, 2016-09-19 12:29:16]

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

[commit-bot: I haz the power, 2016-09-19 12:29:23]

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

[tomhudson, 2016-09-19 13:02:30]

LGTM

https://codereview.chromium.org/2340813006/diff/80001/skia/ext/bitmap_platfor...
File skia/ext/bitmap_platform_device_skia.cc (right):

https://codereview.chromium.org/2340813006/diff/80001/skia/ext/bitmap_platfor...
skia/ext/bitmap_platform_device_skia.cc:55: // pixels? Maybe this won't be
called at all.
Why don't we clean up this other seemingly-obsolete comment while we're in here?

[f(malita), 2016-09-19 13:04:11]

lgtm

[reed1, 2016-09-19 13:06:58]

https://codereview.chromium.org/2340813006/diff/80001/skia/ext/bitmap_platfor...
File skia/ext/bitmap_platform_device_skia.cc (right):

https://codereview.chromium.org/2340813006/diff/80001/skia/ext/bitmap_platfor...
skia/ext/bitmap_platform_device_skia.cc:55: // pixels? Maybe this won't be
called at all.
On 2016/09/19 13:02:30, tomhudson wrote:
> Why don't we clean up this other seemingly-obsolete comment while we're in
here?

I think its still a valid question: the return value is meant to be the "native
handle", but for this backend, there is none. Perhaps we could even return null?

[reed1, 2016-09-19 13:07:03]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-09-19 13:07:09]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-09-19 13:07:25]

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

[tomhudson, 2016-09-19 13:13:47]

On 2016/09/19 13:06:58, reed1 wrote:
>
https://codereview.chromium.org/2340813006/diff/80001/skia/ext/bitmap_platfor...
> File skia/ext/bitmap_platform_device_skia.cc (right):
> 
>
https://codereview.chromium.org/2340813006/diff/80001/skia/ext/bitmap_platfor...
> skia/ext/bitmap_platform_device_skia.cc:55: // pixels? Maybe this won't be
> called at all.
> On 2016/09/19 13:02:30, tomhudson wrote:
> > Why don't we clean up this other seemingly-obsolete comment while we're in
> here?
> 
> I think its still a valid question: the return value is meant to be the
"native
> handle", but for this backend, there is none. Perhaps we could even return
null?

There's code in Chrome that assumes it can get at raw pixel memory.
If we return NULL here, we're breaking that code, or asserting that that code
can only run on other platforms.
Maybe that's reasonable to do, but I'd want to take a survey of the code before
making the choice.

[commit-bot: I haz the power, 2016-09-19 13:40:26]

Description was changed from

==========
Subclasses of SkDevice that overrode onAccessBitmap can remove that code -- it
is not being called.

One caller on platform_skia was calling it (incidentally), but they can call
accessPixels() instead for the same result.

BUG=647756
==========

to

==========
Subclasses of SkDevice that overrode onAccessBitmap can remove that code -- it
is not being called.

One caller on platform_skia was calling it (incidentally), but they can call
accessPixels() instead for the same result.

BUG=647756
==========

[commit-bot: I haz the power, 2016-09-19 13:40:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-19 13:42:35]

Description was changed from

==========
Subclasses of SkDevice that overrode onAccessBitmap can remove that code -- it
is not being called.

One caller on platform_skia was calling it (incidentally), but they can call
accessPixels() instead for the same result.

BUG=647756
==========

to

==========
Subclasses of SkDevice that overrode onAccessBitmap can remove that code -- it
is not being called.

One caller on platform_skia was calling it (incidentally), but they can call
accessPixels() instead for the same result.

BUG=647756

Committed: https://crrev.com/ff7e85cb9b649940e25ef66cbf84910867551f75
Cr-Commit-Position: refs/heads/master@{#419446}
==========

[commit-bot: I haz the power, 2016-09-19 13:42:36]

Patchset 5 (id:??) landed as
https://crrev.com/ff7e85cb9b649940e25ef66cbf84910867551f75
Cr-Commit-Position: refs/heads/master@{#419446}