Mac: correct favicons in bookmarks bar (for pages w/o one and for subfolders).

Mac: correct favicons in bookmarks bar (for pages w/o one and for subfolders).

Bookmarks for pages without a favicon should display a generic "blank page" icon (this also helps avoid a blank button if the bookmark also doesn't have a title). Also, folders within folders also need to display the folder icon.

BUG=23558
TEST=See above; be sure to test things on the bar and in folders, and make sure that pages with favicons are still displayed correctly.

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

[Nico, 2009-10-31 03:33:24]

Drive-by LGTM.

http://codereview.chromium.org/339087/diff/1/2
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/339087/diff/1/2#newcode76
Line 76: // The default (generic) image, also so we can use one copy for all
buttons.
Pick one of "default" and "generic" and use only that. Also, s/also//.

http://codereview.chromium.org/339087/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/339087/diff/1/3#newcode80
Line 80: defaultImage_.reset([rb.GetNSImageNamed(IDR_DEFAULT_FAVICON) retain]);
Probably not worth loading a 16x16 pixmap lazily?

http://codereview.chromium.org/339087/diff/1/3#newcode757
Line 757: if (favIcon.isNull())
if (!favIcon.isNull())
    return gfx::S…);

return defaultImage;

is less confusing in my eyes :-P

[John Grabowski, 2009-11-01 21:35:20]

LGTM

http://codereview.chromium.org/339087/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/339087/diff/1/3#newcode756
Line 756: const SkBitmap& favIcon = bookmarkModel_->GetFavIcon(node);
When bookmarks are first loaded, we have no favicons (they are loaded async). 
Adding this here means we'll always use the default for a quick second.  Perhaps
not a problem but worth knowing.

[pink (ping after 24hrs), 2009-11-02 16:06:32]

LGTM

[viettrungluu, 2009-11-03 16:48:33]

Thanks everyone.

http://codereview.chromium.org/339087/diff/1/2
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/339087/diff/1/2#newcode76
Line 76: // The default (generic) image, also so we can use one copy for all
buttons.
On 2009/10/31 03:33:24, Nico wrote:
> Pick one of "default" and "generic" and use only that. Also, s/also//.

Done.

http://codereview.chromium.org/339087/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/339087/diff/1/3#newcode80
Line 80: defaultImage_.reset([rb.GetNSImageNamed(IDR_DEFAULT_FAVICON) retain]);
On 2009/10/31 03:33:24, Nico wrote:
> Probably not worth loading a 16x16 pixmap lazily?

Probably not, and since the folder icon is also loaded here, there might be some
advantage due to locality of reference (on several levels).

http://codereview.chromium.org/339087/diff/1/3#newcode756
Line 756: const SkBitmap& favIcon = bookmarkModel_->GetFavIcon(node);
On 2009/11/01 21:35:20, John Grabowski wrote:
> When bookmarks are first loaded, we have no favicons (they are loaded async). 
> Adding this here means we'll always use the default for a quick second. 
Perhaps
> not a problem but worth knowing.

Yeah, that occurred to me, but I think it's okay. It's probably less
objectionable than having buttons resize after the quick second (and if the
second doesn't turn out to be quick, we should probably display *something*
anyway).

http://codereview.chromium.org/339087/diff/1/3#newcode757
Line 757: if (favIcon.isNull())
On 2009/10/31 03:33:24, Nico wrote:
> if (!favIcon.isNull())
>     return gfx::S…);
> 
> return defaultImage;
> 
> is less confusing in my eyes :-P

That was my first instinct as well (but I went against instinct since it
involves an extra '!'), so I've changed it. Probably the principle should be
that the default goes at the bottom.

[pink (ping after 24hrs), 2009-11-03 19:42:44]

one more thing...

http://codereview.chromium.org/339087/diff/3002/3004
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/339087/diff/3002/3004#newcode60
Line 60: - (NSImage*)getFavIconForNode:(const BookmarkNode*)node;
the normal obj-c method naming conventions avoid the form "getFoo".
favIconForNode: would work just fine and would be consistent with the rest of
our naming.