Restructure the wrench menu into an action based order

chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc

Index: chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc
diff --git a/chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc b/chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc
index 3f3d042f79fa3d449ef71ca793ae357be0ab84da..3622da65f956bc097b997a2270e5988e2c1133c9 100644
--- a/chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc
+++ b/chrome/browser/ui/toolbar/bookmark_sub_menu_model.cc
@@ -20,11 +20,6 @@ BookmarkSubMenuModel::BookmarkSubMenuModel(
 BookmarkSubMenuModel::~BookmarkSubMenuModel() {}
 
 void BookmarkSubMenuModel::Build(Browser* browser) {
-  AddCheckItemWithStringId(IDC_SHOW_BOOKMARK_BAR, IDS_SHOW_BOOKMARK_BAR);
-  AddItemWithStringId(IDC_SHOW_BOOKMARK_MANAGER, IDS_BOOKMARK_MANAGER);
-#if !defined(OS_CHROMEOS)
-  AddItemWithStringId(IDC_IMPORT_SETTINGS, IDS_IMPORT_SETTINGS_MENU_LABEL);
-#endif
   bool is_submenu_visible =
       delegate()->IsCommandIdVisible(IDC_BOOKMARK_PAGE) ||
       delegate()->IsCommandIdVisible(IDC_BOOKMARK_ALL_TABS);
@@ -32,9 +27,8 @@ void BookmarkSubMenuModel::Build(Browser* browser) {
   is_submenu_visible |=
       delegate()->IsCommandIdVisible(IDC_PIN_TO_START_SCREEN);
 #endif
-  if (is_submenu_visible) {
-    AddSeparator(ui::NORMAL_SEPARATOR);
 
+  if (is_submenu_visible) {
     AddItemWithStringId(IDC_BOOKMARK_PAGE, IDS_BOOKMARK_THIS_PAGE);
     AddItemWithStringId(IDC_BOOKMARK_ALL_TABS, IDS_BOOKMARK_OPEN_PAGES);
 
@@ -42,6 +36,13 @@ void BookmarkSubMenuModel::Build(Browser* browser) {
     AddItemWithStringId(IDC_PIN_TO_START_SCREEN, IDS_PIN_TO_START_SCREEN);
 #endif
   }
+  AddSeparator(ui::NORMAL_SEPARATOR);

[Peter Kasting, 2015/03/30 19:37:02]

Shouldn't this be inside the above conditional?  Otherwise, if the submenu isn't
visible, we'll just have a separator alone atop these other items.

[edwardjung, 2015/03/31 11:44:04]

Yes, good point.

+  AddCheckItemWithStringId(IDC_SHOW_BOOKMARK_BAR, IDS_SHOW_BOOKMARK_BAR);
+  AddItemWithStringId(IDC_SHOW_BOOKMARK_MANAGER, IDS_BOOKMARK_MANAGER);
+#if !defined(OS_CHROMEOS)
+  AddItemWithStringId(IDC_IMPORT_SETTINGS, IDS_IMPORT_SETTINGS_MENU_LABEL);
+#endif
+
 #if defined(OS_MACOSX)
   AddSeparator(ui::NORMAL_SEPARATOR);

[Peter Kasting, 2015/03/30 19:37:02]

This seems really weird.  I assume there's a Mac-specific file somewhere else
that appends more items here?  Maybe whatever code does that should be adding
this separator instead.

I realize this was already like this, but while you're here anyway...

[edwardjung, 2015/03/31 11:44:04]

Removed. Curiously the OSX cocoa code does add a separator before adding the the
extra menu items.

 #endif