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

chrome/browser/ui/ash/launcher/chrome_launcher_controller.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.h"
 
 #include <vector>
 
 #include "ash/ash_switches.h"
 #include "ash/desktop_background/desktop_background_controller.h"
@@ skipping 902 matching lines @@
 void ChromeLauncherController::ToggleShelfAutoHideBehavior(
     aura::Window* root_window) {
   ash::ShelfAutoHideBehavior behavior = GetShelfAutoHideBehavior(root_window) ==
       ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS ?
           ash::SHELF_AUTO_HIDE_BEHAVIOR_NEVER :
           ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS;
   SetShelfAutoHideBehaviorPrefs(behavior, root_window);
   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);
 
   // Check if the gMail app is loaded and it matches the given content.
   // This special treatment is needed to address crbug.com/234268.
   if (app_id.empty() && ContentCanBeHandledByGmailApp(contents))
     app_id = kGmailAppId;
 
   // Check the old |app_id| for a tab. If the contents has changed we need to
   // 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 (app_state == APP_STATE_REMOVED)
+    web_contents_to_app_id_.erase(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);
-    }
-
-    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(
     content::WebContents* contents) {
   DCHECK(contents);
 
   std::string app_id = app_tab_helper_->GetAppID(contents);
 
   if (app_id.empty() && ContentCanBeHandledByGmailApp(contents))
     app_id = kGmailAppId;
@@ skipping 704 matching lines @@
       alignment = ash::SHELF_ALIGNMENT_TOP;
     ash::Shell::GetInstance()->SetShelfAlignment(alignment, *iter);
   }
 }
 
 void ChromeLauncherController::SetShelfBehaviorsFromPrefs() {
   SetShelfAutoHideBehaviorFromPrefs();
   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(
     LauncherItemController* controller,
     const std::string& app_id,
     ash::ShelfItemStatus status,
     int index,
     ash::ShelfItemType shelf_item_type) {
   ash::ShelfID id = model_->next_id();
   CHECK(!HasItemController(id));
   CHECK(controller);
   id_to_item_controller_map_[id] = controller;
   controller->set_shelf_id(id);
 
   ash::ShelfItem item;
   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);
 
   app_icon_loader_->FetchImage(app_id);
 
   SetShelfItemDelegate(id, controller);
 
   return id;
 }
@@ skipping 270 matching lines @@
 }
 
 void ChromeLauncherController::ReleaseProfile() {
   if (app_sync_ui_state_)
     app_sync_ui_state_->RemoveObserver(this);
 
   PrefServiceSyncable::FromProfile(profile_)->RemoveObserver(this);
 
   pref_change_registrar_.RemoveAll();
 }