ImagePixelLocker now manually allocates SkPixmap

ImagePixelLocker now manually allocates SkPixmap

Skia is retracting the SkAutoPixmapStorage class. These are the only two uses of it in Chrome.

Committed: https://crrev.com/a7c239cc4e4100be87bc5b385e2bd41364289014
Cr-Commit-Position: refs/heads/master@{#381700}

[robertphillips, 2016-03-16 18:47:43]

Description was changed from

==========
ImagePixelLocker now manually allocates SkPixmap

Skia is retracting the SkAutoPixmapStorage class. This is the
only use of it in Chrome.
==========

to

==========
ImagePixelLocker now manually allocates SkPixmap

Skia is retracting the SkAutoPixmapStorage class. This is the
only use of it in Chrome.
==========

[robertphillips, 2016-03-16 18:47:43]

robertphillips@google.com changed reviewers:
+ fmalita@chromium.org

[f(malita), 2016-03-16 19:28:29]

https://codereview.chromium.org/1813483002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp
(right):

https://codereview.chromium.org/1813483002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp:54: void*
pixels = sk_malloc_flags(size, 0);
Would it make sense to use a separate SkAutoMalloc m_storage to avoid manual
memory management?

That would likely explode on allocation failures, but I don't think the intent
here is to fail gracefully when OOM (it's just behavior inherited from
SkAutoPixmapStorage).

https://codereview.chromium.org/1813483002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.h
(right):

https://codereview.chromium.org/1813483002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.h:32: SkPixmap
m_pixelStorage;
Let's rename this to m_pixmap.

[robertphillips, 2016-03-16 19:59:10]

https://codereview.chromium.org/1813483002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp
(right):

https://codereview.chromium.org/1813483002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp:54: void*
pixels = sk_malloc_flags(size, 0);
On 2016/03/16 19:28:29, f(malita) wrote:
> Would it make sense to use a separate SkAutoMalloc m_storage to avoid manual
> memory management?
> 
> That would likely explode on allocation failures, but I don't think the intent
> here is to fail gracefully when OOM (it's just behavior inherited from
> SkAutoPixmapStorage).

Done.

https://codereview.chromium.org/1813483002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.h
(right):

https://codereview.chromium.org/1813483002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.h:32: SkPixmap
m_pixelStorage;
On 2016/03/16 19:28:29, f(malita) wrote:
> Let's rename this to m_pixmap.

It is actually back to being more of a pure pixel storage object now.

[f(malita), 2016-03-16 20:12:36]

lgtm

https://codereview.chromium.org/1813483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp
(right):

https://codereview.chromium.org/1813483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp:52: }
nit: no braces for single-line/statement conditional (Blink style).

https://codereview.chromium.org/1813483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp:59:
m_pixelStorage.free();
ImagePixelLockers are locally scoped, so freeing upfront doesn't make a big
difference.  But it certainly looks more rigorous - I'll leave it up to you if
you want to keep it or not :)

[robertphillips, 2016-03-16 20:26:01]

Description was changed from

==========
ImagePixelLocker now manually allocates SkPixmap

Skia is retracting the SkAutoPixmapStorage class. This is the
only use of it in Chrome.
==========

to

==========
ImagePixelLocker now manually allocates SkPixmap

Skia is retracting the SkAutoPixmapStorage class. These are the only two uses of
it in Chrome.
==========

[robertphillips, 2016-03-16 20:30:50]

PTAL - I found a second instance.

https://codereview.chromium.org/1813483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp
(right):

https://codereview.chromium.org/1813483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp:59:
m_pixelStorage.free();
On 2016/03/16 20:12:36, f(malita) wrote:
> ImagePixelLockers are locally scoped, so freeing upfront doesn't make a big
> difference.  But it certainly looks more rigorous - I'll leave it up to you if
> you want to keep it or not :)

Done - removed.

[robertphillips, 2016-03-16 20:31:50]

https://codereview.chromium.org/1813483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp
(right):

https://codereview.chromium.org/1813483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/ImagePixelLocker.cpp:52: }
On 2016/03/16 20:12:36, f(malita) wrote:
> nit: no braces for single-line/statement conditional (Blink style).

Done.

[f(malita), 2016-03-16 20:33:13]

lgtm++

[robertphillips, 2016-03-16 20:34:28]

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

[commit-bot: I haz the power, 2016-03-16 20:35:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813483002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813483002/80001

[commit-bot: I haz the power, 2016-03-16 22:53:19]

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

[commit-bot: I haz the power, 2016-03-16 22:53:20]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-03-17 13:18:02]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2016-03-17 13:18:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813483002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813483002/80001

[commit-bot: I haz the power, 2016-03-17 13:22:23]

Description was changed from

==========
ImagePixelLocker now manually allocates SkPixmap

Skia is retracting the SkAutoPixmapStorage class. These are the only two uses of
it in Chrome.
==========

to

==========
ImagePixelLocker now manually allocates SkPixmap

Skia is retracting the SkAutoPixmapStorage class. These are the only two uses of
it in Chrome.
==========

[commit-bot: I haz the power, 2016-03-17 13:22:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-17 13:24:56]

Description was changed from

==========
ImagePixelLocker now manually allocates SkPixmap

Skia is retracting the SkAutoPixmapStorage class. These are the only two uses of
it in Chrome.
==========

to

==========
ImagePixelLocker now manually allocates SkPixmap

Skia is retracting the SkAutoPixmapStorage class. These are the only two uses of
it in Chrome.

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

[commit-bot: I haz the power, 2016-03-17 13:24:57]

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