Add favicons to Mac bookmark bar.

Add favicons to Mac bookmark bar.
BUG=8381
TEST=Open bookmark bar (Cmd-B).  Add some bookmarks with sites that
have favicons (cnn.com).  See icons in bookmark buttons.  Make sure
color is correct.


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

[TVL, 2009-06-12 19:05:14]

lg

http://codereview.chromium.org/125061/diff/10/12
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/125061/diff/10/12#newcode170
Line 170: NSImage* image = SkBitmapToNSImage(favicon);
do you want to use the nav icon if there is no fav icon?  or will bookmark model
do that already?

http://codereview.chromium.org/125061/diff/10/12#newcode173
Line 173: [cell setImagePosition:NSImageLeft];
will the cell scale the image if it's too big?

http://codereview.chromium.org/125061/diff/10/14
File chrome/browser/cocoa/cocoa_utils.mm (right):

http://codereview.chromium.org/125061/diff/10/14#newcode16
Line 16: free(info);
do you need to check for info being NULL?

http://codereview.chromium.org/125061/diff/10/14#newcode42
Line 42: // but I have not been able to solve this yet.
is a bug filed so we don't forget to circle back to this in the future?  maybe
put the bug here also?

http://codereview.chromium.org/125061/diff/10/14#newcode52
Line 52: int rowwords = rowbytes/4;
do you need to be sure rowbytes is a multiple of 4?

http://codereview.chromium.org/125061/diff/10/14#newcode82
Line 82: icon.rowBytes(), space, info, dataRef,
you already have width, height, rowbytes, no need to fetch them from the icon
again?

http://codereview.chromium.org/125061/diff/10/15
File chrome/browser/cocoa/cocoa_utils_unittest.mm (right):

http://codereview.chromium.org/125061/diff/10/15#newcode43
Line 43: NSColor* color = NSReadPixel(NSMakePoint(17, 17));
make sure 17, 17 is within the size?

http://codereview.chromium.org/125061/diff/10/11
File skia/skia.gyp (right):

http://codereview.chromium.org/125061/diff/10/11#newcode386
Line 386: 
why the blank line?

[John Grabowski, 2009-06-13 00:47:06]

http://codereview.chromium.org/125061/diff/10/12
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/125061/diff/10/12#newcode170
Line 170: NSImage* image = SkBitmapToNSImage(favicon);
On 2009/06/12 19:05:14, TVL wrote:
> do you want to use the nav icon if there is no fav icon?  or will bookmark
model
> do that already?

It does not do that already.  Problem is that the favicons get loaded async.  At
what point do I add the default?  When do I know async loading of a favicon will
never happen to know when to put up a default?

http://codereview.chromium.org/125061/diff/10/12#newcode173
Line 173: [cell setImagePosition:NSImageLeft];
On 2009/06/12 19:05:14, TVL wrote:
> will the cell scale the image if it's too big?

yes.

http://codereview.chromium.org/125061/diff/10/14
File chrome/browser/cocoa/cocoa_utils.mm (right):

http://codereview.chromium.org/125061/diff/10/14#newcode16
Line 16: free(info);
On 2009/06/12 19:05:14, TVL wrote:
> do you need to check for info being NULL?

If NULL we would never get here but I'm happy to add the check.

http://codereview.chromium.org/125061/diff/10/14#newcode42
Line 42: // but I have not been able to solve this yet.
On 2009/06/12 19:05:14, TVL wrote:
> is a bug filed so we don't forget to circle back to this in the future?  maybe
> put the bug here also?

Done

http://codereview.chromium.org/125061/diff/10/14#newcode52
Line 52: int rowwords = rowbytes/4;
On 2009/06/12 19:05:14, TVL wrote:
> do you need to be sure rowbytes is a multiple of 4?

If we get here, format is SkBitmap::kARGB_8888_Config.

http://codereview.chromium.org/125061/diff/10/14#newcode82
Line 82: icon.rowBytes(), space, info, dataRef,
On 2009/06/12 19:05:14, TVL wrote:
> you already have width, height, rowbytes, no need to fetch them from the icon
> again?

No; switched over.

http://codereview.chromium.org/125061/diff/10/15
File chrome/browser/cocoa/cocoa_utils_unittest.mm (right):

http://codereview.chromium.org/125061/diff/10/15#newcode43
Line 43: NSColor* color = NSReadPixel(NSMakePoint(17, 17));
On 2009/06/12 19:05:14, TVL wrote:
> make sure 17, 17 is within the size?

check added

http://codereview.chromium.org/125061/diff/10/11
File skia/skia.gyp (right):

http://codereview.chromium.org/125061/diff/10/11#newcode386
Line 386: 
On 2009/06/12 19:05:14, TVL wrote:
> why the blank line?

Honors the convention for separating logical groups in here.
E.g. "ports" and "images" have spaces around the groups.

[TVL, 2009-06-15 00:41:22]

lgtm