Only add NORMAL separators if menu has at least one item and previous item is not a separator and add RemoveTrailingSeparators.

This CL also cleans up separator logic in several of the derived menu model builder classes.

BUG=158195
TEST=Menus should have separators placed as they did before, and no menu should have a separator at the top, or two adjacent separators anywhere within it.

Review URL: https://chromiumcodereview.appspot.com/12094009

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

13 files changed, 68 insertions, 98 deletions

diff --git a/chrome/browser/background/background_mode_manager.cc b/chrome/browser/background/background_mode_manager.cc
index ce832eb..cfd7b18 100644
--- a/chrome/browser/background/background_mode_manager.cc
+++ b/chrome/browser/background/background_mode_manager.cc
@@ -734,12 +734,14 @@ void BackgroundModeManager::UpdateStatusTrayIconContextMenu() {
     DCHECK(profile_cache_->GetNumberOfProfiles() == size_t(1) ||
            keep_alive_for_test_);
     background_mode_data_.begin()->second->BuildProfileMenu(menu, NULL);
-    menu->AddSeparator(ui::NORMAL_SEPARATOR);
   }
+
+  menu->AddSeparator(ui::NORMAL_SEPARATOR);
   menu->AddCheckItemWithStringId(
       IDC_STATUS_TRAY_KEEP_CHROME_RUNNING_IN_BACKGROUND,
       IDS_STATUS_TRAY_KEEP_CHROME_RUNNING_IN_BACKGROUND);
   menu->AddItemWithStringId(IDC_EXIT, IDS_EXIT);
+
   context_menu_ = menu;
   status_icon_->SetContextMenu(menu);
 }
diff --git a/chrome/browser/extensions/context_menu_matcher.cc b/chrome/browser/extensions/context_menu_matcher.cc
index e778a81..8a1f498 100644
--- a/chrome/browser/extensions/context_menu_matcher.cc
+++ b/chrome/browser/extensions/context_menu_matcher.cc
@@ -52,9 +52,7 @@ void ContextMenuMatcher::AppendExtensionItems(const std::string& extension_id,
 
   // If this is the first extension-provided menu item, and there are other
   // items in the menu, and the last item is not a separator add a separator.
-  if (*index == 0 && menu_model_->GetItemCount() &&
-      menu_model_->GetTypeAt(menu_model_->GetItemCount() - 1) !=
-          ui::MenuModel::TYPE_SEPARATOR)
+  if (*index == 0 && menu_model_->GetItemCount())
     menu_model_->AddSeparator(ui::NORMAL_SEPARATOR);
 
   // Extensions (other than platform apps) are only allowed one top-level slot
@@ -192,15 +190,12 @@ void ContextMenuMatcher::RecursivelyAppendExtensionItems(
         radio_group_id++;
 
         // Auto-append a separator if needed.
-        if (last_type != MenuItem::SEPARATOR)
-          menu_model->AddSeparator(ui::NORMAL_SEPARATOR);
+        menu_model->AddSeparator(ui::NORMAL_SEPARATOR);
       }
 
       menu_model->AddRadioItem(menu_id, title, radio_group_id);
     } else if (item->type() == MenuItem::SEPARATOR) {
-      if (i != items.begin() && last_type != MenuItem::SEPARATOR) {
-        menu_model->AddSeparator(ui::NORMAL_SEPARATOR);
-      }
+      menu_model->AddSeparator(ui::NORMAL_SEPARATOR);
     }
     last_type = item->type();
   }
diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc
index 848c6bd..ce8519a 100644
--- a/chrome/browser/tab_contents/render_view_context_menu.cc
+++ b/chrome/browser/tab_contents/render_view_context_menu.cc
@@ -541,10 +541,7 @@ void RenderViewContextMenu::AppendPlatformAppItems() {
       CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDebugPackedApps)) {
     // Add a separator if there are any items already in the menu.
-    if (menu_model_.GetItemCount() &&
-        menu_model_.GetTypeAt(menu_model_.GetItemCount() - 1) !=
-            ui::MenuModel::TYPE_SEPARATOR)
-      menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
+    menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
 
     menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_RELOAD_PACKAGED_APP,
                                     IDS_CONTENT_CONTEXT_RELOAD_PACKAGED_APP);
@@ -648,8 +645,7 @@ void RenderViewContextMenu::AppendDeveloperItems() {
 
   // In the DevTools popup menu, "developer items" is normally the only
   // section, so omit the separator there.
-  if (menu_model_.GetItemCount() > 0)
-    menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
+  menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
   menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_INSPECTELEMENT,
                                   IDS_CONTENT_CONTEXT_INSPECTELEMENT);
 }
@@ -745,8 +741,7 @@ void RenderViewContextMenu::AppendPluginItems() {
   }
 
   if (params_.media_flags & WebContextMenuData::MediaCanRotate) {
-    if (menu_model_.GetItemCount() > 0)
-      menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
+    menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
     menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_ROTATECW,
                                     IDS_CONTENT_CONTEXT_ROTATECW);
     menu_model_.AddItemWithStringId(IDC_CONTENT_CONTEXT_ROTATECCW,
diff --git a/chrome/browser/ui/app_list/extension_app_item.cc b/chrome/browser/ui/app_list/extension_app_item.cc
index 03d2cb4..56281dc 100644
--- a/chrome/browser/ui/app_list/extension_app_item.cc
+++ b/chrome/browser/ui/app_list/extension_app_item.cc
@@ -547,7 +547,7 @@ ui::MenuModel* ExtensionAppItem::GetContextMenuModel() {
                                                 &index);
 
     if (controller_->CanPin()) {
-      context_menu_model_->AddSeparatorIfNecessary(ui::NORMAL_SEPARATOR);
+      context_menu_model_->AddSeparator(ui::NORMAL_SEPARATOR);
       context_menu_model_->AddItemWithStringId(
           TOGGLE_PIN,
           controller_->IsAppPinned(extension_id_) ?
@@ -562,7 +562,7 @@ ui::MenuModel* ExtensionAppItem::GetContextMenuModel() {
     }
 
     if (!extension->is_platform_app()) {
-      context_menu_model_->AddSeparatorIfNecessary(ui::NORMAL_SEPARATOR);
+      context_menu_model_->AddSeparator(ui::NORMAL_SEPARATOR);
       context_menu_model_->AddCheckItemWithStringId(
           LAUNCH_TYPE_REGULAR_TAB,
           IDS_APP_CONTEXT_MENU_OPEN_REGULAR);
@@ -578,7 +578,7 @@ ui::MenuModel* ExtensionAppItem::GetContextMenuModel() {
           LAUNCH_TYPE_FULLSCREEN,
           IDS_APP_CONTEXT_MENU_OPEN_MAXIMIZED);
 
-      context_menu_model_->AddSeparatorIfNecessary(ui::NORMAL_SEPARATOR);
+      context_menu_model_->AddSeparator(ui::NORMAL_SEPARATOR);
       context_menu_model_->AddItemWithStringId(OPTIONS,
                                                IDS_NEW_TAB_APP_OPTIONS);
       context_menu_model_->AddItemWithStringId(DETAILS,
diff --git a/chrome/browser/ui/ash/launcher/launcher_application_menu_item_model.cc b/chrome/browser/ui/ash/launcher/launcher_application_menu_item_model.cc
index efb7ff9..e54ee22 100644
--- a/chrome/browser/ui/ash/launcher/launcher_application_menu_item_model.cc
+++ b/chrome/browser/ui/ash/launcher/launcher_application_menu_item_model.cc
@@ -49,8 +49,9 @@ void LauncherApplicationMenuItemModel::Build() {
       SetIcon(GetIndexOfCommandId(i), item->icon());
     // The first item is most likely the application name which should get
     // separated from the rest.
-    if (!i && launcher_items_.size() > 1)
+    if (i == 0)
       AddSeparator(ui::NORMAL_SEPARATOR);
   }
+  RemoveTrailingSeparators();
 }
 
diff --git a/chrome/browser/ui/ash/launcher/launcher_context_menu.cc b/chrome/browser/ui/ash/launcher/launcher_context_menu.cc
index 03543ac..7187988 100644
--- a/chrome/browser/ui/ash/launcher/launcher_context_menu.cc
+++ b/chrome/browser/ui/ash/launcher/launcher_context_menu.cc
@@ -124,7 +124,7 @@ void LauncherContextMenu::Init() {
         int index = 0;
         extension_items_->AppendExtensionItems(
             app_id, string16(), &index);
-        AddSeparatorIfNecessary(ui::NORMAL_SEPARATOR);
+        AddSeparator(ui::NORMAL_SEPARATOR);
       }
     }
   }
diff --git a/chrome/browser/ui/gtk/bookmarks/bookmark_sub_menu_model_gtk.cc b/chrome/browser/ui/gtk/bookmarks/bookmark_sub_menu_model_gtk.cc
index 92081c6..c493829 100644
--- a/chrome/browser/ui/gtk/bookmarks/bookmark_sub_menu_model_gtk.cc
+++ b/chrome/browser/ui/gtk/bookmarks/bookmark_sub_menu_model_gtk.cc
@@ -176,19 +176,15 @@ void BookmarkSubMenuModel::MenuWillShow() {
     PopulateMenu();
   }
   bookmark_end_ = GetItemCount();
+
   // We want only one separator after the top-level bookmarks and before the
-  // other node and/or mobile node. Keep track of whether we've added it yet.
-  bool added_separator = false;
-  if (model()->other_node()->GetTotalNodeCount() > 1) {
-    AddSeparator(ui::NORMAL_SEPARATOR);
-    added_separator = true;
+  // other node and/or mobile node.
+  AddSeparator(ui::NORMAL_SEPARATOR);
+  if (model()->other_node()->GetTotalNodeCount() > 1)
     AddSubMenuForNode(model()->other_node());
-  }
-  if (model()->mobile_node()->GetTotalNodeCount() > 1) {
-    if (!added_separator)
-      AddSeparator(ui::NORMAL_SEPARATOR);
+  if (model()->mobile_node()->GetTotalNodeCount() > 1)
     AddSubMenuForNode(model()->mobile_node());
-  }
+  RemoveTrailingSeparators();
 }
 
 void BookmarkSubMenuModel::MenuClosed() {
diff --git a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
index bfc4081..22a9f2b 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
+++ b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
@@ -250,19 +250,15 @@ void RecentTabsSubMenuModel::Build() {
   // ...
   // |model_| only contains navigatable (and hence executable) tab items for
   // other devices.
-  BuildLastClosed();
+  AddItem(IDC_RESTORE_TAB, GetLabelForCommandId(IDC_RESTORE_TAB));
   BuildDevices();
   if (model_.empty()) {
+    AddSeparator(ui::NORMAL_SEPARATOR);
     AddItemWithStringId(IDC_RECENT_TABS_NO_DEVICE_TABS,
                         IDS_RECENT_TABS_NO_DEVICE_TABS);
   }
 }
 
-void RecentTabsSubMenuModel::BuildLastClosed() {
-  AddItem(IDC_RESTORE_TAB, GetLabelForCommandId(IDC_RESTORE_TAB));
-  AddSeparator(ui::NORMAL_SEPARATOR);
-}
-
 void RecentTabsSubMenuModel::BuildDevices() {
   browser_sync::SessionModelAssociator* associator = GetModelAssociator();
   if (!associator)
@@ -277,7 +273,6 @@ void RecentTabsSubMenuModel::BuildDevices() {
 
   const size_t kMaxSessionsToShow = 3;
   size_t num_sessions_added = 0;
-  bool need_separator = false;
   for (size_t i = 0;
        i < sessions.size() && num_sessions_added < kMaxSessionsToShow;
        ++i) {
@@ -315,37 +310,27 @@ void RecentTabsSubMenuModel::BuildDevices() {
     std::sort(tabs_in_session.begin(), tabs_in_session.end(),
               SortTabsByRecency);
 
+    // Add the header for the device session.
+    DCHECK(!session->session_name.empty());
+    AddSeparator(ui::NORMAL_SEPARATOR);
+    AddItem(kDisabledCommandId, UTF8ToUTF16(session->session_name));
+    AddDeviceFavicon(GetItemCount() - 1, session->device_type);
+
     // Build tab menu items from sorted session tabs.
     const size_t kMaxTabsPerSessionToShow = 4;
     for (size_t k = 0;
          k < std::min(tabs_in_session.size(), kMaxTabsPerSessionToShow);
          ++k) {
-      BuildForeignTabItem(session_tag, *tabs_in_session[k],
-          // Only need |session_name| for the first tab of the session.
-          !k ? session->session_name : std::string(), session->device_type,
-          need_separator);
-      need_separator = false;
+      BuildForeignTabItem(session_tag, *tabs_in_session[k]);
     }  // for all tabs in one session
 
     ++num_sessions_added;
-    need_separator = true;
   }  // for all sessions
 }
 
 void RecentTabsSubMenuModel::BuildForeignTabItem(
     const std::string& session_tag,
-    const SessionTab& tab,
-    const std::string& session_name,
-    browser_sync::SyncedSession::DeviceType device_type,
-    bool need_separator) {
-  if (need_separator)
-    AddSeparator(ui::NORMAL_SEPARATOR);
-
-  if (!session_name.empty()) {
-    AddItem(kDisabledCommandId, UTF8ToUTF16(session_name));
-    AddDeviceFavicon(GetItemCount() - 1, device_type);
-  }
-
+    const SessionTab& tab) {
   const TabNavigation& current_navigation =
       tab.navigations.at(tab.normalized_navigation_index());
   NavigationItem item(session_tag, tab.tab_id.id(),
diff --git a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
index 6f726ba..6f7f70b 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
+++ b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
@@ -61,14 +61,10 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
 
   // Build the menu items by populating the model.
   void Build();
-  void BuildLastClosed();
   void BuildDevices();
   void BuildForeignTabItem(
       const std::string& session_tag,
-      const SessionTab& tab,
-      const std::string& session_name,
-      browser_sync::SyncedSession::DeviceType device_type,
-      bool need_separator);
+      const SessionTab& tab);
   void AddDeviceFavicon(int index_in_menu,
                         browser_sync::SyncedSession::DeviceType device_type);
   void AddTabFavicon(int model_index, int command_id, const GURL& url);
diff --git a/chrome/browser/ui/views/frame/system_menu_model_builder.cc b/chrome/browser/ui/views/frame/system_menu_model_builder.cc
index 73e1bb4..0401182 100644
--- a/chrome/browser/ui/views/frame/system_menu_model_builder.cc
+++ b/chrome/browser/ui/views/frame/system_menu_model_builder.cc
@@ -25,26 +25,16 @@ SystemMenuModelBuilder::~SystemMenuModelBuilder() {
 }
 
 void SystemMenuModelBuilder::Init() {
-  bool needs_trailing_separator = false;
-  ui::SimpleMenuModel* model = CreateMenuModel(&needs_trailing_separator);
+  ui::SimpleMenuModel* model = new ui::SimpleMenuModel(&menu_delegate_);
   menu_model_.reset(model);
   BuildMenu(model);
-  if (needs_trailing_separator)
-    model->AddSeparator(ui::NORMAL_SEPARATOR);
-}
-
-ui::SimpleMenuModel* SystemMenuModelBuilder::CreateMenuModel(
-    bool* needs_trailing_separator) {
 #if defined(OS_WIN)
   // On Windows with HOST_DESKTOP_TYPE_NATIVE we put the menu items in the
   // system menu (not at the end). Doing this necessitates adding a trailing
   // separator.
-  *needs_trailing_separator =
-      (browser()->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_NATIVE);
-#else
-  *needs_trailing_separator = false;
+  if (browser()->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_NATIVE)
+    model->AddSeparator(ui::NORMAL_SEPARATOR);
 #endif
-  return new ui::SimpleMenuModel(&menu_delegate_);
 }
 
 void SystemMenuModelBuilder::BuildMenu(ui::SimpleMenuModel* model) {
diff --git a/chrome/browser/ui/views/frame/system_menu_model_builder.h b/chrome/browser/ui/views/frame/system_menu_model_builder.h
index d68aeb3..280d5de 100644
--- a/chrome/browser/ui/views/frame/system_menu_model_builder.h
+++ b/chrome/browser/ui/views/frame/system_menu_model_builder.h
@@ -34,11 +34,6 @@ class SystemMenuModelBuilder {
  private:
   Browser* browser() { return menu_delegate_.browser(); }
 
-  // Creates and returns the MenuModel. This does not populate the menu.
-  // |needs_trailing_separator| is set to true if the menu needs a separator
-  // after all items have been added.
-  ui::SimpleMenuModel* CreateMenuModel(bool* needs_trailing_separator);
-
   // Populates |model| with the appropriate contents.
   void BuildMenu(ui::SimpleMenuModel* model);
   void BuildSystemMenuForBrowserWindow(ui::SimpleMenuModel* model);
diff --git a/ui/base/models/simple_menu_model.cc b/ui/base/models/simple_menu_model.cc
index 139bc22..5e947aa 100644
--- a/ui/base/models/simple_menu_model.cc
+++ b/ui/base/models/simple_menu_model.cc
@@ -81,18 +81,6 @@ void SimpleMenuModel::AddItemWithStringId(int command_id, int string_id) {
   AddItem(command_id, l10n_util::GetStringUTF16(string_id));
 }
 
-void SimpleMenuModel::AddSeparator(MenuSeparatorType separator_type) {
-#if !defined(USE_AURA)
-  if (separator_type != NORMAL_SEPARATOR) {
-    NOTIMPLEMENTED();
-  }
-#endif
-  DCHECK(items_.empty() || items_.back().type != TYPE_SEPARATOR);
-  Item item = { kSeparatorId, string16(), gfx::Image(), TYPE_SEPARATOR,
-                -1, NULL, NULL , separator_type };
-  AppendItem(item);
-}
-
 void SimpleMenuModel::AddCheckItem(int command_id, const string16& label) {
   Item item = { command_id, label, gfx::Image(), TYPE_CHECK, -1, NULL,
                 NULL, NORMAL_SEPARATOR };
@@ -115,10 +103,30 @@ void SimpleMenuModel::AddRadioItemWithStringId(int command_id, int string_id,
   AddRadioItem(command_id, l10n_util::GetStringUTF16(string_id), group_id);
 }
 
-void SimpleMenuModel::AddSeparatorIfNecessary(MenuSeparatorType separator_type)
+void SimpleMenuModel::AddSeparator(MenuSeparatorType separator_type)
 {
-  if (!items_.empty() && items_.back().type != TYPE_SEPARATOR)
-    AddSeparator(separator_type);
+  if (items_.empty()) {
+    if (separator_type == NORMAL_SEPARATOR) {
+      return;
+    }
+    DCHECK_EQ(SPACING_SEPARATOR, separator_type);
+  } else if (items_.back().type == TYPE_SEPARATOR) {
+    DCHECK_EQ(NORMAL_SEPARATOR, separator_type);
+    DCHECK_EQ(NORMAL_SEPARATOR, items_.back().separator_type);
+    return;
+  }
+#if !defined(USE_AURA)
+  if (separator_type != NORMAL_SEPARATOR)
+    NOTIMPLEMENTED();
+#endif
+  Item item = { kSeparatorId, string16(), gfx::Image(), TYPE_SEPARATOR,
+                -1, NULL, NULL , separator_type };
+  AppendItem(item);
+}
+
+void SimpleMenuModel::RemoveTrailingSeparators() {
+  while (!items_.empty() && items_.back().type == TYPE_SEPARATOR)
+    items_.pop_back();
 }
 
 void SimpleMenuModel::AddButtonItem(int command_id,
diff --git a/ui/base/models/simple_menu_model.h b/ui/base/models/simple_menu_model.h
index 8aa9d9b..06917f3 100644
--- a/ui/base/models/simple_menu_model.h
+++ b/ui/base/models/simple_menu_model.h
@@ -74,14 +74,21 @@ class UI_EXPORT SimpleMenuModel : public MenuModel {
   // Methods for adding items to the model.
   void AddItem(int command_id, const string16& label);
   void AddItemWithStringId(int command_id, int string_id);
-  void AddSeparator(MenuSeparatorType separator_type);
   void AddCheckItem(int command_id, const string16& label);
   void AddCheckItemWithStringId(int command_id, int string_id);
   void AddRadioItem(int command_id, const string16& label, int group_id);
   void AddRadioItemWithStringId(int command_id, int string_id, int group_id);
 
-  // Adds a separator if the menu is empty, or the last item is not a separator.
-  void AddSeparatorIfNecessary(MenuSeparatorType separator_type);
+  // Adds a separator of the specified type to the model.
+  // - Adding a separator after another separator is always invalid if they
+  //   differ in type, but silently ignored if they are both NORMAL.
+  // - Adding a separator to an empty model is invalid, unless they are NORMAL
+  //   or SPACING. NORMAL separators are silently ignored if the model is empty.
+  void AddSeparator(MenuSeparatorType separator_type);
+
+  // Removes separators until the model's last entry is not a separator, or the
+  // model is empty.
+  void RemoveTrailingSeparators();
 
   // These three methods take pointers to various sub-models. These models
   // should be owned by the same owner of this SimpleMenuModel.