Remove uses of SkBitmap::Config (deprecated) from core/ and platform/.

Remove uses of SkBitmap::Config (deprecated) from core/ and platform/.

These should be replaced by SkColorType and SkImageInfo.
In many cases, shortcut methods like SkBitmap::allocN32Pixels can be used.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169464

[jbroman, 2014-03-15 23:48:02]

My understanding is that Skia consumers are intended to be moving away from
SkBitmap::Config (if I'm reading reed's skia-discuss email properly). Moving
away from deprecated APIs seems like a Good Thing™, so have a look.

[Stephen Chennney, 2014-03-17 12:25:37]

Added reed@ to be sure we're doing this the right way.

[reed1, 2014-03-17 13:25:17]

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Def...
File Source/platform/graphics/DeferredImageDecoderTest.cpp (right):

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Def...
Source/platform/graphics/DeferredImageDecoderTest.cpp:66: return new
SkCanvas(bitmap);
There is an even nicer way to to this...

return SkCanvas::NewRasterN32(width, height);

One advantage is that it will return NULL if it cannot allocate the canvas or
its pixels.

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Una...
File Source/platform/graphics/UnacceleratedImageBufferSurface.cpp (right):

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Una...
Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:43:
bitmap.allocN32Pixels(size.width(), size.height());
allocN32Pixels returns false on error, so you can use that to know if you should
return early.

if (!bitmap.allocN32Pixels(...)) return;

Alternatively, you perhaps should call SkCanvas::NewRasterN32(...) here, which
returns NULL on failure, skipping the need to create a temp bitmap which you
(the caller) don't need.

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/ski...
File Source/platform/graphics/skia/GaneshUtils.cpp (right):

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/ski...
Source/platform/graphics/skia/GaneshUtils.cpp:39: bool
ensureTextureBackedSkBitmap(GrContext* gr, SkBitmap& bitmap, const IntSize&
size, GrSurfaceOrigin origin, GrPixelConfig config)
Who calls this? Ganesh wants to move away from the dualism that bitmaps can have
pixels or textures behind them. Don't want to hold up this CL over this, but its
something we should look into.

https://codereview.chromium.org/201213002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/201213002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/ImageFrame.cpp:111:
m_bitmap->bitmap().setConfig(SkImageInfo::MakeN32Premul(newWidth, newHeight));
In the future, can we change over to using SkPixelRefFactory? That would allow
this to be an atomic operation, instead of setConfig + alloc.

    SkBitmap::allocPixels(const SkImageInfo&, SkPixelRefFactory*, SkColorTable*)

Not meant to block this CL...

[Stephen Chennney, 2014-03-17 14:57:34]

I'm happy with the overall direction. When addressing Mike's comments, please
make sure that the callers can handle null return values. I'll do a final pass
once you have a new patch up.

[jbroman, 2014-03-17 16:46:37]

Added explicit handling for the allocation-failure case in several places (note
that several of these assumed success before), as requested.

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Def...
File Source/platform/graphics/DeferredImageDecoderTest.cpp (right):

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Def...
Source/platform/graphics/DeferredImageDecoderTest.cpp:66: return new
SkCanvas(bitmap);
On 2014/03/17 13:25:18, reed1 wrote:
> There is an even nicer way to to this...
> 
> return SkCanvas::NewRasterN32(width, height);
> 
> One advantage is that it will return NULL if it cannot allocate the canvas or
> its pixels.

Done, and inlined at the call site.

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:783:
misspellBitmap[index]->allocN32Pixels(rowPixels, colPixels);
This does not handle the case where the allocation fails (and false is
returned), but neither did it before.

The core seems to be assuming that this cannot happen, since draw[12]xMarker
will call getAddr32 on the result regardless, which triggers an SkASSERT and
then (if asserts aren't enabled returns and uses an address near 0x0).

I'd appreciate schenney's opinion on whether, or how, we should handle this
case. One option might be to free the SkBitmap and early-return.

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:838:
misspellBitmap[index]->allocN32Pixels(rowPixels, colPixels);
And here as well.

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Una...
File Source/platform/graphics/UnacceleratedImageBufferSurface.cpp (right):

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Una...
Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:43:
bitmap.allocN32Pixels(size.width(), size.height());
On 2014/03/17 13:25:18, reed1 wrote:
> allocN32Pixels returns false on error, so you can use that to know if you
should
> return early.
> 
> if (!bitmap.allocN32Pixels(...)) return;
> 
> Alternatively, you perhaps should call SkCanvas::NewRasterN32(...) here, which
> returns NULL on failure, skipping the need to create a temp bitmap which you
> (the caller) don't need.

Done. Didn't realize SkCanvas::NewRasterN32 existed. Refactored in a way that
should not affect the overall behaviour of this constructor.

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/ski...
File Source/platform/graphics/skia/GaneshUtils.cpp (right):

https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/ski...
Source/platform/graphics/skia/GaneshUtils.cpp:39: bool
ensureTextureBackedSkBitmap(GrContext* gr, SkBitmap& bitmap, const IntSize&
size, GrSurfaceOrigin origin, GrPixelConfig config)
On 2014/03/17 13:25:18, reed1 wrote:
> Who calls this? Ganesh wants to move away from the dualism that bitmaps can
have
> pixels or textures behind them. Don't want to hold up this CL over this, but
its
> something we should look into.

WebMediaClientPlayerImpl and WebGLImageBufferSurface. I don't know enough about
Ganesh to know why they call this, though.

https://codereview.chromium.org/201213002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/201213002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/ImageFrame.cpp:111:
m_bitmap->bitmap().setConfig(SkImageInfo::MakeN32Premul(newWidth, newHeight));
On 2014/03/17 13:25:18, reed1 wrote:
> In the future, can we change over to using SkPixelRefFactory? That would allow
> this to be an atomic operation, instead of setConfig + alloc.
> 
>     SkBitmap::allocPixels(const SkImageInfo&, SkPixelRefFactory*,
SkColorTable*)
> 
> Not meant to block this CL...

I don't know enough about the discardable/external memory allocators to know how
easy that would be. Is SkPixelRefFactory intended to supersede
SkBitmap::Allocator everywhere going forward?

I'd be happy to leave a FIXME, here or elsewhere, if you'd like. Or we could
file a bug.

[reed1, 2014-03-17 17:35:04]

On 2014/03/17 16:46:37, jbroman wrote:
> Added explicit handling for the allocation-failure case in several places
(note
> that several of these assumed success before), as requested.
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Def...
> File Source/platform/graphics/DeferredImageDecoderTest.cpp (right):
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Def...
> Source/platform/graphics/DeferredImageDecoderTest.cpp:66: return new
> SkCanvas(bitmap);
> On 2014/03/17 13:25:18, reed1 wrote:
> > There is an even nicer way to to this...
> > 
> > return SkCanvas::NewRasterN32(width, height);
> > 
> > One advantage is that it will return NULL if it cannot allocate the canvas
or
> > its pixels.
> 
> Done, and inlined at the call site.
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Gra...
> File Source/platform/graphics/GraphicsContext.cpp (right):
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Gra...
> Source/platform/graphics/GraphicsContext.cpp:783:
> misspellBitmap[index]->allocN32Pixels(rowPixels, colPixels);
> This does not handle the case where the allocation fails (and false is
> returned), but neither did it before.
> 
> The core seems to be assuming that this cannot happen, since draw[12]xMarker
> will call getAddr32 on the result regardless, which triggers an SkASSERT and
> then (if asserts aren't enabled returns and uses an address near 0x0).
> 
> I'd appreciate schenney's opinion on whether, or how, we should handle this
> case. One option might be to free the SkBitmap and early-return.
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Gra...
> Source/platform/graphics/GraphicsContext.cpp:838:
> misspellBitmap[index]->allocN32Pixels(rowPixels, colPixels);
> And here as well.
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Una...
> File Source/platform/graphics/UnacceleratedImageBufferSurface.cpp (right):
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/Una...
> Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:43:
> bitmap.allocN32Pixels(size.width(), size.height());
> On 2014/03/17 13:25:18, reed1 wrote:
> > allocN32Pixels returns false on error, so you can use that to know if you
> should
> > return early.
> > 
> > if (!bitmap.allocN32Pixels(...)) return;
> > 
> > Alternatively, you perhaps should call SkCanvas::NewRasterN32(...) here,
which
> > returns NULL on failure, skipping the need to create a temp bitmap which you
> > (the caller) don't need.
> 
> Done. Didn't realize SkCanvas::NewRasterN32 existed. Refactored in a way that
> should not affect the overall behaviour of this constructor.
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/ski...
> File Source/platform/graphics/skia/GaneshUtils.cpp (right):
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/graphics/ski...
> Source/platform/graphics/skia/GaneshUtils.cpp:39: bool
> ensureTextureBackedSkBitmap(GrContext* gr, SkBitmap& bitmap, const IntSize&
> size, GrSurfaceOrigin origin, GrPixelConfig config)
> On 2014/03/17 13:25:18, reed1 wrote:
> > Who calls this? Ganesh wants to move away from the dualism that bitmaps can
> have
> > pixels or textures behind them. Don't want to hold up this CL over this, but
> its
> > something we should look into.
> 
> WebMediaClientPlayerImpl and WebGLImageBufferSurface. I don't know enough
about
> Ganesh to know why they call this, though.
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/image-decode...
> File Source/platform/image-decoders/ImageFrame.cpp (right):
> 
>
https://codereview.chromium.org/201213002/diff/1/Source/platform/image-decode...
> Source/platform/image-decoders/ImageFrame.cpp:111:
> m_bitmap->bitmap().setConfig(SkImageInfo::MakeN32Premul(newWidth, newHeight));
> On 2014/03/17 13:25:18, reed1 wrote:
> > In the future, can we change over to using SkPixelRefFactory? That would
allow
> > this to be an atomic operation, instead of setConfig + alloc.
> > 
> >     SkBitmap::allocPixels(const SkImageInfo&, SkPixelRefFactory*,
> SkColorTable*)
> > 
> > Not meant to block this CL...
> 
> I don't know enough about the discardable/external memory allocators to know
how
> easy that would be. Is SkPixelRefFactory intended to supersede
> SkBitmap::Allocator everywhere going forward?
> 
> I'd be happy to leave a FIXME, here or elsewhere, if you'd like. Or we could
> file a bug.

No need for a bug, as it will naturally come up later when I try to refactor our
API.

[reed1, 2014-03-17 17:36:51]

lgtm

https://codereview.chromium.org/201213002/diff/20001/Source/core/frame/ImageB...
File Source/core/frame/ImageBitmapTest.cpp (right):

https://codereview.chromium.org/201213002/diff/20001/Source/core/frame/ImageB...
Source/core/frame/ImageBitmapTest.cpp:57:
ASSERT_TRUE(m_bitmap.allocN32Pixels(10, 10));
I presume ASSERT_TRUE executes its argument in debug *and* release builds?

[jbroman, 2014-03-17 17:41:19]

On 2014/03/17 17:36:51, reed1 wrote:
> lgtm
> 
>
https://codereview.chromium.org/201213002/diff/20001/Source/core/frame/ImageB...
> File Source/core/frame/ImageBitmapTest.cpp (right):
> 
>
https://codereview.chromium.org/201213002/diff/20001/Source/core/frame/ImageB...
> Source/core/frame/ImageBitmapTest.cpp:57:
> ASSERT_TRUE(m_bitmap.allocN32Pixels(10, 10));
> I presume ASSERT_TRUE executes its argument in debug *and* release builds?

Correct. In googletest parlance, EXPECT_* are checks that allow testing to
continue, and ASSERT_* are checks that abort the test if they fail, but neither
is compiled out in debug builds.

[jbroman, 2014-03-17 17:46:32]

On 2014/03/17 17:41:19, jbroman wrote:
> is compiled out in debug builds.
or release builds :)

[Stephen Chennney, 2014-03-18 12:47:56]

Apparently I didn't send this yesterday.

https://codereview.chromium.org/201213002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/201213002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:783:
misspellBitmap[index]->allocN32Pixels(rowPixels, colPixels);
On failure, release and return without drawing, please. We've seen bugs with
this kind of thing before.

https://codereview.chromium.org/201213002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:838:
misspellBitmap[index]->allocN32Pixels(rowPixels, colPixels);
Ditto.

https://codereview.chromium.org/201213002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/UnacceleratedImageBufferSurface.cpp (right):

https://codereview.chromium.org/201213002/diff/20001/Source/platform/graphics...
Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:41: ,
m_canvas(adoptPtr(SkCanvas::NewRasterN32(size.width(), size.height())))
I understand that this CL is not changing the behavior, but is this not one of
the cases where we are violating ref counting semantics? If so, I would tend to
fix it now.

[jbroman, 2014-03-18 13:44:52]

https://codereview.chromium.org/201213002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/201213002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:783:
misspellBitmap[index]->allocN32Pixels(rowPixels, colPixels);
On 2014/03/18 12:47:57, Stephen Chenney wrote:
> On failure, release and return without drawing, please. We've seen bugs with
> this kind of thing before.

Done. Rather than try to use "delete" here (which is prone to being missed if
other returns appear), I've opted to use an SkBitmap on the stack to do the
drawing (which is properly freed if we cannot allocate pixels), and then move it
into misspellBitmap after it succeeds.

https://codereview.chromium.org/201213002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/UnacceleratedImageBufferSurface.cpp (right):

https://codereview.chromium.org/201213002/diff/20001/Source/platform/graphics...
Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:41: ,
m_canvas(adoptPtr(SkCanvas::NewRasterN32(size.width(), size.height())))
On 2014/03/18 12:47:57, Stephen Chenney wrote:
> I understand that this CL is not changing the behavior, but is this not one of
> the cases where we are violating ref counting semantics? If so, I would tend
to
> fix it now.

Yes, it is; SkCanvas extends SkRefCnt (so it's possible for others to take
references and have the canvas freed from under them). I'm happy to change this
to a RefPtr.

[Stephen Chennney, 2014-03-18 14:15:58]

Great. LGTM.

[jbroman, 2014-03-18 14:20:11]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-03-18 14:20:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/201213002/40001

[commit-bot: I haz the power, 2014-03-18 19:14:12]

Change committed as 169464