Use ChromeLauncherControllerImpl::GetItem; cleanup.

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

 // Copyright 2013 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 "chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h"
 
 #include <stddef.h>
 
 #include <vector>
 
@@ skipping 246 matching lines @@
 ChromeLauncherControllerImpl::~ChromeLauncherControllerImpl() {
   // Reset the BrowserStatusMonitor as it has a weak pointer to this.
   browser_status_monitor_.reset();
 
   // Reset the app window controllers here since it has a weak pointer to this.
   app_window_controllers_.clear();
 
   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 &&
         model_->items()[index].type != ash::TYPE_BROWSER_SHORTCUT)
       model_->RemoveItemAt(index);
   }
 
   // Release all profile dependent resources.
   ReleaseProfile();
 
@@ skipping 23 matching lines @@
 
 const ash::ShelfItem& ChromeLauncherControllerImpl::GetItem(
     ash::ShelfID id) const {
   const int index = model_->ItemIndexByID(id);
   DCHECK_GE(index, 0);
   return model_->items()[index];
 }
 
 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);
   }
 }
 
 void ChromeLauncherControllerImpl::SetItemController(
     ash::ShelfID id,
     LauncherItemController* controller) {
   CHECK(controller);
   IDToItemControllerMap::iterator iter = id_to_item_controller_map_.find(id);
   CHECK(iter != id_to_item_controller_map_.end());
   controller->set_shelf_id(id);
@@ skipping 16 matching lines @@
     iter->second->set_shelf_id(id);
     // Existing controller is destroyed and replaced by registering again.
     SetShelfItemDelegate(id, iter->second);
   } else {
     LauncherItemClosed(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;
   }
 
   SyncPinPosition(id);
 }
 
 void ChromeLauncherControllerImpl::Unpin(ash::ShelfID id) {
   UnpinAndUpdatePrefs(id, true /* update_prefs */);
 }
 
 void ChromeLauncherControllerImpl::UnpinAndUpdatePrefs(ash::ShelfID id,
                                                        bool update_prefs) {
   LauncherItemController* controller = GetLauncherItemController(id);
   CHECK(controller);
 
   if (update_prefs) {
     ash::launcher::RemovePinPosition(
         profile(), ash::launcher::AppLauncherId(GetAppIDForShelfID(id),
                                                 GetLaunchIDForShelfID(id)));
   }
 
   if (GetItem(id).status != ash::STATUS_CLOSED || controller->locked())
     UnpinRunningAppInternal(model_->ItemIndexByID(id));
   else
     LauncherItemClosed(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) {
   if (!HasShelfIDToAppIDMapping(id))
     return;  // May happen if item closed with menu open.
 
   if (IsPinned(id))
     Unpin(id);
   else
     Pin(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());
 }
 
 void ChromeLauncherControllerImpl::LockV1AppWithID(const std::string& app_id) {
   ash::ShelfID id = GetShelfIDForAppID(app_id);
   if (id == ash::kInvalidShelfID) {
     CreateAppShortcutLauncherItemWithType(ash::launcher::AppLauncherId(app_id),
                                           model_->item_count(), ash::TYPE_APP);
     id = GetShelfIDForAppID(app_id);
@@ skipping 20 matching lines @@
 }
 
 void ChromeLauncherControllerImpl::Close(ash::ShelfID id) {
   ash::ShelfItemDelegate* delegate = model_->GetShelfItemDelegate(id);
   if (!delegate)
     return;  // May happen if menu closed.
   delegate->Close();
 }
 
 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) {
   if (!HasShelfIDToAppIDMapping(id))
     return false;
 
   std::string app_id = GetAppIDForShelfID(id);
   const Extension* extension = GetExtensionForAppID(app_id, profile());
   // An extension can be synced / updated at any time and therefore not be
   // available.
@@ skipping 31 matching lines @@
     return extensions::LAUNCH_TYPE_DEFAULT;
 
   return extensions::GetLaunchType(extensions::ExtensionPrefs::Get(profile()),
                                    extension);
 }
 
 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);
 }
 
 void ChromeLauncherControllerImpl::SetLaunchType(
     ash::ShelfID id,
     extensions::LaunchType launch_type) {
   LauncherItemController* controller = GetLauncherItemController(id);
   if (!controller)
     return;
@@ skipping 55 matching lines @@
     int browser_index = model_->GetItemIndexForType(ash::TYPE_BROWSER_SHORTCUT);
     return model_->items()[browser_index].id;
   }
 
   return id;
 }
 
 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);
     DCHECK(controller);
     AppShortcutLauncherItemController* app_controller =
         static_cast<AppShortcutLauncherItemController*>(controller);
     app_controller->set_refocus_url(url);
   } else {
     NOTREACHED() << "Invalid launcher type";
   }
@@ skipping 64 matching lines @@
 void ChromeLauncherControllerImpl::AdditionalUserAddedToSession(
     Profile* profile) {
   // Switch the running applications to the new user.
   for (auto& controller : app_window_controllers_)
     controller->AdditionalUserAddedToSession(profile);
 }
 
 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*>
 ChromeLauncherControllerImpl::GetV1ApplicationsFromAppId(
     const std::string& app_id) {
   ash::ShelfID id = GetShelfIDForAppID(app_id);
   // If there is no such item pinned to the launcher, no menu gets created.
   if (id != ash::kInvalidShelfID &&
       GetItem(id).type == ash::TYPE_APP_SHORTCUT) {
     LauncherItemController* controller = GetLauncherItemController(id);
@@ skipping 67 matching lines @@
     const extensions::Extension* extension =
         GetExtensionForAppID(app_id, profile());
     if (extension)
       return base::UTF8ToUTF16(extension->name());
   }
   return l10n_util::GetStringUTF16(IDS_NEW_TAB_TITLE);
 }
 
 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.";
   return nullptr;
 }
 
 LauncherItemController* ChromeLauncherControllerImpl::GetLauncherItemController(
     const ash::ShelfID id) {
   if (!HasShelfIDToAppIDMapping(id))
     return nullptr;
@@ skipping 137 matching lines @@
   if (GetPinnableForAppID(shelf_app_id, profile()) ==
       AppListControllerDelegate::PIN_EDITABLE)
     DoPinAppWithID(shelf_app_id);
   else
     NOTREACHED();
 }
 
 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;
 }
 
 void ChromeLauncherControllerImpl::UnpinAppWithID(const std::string& app_id) {
   const std::string shelf_app_id =
       ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(app_id);
   if (GetPinnableForAppID(shelf_app_id, profile()) ==
       AppListControllerDelegate::PIN_EDITABLE)
@@ skipping 82 matching lines @@
     return;
 
   // Find the first insertion point for running applications.
   int running_index = model_->FirstRunningAppIndex();
   for (RunningAppListIds::iterator app_id = app_id_list->second.begin();
        app_id != app_id_list->second.end(); ++app_id) {
     ash::ShelfID shelf_id = GetShelfIDForAppID(*app_id);
     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++;
       }
     }
   }
 }
 
 ash::ShelfID
 ChromeLauncherControllerImpl::CreateAppShortcutLauncherItemWithType(
@@ skipping 46 matching lines @@
                                                     bool update_prefs) {
   ash::ShelfID shelf_id = GetShelfIDForAppID(app_id);
   if (shelf_id && IsPinned(shelf_id))
     UnpinAndUpdatePrefs(shelf_id, update_prefs);
 }
 
 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);
   // The |ShelfModel|'s weight system might reposition the item to a
   // new index, so we get the index again.
   running_index = model_->ItemIndexByID(shelf_id);
   if (running_index < index)
     --index;
   if (running_index != index)
     model_->Move(running_index, index);
@@ skipping 399 matching lines @@
     LauncherItemController* controller = GetLauncherItemController(item.id);
     if (!controller || controller->image_set_by_controller())
       continue;
     item.image = image;
     if (arc_deferred_launcher_)
       arc_deferred_launcher_->MaybeApplySpinningEffect(id, &item.image);
     model_->Set(index, item);
     // It's possible we're waiting on more than one item, so don't break.
   }
 }