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

ui/base/models/simple_menu_model.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/base/models/simple_menu_model.h"
 
 #include "base/bind.h"
 #include "base/message_loop.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/image/image_skia.h"
@@ skipping 167 matching lines @@
   InsertItemAtIndex(item, index);
 }
 
 void SimpleMenuModel::InsertSubMenuWithStringIdAt(
     int index, int command_id, int string_id, MenuModel* model) {
   InsertSubMenuAt(index, command_id, l10n_util::GetStringUTF16(string_id),
                   model);
 }
 
 void SimpleMenuModel::SetIcon(int index, const gfx::ImageSkia& icon) {
+  ValidateItemIndex(index);
   items_[index].icon = icon;
 }
 
 void SimpleMenuModel::Clear() {
   items_.clear();
 }
 
 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)));
     }
   }
   return -1;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // SimpleMenuModel, MenuModel implementation:
 
 bool SimpleMenuModel::HasIcons() const {
   for (ItemVector::const_iterator i = items_.begin(); i != items_.end(); ++i) {
     if (!i->icon.isNull())
       return true;
   }
 
   return false;
 }
 
 int SimpleMenuModel::GetItemCount() const {
   return static_cast<int>(items_.size());
 }
 
 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 {
   if (delegate_)
     return delegate_->IsItemForCommandIdDynamic(GetCommandIdAt(index));
   return false;
 }
 
 bool SimpleMenuModel::GetAcceleratorAt(int index,
                                        ui::Accelerator* accelerator) const {
   if (delegate_) {
     return delegate_->GetAcceleratorForCommandId(GetCommandIdAt(index),
                                                  accelerator);
   }
   return false;
 }
 
 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;
 
   *icon = items_[index].icon;
   return true;
 }
 
 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);
 }
 
 void SimpleMenuModel::HighlightChangedTo(int index) {
   if (delegate_)
     delegate_->CommandIdHighlighted(GetCommandIdAt(index));
 }
 
 void SimpleMenuModel::ActivatedAt(int index) {
   if (delegate_)
     delegate_->ExecuteCommand(GetCommandIdAt(index));
 }
 
 void SimpleMenuModel::ActivatedAt(int index, int event_flags) {
   if (delegate_)
     delegate_->ExecuteCommand(GetCommandIdAt(index), event_flags);
 }
 
 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() {
   if (delegate_)
     delegate_->MenuWillShow(this);
 }
 
 void SimpleMenuModel::MenuClosed() {
   // Due to how menus work on the different platforms, ActivatedAt will be
   // called after this.  It's more convenient for the delegate to be called
@@ skipping 13 matching lines @@
     delegate_->MenuClosed(this);
 }
 
 int SimpleMenuModel::FlipIndex(int index) const {
   return index;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // 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);
 }
 
 void SimpleMenuModel::InsertItemAtIndex(const Item& item, int index) {
   ValidateItem(item);
   items_.insert(items_.begin() + FlipIndex(index), item);
 }
 
 void SimpleMenuModel::ValidateItem(const Item& item) {
 #ifndef NDEBUG
   if (item.type == TYPE_SEPARATOR) {
     DCHECK_EQ(item.command_id, kSeparatorId);
   } else {
     DCHECK_GE(item.command_id, 0);
   }
 #endif  // NDEBUG
 }
 
 }  // namespace ui