Add file icons to chrome://downloads/ on the Mac. Add Skia helper CGImageToSkBitmap().

Add file icons to chrome://downloads/ on the Mac. Add Skia helper CGImageToSkBitmap().

[Amanda Walker, 2009-06-12 17:13:34]

http://codereview.chromium.org/118488/diff/1/3
File chrome/browser/icon_loader_mac.mm (right):

http://codereview.chromium.org/118488/diff/1/3#newcode22
Line 22: for (NSBitmapImageRep* rep in imageReps) {
This loop assumes that there will be a 32x32 or 16x16 representation.  It would
be more general (and should be about as fast) to just draw the NSImage into a
32x32 or 16x16 CGImage.

http://codereview.chromium.org/118488/diff/1/3#newcode30
Line 30: bitmap_ = new SkBitmap(gfx::CGImageToSkBitmap(image));
Hmm.  Better Chromium coding style would be to have gfx::CGImageToSkBitmap do
the "new" itself and return the SkBitmap *, or take a parameter of type SkBitmap
* that it fills in, rather than relying on copy constructors and temp objects.

http://codereview.chromium.org/118488/diff/1/5
File skia/ext/skia_utils_mac.cc (right):

http://codereview.chromium.org/118488/diff/1/5#newcode90
Line 90: // We need to rotate the canvas so that Core Graphics drawing happens
right
"invert the y axis of" would be more accurate than "rotate" here

[---DO NOT USE---rsesek, 2009-06-16 05:35:20]

http://codereview.chromium.org/118488/diff/1/3
File chrome/browser/icon_loader_mac.mm (right):

http://codereview.chromium.org/118488/diff/1/3#newcode22
Line 22: for (NSBitmapImageRep* rep in imageReps) {
On 2009/06/12 17:13:34, Amanda Walker wrote:
> This loop assumes that there will be a 32x32 or 16x16 representation.  It
would
> be more general (and should be about as fast) to just draw the NSImage into a
> 32x32 or 16x16 CGImage.

We now have NSImageToSkBitmap() which takes a size argument.

http://codereview.chromium.org/118488/diff/1/3#newcode30
Line 30: bitmap_ = new SkBitmap(gfx::CGImageToSkBitmap(image));
On 2009/06/12 17:13:34, Amanda Walker wrote:
> Hmm.  Better Chromium coding style would be to have gfx::CGImageToSkBitmap do
> the "new" itself and return the SkBitmap *, or take a parameter of type
SkBitmap
> * that it fills in, rather than relying on copy constructors and temp objects.

Both CGImageToSkBitmap() and NSImageToSkBitmap() now return |SkBitmap*|.

http://codereview.chromium.org/118488/diff/1/5
File skia/ext/skia_utils_mac.cc (right):

http://codereview.chromium.org/118488/diff/1/5#newcode90
Line 90: // We need to rotate the canvas so that Core Graphics drawing happens
right
On 2009/06/12 17:13:34, Amanda Walker wrote:
> "invert the y axis of" would be more accurate than "rotate" here

Done.

[Amanda Walker, 2009-06-16 14:42:27]

LGTM

[Nico, 2009-06-16 15:00:08]

On 2009/06/16 05:35:20, rsesek wrote:
> http://codereview.chromium.org/118488/diff/1/3
> File chrome/browser/icon_loader_mac.mm (right):
> 
> http://codereview.chromium.org/118488/diff/1/3#newcode22
> Line 22: for (NSBitmapImageRep* rep in imageReps) {
> On 2009/06/12 17:13:34, Amanda Walker wrote:
> > This loop assumes that there will be a 32x32 or 16x16 representation.  It
> would
> > be more general (and should be about as fast) to just draw the NSImage into
a
> > 32x32 or 16x16 CGImage.
> 
> We now have NSImageToSkBitmap() which takes a size argument.
> 
> http://codereview.chromium.org/118488/diff/1/3#newcode30
> Line 30: bitmap_ = new SkBitmap(gfx::CGImageToSkBitmap(image));
> On 2009/06/12 17:13:34, Amanda Walker wrote:
> > Hmm.  Better Chromium coding style would be to have gfx::CGImageToSkBitmap
do
> > the "new" itself and return the SkBitmap *, or take a parameter of type
> SkBitmap
> > * that it fills in, rather than relying on copy constructors and temp
objects.
> 
> Both CGImageToSkBitmap() and NSImageToSkBitmap() now return |SkBitmap*|.
> 
> http://codereview.chromium.org/118488/diff/1/5
> File skia/ext/skia_utils_mac.cc (right):
> 
> http://codereview.chromium.org/118488/diff/1/5#newcode90
> Line 90: // We need to rotate the canvas so that Core Graphics drawing happens
> right
> On 2009/06/12 17:13:34, Amanda Walker wrote:
> > "invert the y axis of" would be more accurate than "rotate" here
> 
> Done.

For the records; already mentioned this on IRC:

Drive-by codereivew! I don't think the cgimage->skbitmap conversion function
handles transparency correctly. See
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/image_decoder.cc and
its last commit on how that's done.

And while you're on it, you could move
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/image_decoder.cc to
your new function (and perhaps see if some of the code in there might be a good
fit for your new function) ;-)

[---DO NOT USE---rsesek, 2009-06-16 15:42:22]

> For the records; already mentioned this on IRC:
> 
> Drive-by codereivew! I don't think the cgimage->skbitmap conversion function
> handles transparency correctly. See
> http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/image_decoder.cc
and
> its last commit on how that's done.
> 
> And while you're on it, you could move
> http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/image_decoder.cc
to
> your new function (and perhaps see if some of the code in there might be a
good
> fit for your new function) ;-)

Thanks. Both of these are done.

[Nico, 2009-06-16 15:54:16]

More drive-by. If Amanda says something that contradicts what I say, listen to
her.

http://codereview.chromium.org/118488/diff/1006/1012
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/118488/diff/1006/1012#newcode107
Line 107: CGContextDrawImage(context, rect, image);
It looks like this function copies the image twice. First time is here…

http://codereview.chromium.org/118488/diff/1006/1012#newcode117
Line 117: src.copyTo(bitmap, bitmap->config(), NULL);
…second time might be here. This at least relies on skia to be smart. The code
in image_decoder.cc always does just one copy, even if skia turns out to be
dump.

http://codereview.chromium.org/118488/diff/1006/1012#newcode121
Line 121: return bitmap;  // The caller owns the bitmap.
Can't you return a stack-allocatedSkBitmap? Then callers that want bitmaps on
the stack don't have to deal with deleting the result, while callers that prefer
heap-allocated bitmaps can always call `new
SkBitmap(CGImageToSkBitmap(cgImage))` and get the same effect.

[Mark Mentovai, 2009-06-16 16:52:28]

Unsolicited review for you...

http://codereview.chromium.org/118488/diff/1006/1011
File skia/ext/skia_utils_mac.h (right):

http://codereview.chromium.org/118488/diff/1006/1011#newcode54
Line 54: // Converts a CGImage to a SkBitmap. Caller is responsible for the
SkBitmap.
"Caller is responsible for" means "caller owns?"

http://codereview.chromium.org/118488/diff/1006/1011#newcode54
Line 54: // Converts a CGImage to a SkBitmap. Caller is responsible for the
SkBitmap.
Maybe you should say that the SkBitmap is allocated with new, so that people
know to free it with delete.

http://codereview.chromium.org/118488/diff/1006/1012
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/118488/diff/1006/1012#newcode6
Line 6: #include "skia/ext/bitmap_platform_device_mac.h"
Move this one down below base/logging.h.

http://codereview.chromium.org/118488/diff/1006/1012#newcode8
Line 8: #import <Cocoa/Cocoa.h>
I think that this should be <AppKit/AppKit.h>, and similarly, in the .gyp file,
you should link against AppKit instead of Cocoa.  You're not using any non-Cocoa
parts of AppKit here.

http://codereview.chromium.org/118488/diff/1006/1012#newcode119
Line 119: delete device;
This is itching to turn into a leak when someone comes along and adds an early
return before this delete.  You should hold |device| in a scoped_ptr.

http://codereview.chromium.org/118488/diff/1006/1012#newcode121
Line 121: return bitmap;  // The caller owns the bitmap.
Perhaps this function should take a SkBitmap* argument instead of
heap-allocating one and returning it to the caller.  That way, the caller can
pass an SkBitmap from the stack with & or heap-allocate an object as required.

[Mark Mentovai, 2009-06-16 17:05:49]

http://codereview.chromium.org/118488/diff/2017/1025
File chrome/browser/icon_loader_mac.mm (right):

http://codereview.chromium.org/118488/diff/2017/1025#newcode5
Line 5: #import <Cocoa/Cocoa.h>
<AppKit/AppKit.h> again here - and put it after icon_loader.h.

http://codereview.chromium.org/118488/diff/2017/1025#newcode28
Line 28: // We own the bitmap that comes out of NSImageToSkBitmap().
This comment is senseless now.

http://codereview.chromium.org/118488/diff/2017/1026
File chrome/browser/icon_manager_mac.mm (right):

http://codereview.chromium.org/118488/diff/2017/1026#newcode5
Line 5: #import <Cocoa/Cocoa.h>
Not needed?I don't see why this is needed.

[---DO NOT USE---rsesek, 2009-06-16 17:14:45]

http://codereview.chromium.org/118488/diff/1006/1012
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/118488/diff/1006/1012#newcode6
Line 6: #include "skia/ext/bitmap_platform_device_mac.h"
On 2009/06/16 16:52:31, Mark Mentovai wrote:
> Move this one down below base/logging.h.

Done.

http://codereview.chromium.org/118488/diff/1006/1012#newcode8
Line 8: #import <Cocoa/Cocoa.h>
On 2009/06/16 16:52:31, Mark Mentovai wrote:
> I think that this should be <AppKit/AppKit.h>, and similarly, in the .gyp
file,
> you should link against AppKit instead of Cocoa.  You're not using any
non-Cocoa
> parts of AppKit here.

Done.

http://codereview.chromium.org/118488/diff/1006/1012#newcode107
Line 107: CGContextDrawImage(context, rect, image);
On 2009/06/16 15:54:16, thakis wrote:
> It looks like this function copies the image twice. First time is here…

Done.

http://codereview.chromium.org/118488/diff/1006/1012#newcode117
Line 117: src.copyTo(bitmap, bitmap->config(), NULL);
On 2009/06/16 15:54:16, thakis wrote:
> …second time might be here. This at least relies on skia to be smart. The code
> in image_decoder.cc always does just one copy, even if skia turns out to be
> dump.

Done.

http://codereview.chromium.org/118488/diff/1006/1012#newcode119
Line 119: delete device;
On 2009/06/16 16:52:31, Mark Mentovai wrote:
> This is itching to turn into a leak when someone comes along and adds an early
> return before this delete.  You should hold |device| in a scoped_ptr.

Done.

http://codereview.chromium.org/118488/diff/1006/1012#newcode121
Line 121: return bitmap;  // The caller owns the bitmap.
On 2009/06/16 15:54:16, thakis wrote:
> Can't you return a stack-allocatedSkBitmap? Then callers that want bitmaps on
> the stack don't have to deal with deleting the result, while callers that
prefer
> heap-allocated bitmaps can always call `new
> SkBitmap(CGImageToSkBitmap(cgImage))` and get the same effect.

Done.

http://codereview.chromium.org/118488/diff/1006/1012#newcode121
Line 121: return bitmap;  // The caller owns the bitmap.
On 2009/06/16 16:52:31, Mark Mentovai wrote:
> Perhaps this function should take a SkBitmap* argument instead of
> heap-allocating one and returning it to the caller.  That way, the caller can
> pass an SkBitmap from the stack with & or heap-allocate an object as required.

We just return |SkBitmap| from the stack now.

http://codereview.chromium.org/118488/diff/2017/1025
File chrome/browser/icon_loader_mac.mm (right):

http://codereview.chromium.org/118488/diff/2017/1025#newcode5
Line 5: #import <Cocoa/Cocoa.h>
On 2009/06/16 17:05:50, Mark Mentovai wrote:
> <AppKit/AppKit.h> again here - and put it after icon_loader.h.

Done.

http://codereview.chromium.org/118488/diff/2017/1025#newcode28
Line 28: // We own the bitmap that comes out of NSImageToSkBitmap().
On 2009/06/16 17:05:50, Mark Mentovai wrote:
> This comment is senseless now.

Done.

http://codereview.chromium.org/118488/diff/2017/1026
File chrome/browser/icon_manager_mac.mm (right):

http://codereview.chromium.org/118488/diff/2017/1026#newcode5
Line 5: #import <Cocoa/Cocoa.h>
On 2009/06/16 17:05:50, Mark Mentovai wrote:
> Not needed?I don't see why this is needed.

Keep meaning to nix that but I keep forgetting to CL it. Done.

[Mark Mentovai, 2009-06-16 17:18:48]

lg

[Amanda Walker, 2009-06-16 17:20:46]

lgtm

[Nico, 2009-06-16 18:18:07]

Not lgtm for me yet, sorry.

http://codereview.chromium.org/118488/diff/2035/1044
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/118488/diff/2035/1044#newcode124
Line 124: CGColorSpaceRef color_space =
Use a RetainPtr for this.

http://codereview.chromium.org/118488/diff/2035/1044#newcode130
Line 130: CGContextRef context =
Use a RetainPtr for this.

http://codereview.chromium.org/118488/diff/2035/1044#newcode149
Line 149: operation:NSCompositeDestinationOver
You still want to use NSCompositeCopy here.

[---DO NOT USE---rsesek, 2009-06-16 18:38:37]

http://codereview.chromium.org/118488/diff/2035/1044
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/118488/diff/2035/1044#newcode124
Line 124: CGColorSpaceRef color_space =
On 2009/06/16 18:18:07, thakis wrote:
> Use a RetainPtr for this.

Done.

http://codereview.chromium.org/118488/diff/2035/1044#newcode130
Line 130: CGContextRef context =
On 2009/06/16 18:18:07, thakis wrote:
> Use a RetainPtr for this.

Done.

http://codereview.chromium.org/118488/diff/2035/1044#newcode149
Line 149: operation:NSCompositeDestinationOver
On 2009/06/16 18:18:07, thakis wrote:
> You still want to use NSCompositeCopy here.

Done.

[Nico, 2009-06-16 18:48:33]

Sorry for seeing all this on the first pass :-/

http://codereview.chromium.org/118488/diff/1048/1054
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/118488/diff/1048/1054#newcode122
Line 122: bitmap.eraseARGB(0, 0, 0, 0);
Since you're using NSCompositeCopy, all the image data is overwritten with the
data from image, so this should be unnecessary (at least when size and |image|s
size match)?

http://codereview.chromium.org/118488/diff/1048/1054#newcode148
Line 148: [image drawAtPoint:NSMakePoint(0, 0)
What happens if the image's size and size doesn't match? Don't you want to scale
the image in that case?

[Amanda Walker, 2009-06-16 18:56:23]

http://codereview.chromium.org/118488/diff/1048/1054
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/118488/diff/1048/1054#newcode148
Line 148: [image drawAtPoint:NSMakePoint(0, 0)
NSImage will scale it.

[Nico, 2009-06-16 19:32:11]

lgtm once line 122 ( bitmap.eraseARGB(0, 0, 0, 0);) is gone and you tested that
everything still works ;-)

http://codereview.chromium.org/118488/diff/1048/1054
File skia/ext/skia_utils_mac.mm (right):

http://codereview.chromium.org/118488/diff/1048/1054#newcode148
Line 148: [image drawAtPoint:NSMakePoint(0, 0)
On 2009/06/16 18:56:23, Amanda Walker wrote:
> NSImage will scale it.

Oh, brain fart. Then the eraseARGB() up there is unnecessary at all times.

[Mark Mentovai, 2009-06-16 19:57:45]

Committed r18517