Use ChromeLauncherControllerImpl::GetItem; cleanup.

chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc

Index: chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
index 9de260a325590655f1b9467623c86c5a90cd2888..68b70624a5964b9267f0c5f491279cebd3d92b69 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
@@ -264,9 +264,8 @@ ChromeLauncherControllerImpl::~ChromeLauncherControllerImpl() {
   model_->RemoveObserver(this);
   if (ash::Shell::HasInstance())
     ash::Shell::GetInstance()->window_tree_host_manager()->RemoveObserver(this);
-  for (IDToItemControllerMap::iterator i = id_to_item_controller_map_.begin();
-       i != id_to_item_controller_map_.end(); ++i) {
-    int index = model_->ItemIndexByID(i->first);
+  for (const auto& pair : id_to_item_controller_map_) {
+    int index = model_->ItemIndexByID(pair.first);
     // A "browser proxy" is not known to the model and this removal does
     // therefore not need to be propagated to the model.
     if (index != -1 &&
@@ -310,23 +309,19 @@ const ash::ShelfItem& ChromeLauncherControllerImpl::GetItem(
 
 void ChromeLauncherControllerImpl::SetItemType(ash::ShelfID id,
                                                ash::ShelfItemType type) {
-  const int index = model_->ItemIndexByID(id);
-  DCHECK_GE(index, 0);
-  ash::ShelfItem item = model_->items()[index];
+  ash::ShelfItem item = GetItem(id);
   if (item.type != type) {
     item.type = type;
-    model_->Set(index, item);
+    model_->Set(model_->ItemIndexByID(id), item);

[James Cook, 2016/12/06 00:27:01]

ShelfModel::Set() has a DCHECK that the index is in range, which is good. What
do you think about also adding an early return if the index is bad, just in case
there's some bad code in production?

[msw, 2016/12/06 00:30:40]

Bad code would have already crashed in GetItem, if the index is invalid.
GetItem could return a ptr, using null for ids without an item; WDYT?

[msw, 2016/12/06 01:56:22]

It just occurred to me that you meant an early return in ShelfModel::Set.
I uploaded a patch set which does that and changes GetItem to return a ptr.
Let me know what you think; I can easily change it up as you see fit.

   }
 }
 
 void ChromeLauncherControllerImpl::SetItemStatus(ash::ShelfID id,
                                                  ash::ShelfItemStatus status) {
-  const int index = model_->ItemIndexByID(id);
-  DCHECK_GE(index, 0);
-  ash::ShelfItem item = model_->items()[index];
+  ash::ShelfItem item = GetItem(id);
   if (item.status != status) {
     item.status = status;
-    model_->Set(index, item);
+    model_->Set(model_->ItemIndexByID(id), item);
   }
 }
 
@@ -363,15 +358,10 @@ void ChromeLauncherControllerImpl::CloseLauncherItem(ash::ShelfID id) {
 
 void ChromeLauncherControllerImpl::Pin(ash::ShelfID id) {
   DCHECK(HasShelfIDToAppIDMapping(id));
-
-  int index = model_->ItemIndexByID(id);
-  DCHECK_GE(index, 0);
-
-  ash::ShelfItem item = model_->items()[index];
-
+  ash::ShelfItem item = GetItem(id);
   if (item.type == ash::TYPE_APP) {
     item.type = ash::TYPE_APP_SHORTCUT;
-    model_->Set(index, item);
+    model_->Set(model_->ItemIndexByID(id), item);
   } else if (item.type != ash::TYPE_APP_SHORTCUT) {
     return;
   }
@@ -401,11 +391,10 @@ void ChromeLauncherControllerImpl::UnpinAndUpdatePrefs(ash::ShelfID id,
 }
 
 bool ChromeLauncherControllerImpl::IsPinned(ash::ShelfID id) {
-  int index = model_->ItemIndexByID(id);
-  if (index < 0)
+  if (model_->ItemIndexByID(id) < 0)
     return false;
-  ash::ShelfItemType type = model_->items()[index].type;
-  return (type == ash::TYPE_APP_SHORTCUT || type == ash::TYPE_BROWSER_SHORTCUT);
+  ash::ShelfItemType type = GetItem(id).type;
+  return type == ash::TYPE_APP_SHORTCUT || type == ash::TYPE_BROWSER_SHORTCUT;
 }
 
 void ChromeLauncherControllerImpl::TogglePinned(ash::ShelfID id) {
@@ -419,12 +408,10 @@ void ChromeLauncherControllerImpl::TogglePinned(ash::ShelfID id) {
 }
 
 bool ChromeLauncherControllerImpl::IsPinnable(ash::ShelfID id) const {
-  int index = model_->ItemIndexByID(id);
-  if (index == -1)
+  if (model_->ItemIndexByID(id) < 0)
     return false;
 
-  ash::ShelfItemType type = model_->items()[index].type;
-  std::string app_id;
+  ash::ShelfItemType type = GetItem(id).type;
   return ((type == ash::TYPE_APP_SHORTCUT || type == ash::TYPE_APP) &&
           model_->GetShelfItemDelegate(id)->CanPin());
 }
@@ -465,8 +452,8 @@ void ChromeLauncherControllerImpl::Close(ash::ShelfID id) {
 }
 
 bool ChromeLauncherControllerImpl::IsOpen(ash::ShelfID id) {
-  const int index = model_->ItemIndexByID(id);
-  return index >= 0 && model_->items()[index].status != ash::STATUS_CLOSED;
+  return model_->ItemIndexByID(id) >= 0 &&
+         GetItem(id).status != ash::STATUS_CLOSED;
 }
 
 bool ChromeLauncherControllerImpl::IsPlatformApp(ash::ShelfID id) {
@@ -518,9 +505,9 @@ void ChromeLauncherControllerImpl::SetLauncherItemImage(
     ash::ShelfID shelf_id,
     const gfx::ImageSkia& image) {
   int index = model_->ItemIndexByID(shelf_id);
-  if (index == -1)
+  if (index < 0)
     return;
-  ash::ShelfItem item = model_->items()[index];
+  ash::ShelfItem item = GetItem(shelf_id);
   item.image = image;
   model_->Set(index, item);
 }
@@ -596,13 +583,12 @@ ash::ShelfID ChromeLauncherControllerImpl::GetShelfIDForWebContents(
 void ChromeLauncherControllerImpl::SetRefocusURLPatternForTest(
     ash::ShelfID id,
     const GURL& url) {
-  int index = model_->ItemIndexByID(id);
-  if (index == -1) {
+  if (model_->ItemIndexByID(id) < 0) {
     NOTREACHED() << "Invalid launcher id";
     return;
   }
 
-  ash::ShelfItemType type = model_->items()[index].type;
+  ash::ShelfItemType type = GetItem(id).type;
   if ((type == ash::TYPE_APP_SHORTCUT || type == ash::TYPE_APP) &&
       !IsPlatformApp(id)) {
     LauncherItemController* controller = GetLauncherItemController(id);
@@ -687,13 +673,9 @@ void ChromeLauncherControllerImpl::AdditionalUserAddedToSession(
 ChromeLauncherAppMenuItems ChromeLauncherControllerImpl::GetApplicationList(
     const ash::ShelfItem& item,
     int event_flags) {
-  // Make sure that there is a controller associated with the id and that the
-  // extension itself is a valid application and not a panel.
   LauncherItemController* controller = GetLauncherItemController(item.id);
-  if (!controller || !GetShelfIDForAppID(controller->app_id()))
-    return ChromeLauncherAppMenuItems();
-
-  return controller->GetApplicationList(event_flags);
+  return controller ? controller->GetApplicationList(event_flags)
+                    : ChromeLauncherAppMenuItems();
 }
 
 std::vector<content::WebContents*>
@@ -781,12 +763,9 @@ base::string16 ChromeLauncherControllerImpl::GetAppListTitle(
 
 BrowserShortcutLauncherItemController*
 ChromeLauncherControllerImpl::GetBrowserShortcutLauncherItemController() {
-  for (IDToItemControllerMap::iterator i = id_to_item_controller_map_.begin();
-       i != id_to_item_controller_map_.end(); ++i) {
-    int index = model_->ItemIndexByID(i->first);
-    const ash::ShelfItem& item = model_->items()[index];
-    if (item.type == ash::TYPE_BROWSER_SHORTCUT)
-      return static_cast<BrowserShortcutLauncherItemController*>(i->second);
+  for (const auto& pair : id_to_item_controller_map_) {
+    if (GetItem(pair.first).type == ash::TYPE_BROWSER_SHORTCUT)
+      return static_cast<BrowserShortcutLauncherItemController*>(pair.second);
   }
   NOTREACHED()
       << "There should be always be a BrowserShortcutLauncherItemController.";
@@ -944,10 +923,8 @@ void ChromeLauncherControllerImpl::PinAppWithID(const std::string& app_id) {
 bool ChromeLauncherControllerImpl::IsAppPinned(const std::string& app_id) {
   const std::string shelf_app_id =
       ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(app_id);
-  for (IDToItemControllerMap::const_iterator i =
-           id_to_item_controller_map_.begin();
-       i != id_to_item_controller_map_.end(); ++i) {
-    if (IsPinned(i->first) && i->second->app_id() == shelf_app_id)
+  for (const auto& pair : id_to_item_controller_map_) {
+    if (IsPinned(pair.first) && pair.second->app_id() == shelf_app_id)
       return true;
   }
   return false;
@@ -1050,7 +1027,7 @@ void ChromeLauncherControllerImpl::RestoreUnpinnedRunningApplicationOrder(
     if (shelf_id) {
       int app_index = model_->ItemIndexByID(shelf_id);
       DCHECK_GE(app_index, 0);
-      if (model_->items()[app_index].type == ash::TYPE_APP) {
+      if (GetItem(shelf_id).type == ash::TYPE_APP) {
         if (running_index != app_index)
           model_->Move(running_index, app_index);
         running_index++;
@@ -1117,7 +1094,7 @@ void ChromeLauncherControllerImpl::PinRunningAppInternal(
     int index,
     ash::ShelfID shelf_id) {
   int running_index = model_->ItemIndexByID(shelf_id);
-  ash::ShelfItem item = model_->items()[running_index];
+  ash::ShelfItem item = GetItem(shelf_id);
   DCHECK_EQ(item.type, ash::TYPE_APP);
   item.type = ash::TYPE_APP_SHORTCUT;
   model_->Set(running_index, item);