Fixes couple of issues with bookmarks in wrench menu:

. Crash when deleting via context menu. This was happening because
  BookmarkMenuDelegate didn't keep a handle to the menu item it added
  the items to.
. If other bookmarks folder is empty it wouldn't show a menu item for
  (empty).
. Deleting the last item in the other folder would prematurely close
  the menu.
. Hit DCHECK when adding empty menu (long standing issue).

BUG=83746
TEST=from the wrench menu on windows right click a bookmark, delete
     it, and make sure you don't crash.
R=ben@chromium.org

Review URL: http://codereview.chromium.org/7066034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@86634 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc

@@ -41,6 +41,7 @@ BookmarkMenuDelegate::BookmarkMenuDelegate(Profile* profile,
       parent_(parent),
       menu_(NULL),
       for_drop_(false),
+      parent_menu_item_(NULL),
       next_menu_id_(first_menu_id),
       real_delegate_(NULL),
       is_mutating_model_(false) {
@@ -59,6 +60,7 @@ void BookmarkMenuDelegate::Init(views::MenuDelegate* real_delegate,
   profile_->GetBookmarkModel()->AddObserver(this);
   real_delegate_ = real_delegate;
   if (parent) {
+    parent_menu_item_ = parent;
     BuildMenu(node, start_child_index, parent, &next_menu_id_);
     if (show_options == SHOW_OTHER_FOLDER)
       BuildOtherFolderMenu(parent, &next_menu_id_);
@@ -69,6 +71,7 @@ void BookmarkMenuDelegate::Init(views::MenuDelegate* real_delegate,
 
 void BookmarkMenuDelegate::SetActiveMenu(const BookmarkNode* node,
                                          int start_index) {
+  DCHECK(!parent_menu_item_);
   if (!node_to_menu_map_[node])
     CreateMenu(node, start_index, HIDE_OTHER_FOLDER);
   menu_ = node_to_menu_map_[node];
@@ -279,6 +282,13 @@ void BookmarkMenuDelegate::BookmarkNodeFaviconChanged(
       return;
     }
   }
+
+  if (parent_menu_item_) {
+    MenuItemView* menu_item = parent_menu_item_->GetMenuItemByID(
+        menu_pair->second);
+    if (menu_item)
+      menu_item->SetIcon(model->GetFavicon(node));
+  }
 }
 
 void BookmarkMenuDelegate::WillRemoveBookmarks(
@@ -366,7 +376,8 @@ MenuItemView* BookmarkMenuDelegate::GetMenuByID(int id) {
     if (menu)
       return menu;
   }
-  return NULL;
+
+  return parent_menu_item_ ? parent_menu_item_->GetMenuItemByID(id) : NULL;
 }
 
 void BookmarkMenuDelegate::WillRemoveBookmarksImpl(

chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h

@@ -174,6 +174,10 @@ class BookmarkMenuDelegate : public BaseBookmarkModelObserver,
   // Is the menu being shown for a drop?
   bool for_drop_;
 
+  // If non-NULL this is the |parent| passed to Init and is NOT owned by us.
+  views::MenuItemView* parent_menu_item_;
+
+  // Maps from node to menu. This is used if a NULL parent is passed to Init().
   NodeToMenuMap node_to_menu_map_;
 
   // ID of the next menu item.

views/controls/menu/menu_item_view.cc

@@ -37,10 +37,10 @@ class EmptyMenuMenuItem : public MenuItemView {
  public:
   explicit EmptyMenuMenuItem(MenuItemView* parent)
       : MenuItemView(parent, 0, NORMAL) {
-    SetTitle(UTF16ToWide(
-        l10n_util::GetStringUTF16(IDS_APP_MENU_EMPTY_SUBMENU)));
     // Set this so that we're not identified as a normal menu item.
     SetID(kEmptyMenuItemViewID);
+    SetTitle(UTF16ToWide(
+        l10n_util::GetStringUTF16(IDS_APP_MENU_EMPTY_SUBMENU)));
     SetEnabled(false);
   }
 
@@ -738,6 +738,11 @@ int MenuItemView::GetChildPreferredWidth() {
 }
 
 string16 MenuItemView::GetAcceleratorText() {
+  if (GetID() == kEmptyMenuItemViewID) {
+    // Don't query the delegate for menus that represent no children.
+    return string16();
+  }
+
   Accelerator accelerator;
   return (GetDelegate() &&
           GetDelegate()->GetAccelerator(GetCommand(), &accelerator)) ?