Fix the errors of ui unittests caused by packing colors for Android

Fix the errors of ui unittests caused by packing colors  for Android

Get the platform-independent result and compare them with the
values of SkColor which is the platform-independent ARGB format

BUG=138933
TEST=run_tests.py -s ui_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150772

[yongsheng, 2012-07-25 14:47:48]

on Android, it packs colors as RGBA while other platforms uses ARGB. so use the
macros SK_A32_SHIFT, etc to generate corresponding colors.

Use the inline function SetPackedColor in these 3 files. seems a little bit
duplicated. If you think it could be put in a common file, i'm glad to do it(not
sure what directory the file should be put into).

Owners, please help review it. Thanks.

[Robert Sesek, 2012-07-25 15:36:53]

http://codereview.chromium.org/10823012/diff/1/ui/gfx/image/image_unittest.cc
File ui/gfx/image/image_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/1/ui/gfx/image/image_unittest.cc...
ui/gfx/image/image_unittest.cc:201: #define ColorRed SetPackedColor(0xFF, 0xFF,
0x00, 0x00)
Since you only use this once, is there a point to defining this as a macro? If
not, please inline it at your callsite, or turn it into a function.

[Ben Goodger (Google), 2012-07-25 15:51:33]

Do you really need 5 people to review this change?

[yongsheng, 2012-07-26 01:21:48]

On 2012/07/25 15:51:33, Ben Goodger (Google) wrote:
> Do you really need 5 people to review this change?
I think no. I add 5 people for I don't know whether some of you are on vacation
thus there will be no response. thanks a lot.

[yongsheng, 2012-07-26 01:24:18]

what about other files? anyone who can help review them?

http://codereview.chromium.org/10823012/diff/1/ui/gfx/image/image_unittest.cc
File ui/gfx/image/image_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/1/ui/gfx/image/image_unittest.cc...
ui/gfx/image/image_unittest.cc:201: #define ColorRed SetPackedColor(0xFF, 0xFF,
0x00, 0x00)
On 2012/07/25 15:36:53, rsesek wrote:
> Since you only use this once, is there a point to defining this as a macro? If
> not, please inline it at your callsite, or turn it into a function.
ok, got it. thanks.

[yongsheng, 2012-07-26 02:34:32]

michaelbai, since you're the owner of changing the behaviors of skia for
android, any comments for this? thanks.

[michaelbai, 2012-07-26 16:48:29]

Thanks for handing this, I just give a quick look, the obvious issue seemed that
you shouldn't define SetPackedColor in each unittests files.

Could you find a place to put it?

I also added the owners of ui and webkit directory in the reviewers

[Ami GONE FROM CHROMIUM, 2012-07-26 17:20:03]

http://codereview.chromium.org/10823012/diff/9/webkit/media/skcanvas_video_re...
File webkit/media/skcanvas_video_renderer_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/9/webkit/media/skcanvas_video_re...
webkit/media/skcanvas_video_renderer_unittest.cc:137: EXPECT_EQ(ColorRed,
GetColor(fast_path_canvas()));
I don't know skia very much at all, but skimming
third_party/skia/include/core/SkColor.h this change seems wrong.  SkColor is
meant to be platform-configuration-independent (as opposed to the
platform-specific SkPMColor).
In fact SkBitmap::getColor() un-pre-multiplies its return value from SkPMColor
to SkColor.

Does this mean this change should be reverted and actually a skia bug fixed?

[yongsheng, 2012-07-27 00:05:34]

On 2012/07/26 16:48:29, michaelbai wrote:
> Thanks for handing this, I just give a quick look, the obvious issue seemed
that
> you shouldn't define SetPackedColor in each unittests files.
> 
> Could you find a place to put it?
> 
> I also added the owners of ui and webkit directory in the reviewers
yes, you're right. I also mention this previously. But i'm not sure which
directory is suitable to put the common file.

[yongsheng, 2012-07-27 00:10:14]

On 2012/07/26 17:20:03, Ami Fischman wrote:
>
http://codereview.chromium.org/10823012/diff/9/webkit/media/skcanvas_video_re...
> File webkit/media/skcanvas_video_renderer_unittest.cc (right):
> 
>
http://codereview.chromium.org/10823012/diff/9/webkit/media/skcanvas_video_re...
> webkit/media/skcanvas_video_renderer_unittest.cc:137: EXPECT_EQ(ColorRed,
> GetColor(fast_path_canvas()));
> I don't know skia very much at all, but skimming
> third_party/skia/include/core/SkColor.h this change seems wrong.  SkColor is
> meant to be platform-configuration-independent (as opposed to the
> platform-specific SkPMColor).
If you look through SkColor.h and you'll find that the color defined in this
file is packed with specific format ARGB(platform-independent). However, in the
unit tests, they get the raw buffer from bitmap which is obviously not platform
independent. For Android, it's ABGR.
> In fact SkBitmap::getColor() un-pre-multiplies its return value from SkPMColor
> to SkColor.
> 
> Does this mean this change should be reverted and actually a skia bug fixed?
All, anyone who can help a skia expert for this issue? thanks a lot!

thanks for your review.

[Ami GONE FROM CHROMIUM, 2012-07-27 15:35:16]

http://codereview.chromium.**org/10823012/diff/9/webkit/**
media/skcanvas_video_renderer_**unittest.cc#newcode137<http://codereview.chromium.org/10823012/diff/9/webkit/media/skcanvas_video_renderer_unittest.cc#newcode137>

>  webkit/media/skcanvas_video_**renderer_unittest.cc:137:
>> EXPECT_EQ(ColorRed,
>> GetColor(fast_path_canvas()));
>> I don't know skia very much at all, but skimming
>> third_party/skia/include/core/**SkColor.h this change seems wrong.
>>  SkColor is
>> meant to be platform-configuration-**independent (as opposed to the
>> platform-specific SkPMColor).
>>
> If you look through SkColor.h and you'll find that the color defined in
> this
> file is packed with specific format ARGB(platform-independent). However,
> in the
> unit tests, they get the raw buffer from bitmap which is obviously not
> platform
> independent. For Android, it's ABGR.


I don't see that.  The unittest is calling SkBitmap::getColor(), which
returns an SkColor, which is documented to be platform-independent.  So why
do you need to have platform-dependent code in setting expectations on it?

>
>  In fact SkBitmap::getColor() un-pre-multiplies its return value from
>> SkPMColor
>> to SkColor.
>>
>
>  Does this mean this change should be reverted and actually a skia bug
>> fixed?
>>
> All, anyone who can help a skia expert for this issue? thanks a lot!
>

http://code.google.com/p/skia/

[yongsheng, 2012-07-29 01:10:42]

On 2012/07/27 15:35:16, Ami Fischman wrote:
> http://codereview.chromium.**org/10823012/diff/9/webkit/**
>
media/skcanvas_video_renderer_**unittest.cc#newcode137<http://codereview.chromium.org/10823012/diff/9/webkit/media/skcanvas_video_renderer_unittest.cc#newcode137>
> 
> >  webkit/media/skcanvas_video_**renderer_unittest.cc:137:
> >> EXPECT_EQ(ColorRed,
> >> GetColor(fast_path_canvas()));
> >> I don't know skia very much at all, but skimming
> >> third_party/skia/include/core/**SkColor.h this change seems wrong.
> >>  SkColor is
> >> meant to be platform-configuration-**independent (as opposed to the
> >> platform-specific SkPMColor).
> >>
> > If you look through SkColor.h and you'll find that the color defined in
> > this
> > file is packed with specific format ARGB(platform-independent). However,
> > in the
> > unit tests, they get the raw buffer from bitmap which is obviously not
> > platform
> > independent. For Android, it's ABGR.
> 
> 
> I don't see that.  The unittest is calling SkBitmap::getColor(), which
> returns an SkColor, which is documented to be platform-independent.  So why
> do you need to have platform-dependent code in setting expectations on it?
See here:
SkAutoLockPixels auto_lock(*bitmap);
   uint32_t* pixel = bitmap->getAddr32(10, 10);
   EXPECT_EQ(SK_ColorRED, *pixel);
It gets the raw buffer to compare. 
I'll continue to investigate others.
> >
> >  In fact SkBitmap::getColor() un-pre-multiplies its return value from
> >> SkPMColor
> >> to SkColor.
> >>
> >
> >  Does this mean this change should be reverted and actually a skia bug
> >> fixed?
> >>
> > All, anyone who can help a skia expert for this issue? thanks a lot!
> >
> 
> http://code.google.com/p/skia/

[Robert Sesek, 2012-07-31 00:11:00]

ui/gfx/image LGTM

[sky, 2012-08-02 21:02:00]

http://codereview.chromium.org/10823012/diff/9/ui/gfx/image/image_unittest.cc
File ui/gfx/image/image_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/9/ui/gfx/image/image_unittest.cc...
ui/gfx/image/image_unittest.cc:197: inline uint32_t SetPackedColor(uint8_t a,
uint8_t r, uint8_t g, uint8_t b) {
There are functions in skia for creating a color from a specific argb. Can you
use it?

http://codereview.chromium.org/10823012/diff/9/ui/gfx/skbitmap_operations_uni...
File ui/gfx/skbitmap_operations_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/9/ui/gfx/skbitmap_operations_uni...
ui/gfx/skbitmap_operations_unittest.cc:23: inline uint32_t
SetPackedColor(uint8_t a, uint8_t r, uint8_t g, uint8_t b) {
Same comment as other file.

[yongsheng, 2012-08-06 06:37:21]

really thanks for all of your comments. 
I rethink about this and refine this patch according your comments and reed's
comments in http://codereview.chromium.org/10831031.

So i'll fix issues in ui_unittests and will create a new issue for
skcanvas_video_renderer. It's deeper there.

http://codereview.chromium.org/10823012/diff/9/webkit/media/skcanvas_video_re...
File webkit/media/skcanvas_video_renderer_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/9/webkit/media/skcanvas_video_re...
webkit/media/skcanvas_video_renderer_unittest.cc:137: EXPECT_EQ(ColorRed,
GetColor(fast_path_canvas()));
On 2012/07/26 17:20:03, Ami Fischman wrote:
> I don't know skia very much at all, but skimming
> third_party/skia/include/core/SkColor.h this change seems wrong.  SkColor is
> meant to be platform-configuration-independent (as opposed to the
> platform-specific SkPMColor).
> In fact SkBitmap::getColor() un-pre-multiplies its return value from SkPMColor
> to SkColor.
> 
> Does this mean this change should be reverted and actually a skia bug fixed?
yes, i think you're right after I read the code in skia.
SkBitmap::getColor() should return platform-independent value. 

These failures are due to the conversion between YUV and argb32 has the
assumption: the target bitmap internal format
is argb32 while it's not true for chrome for Android. So it's a bug in
webkit/media/skcanvas_video_renderer.cc. I think it's better to split this bug
with current other failures: this issue is more complex. I'll create a new issue
for this issue.

[yongsheng, 2012-08-06 06:55:42]

> These failures are due to the conversion between YUV and argb32 has the
> assumption: the target bitmap internal format
> is argb32 while it's not true for chrome for Android. So it's a bug in
> webkit/media/skcanvas_video_renderer.cc. I think it's better to split this bug
> with current other failures: this issue is more complex. I'll create a new
issue
> for this issue.
To make it easier to be landed, this issue fixes 3 failures in ui_unittests.
Please help review it.

[reed1, 2012-08-06 12:42:43]

http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
File ui/gfx/skbitmap_operations_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
ui/gfx/skbitmap_operations_unittest.cc:480: // Set PMColors into the bitmap
The last value is not a valid PMColor, for its alpha is < green and blue! What
color are you trying to represent?

[yongsheng, 2012-08-07 01:18:40]

rsesek, since you're the owner, do you know the reason?

http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
File ui/gfx/skbitmap_operations_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
ui/gfx/skbitmap_operations_unittest.cc:480: // Set PMColors into the bitmap
On 2012/08/06 12:42:43, reed1 wrote:
> The last value is not a valid PMColor, for its alpha is < green and blue! What
> color are you trying to represent?
yes, so use  SkPackARGB32NoCheck here.
I'm not the owner of this case but i guess it's for testing purpose?

[reed1, 2012-08-07 12:55:26]

On 2012/08/07 01:18:40, yongsheng wrote:
> rsesek, since you're the owner, do you know the reason?
> 
>
http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
> File ui/gfx/skbitmap_operations_unittest.cc (right):
> 
>
http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
> ui/gfx/skbitmap_operations_unittest.cc:480: // Set PMColors into the bitmap
> On 2012/08/06 12:42:43, reed1 wrote:
> > The last value is not a valid PMColor, for its alpha is < green and blue!
What
> > color are you trying to represent?
> yes, so use  SkPackARGB32NoCheck here.
> I'm not the owner of this case but i guess it's for testing purpose?

Lets not commit until we understand the purpose. From my view, this is a bug.

[Robert Sesek, 2012-08-07 13:47:20]

On 2012/08/07 12:55:26, reed1 wrote:
> On 2012/08/07 01:18:40, yongsheng wrote:
> > rsesek, since you're the owner, do you know the reason?
> > 
> >
>
http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
> > File ui/gfx/skbitmap_operations_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
> > ui/gfx/skbitmap_operations_unittest.cc:480: // Set PMColors into the bitmap
> > On 2012/08/06 12:42:43, reed1 wrote:
> > > The last value is not a valid PMColor, for its alpha is < green and blue!
> What
> > > color are you trying to represent?
> > yes, so use  SkPackARGB32NoCheck here.
> > I'm not the owner of this case but i guess it's for testing purpose?
> 
> Lets not commit until we understand the purpose. From my view, this is a bug.

This file was moved into the directory, so I don't know. git blame says the test
was added here: http://codereview.chromium.org/2963009

[tony, 2012-08-07 23:33:43]

+nico, the author of the test.

[Nico, 2012-08-07 23:39:40]

lgtm

http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
File ui/gfx/skbitmap_operations_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
ui/gfx/skbitmap_operations_unittest.cc:480: // Set PMColors into the bitmap
On 2012/08/07 01:18:40, yongsheng wrote:
> On 2012/08/06 12:42:43, reed1 wrote:
> > The last value is not a valid PMColor, for its alpha is < green and blue!
What
> > color are you trying to represent?
> yes, so use  SkPackARGB32NoCheck here.
> I'm not the owner of this case but i guess it's for testing purpose?

Right, I think it's just there to check that boundary cases don't blow up but
are handled somewhat sensibly.

[yongsheng, 2012-08-08 00:20:35]

any other concerns?

http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
File ui/gfx/skbitmap_operations_unittest.cc (right):

http://codereview.chromium.org/10823012/diff/14002/ui/gfx/skbitmap_operations...
ui/gfx/skbitmap_operations_unittest.cc:480: // Set PMColors into the bitmap
On 2012/08/07 23:39:40, Nico wrote:
> On 2012/08/07 01:18:40, yongsheng wrote:
> > On 2012/08/06 12:42:43, reed1 wrote:
> > > The last value is not a valid PMColor, for its alpha is < green and blue!
> What
> > > color are you trying to represent?
> > yes, so use  SkPackARGB32NoCheck here.
> > I'm not the owner of this case but i guess it's for testing purpose?
> 
> Right, I think it's just there to check that boundary cases don't blow up but
> are handled somewhat sensibly. 
great, thank you, Nico.

[sky, 2012-08-08 04:15:43]

LGTM

[yongsheng, 2012-08-08 08:37:43]

On 2012/08/08 04:15:43, sky wrote:
> LGTM
thanks, I'll wait for more comments. If no, i'll commit it tomorrow.

[yongsheng, 2012-08-09 08:18:48]

On 2012/08/08 08:37:43, yongsheng wrote:
> On 2012/08/08 04:15:43, sky wrote:
> > LGTM
> thanks, I'll wait for more comments. If no, i'll commit it tomorrow.
the failure is from unit_tests, not by this fix.

[commit-bot: I haz the power, 2012-08-09 08:19:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10823012/3004

[commit-bot: I haz the power, 2012-08-09 09:54:40]

Change committed as 150772