make allocPixels throw on failure

make allocPixels throw on failure

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/848250415eddc54075f7eb8795e8db79e749c6ab

[reed1, 2014-08-29 17:52:48]

reed@google.com changed reviewers:
+ fmalita@google.com, mtklein@google.com

[reed1, 2014-08-29 17:52:49]

WIP

[mtklein, 2014-08-29 17:59:49]

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

https://codereview.chromium.org/510423005/diff/1/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:229: bool allocPixelsCheck(const SkImageInfo&,
SkPixelRefFactory*, SkColorTable*);
tryAllocPixels?  allocPixelsCheck sounds a little too much like CHECK to me,
which made me parse the name with the wrong "alloc pixels or crash" meaning.

[f(malita), 2014-08-29 18:21:14]

fmalita@chromium.org changed reviewers:
+ fmalita@chromium.org

[f(malita), 2014-08-29 18:21:15]

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

https://codereview.chromium.org/510423005/diff/1/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:260: *  On failure, the bitmap will be set to empty and
return false.
Should also update the docs at some point.

[reed1, 2014-09-02 12:23:23]

On 2014/08/29 17:59:49, mtklein wrote:
> https://codereview.chromium.org/510423005/diff/1/include/core/SkBitmap.h
> File include/core/SkBitmap.h (right):
> 
>
https://codereview.chromium.org/510423005/diff/1/include/core/SkBitmap.h#newc...
> include/core/SkBitmap.h:229: bool allocPixelsCheck(const SkImageInfo&,
> SkPixelRefFactory*, SkColorTable*);
> tryAllocPixels?  allocPixelsCheck sounds a little too much like <span
id="abbrex-spnAbbr_7926" class="abbrex-Abbr abbrex-Abbr_CHECK abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">CHECK</span> to me,
> which made me parse the name with the wrong "alloc pixels or crash" meaning.

Oops, just read this comment about "Check". Will consider other schemes.

[reed1, 2014-09-02 13:43:04]

ptal

[reed1, 2014-09-02 14:22:27]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-02 14:23:04]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/510423005/80001

[reed1, 2014-09-02 14:23:05]

I *think* the canary failure is just that that bot did not re-run runhooks, to
pick up the change in the gypi.

[commit-bot: I haz the power, 2014-09-02 14:23:05]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-02 20:23 UTC

[commit-bot: I haz the power, 2014-09-02 14:28:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[commit-bot: I haz the power, 2014-09-02 14:29:56]

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

[commit-bot: I haz the power, 2014-09-02 14:29:57]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[reed1, 2014-09-02 15:29:12]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-02 15:29:54]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/510423005/90001

[commit-bot: I haz the power, 2014-09-02 15:29:55]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-02 21:29 UTC

[commit-bot: I haz the power, 2014-09-02 15:36:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[commit-bot: I haz the power, 2014-09-02 15:40:08]

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

[commit-bot: I haz the power, 2014-09-02 15:40:10]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[reed1, 2014-09-02 16:10:12]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-02 16:10:28]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/510423005/130001

[commit-bot: I haz the power, 2014-09-02 16:10:29]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-02 22:10 UTC

[commit-bot: I haz the power, 2014-09-02 16:18:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[commit-bot: I haz the power, 2014-09-02 16:21:53]

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

[commit-bot: I haz the power, 2014-09-02 16:21:54]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)

[reed1, 2014-09-02 16:57:48]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-02 16:58:33]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/510423005/150001

[commit-bot: I haz the power, 2014-09-02 16:58:34]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-02 22:58 UTC

[reed1, 2014-09-02 17:46:48]

PTAL

[mtklein, 2014-09-02 18:50:26]

Some nice cleanup across our tests here.

https://codereview.chromium.org/510423005/diff/150001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/510423005/diff/150001/include/core/SkBitmap.h...
include/core/SkBitmap.h:231: bool tryAllocPixels(const SkImageInfo&,
SkPixelRefFactory*, SkColorTable*);
Can't SK_WARN_UNUSED_RESULT here too?

https://codereview.chromium.org/510423005/diff/150001/tests/DrawBitmapRectTes...
File tests/DrawBitmapRectTest.cpp (right):

https://codereview.chromium.org/510423005/diff/150001/tests/DrawBitmapRectTes...
tests/DrawBitmapRectTest.cpp:193: if (bm.tryAllocN32Pixels(width, height)) {
This one's pretty surprising. The test's exploiting this behavior already?

[reed1, 2014-09-02 19:35:28]

https://codereview.chromium.org/510423005/diff/150001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/510423005/diff/150001/include/core/SkBitmap.h...
include/core/SkBitmap.h:231: bool tryAllocPixels(const SkImageInfo&,
SkPixelRefFactory*, SkColorTable*);
On 2014/09/02 18:50:26, mtklein wrote:
> Can't SK_WARN_UNUSED_RESULT here too?

Done.

https://codereview.chromium.org/510423005/diff/150001/tests/DrawBitmapRectTes...
File tests/DrawBitmapRectTest.cpp (right):

https://codereview.chromium.org/510423005/diff/150001/tests/DrawBitmapRectTes...
tests/DrawBitmapRectTest.cpp:193: if (bm.tryAllocN32Pixels(width, height)) {
On 2014/09/02 18:50:26, mtklein wrote:
> This one's pretty surprising. The test's exploiting this behavior already?

The test explicitly passes in 0xFFFF x 0xFFFF for the dimensions, which it
expects will fail (not enough ram)

[reed1, 2014-09-02 19:36:47]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-02 19:36:49]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/510423005/170001

[commit-bot: I haz the power, 2014-09-02 19:36:50]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-03 01:36 UTC

[commit-bot: I haz the power, 2014-09-02 19:37:01]

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

[commit-bot: I haz the power, 2014-09-02 19:37:02]

Failed to apply patch for tests/ScaledImageCache.cpp:
While running git apply --index -p1;
  error: tests/ScaledImageCache.cpp: does not exist in index

Patch:       tests/ScaledImageCache.cpp
Index: tests/ScaledImageCache.cpp
diff --git a/tests/ScaledImageCache.cpp b/tests/ScaledImageCache.cpp
index
b71c7443e57ad6ad7c26a7f72c8078415a9cd9c4..454f8c20423e106e3be7c5482bf54bef7c54f4d7
100644
--- a/tests/ScaledImageCache.cpp
+++ b/tests/ScaledImageCache.cpp
@@ -27,7 +27,7 @@ static bool test_scaled_image_cache_useage() {
     SkAutoTUnref<SkCanvas> canvas(
             SkCanvas::NewRasterN32(kCanvasSize, kCanvasSize));
     SkBitmap bitmap;
-    SkAssertResult(bitmap.allocN32Pixels(kBitmapSize, kBitmapSize));
+    bitmap.allocN32Pixels(kBitmapSize, kBitmapSize);
     bitmap.eraseColor(0xFFFFFFFF);
     SkScalar scale = SkIntToScalar(kScale);
     SkScalar scaledSize = SkIntToScalar(kBitmapSize) * scale;

[mtklein, 2014-09-02 19:38:13]

lgtm

[reed1, 2014-09-02 19:42:14]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-02 19:43:04]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/510423005/190001

[commit-bot: I haz the power, 2014-09-02 19:50:51]

Committed patchset #11 (id:190001) as 848250415eddc54075f7eb8795e8db79e749c6ab