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

2 files changed, 33 insertions, 18 deletions

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.