switch GatherPixelRefs to use SkBaseDevice instead of SkBitmapDevice

switch GatherPixelRefs to use SkBaseDevice instead of SkBitmapDevice

BUG=
R=robertphillips@google.com, scroggo@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12259

[scroggo, 2013-11-12 18:21:59]

https://codereview.chromium.org/70473003/diff/1/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/70473003/diff/1/include/core/SkDevice.h#newco...
include/core/SkDevice.h:88: bounds->setXYWH(origin.x(), origin.y(),
this->width(), this->height());
Check for NULL or assert?

https://codereview.chromium.org/70473003/diff/1/src/utils/SkPictureUtils.cpp
File src/utils/SkPictureUtils.cpp (right):

https://codereview.chromium.org/70473003/diff/1/src/utils/SkPictureUtils.cpp#...
src/utils/SkPictureUtils.cpp:187: virtual SkBaseDevice*
onCreateCompatibleDevice(SkBitmap::Config config,
nit: newline between functions.

https://codereview.chromium.org/70473003/diff/1/src/utils/SkPictureUtils.cpp#...
src/utils/SkPictureUtils.cpp:193: virtual void flush() SK_OVERRIDE {}
nit: newline between functions.

https://codereview.chromium.org/70473003/diff/1/src/utils/SkPictureUtils.cpp#...
src/utils/SkPictureUtils.cpp:255: SkBitmap emptyBitmap;
Can this be removed?

[reed1, 2013-11-12 18:33:00]

I vote to leave the missing newlines missing, just to keep all that crap compact
and (slightly) easier to read/skip. However, will fix if the reviewers ask again
:)

https://codereview.chromium.org/70473003/diff/1/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/70473003/diff/1/include/core/SkDevice.h#newco...
include/core/SkDevice.h:88: bounds->setXYWH(origin.x(), origin.y(),
this->width(), this->height());
On 2013/11/12 18:22:00, scroggo wrote:
> Check for NULL or assert?

Done.

[robertphillips, 2013-11-12 18:36:24]

lgtm - it's a start.

https://codereview.chromium.org/70473003/diff/1/src/utils/SkPictureUtils.cpp
File src/utils/SkPictureUtils.cpp (right):

https://codereview.chromium.org/70473003/diff/1/src/utils/SkPictureUtils.cpp#...
src/utils/SkPictureUtils.cpp:90: // TODO: allow this call to return failure, or
move to SkBitmapDevice only.
I would like it to be on SkBitmapDevice only (although that is going to be
tough).

https://codereview.chromium.org/70473003/diff/1/src/utils/SkPictureUtils.cpp#...
src/utils/SkPictureUtils.cpp:183: 
Can this move to SkBitmapDevice?

https://codereview.chromium.org/70473003/diff/1/src/utils/SkPictureUtils.cpp#...
src/utils/SkPictureUtils.cpp:186: }
I would think this could work - in theory.

[scroggo, 2013-11-12 18:37:30]

On 2013/11/12 18:33:00, reed1 wrote:
> I vote to leave the missing newlines missing, just to keep all that crap
compact
> and (slightly) easier to read/skip. However, will fix if the reviewers ask
again
> :)

Fine with me.

https://codereview.chromium.org/70473003/diff/80001/src/utils/SkPictureUtils.cpp
File src/utils/SkPictureUtils.cpp (right):

https://codereview.chromium.org/70473003/diff/80001/src/utils/SkPictureUtils....
src/utils/SkPictureUtils.cpp:49: *  FIXME: Derive from SkBaseDevice.
I guess you just rebased and got this, but it's no longer needed.

https://codereview.chromium.org/70473003/diff/80001/src/utils/SkPictureUtils....
src/utils/SkPictureUtils.cpp:256: SkBitmap emptyBitmap;
emptyBitmap is no longer used

[reed1, 2013-11-12 21:21:24]

https://codereview.chromium.org/70473003/diff/80001/src/utils/SkPictureUtils.cpp
File src/utils/SkPictureUtils.cpp (right):

https://codereview.chromium.org/70473003/diff/80001/src/utils/SkPictureUtils....
src/utils/SkPictureUtils.cpp:49: *  FIXME: Derive from SkBaseDevice.
On 2013/11/12 18:37:31, scroggo wrote:
> I guess you just rebased and got this, but it's no longer needed.

Done.

https://codereview.chromium.org/70473003/diff/80001/src/utils/SkPictureUtils....
src/utils/SkPictureUtils.cpp:256: SkBitmap emptyBitmap;
On 2013/11/12 18:37:31, scroggo wrote:
> emptyBitmap is no longer used

Done.

[scroggo, 2013-11-12 21:28:48]

https://codereview.chromium.org/70473003/diff/170001/src/utils/SkPictureUtils...
File src/utils/SkPictureUtils.cpp (right):

https://codereview.chromium.org/70473003/diff/170001/src/utils/SkPictureUtils...
src/utils/SkPictureUtils.cpp:170: return SkNEW_ARGS(GatherPixelRefDevice,
(width, height, fPRSet));
Does this mean that if someone calls createCompatibleDevice the new device will
share the PixelRefSet? Is that intended?

[reed1, 2013-11-12 21:37:41]

https://codereview.chromium.org/70473003/diff/170001/src/utils/SkPictureUtils...
File src/utils/SkPictureUtils.cpp (right):

https://codereview.chromium.org/70473003/diff/170001/src/utils/SkPictureUtils...
src/utils/SkPictureUtils.cpp:170: return SkNEW_ARGS(GatherPixelRefDevice,
(width, height, fPRSet));
On 2013/11/12 21:28:48, scroggo wrote:
> Does this mean that if someone calls createCompatibleDevice the new device
will
> share the PixelRefSet? Is that intended?

I think we can assert that we only get called for savelayer, in which case I
think this is the correct behavior.

[reed1, 2013-11-12 22:05:34]

ptal

[scroggo, 2013-11-12 22:10:25]

On 2013/11/12 22:05:34, reed1 wrote:
> ptal

Sorry, I thought I had already approved this.

LGTM

[reed1, 2013-11-12 22:14:16]

Committed patchset #4 manually as r12259 (presubmit successful).