Simplifying launcher handling by removing an unnecessary data structure and some functions

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

Index: chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
index 7dd041107b0d4baca8e89f43074dd030524da2d9..fe7d3633fded1d51eee800f1f047a19f89383404 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
@@ -920,31 +920,6 @@ void ChromeLauncherController::ToggleShelfAutoHideBehavior(
   return;
 }
 
-void ChromeLauncherController::RemoveTabFromRunningApp(
-    WebContents* tab,
-    const std::string& app_id) {
-  web_contents_to_app_id_.erase(tab);
-  // BrowserShortcutLauncherItemController::UpdateBrowserItemState() will update
-  // the state when no application is associated with the tab.
-  if (app_id.empty())
-    return;
-
-  AppIDToWebContentsListMap::iterator i_app_id =
-      app_id_to_web_contents_list_.find(app_id);
-  if (i_app_id != app_id_to_web_contents_list_.end()) {
-    WebContentsList* tab_list = &i_app_id->second;
-    tab_list->remove(tab);
-    ash::ShelfItemStatus status = ash::STATUS_RUNNING;
-    if (tab_list->empty()) {
-      app_id_to_web_contents_list_.erase(i_app_id);
-      status = ash::STATUS_CLOSED;
-    }
-    ash::ShelfID id = GetShelfIDForAppID(app_id);
-    if (id)
-      SetItemStatus(id, status);
-  }
-}
-
 void ChromeLauncherController::UpdateAppState(content::WebContents* contents,
                                               AppState app_state) {
   std::string app_id = app_tab_helper_->GetAppID(contents);
@@ -958,43 +933,24 @@ void ChromeLauncherController::UpdateAppState(content::WebContents* contents,
   // remove it from the previous app.
   if (web_contents_to_app_id_.find(contents) != web_contents_to_app_id_.end()) {
     std::string last_app_id = web_contents_to_app_id_[contents];
-    if (last_app_id != app_id)
-      RemoveTabFromRunningApp(contents, last_app_id);
+    if (last_app_id != app_id) {
+      ash::ShelfID id = GetShelfIDForAppID(last_app_id);
+      if (id) {
+        // We remove the content before checking existence with GetAppState.

[sky, 2014/03/26 21:59:35]

Again, document why. This comment just documents code, not why order matters
here.

[Mr4D (OOO till 08-26), 2014/03/27 14:50:54]

I do not see it this way. It says that it removes the content before GetAppState
gets called which would otherwise find it as active. Sure the reader needs to
think a little bit.. I add a few more words. Done.

+        web_contents_to_app_id_.erase(contents);
+        SetItemStatus(id, GetAppState(last_app_id));
+      }
+    }
   }
 
   web_contents_to_app_id_[contents] = app_id;

[sky, 2014/03/26 21:59:35]

Seems silly to have a remove 2 lines later. How about an else for the if on 949.

[Mr4D (OOO till 08-26), 2014/03/27 14:50:54]

Done.

 
-  if (app_state == APP_STATE_REMOVED) {
-    // The tab has gone away.
-    RemoveTabFromRunningApp(contents, app_id);
-  } else if (!app_id.empty()) {
-    WebContentsList& tab_list(app_id_to_web_contents_list_[app_id]);
-    WebContentsList::const_iterator i_tab =
-        std::find(tab_list.begin(), tab_list.end(), contents);
-
-    if (i_tab == tab_list.end())
-      tab_list.push_back(contents);
-
-    if (app_state == APP_STATE_INACTIVE || app_state == APP_STATE_ACTIVE) {
-      if (i_tab != tab_list.begin()) {
-        // Going to running state, but wasn't the front tab, indicating that a
-        // new tab has already become active.
-        return;
-      }
-    }
-
-    if (app_state == APP_STATE_ACTIVE || app_state == APP_STATE_WINDOW_ACTIVE) {
-      tab_list.remove(contents);
-      tab_list.push_front(contents);
-    }
+  if (app_state == APP_STATE_REMOVED)
+    web_contents_to_app_id_.erase(contents);
 
-    ash::ShelfID id = GetShelfIDForAppID(app_id);
-    if (id) {
-      // If the window is active, mark the app as active.
-      SetItemStatus(id, app_state == APP_STATE_WINDOW_ACTIVE ?
-          ash::STATUS_ACTIVE : ash::STATUS_RUNNING);
-    }
-  }
+  ash::ShelfID id = GetShelfIDForAppID(app_id);
+  if (id)
+    SetItemStatus(id, GetAppState(app_id));
 }
 
 ash::ShelfID ChromeLauncherController::GetShelfIDForWebContents(
@@ -1719,35 +1675,24 @@ void ChromeLauncherController::SetShelfBehaviorsFromPrefs() {
   SetShelfAlignmentFromPrefs();
 }
 
-WebContents* ChromeLauncherController::GetLastActiveWebContents(
-    const std::string& app_id) {
-  AppIDToWebContentsListMap::iterator i =
-      app_id_to_web_contents_list_.find(app_id);
-  if (i == app_id_to_web_contents_list_.end())
-    return NULL;
-
-  // There are many crash records (crbug.com/341250) which indicate that the
-  // app_id_to_web_contents_list_ contains deleted content entries - so there
-  // must be a way that the content does not get properly updated. To fix
-  // M33 and M34 we filter out the invalid items here, but this should be
-  // addressed by a later patch correctly. Looking at the code however, the
-  // real culprit is possibly BrowserStatusMonitor::UpdateAppItemState which
-  // does not call "UpdateAppState(.., APP_STATE_REMOVED)" because due to a
-  // Browser::SwapTabContent operation it isn't able to get the browser. I
-  // think that the real patch is to call anyway when APP_STATE_REMOVED is
-  // requested, but for a backport that seems risky.
-  WebContentsList* list = &i->second;
-  while (!list->empty()) {
-    WebContents* contents = *list->begin();
-    if (chrome::FindBrowserWithWebContents(contents))
-      return contents;
-    list->erase(list->begin());
-    // This might not be necessary, but since we do not know why the lists
-    // diverged we also erase it since it cannot be correct either.
-    web_contents_to_app_id_.erase(contents);
+ash::ShelfItemStatus ChromeLauncherController::GetAppState(
+    const::std::string& app_id) {
+  ash::ShelfItemStatus status = ash::STATUS_CLOSED;
+  WebContentsToAppIDMap::iterator it = web_contents_to_app_id_.begin();

[sky, 2014/03/26 21:59:35]

nit: for loop?

[Mr4D (OOO till 08-26), 2014/03/27 14:50:54]

Done.

+  while (it != web_contents_to_app_id_.end()) {
+    if (it->second == app_id) {
+      Browser* browser = chrome::FindBrowserWithWebContents(it->first);
+      DCHECK(browser);

[sky, 2014/03/26 21:59:35]

Are you sure this DCHECK is valid here? Seems to me there are a bunch of invalid
assummptions in BrowserStatusMonitor that could lead to improve cleanup. I
suppose that is separate from this patch though.

[Mr4D (OOO till 08-26), 2014/03/27 14:50:54]

This DCHECK was added to identify an ilegal state which was the reason for this
cleanup: a page got added but never removed. I think that it is a good test to
have, but it could use a comment to point this out. - Done.

For BrowserStatusMonitor: I looked in the past over that module several times -
and will do so again.

+      if (browser->window()->IsActive()) {
+        return browser->tab_strip_model()->GetActiveWebContents() == it->first ?
+            ash::STATUS_ACTIVE : ash::STATUS_RUNNING;
+      } else {
+        status = ash::STATUS_RUNNING;
+      }
+    }
+    ++it;
   }
-  app_id_to_web_contents_list_.erase(app_id);
-  return NULL;
+  return status;
 }
 
 ash::ShelfID ChromeLauncherController::InsertAppLauncherItem(
@@ -1766,15 +1711,10 @@ ash::ShelfID ChromeLauncherController::InsertAppLauncherItem(
   item.type = shelf_item_type;
   item.image = extensions::IconsInfo::GetDefaultAppIcon();
 
-  WebContents* active_tab = GetLastActiveWebContents(app_id);
-  if (active_tab) {
-    Browser* browser = chrome::FindBrowserWithWebContents(active_tab);
-    DCHECK(browser);
-    if (browser->window()->IsActive())
-      status = ash::STATUS_ACTIVE;
-    else
-      status = ash::STATUS_RUNNING;
-  }
+  ash::ShelfItemStatus new_state = GetAppState(app_id);
+  if (new_state != ash::STATUS_CLOSED)
+    status = new_state;
+
   item.status = status;
 
   model_->AddAt(index, item);