Remove use of SkBitmap::Config (deprecated) from Source/web/tests/.

Remove use of SkBitmap::Config (deprecated) from Source/web/tests/.

BUG=

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

[jbroman, 2014-03-20 19:44:27]

Let me know if this looks good (and uses the best/most current Skia APIs).

In particular:
- I haven't yet checked the result of allocN32Pixels in some of the TestImage
helpers. I can't directly ASSERT_TRUE from here, so checking it would involve
some plumbing which I'm not sure is worth it (because the tests will fail anyway
if there are no pixels).
- In GraphicsContextTest.RecordingTotalMatrix, I've used the SkCanvas
two-argument constructor to set the unknown color type, which is what kNo_Config
does. Is this the best way to do this?
- Passing an SkAlphaType when building a bitmap canvas seems to require building
an SkImageInfo, so it looks like passing the isOpaque boolean argument is
easier/preferred.

After this, I think the only remaining use of SkBitmap::Config in Blink is in
WebViewImpl (but I'm not quite sure what the right way to respond to allocPixels
returning false is in that case).

If this LGTY, I'll pass this along to a Source/web/ owner afterwards.
(Incidentally, shouldn't some of these tests be moved to platform/? Is there a
reason they have to live over here?)

[Stephen Chennney, 2014-03-20 20:29:08]

On 2014/03/20 19:44:27, jbroman wrote:
> Let me know if this looks good (and uses the best/most current Skia APIs).
> 
> In particular:
> - I haven't yet checked the result of allocN32Pixels in some of the TestImage
> helpers. I can't directly ASSERT_TRUE from here, so checking it would involve
> some plumbing which I'm not sure is worth it (because the tests will fail
anyway
> if there are no pixels).
> - In GraphicsContextTest.RecordingTotalMatrix, I've used the SkCanvas
> two-argument constructor to set the unknown color type, which is what
kNo_Config
> does. Is this the best way to do this?
> - Passing an SkAlphaType when building a bitmap canvas seems to require
building
> an SkImageInfo, so it looks like passing the isOpaque boolean argument is
> easier/preferred.
> 
> After this, I think the only remaining use of SkBitmap::Config in Blink is in
> WebViewImpl (but I'm not quite sure what the right way to respond to
allocPixels
> returning false is in that case).
> 
> If this LGTY, I'll pass this along to a Source/web/ owner afterwards.
> (Incidentally, shouldn't some of these tests be moved to platform/? Is there a
> reason they have to live over here?)

It's fine to ignore failures in tests. As you say, the test will fail somehow so
we'll catch the problem.

Regarding tests in web that could be in platform, there was a patch moving some
of these but I think it's landed, so the one's you're seeing are available to
move. The tests were originally in web because they need test infrastructure
that is only available there. However, there is now a target in one of the
platform.gypi files that includes platform tests that must run as web unit
tests, so these files can be moved to the appropriate platform location and
added to that list of things that run with web tests.

I would do the move in a follow up patch. Don't be surprised if someone else
jumps in before you - I think an external contributor is grabbing these simple
moves.

Your change is LGTM from me. Mike should sign off too.

[reed1, 2014-03-21 15:18:40]

lgtm w/ suggestions about ASSERT_TRUE

https://codereview.chromium.org/204353005/diff/20001/Source/web/tests/DragIma...
File Source/web/tests/DragImageTest.cpp (right):

https://codereview.chromium.org/204353005/diff/20001/Source/web/tests/DragIma...
Source/web/tests/DragImageTest.cpp:67:
m_nativeImage->bitmap().allocN32Pixels(size.width(), size.height());
ASSERT_TRUE() ?

https://codereview.chromium.org/204353005/diff/20001/Source/web/tests/ImageLa...
File Source/web/tests/ImageLayerChromiumTest.cpp (right):

https://codereview.chromium.org/204353005/diff/20001/Source/web/tests/ImageLa...
Source/web/tests/ImageLayerChromiumTest.cpp:58:
m_nativeImage->bitmap().allocN32Pixels(size.width(), size.height(), isOpaque);
ASSERT_TRUE() ?

[jbroman, 2014-03-21 15:39:33]

Unfortunately, ASSERT_TRUE uses "return", and isn't legal in this context. Best
I can do without extra plumbing is EXPECT_TRUE.

https://codereview.chromium.org/204353005/diff/20001/Source/web/tests/DragIma...
File Source/web/tests/DragImageTest.cpp (right):

https://codereview.chromium.org/204353005/diff/20001/Source/web/tests/DragIma...
Source/web/tests/DragImageTest.cpp:67:
m_nativeImage->bitmap().allocN32Pixels(size.width(), size.height());
On 2014/03/21 15:18:40, reed1 wrote:
> ASSERT_TRUE() ?

Unfortunately, this isn't directly in the test or its SetUp/TearDown.
ASSERT_TRUE works by emitting a message (by accessing a singleton) and then
returning. (But this is in a constructor.)

I look a  little more closely at exactly how gtest handles ASSERT_TRUE and
EXPECT_TRUE. Apparently it *is* okay to put a non-fatal EXPECT_TRUE here, so I'm
adding that (even though it will still result in more errors later in the test,
this might help in debugging), but I don't think it's worth the complexity of
plumbing the boolean back to the test to ASSERT_TRUE.

https://codereview.chromium.org/204353005/diff/20001/Source/web/tests/ImageLa...
File Source/web/tests/ImageLayerChromiumTest.cpp (right):

https://codereview.chromium.org/204353005/diff/20001/Source/web/tests/ImageLa...
Source/web/tests/ImageLayerChromiumTest.cpp:58:
m_nativeImage->bitmap().allocN32Pixels(size.width(), size.height(), isOpaque);
On 2014/03/21 15:18:40, reed1 wrote:
> ASSERT_TRUE() ?

Ditto.

[jbroman, 2014-03-21 15:41:02]

+abarth for Source/web/ OWNERS.

[abarth-chromium, 2014-03-21 15:51:49]

lgtm

[jbroman, 2014-03-21 15:53:13]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-03-21 15:53:17]

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

[commit-bot: I haz the power, 2014-03-21 16:48:28]

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

[commit-bot: I haz the power, 2014-03-21 16:48:29]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jbroman, 2014-03-21 16:48:51]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-03-21 16:48:54]

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

[commit-bot: I haz the power, 2014-03-21 16:52:11]

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

[commit-bot: I haz the power, 2014-03-21 16:52:11]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jbroman, 2014-03-21 16:54:08]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-03-21 16:54:17]

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

[commit-bot: I haz the power, 2014-03-21 18:03:33]

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

[commit-bot: I haz the power, 2014-03-21 18:03:33]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jbroman, 2014-03-21 20:32:16]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-03-21 20:32:22]

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

[commit-bot: I haz the power, 2014-03-21 20:35:12]

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

[commit-bot: I haz the power, 2014-03-21 20:35:13]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jbroman, 2014-03-21 20:35:47]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2014-03-21 20:35:59]

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

[commit-bot: I haz the power, 2014-03-22 04:16:42]

Change committed as 169778