Cleanup: Remove vector::at() calls in SimpleMenuModel. Do CHECKs instead.

ui/base/models/simple_menu_model.cc

Index: ui/base/models/simple_menu_model.cc
===================================================================
--- ui/base/models/simple_menu_model.cc	(revision 139648)
+++ ui/base/models/simple_menu_model.cc	(working copy)
@@ -185,6 +185,7 @@
 }
 
 void SimpleMenuModel::SetIcon(int index, const gfx::ImageSkia& icon) {
+  ValidateItemIndex(index);
   items_[index].icon = icon;
 }
 
@@ -194,7 +195,7 @@
 
 int SimpleMenuModel::GetIndexOfCommandId(int command_id) {
   for (ItemVector::iterator i = items_.begin(); i != items_.end(); ++i) {
-    if ((*i).command_id == command_id) {
+    if (i->command_id == command_id) {
       return FlipIndex(static_cast<int>(std::distance(items_.begin(), i)));
     }
   }
@@ -218,17 +219,23 @@
 }
 
 MenuModel::ItemType SimpleMenuModel::GetTypeAt(int index) const {
-  return items_.at(FlipIndex(index)).type;
+  int item_index = FlipIndex(index);
+  ValidateItemIndex(item_index);
+  return items_[item_index].type;
 }
 
 int SimpleMenuModel::GetCommandIdAt(int index) const {
-  return items_.at(FlipIndex(index)).command_id;
+  int item_index = FlipIndex(index);
+  ValidateItemIndex(item_index);
+  return items_[item_index].command_id;
 }
 
 string16 SimpleMenuModel::GetLabelAt(int index) const {
   if (IsItemDynamicAt(index))
     return delegate_->GetLabelForCommandId(GetCommandIdAt(index));
-  return items_.at(FlipIndex(index)).label;
+  int item_index = FlipIndex(index);
+  ValidateItemIndex(item_index);
+  return items_[item_index].label;
 }
 
 bool SimpleMenuModel::IsItemDynamicAt(int index) const {
@@ -249,20 +256,22 @@
 bool SimpleMenuModel::IsItemCheckedAt(int index) const {
   if (!delegate_)
     return false;
-  int item_index = FlipIndex(index);
-  MenuModel::ItemType item_type = items_[item_index].type;
+  MenuModel::ItemType item_type = GetTypeAt(index);
   return (item_type == TYPE_CHECK || item_type == TYPE_RADIO) ?
       delegate_->IsCommandIdChecked(GetCommandIdAt(index)) : false;
 }
 
 int SimpleMenuModel::GetGroupIdAt(int index) const {
-  return items_.at(FlipIndex(index)).group_id;
+  int item_index = FlipIndex(index);
+  ValidateItemIndex(item_index);
+  return items_[item_index].group_id;
 }
 
 bool SimpleMenuModel::GetIconAt(int index, gfx::ImageSkia* icon) {
   if (IsItemDynamicAt(index))
     return delegate_->GetIconForCommandId(GetCommandIdAt(index), icon);
 
+  ValidateItemIndex(index);
   if (items_[index].icon.isNull())
     return false;
 
@@ -271,21 +280,21 @@
 }
 
 ButtonMenuItemModel* SimpleMenuModel::GetButtonMenuItemAt(int index) const {
-  return items_.at(FlipIndex(index)).button_model;
+  int item_index = FlipIndex(index);
+  ValidateItemIndex(item_index);
+  return items_[item_index].button_model;
 }
 
 bool SimpleMenuModel::IsEnabledAt(int index) const {
   int command_id = GetCommandIdAt(index);
-  if (!delegate_ || command_id == kSeparatorId ||
-      items_.at(FlipIndex(index)).button_model)
+  if (!delegate_ || command_id == kSeparatorId || GetButtonMenuItemAt(index))
     return true;
   return delegate_->IsCommandIdEnabled(command_id);
 }
 
 bool SimpleMenuModel::IsVisibleAt(int index) const {
   int command_id = GetCommandIdAt(index);
-  if (!delegate_ || command_id == kSeparatorId ||
-      items_.at(FlipIndex(index)).button_model)
+  if (!delegate_ || command_id == kSeparatorId || GetButtonMenuItemAt(index))
     return true;
   return delegate_->IsCommandIdVisible(command_id);
 }
@@ -306,7 +315,9 @@
 }
 
 MenuModel* SimpleMenuModel::GetSubmenuModelAt(int index) const {
-  return items_.at(FlipIndex(index)).submenu;
+  int item_index = FlipIndex(index);
+  ValidateItemIndex(item_index);
+  return items_[item_index].submenu;
 }
 
 void SimpleMenuModel::MenuWillShow() {
@@ -340,6 +351,11 @@
 ////////////////////////////////////////////////////////////////////////////////
 // SimpleMenuModel, Private:
 
+void SimpleMenuModel::ValidateItemIndex(int index) const {

[Peter Kasting, 2012/05/31 04:49:25]

Nit: If you make this method return its argument rather than void, then most of
the callers can inline it to reduce duplicated boilerplate:

int item_index = FlipIndex(index);
ValidateItemIndex(item_index);
do_something(item_index);

-->

do_something(ValidateIndex(FlipIndex(index)));

I find this a little easier to read too.

[Lei Zhang, 2012/05/31 06:22:05]

Done.

+  CHECK_LE(0, index);

[Peter Kasting, 2012/05/31 04:49:25]

Nit: For inequalities other than NE, go ahead and write in natural reading
order:

CHECK_GE(index, 0);
CHECK_LT(static_cast<size_t>(index), items_.size());

[Lei Zhang, 2012/05/31 06:22:05]

Done. I always forget you only have to do that with gtest ASSERT_FOO to make the
error messages come out correctly.

+  CHECK_GT(items_.size(), static_cast<size_t>(index));
+}
+
 void SimpleMenuModel::AppendItem(const Item& item) {
   ValidateItem(item);
   items_.push_back(item);