Add support for PNG representation in gfx::Image

Add support for PNG representation in gfx::Image

Currently all conversions to or from PNG will request (and so possibly
create and cache) an ImageSkia intermediate representation.

Depends on http://codereview.chromium.org/10799014/

BUG=123865
TEST=

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

[cjhopman, 2012-07-19 21:52:19]

This change doesn't touch any of the users of gfx::Image, specifically, callers
still use ImageFromPNGEncodedData() rather than creating the Image directly. To
keep the same behavior, that function attempts a conversion to ImageSkia and
returns NULL on failure. As a followup to this CL I will look at changing all
users to create an Image directly and removing the two PNG-related functions
from image_util.h.

Also, I will look at using the conversions in resource_bundle so that the
intermediate ImageSkias are unnecessary.

[cjhopman, 2012-07-19 21:53:27]

sky: ui/
rsesek: ui/gfx/image/

[Robert Sesek, 2012-07-19 23:45:07]

http://codereview.chromium.org/10799014/diff/8006/ui/gfx/image/image_png.h
File ui/gfx/image/image_png.h (right):

http://codereview.chromium.org/10799014/diff/8006/ui/gfx/image/image_png.h#ne...
ui/gfx/image/image_png.h:18: class UI_EXPORT ImagePNG {
Why do we need ImagePNG? I think everything can be on gfx::Image with an
internal rep for the PNG bytes. Just have a constructor for Image(const unsigned
char* input, size_t input) and internally store that as a vector of the data.
The Copy/To methods can be implemented with the vector of bytes.

[cjhopman, 2012-07-20 17:28:24]

http://codereview.chromium.org/10799014/diff/8006/ui/gfx/image/image_png.h
File ui/gfx/image/image_png.h (right):

http://codereview.chromium.org/10799014/diff/8006/ui/gfx/image/image_png.h#ne...
ui/gfx/image/image_png.h:18: class UI_EXPORT ImagePNG {
On 2012/07/19 23:45:07, rsesek wrote:
> Why do we need ImagePNG? I think everything can be on gfx::Image with an
> internal rep for the PNG bytes. Just have a constructor for Image(const
unsigned
> char* input, size_t input) and internally store that as a vector of the data.
> The Copy/To methods can be implemented with the vector of bytes.

Done. Yeah, that works for me, we don't need ImagePNG.

[Robert Sesek, 2012-07-20 21:03:00]

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:108: explicit ImageRepPNG(const unsigned char* input,
size_t input_size)
Don't need to mark this constructor as explicit.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:328: // through to that case
nit: full-stop

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:338: GetRepresentation(kImageRepSkia,
true)->AsImageRepSkia();
nit: continuations should be intended 4

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:362: GetRepresentation(kImageRepPNG,
true)->AsImageRepPNG();
nit: indent correctly

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:364: internal::ImageSkiaFromPNG(png_rep->image())));
You only call this once. Why not have this function return a pointer so that you
don't need to invoke the copy constructor?

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:400: ToImageSkia();
The comment at ToNSImage() applies here, too.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:437: ToImageSkia();
This is a double-conversion. Instead you could use the platform PNG decoder to
return an NSImage with a single conversion.

The ResourceBundle already has the logic for loading raw PNG data into the
native platform type. I'm not sure how much sense it makes to unify that logic,
but I think these double conversions are unfortunate.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h#newcode39
ui/gfx/image/image.h:39: class ImagePNG;
Remove

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h#newcode70
ui/gfx/image/image.h:70: explicit Image(const std::vector<unsigned char>& png);
I wonder if you even need this constructor. A client that has a vector can
always use .front() and .size() to the constructor above.

What do you think?

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h#newcod...
ui/gfx/image/image.h:108: const std::vector<unsigned char>* ToImagePNG() const;
I'm wondering if this should return a const ref. Reasons against are that it's a
difference in the API from all the other To*() functions, and it makes it easier
to perform a vector copy.

[cjhopman, 2012-07-24 01:44:12]

I've never touched Objective-C, so feel free to tear that code apart. I think it
is correct (and the new tests should verify at least the normal case), but
particularly style, memory management, and error handling I am less confident
about.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:108: explicit ImageRepPNG(const unsigned char* input,
size_t input_size)
On 2012/07/20 21:03:01, rsesek wrote:
> Don't need to mark this constructor as explicit.

Done.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:328: // through to that case
On 2012/07/20 21:03:01, rsesek wrote:
> nit: full-stop

Done.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:338: GetRepresentation(kImageRepSkia,
true)->AsImageRepSkia();
On 2012/07/20 21:03:01, rsesek wrote:
> nit: continuations should be intended 4

Done.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:362: GetRepresentation(kImageRepPNG,
true)->AsImageRepPNG();
On 2012/07/20 21:03:01, rsesek wrote:
> nit: indent correctly

Done.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:364: internal::ImageSkiaFromPNG(png_rep->image())));
On 2012/07/20 21:03:01, rsesek wrote:
> You only call this once. Why not have this function return a pointer so that
you
> don't need to invoke the copy constructor?

Done.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:400: ToImageSkia();
On 2012/07/20 21:03:01, rsesek wrote:
> The comment at ToNSImage() applies here, too.

Done.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:437: ToImageSkia();
On 2012/07/20 21:03:01, rsesek wrote:
> This is a double-conversion. Instead you could use the platform PNG decoder to
> return an NSImage with a single conversion.
> 
> The ResourceBundle already has the logic for loading raw PNG data into the
> native platform type. I'm not sure how much sense it makes to unify that
logic,
> but I think these double conversions are unfortunate.

Done. Also removed the intermediate conversions in the ToImagePNG() function.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h#newcode39
ui/gfx/image/image.h:39: class ImagePNG;
On 2012/07/20 21:03:01, rsesek wrote:
> Remove

Done.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h#newcode70
ui/gfx/image/image.h:70: explicit Image(const std::vector<unsigned char>& png);
On 2012/07/20 21:03:01, rsesek wrote:
> I wonder if you even need this constructor. A client that has a vector can
> always use .front() and .size() to the constructor above.
> 
> What do you think?

Agreed. Done.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h#newcod...
ui/gfx/image/image.h:108: const std::vector<unsigned char>* ToImagePNG() const;
On 2012/07/20 21:03:01, rsesek wrote:
> I'm wondering if this should return a const ref. Reasons against are that it's
a
> difference in the API from all the other To*() functions, and it makes it
easier
> to perform a vector copy.

I think my preference would be to keep it consistent with the other To*()
functions, but I'm not really opposed to changing it if you'd prefer it as a
reference.

[Robert Sesek, 2012-07-24 18:59:16]

Overall, I like the direction this is heading!

I wonder about the failure case for the conversions, though. I think returning
an empty bitmap may not be right. I wonder if they should return NULL on failure
and end with a crash, or if it should what ResourceBundle does and return an
empty red image to visibly show failure.

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/10799014/diff/5008/ui/gfx/image/image.h#newcod...
ui/gfx/image/image.h:108: const std::vector<unsigned char>* ToImagePNG() const;
On 2012/07/24 01:44:13, cjhopman wrote:
> On 2012/07/20 21:03:01, rsesek wrote:
> > I'm wondering if this should return a const ref. Reasons against are that
it's
> a
> > difference in the API from all the other To*() functions, and it makes it
> easier
> > to perform a vector copy.
> 
> I think my preference would be to keep it consistent with the other To*()
> functions, but I'm not really opposed to changing it if you'd prefer it as a
> reference.
> 

Agree. Let's keep these as pointers.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:53: // loader.  We must do this before calling
gdk_pixbuf_loader_get_pixbuf.
"We must do this" -> "This must be done"

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:73: pixbuf, &image, &image_size, "png", &error, NULL)) {
nit: continuations are indented 4 from this (

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:83: #endif
  // defined(TOOLKIT_GTK)

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:97: if
(!gfx::PNGCodec::EncodeBGRASkBitmap(*skia->bitmap(), false, png)) {
nit: over-indented

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:169: std::vector<unsigned char>& image() { return image_;
}
Generally non-const-ref is not used. Make this a pointer?

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.h#newcode66
ui/gfx/image/image.h:66: // Creates a new image by copying the png-encoded input
for use as the default
nit: PNG

Also add a note about how to create this with a vector of bytes.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm
File ui/gfx/image/image_mac.mm (right):

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm#n...
ui/gfx/image/image_mac.mm:16: CGImageRef cgimage = [nsimage
CGImageForProposedRect:NULL
These functions should only be indented 2 spaces. Also, the convention in an
ObjC method call is to align the ':' vertically.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm#n...
ui/gfx/image/image_mac.mm:16: CGImageRef cgimage = [nsimage
CGImageForProposedRect:NULL
Why do you need to go through CGImage? Can you not just use +[NSBitmapImageRep
representationOfImageRepsInArray:usingType:properties:]?

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm#n...
ui/gfx/image/image_mac.mm:19: scoped_nsobject<NSBitmapImageRep> nsbitmap(
naming: ns_bitmap and cg_image

[cjhopman, 2012-07-25 21:13:29]

I agree that just returning an empty image is not the best way to handle
failures.

I have changed the PNG decodes to return a 16x16 red image on failure. I don't
particularly like the idea of crashing on decode failures as we have little
control over the source of the PNG data and at least the PNG->Skia decode may
not handle all valid PNGs. I think generating the red image is the best way to
go for this.

As for PNG encoding, I think that failures should be treated as errors (and
crash) since we can expect that all platform/skia images are valid and that
those encoders work correctly.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:53: // loader.  We must do this before calling
gdk_pixbuf_loader_get_pixbuf.
On 2012/07/24 18:59:17, rsesek wrote:
> "We must do this" -> "This must be done"

Done.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:73: pixbuf, &image, &image_size, "png", &error, NULL)) {
On 2012/07/24 18:59:17, rsesek wrote:
> nit: continuations are indented 4 from this (

Done.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:83: #endif
On 2012/07/24 18:59:17, rsesek wrote:
>   // defined(TOOLKIT_GTK)

Done.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:88: #endif
// defined(OS_MACOSX) here? or unnecessary since it's close to the #if defined?

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:97: if
(!gfx::PNGCodec::EncodeBGRASkBitmap(*skia->bitmap(), false, png)) {
On 2012/07/24 18:59:17, rsesek wrote:
> nit: over-indented

Done.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:169: std::vector<unsigned char>& image() { return image_;
}
On 2012/07/24 18:59:17, rsesek wrote:
> Generally non-const-ref is not used. Make this a pointer?

Done.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.h#newcode66
ui/gfx/image/image.h:66: // Creates a new image by copying the png-encoded input
for use as the default
On 2012/07/24 18:59:17, rsesek wrote:
> nit: PNG
> 
> Also add a note about how to create this with a vector of bytes.

Done.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm
File ui/gfx/image/image_mac.mm (right):

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm#n...
ui/gfx/image/image_mac.mm:16: CGImageRef cgimage = [nsimage
CGImageForProposedRect:NULL
On 2012/07/24 18:59:17, rsesek wrote:
> These functions should only be indented 2 spaces. Also, the convention in an
> ObjC method call is to align the ':' vertically.

Done.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm#n...
ui/gfx/image/image_mac.mm:16: CGImageRef cgimage = [nsimage
CGImageForProposedRect:NULL
On 2012/07/24 18:59:17, rsesek wrote:
> Why do you need to go through CGImage? Can you not just use +[NSBitmapImageRep
> representationOfImageRepsInArray:usingType:properties:]?

I really don't know about this stuff, so here's some links where I got my
information:
http://cocoadev.com/wiki/NSImageToJPEG
http://cocoadev.com/wiki/NSBitmapImageRep

From these (and some cocoa-dev ml posts) it seems that
representationOfImageRepsInArray requires that the NSImage have an
NSBitmapImageRep which is not always the case. Going through a CGImage with
CGImageForProposedRect is suggested as a performant way of getting a
NSBitmapImageRep from an NSImage.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm#n...
ui/gfx/image/image_mac.mm:19: scoped_nsobject<NSBitmapImageRep> nsbitmap(
On 2012/07/24 18:59:17, rsesek wrote:
> naming: ns_bitmap and cg_image

Done.

[Robert Sesek, 2012-07-31 14:07:16]

This is almost there -- just a few more comments.

Also, feel free to ping me if I don't respond in more than 24 hours. I try and
keep code review latency low, but this slipped off my screen.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image.cc#newco...
ui/gfx/image/image.cc:88: #endif
On 2012/07/25 21:13:29, cjhopman wrote:
> // defined(OS_MACOSX) here? or unnecessary since it's close to the #if
defined?

Optional. I'd maybe do it just for consistency.

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm
File ui/gfx/image/image_mac.mm (right):

http://codereview.chromium.org/10799014/diff/6019/ui/gfx/image/image_mac.mm#n...
ui/gfx/image/image_mac.mm:16: CGImageRef cgimage = [nsimage
CGImageForProposedRect:NULL
On 2012/07/25 21:13:29, cjhopman wrote:
> On 2012/07/24 18:59:17, rsesek wrote:
> > Why do you need to go through CGImage? Can you not just use
+[NSBitmapImageRep
> > representationOfImageRepsInArray:usingType:properties:]?
> 
> I really don't know about this stuff, so here's some links where I got my
> information:
> http://cocoadev.com/wiki/NSImageToJPEG
> http://cocoadev.com/wiki/NSBitmapImageRep
> 
> From these (and some cocoa-dev ml posts) it seems that
> representationOfImageRepsInArray requires that the NSImage have an
> NSBitmapImageRep which is not always the case. Going through a CGImage with
> CGImageForProposedRect is suggested as a performant way of getting a
> NSBitmapImageRep from an NSImage.

Thanks for the links. This looks right, then.

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image.cc#newc...
ui/gfx/image/image.cc:62: pixbuf = gdk_pixbuf_new(GDK_COLORSPACE_RGB, TRUE, 8,
32, 32);
Does this leak the pixbuf because this is gdk_pixbuf_new() will have the
refcount = 1, and then you g_object_ref() it below, making it 2?

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image_mac.mm
File ui/gfx/image/image_mac.mm (right):

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image_mac.mm#...
ui/gfx/image/image_mac.mm:38: NSRectFill(NSMakeRect(0, 0, 16, 16));
nit: you could declare this rect as a local, and then use rect.size as the param
to |-initWithSize:| to be DRY

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image_unittes...
File ui/gfx/image/image_unittest.cc (right):

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image_unittes...
ui/gfx/image/image_unittest.cc:142: 
nit: extra blank lines. Also, do you think it's possible to test the failure
cases? Those are important, too.

[cjhopman, 2012-08-03 22:15:25]

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image.cc#newc...
ui/gfx/image/image.cc:62: pixbuf = gdk_pixbuf_new(GDK_COLORSPACE_RGB, TRUE, 8,
32, 32);
On 2012/07/31 14:07:17, rsesek wrote:
> Does this leak the pixbuf because this is gdk_pixbuf_new() will have the
> refcount = 1, and then you g_object_ref() it below, making it 2?

Yes, you're right. A ref should only be added when decoding is successful. Fixed
now.

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image_mac.mm
File ui/gfx/image/image_mac.mm (right):

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image_mac.mm#...
ui/gfx/image/image_mac.mm:38: NSRectFill(NSMakeRect(0, 0, 16, 16));
On 2012/07/31 14:07:17, rsesek wrote:
> nit: you could declare this rect as a local, and then use rect.size as the
param
> to |-initWithSize:| to be DRY

Done.

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image_unittes...
File ui/gfx/image/image_unittest.cc (right):

http://codereview.chromium.org/10799014/diff/26003/ui/gfx/image/image_unittes...
ui/gfx/image/image_unittest.cc:142: 
On 2012/07/31 14:07:17, rsesek wrote:
> nit: extra blank lines. Also, do you think it's possible to test the failure
> cases? Those are important, too.

Done. Added tests for the failure cases also.

[cjhopman, 2012-08-03 22:17:01]

The last patch also changed PNG encode failures to crashes. As mentioned before
this is expected to never happen.

[cjhopman, 2012-08-03 23:07:06]

On mac_rel, UiGfxImageTest::CheckColor is failing because I changed the test
utils to create a green image instead of a red one (to properly test the PNG
decode failure case). I will fix this, but while I'm at it, that test seems to
fit much better in ui/gfx/image/ than chrome/browser/ui/tests, how do you feel
about me moving it?

[sky, 2012-08-06 14:19:05]

As rsesek is reviewing the image related changes I only looked at the gyp files,
which LGTM.

[Robert Sesek, 2012-08-06 21:54:04]

LGTM!

On 2012/08/03 23:07:06, cjhopman wrote:
> On mac_rel, UiGfxImageTest::CheckColor is failing because I changed the test
> utils to create a green image instead of a red one (to properly test the PNG
> decode failure case). I will fix this, but while I'm at it, that test seems to
> fit much better in ui/gfx/image/ than chrome/browser/ui/tests, how do you feel
> about me moving it?

See the bottom of ui/gfx/image/image_unittest.cc for why they live in
c/b/u/tests. They need to stay there if things have not changed.

http://codereview.chromium.org/10799014/diff/29011/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

http://codereview.chromium.org/10799014/diff/29011/ui/gfx/image/image.cc#newc...
ui/gfx/image/image.cc:102: }
nit: blank line after

[cjhopman, 2012-08-06 22:25:33]

On 2012/08/06 21:54:04, rsesek wrote:
> LGTM!
> 
> On 2012/08/03 23:07:06, cjhopman wrote:
> > On mac_rel, UiGfxImageTest::CheckColor is failing because I changed the test
> > utils to create a green image instead of a red one (to properly test the PNG
> > decode failure case). I will fix this, but while I'm at it, that test seems
to
> > fit much better in ui/gfx/image/ than chrome/browser/ui/tests, how do you
feel
> > about me moving it?
> 
> See the bottom of ui/gfx/image/image_unittest.cc for why they live in
> c/b/u/tests. They need to stay there if things have not changed.

The comment there is true for the other UiGfxImageTest testcases in c/b/u/tests
but doesn't seem to be the case for CheckColor. For now, I've just fixed the
test.

> 
> http://codereview.chromium.org/10799014/diff/29011/ui/gfx/image/image.cc
> File ui/gfx/image/image.cc (right):
> 
>
http://codereview.chromium.org/10799014/diff/29011/ui/gfx/image/image.cc#newc...
> ui/gfx/image/image.cc:102: }
> nit: blank line after

[Robert Sesek, 2012-08-06 22:29:52]

++LGTM

[commit-bot: I haz the power, 2012-08-06 22:33:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/10799014/29013

[commit-bot: I haz the power, 2012-08-06 23:12:12]

Try job failure for 10799014-29013 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-07 00:17:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/10799014/29013

[commit-bot: I haz the power, 2012-08-07 01:41:42]

Change committed as 150228