Added menus for bookmark bar folders.

Added menus for bookmark bar folders.  This is NOT based on the Cole
prototype; it is an attempt to get something functional in the short
term, and have a visual baseline before doing something new.

Added folder icons for bookmark bar folder buttons.  Added an "off the
side" button/menu for bookmark buttons which don't fit on the bar.
Updated "Add page..." item to allow creating bookmarks in the folders
(if selected over a folder button).

BUG=http://crbug.com/8381

TEST=Here we go:
1) Make sure bookmark bar folders have the "folder" icon.
2) Right click on a folder --> Add Page, and add a bookmark.
   Make sure bookmark is now in the folder, not at the top level.
3) (Oh, you just implicitly verified you can open bookmark folders!)
4) Add 5 bookmarks then shrink the window thinner so all bookmark
  buttons don't fit.  Make sure "off the right" button gets enabled
  (on right side of bar) and shows bookmarks in a pop-up menu (when
  clicked) that don't completely fit on the bar.
5) Make it super-wide so the all fit and make sure "off the right"
  button is disabled.
6) Add a bunch of bookmarks to a folder; make sure they all work.
7) Add nested folders (by editing the bookmark pref file and restarting
   Chrome) and make sure bookmark folder buttons have nested/cascading
   menus.


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

[John Grabowski, 2009-07-23 20:39:33]

+cc pink for milestone tracking
+cc thomasvl for l10n issue tracking

[TVL, 2009-07-23 21:05:20]

http://codereview.chromium.org/159286/diff/1001/1002
File chrome/app/nibs/BookmarkBar.xib (right):

http://codereview.chromium.org/159286/diff/1001/1002#newcode64
Line 64: <int key="NSfFlags">16</int>
why do you have to set the font?  seem like that could give us issues with other
langs?

[John Grabowski, 2009-07-23 21:14:35]

I didn't set the font explicitly; I think this is an artifact of the button
I chose ("Square").This button doesn't even have text; it has the chevron
for the "off the side" menu.

I'll try and tweak the xib for clarity.

jrg


On Thu, Jul 23, 2009 at 2:05 PM, <thomasvl@chromium.org> wrote:

>
> http://codereview.chromium.org/159286/diff/1001/1002
> File chrome/app/nibs/BookmarkBar.xib (right):
>
> http://codereview.chromium.org/159286/diff/1001/1002#newcode64
> Line 64: <int key="NSfFlags">16</int>
> why do you have to set the font?  seem like that could give us issues
> with other langs?
>
>
> http://codereview.chromium.org/159286
>

[rohitrao (ping after 24h), 2009-07-23 22:23:35]

LGTM

http://codereview.chromium.org/159286/diff/1001/1004
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/159286/diff/1001/1004#newcode282
Line 282: // Add the given bookmark node (and all it's children) to menu, one
s/it's/its/

Can you say "Recursively add..." in the comment, to make it explicit?

http://codereview.chromium.org/159286/diff/1001/1004#newcode506
Line 506: if (!favicon.isNull()) {
Is there a fallback for missing favicons?  Should we put in a stock icon?

http://codereview.chromium.org/159286/diff/1001/1005
File chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm (right):

http://codereview.chromium.org/159286/diff/1001/1005#newcode29
Line 29: // TODO(jrg): add a better heuristic?  I'd really like to trim this
Can you use one of the text elider functions?  See app/gfx/text_elider.h.

[John Grabowski, 2009-07-23 23:16:18]

Note this includes a change to remove the font setting on the button (a la tvl's
comments)

http://codereview.chromium.org/159286/diff/1001/1004
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/159286/diff/1001/1004#newcode282
Line 282: // Add the given bookmark node (and all it's children) to menu, one
On 2009/07/23 22:23:35, rohitrao wrote:
> s/it's/its/
> 
> Can you say "Recursively add..." in the comment, to make it explicit?

Yes; done.

http://codereview.chromium.org/159286/diff/1001/1004#newcode506
Line 506: if (!favicon.isNull()) {
On 2009/07/23 22:23:35, rohitrao wrote:
> Is there a fallback for missing favicons?  Should we put in a stock icon?

It's a bit more complicated.  At this point, favicons may not be available yet
(delayed load for launch speed).  See nodeFavIconLoaded:node: lower in this file
for the callback when the icon gets loaded.

I am doubtful adding a stock icon is good here, since icons will "flash wrong"
on launch.

http://codereview.chromium.org/159286/diff/1001/1005
File chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm (right):

http://codereview.chromium.org/159286/diff/1001/1005#newcode29
Line 29: // TODO(jrg): add a better heuristic?  I'd really like to trim this
On 2009/07/23 22:23:35, rohitrao wrote:
> Can you use one of the text elider functions?  See app/gfx/text_elider.h.

Ooh... yes I can!