Handling browser item status in BrowserShortcutLauncherItemController

Move the role of handling browser item status from ChromeLauncherController
to BrowserShortcutLauncherItemController.

R=skuhne@chromium.org
BUG=NONE
TEST=browser_tests, unit_tests

Review URL: https://chromiumcodereview.appspot.com/23534022

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221823 0039d316-1c4b-4281-b951-d872f2087c98

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

@@ -7,6 +7,8 @@
 #include <vector>
 
 #include "ash/launcher/launcher.h"
+#include "ash/launcher/launcher_model.h"
+#include "ash/launcher/launcher_util.h"
 #include "ash/shell.h"
 #include "ash/wm/window_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -19,6 +21,7 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/ash_resources.h"
@@ -48,6 +51,53 @@ BrowserShortcutLauncherItemController::
     ~BrowserShortcutLauncherItemController() {
 }
 
+void BrowserShortcutLauncherItemController::UpdateBrowserItemState() {
+  // The shell will not be available for win7_aura unittests like
+  // ChromeLauncherControllerTest.BrowserMenuGeneration.
+  if (!ash::Shell::HasInstance())
+    return;
+
+  ash::LauncherModel* model = launcher_controller()->model();
+
+  // Determine the new browser's active state and change if necessary.
+  size_t browser_index = ash::launcher::GetBrowserItemIndex(*model);
+  DCHECK_GE(browser_index, 0u);
+  ash::LauncherItem browser_item = model->items()[browser_index];
+  ash::LauncherItemStatus browser_status = ash::STATUS_CLOSED;
+
+  aura::Window* window = ash::wm::GetActiveWindow();
+  if (window) {
+    // Check if the active browser / tab is a browser which is not an app,
+    // a windowed app, a popup or any other item which is not a browser of
+    // interest.
+    Browser* browser = chrome::FindBrowserWithWindow(window);
+    if (IsBrowserRepresentedInBrowserList(browser)) {
+      browser_status = ash::STATUS_ACTIVE;
+      // If an app is running in active window, browser item status cannot be
+      // active.
+      if (launcher_controller()->GetIDByWindow(window) != browser_item.id)
+        browser_status = ash::STATUS_RUNNING;
+    }
+  }
+
+  if (browser_status == ash::STATUS_CLOSED) {
+    const BrowserList* ash_browser_list =
+        BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH);
+    for (BrowserList::const_reverse_iterator it =
+             ash_browser_list->begin_last_active();
+         it != ash_browser_list->end_last_active() &&
+         browser_status == ash::STATUS_CLOSED; ++it) {
+      if (IsBrowserRepresentedInBrowserList(*it))
+        browser_status = ash::STATUS_RUNNING;
+    }
+  }
+
+  if (browser_status != browser_item.status) {
+    browser_item.status = browser_status;
+    model->Set(browser_index, browser_item);
+  }
+}
+
 string16 BrowserShortcutLauncherItemController::GetTitle() {
   return l10n_util::GetStringUTF16(IDS_PRODUCT_NAME);
 }
@@ -152,7 +202,7 @@ BrowserShortcutLauncherItemController::GetApplicationList(int event_flags) {
       continue;
     if (browser->is_type_tabbed())
       found_tabbed_browser = true;
-    else if (!launcher_controller()->IsBrowserRepresentedInBrowserList(browser))
+    else if (!IsBrowserRepresentedInBrowserList(browser))
       continue;
     TabStripModel* tab_strip = browser->tab_strip_model();
     if (tab_strip->active_index() == -1)
@@ -218,7 +268,7 @@ void BrowserShortcutLauncherItemController::ActivateOrAdvanceToNextBrowser() {
   for (BrowserList::const_iterator it =
            ash_browser_list->begin();
        it != ash_browser_list->end(); ++it) {
-    if (launcher_controller()->IsBrowserRepresentedInBrowserList(*it))
+    if (IsBrowserRepresentedInBrowserList(*it))
       items.push_back(*it);
   }
   // If there are no suitable browsers we create a new one.
@@ -249,7 +299,7 @@ void BrowserShortcutLauncherItemController::ActivateOrAdvanceToNextBrowser() {
                                           true,
                                           chrome::HOST_DESKTOP_TYPE_ASH);
       if (!browser ||
-          !launcher_controller()->IsBrowserRepresentedInBrowserList(browser))
+          !IsBrowserRepresentedInBrowserList(browser))
         browser = items[0];
     }
   }
@@ -257,3 +307,14 @@ void BrowserShortcutLauncherItemController::ActivateOrAdvanceToNextBrowser() {
   browser->window()->Show();
   browser->window()->Activate();
 }
+
+bool BrowserShortcutLauncherItemController::IsBrowserRepresentedInBrowserList(
+    Browser* browser) {
+  return (browser &&
+          (browser->is_type_tabbed() ||
+           !browser->is_app() ||
+           !browser->is_type_popup() ||
+           launcher_controller()->
+               GetLauncherIDForAppID(web_app::GetExtensionIdFromApplicationName(
+                   browser->app_name())) <= 0));
+}

chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h

@@ -27,6 +27,9 @@ class BrowserShortcutLauncherItemController : public LauncherItemController {
 
   virtual ~BrowserShortcutLauncherItemController();
 
+  // Updates the activation state of the Broswer item.
+  void UpdateBrowserItemState();
+
   // LauncherItemController overrides:
   virtual string16 GetTitle() OVERRIDE;
   virtual bool IsCurrentlyShownInWindow(aura::Window* window) const OVERRIDE;
@@ -55,6 +58,10 @@ class BrowserShortcutLauncherItemController : public LauncherItemController {
   // Activate a browser - or advance to the next one on the list.
   void ActivateOrAdvanceToNextBrowser();
 
+  // Returns true when the given |browser| is listed in the browser application
+  // list.
+  bool IsBrowserRepresentedInBrowserList(Browser* browser);
+
   Profile* profile_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowserShortcutLauncherItemController);

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

@@ -6,6 +6,7 @@
 
 #include "ash/shell.h"
 #include "ash/wm/window_util.h"
+#include "chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -65,19 +66,15 @@ BrowserStatusMonitor::~BrowserStatusMonitor() {
 void BrowserStatusMonitor::OnWindowActivated(aura::Window* gained_active,
                                              aura::Window* lost_active) {
   Browser* browser = chrome::FindBrowserWithWindow(lost_active);
-  content::WebContents* active_contents = NULL;
-
   if (browser) {
-    active_contents = browser->tab_strip_model()->GetActiveWebContents();
-    if (active_contents)
-      UpdateAppState(active_contents);
+    UpdateAppAndBrowserState(
+        browser->tab_strip_model()->GetActiveWebContents());
   }
 
   browser = chrome::FindBrowserWithWindow(gained_active);
   if (browser) {
-    active_contents = browser->tab_strip_model()->GetActiveWebContents();
-    if (active_contents)
-      UpdateAppState(active_contents);
+    UpdateAppAndBrowserState(
+        browser->tab_strip_model()->GetActiveWebContents());
   }
 }
 
@@ -108,7 +105,7 @@ void BrowserStatusMonitor::OnBrowserRemoved(Browser* browser) {
     launcher_controller_->UnlockV1AppWithID(browser_to_app_id_map_[browser]);
     browser_to_app_id_map_.erase(browser);
   }
-  launcher_controller_->UpdateBrowserItemStatus();
+  UpdateBrowserItemState();
 }
 
 void BrowserStatusMonitor::OnDisplayBoundsChanged(
@@ -147,29 +144,33 @@ void BrowserStatusMonitor::ActiveTabChanged(content::WebContents* old_contents,
   // Update immediately on a tab change.
   if (browser &&
       (TabStripModel::kNoTab !=
-           browser->tab_strip_model()->GetIndexOfWebContents(old_contents)))
-    UpdateAppState(old_contents);
+           browser->tab_strip_model()->GetIndexOfWebContents(old_contents))) {
+    launcher_controller_->UpdateAppState(
+        old_contents,
+        ChromeLauncherController::APP_STATE_INACTIVE);
+  }
 
-  UpdateAppState(new_contents);
+  UpdateAppAndBrowserState(new_contents);
 }
 
 void BrowserStatusMonitor::TabInsertedAt(content::WebContents* contents,
                                          int index,
                                          bool foreground) {
-  UpdateAppState(contents);
+  UpdateAppAndBrowserState(contents);
 }
 
 void BrowserStatusMonitor::TabDetachedAt(content::WebContents* contents,
                                          int index) {
   launcher_controller_->UpdateAppState(
       contents, ChromeLauncherController::APP_STATE_REMOVED);
+  UpdateBrowserItemState();
 }
 
 void BrowserStatusMonitor::TabChangedAt(
     content::WebContents* contents,
     int index,
     TabStripModelObserver::TabChangeType change_type) {
-  UpdateAppState(contents);
+  UpdateAppAndBrowserState(contents);
 }
 
 void BrowserStatusMonitor::TabReplacedAt(TabStripModel* tab_strip_model,
@@ -179,23 +180,29 @@ void BrowserStatusMonitor::TabReplacedAt(TabStripModel* tab_strip_model,
   launcher_controller_->UpdateAppState(
       old_contents,
       ChromeLauncherController::APP_STATE_REMOVED);
-  UpdateAppState(new_contents);
+  UpdateAppAndBrowserState(new_contents);
 }
 
-void BrowserStatusMonitor::UpdateAppState(content::WebContents* contents) {
-  if (!contents)
-    return;
-
-  ChromeLauncherController::AppState app_state =
-      ChromeLauncherController::APP_STATE_INACTIVE;
+void BrowserStatusMonitor::UpdateAppAndBrowserState(
+    content::WebContents* contents) {
+  if (contents) {
+    ChromeLauncherController::AppState app_state =
+        ChromeLauncherController::APP_STATE_INACTIVE;
+
+    Browser* browser = chrome::FindBrowserWithWebContents(contents);
+    if (browser->tab_strip_model()->GetActiveWebContents() == contents) {
+      if (browser->window()->IsActive())
+        app_state = ChromeLauncherController::APP_STATE_WINDOW_ACTIVE;
+      else
+        app_state = ChromeLauncherController::APP_STATE_ACTIVE;
+    }
 
-  Browser* browser = chrome::FindBrowserWithWebContents(contents);
-  if (browser->tab_strip_model()->GetActiveWebContents() == contents) {
-    if (browser->window()->IsActive())
-      app_state = ChromeLauncherController::APP_STATE_WINDOW_ACTIVE;
-    else
-      app_state = ChromeLauncherController::APP_STATE_ACTIVE;
+    launcher_controller_->UpdateAppState(contents, app_state);
   }
+  UpdateBrowserItemState();
+}
 
-  launcher_controller_->UpdateAppState(contents, app_state);
+void BrowserStatusMonitor::UpdateBrowserItemState() {
+  launcher_controller_->GetBrowserShortcutLauncherItemController()->
+      UpdateBrowserItemState();
 }

chrome/browser/ui/ash/launcher/browser_status_monitor.h

@@ -79,8 +79,12 @@ class BrowserStatusMonitor : public aura::client::ActivationChangeObserver,
  private:
   typedef std::map<Browser*, std::string> BrowserToAppIDMap;
 
-  // Update app state for |contents|.
-  void UpdateAppState(content::WebContents* contents);
+  // Update app state for |contents| and browser state.
+  void UpdateAppAndBrowserState(content::WebContents* contents);
+
+  // A shortcut to call the BrowserShortcutLauncherItemController's
+  // UpdateBrowserItemState().
+  void UpdateBrowserItemState();
 
   ChromeLauncherController* launcher_controller_;
 

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

@@ -348,11 +348,7 @@ void ChromeLauncherController::SetItemStatus(
     ash::LauncherItem item = model_->items()[index];
     item.status = status;
     model_->Set(index, item);
-
-    if (model_->items()[index].type == ash::TYPE_BROWSER_SHORTCUT)
-      return;
   }
-  UpdateBrowserItemStatus();
 }
 
 void ChromeLauncherController::SetItemController(
@@ -819,13 +815,6 @@ void ChromeLauncherController::UpdateAppState(content::WebContents* contents,
       RemoveTabFromRunningApp(contents, last_app_id);
   }
 
-  if (app_id.empty()) {
-    // Even if there is no application running, we should update the activation
-    // state of the associated browser.
-    UpdateBrowserItemStatus();
-    return;
-  }
-
   web_contents_to_app_id_[contents] = app_id;
 
   if (app_state == APP_STATE_REMOVED) {
@@ -855,7 +844,6 @@ void ChromeLauncherController::UpdateAppState(content::WebContents* contents,
           ash::STATUS_ACTIVE : ash::STATUS_RUNNING);
     }
   }
-  UpdateBrowserItemStatus();
 }
 
 void ChromeLauncherController::SetRefocusURLPatternForTest(ash::LauncherID id,
@@ -1233,55 +1221,6 @@ ash::LauncherID ChromeLauncherController::CreateAppShortcutLauncherItemWithType(
   return launcher_id;
 }
 
-void ChromeLauncherController::UpdateBrowserItemStatus() {
-  // This check needs for win7_aura. UpdateBrowserItemStatus() access Shell.
-  // Without this ChromeLauncherControllerTest.BrowserMenuGeneration test will
-  // fail.
-  if (!ash::Shell::HasInstance())
-    return;
-
-  // Determine the new browser's active state and change if necessary.
-  size_t browser_index = ash::launcher::GetBrowserItemIndex(*model_);
-  DCHECK_GE(browser_index, 0u);
-  ash::LauncherItem browser_item = model_->items()[browser_index];
-  ash::LauncherItemStatus browser_status = ash::STATUS_CLOSED;
-
-  aura::Window* window = ash::wm::GetActiveWindow();
-  if (window) {
-    // Check if the active browser / tab is a browser which is not an app,
-    // a windowed app, a popup or any other item which is not a browser of
-    // interest.
-    Browser* browser = chrome::FindBrowserWithWindow(window);
-    if (IsBrowserRepresentedInBrowserList(browser)) {
-      browser_status = ash::STATUS_ACTIVE;
-      const ash::LauncherItems& items = model_->items();
-      // If another launcher item has claimed to be active, we don't.
-      for (size_t i = 0;
-           i < items.size() && browser_status == ash::STATUS_ACTIVE; ++i) {
-        if (i != browser_index && items[i].status == ash::STATUS_ACTIVE)
-          browser_status = ash::STATUS_RUNNING;
-      }
-    }
-  }
-
-  if (browser_status == ash::STATUS_CLOSED) {
-    const BrowserList* ash_browser_list =
-        BrowserList::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH);
-    for (BrowserList::const_reverse_iterator it =
-             ash_browser_list->begin_last_active();
-         it != ash_browser_list->end_last_active() &&
-         browser_status == ash::STATUS_CLOSED; ++it) {
-      if (IsBrowserRepresentedInBrowserList(*it))
-        browser_status = ash::STATUS_RUNNING;
-    }
-  }
-
-  if (browser_status != browser_item.status) {
-    browser_item.status = browser_status;
-    model_->Set(browser_index, browser_item);
-  }
-}
-
 Profile* ChromeLauncherController::GetProfileForNewWindows() {
   return ProfileManager::GetDefaultProfileOrOffTheRecord();
 }
@@ -1553,30 +1492,21 @@ ChromeLauncherController::GetV1ApplicationsFromController(
   return app_controller->GetRunningApplications();
 }
 
-bool ChromeLauncherController::IsBrowserRepresentedInBrowserList(
-    Browser* browser) {
-  return (browser &&
-          (browser->is_type_tabbed() ||
-           !browser->is_app() ||
-           !browser->is_type_popup() ||
-           GetLauncherIDForAppID(web_app::GetExtensionIdFromApplicationName(
-               browser->app_name())) <= 0));
-}
-
-LauncherItemController*
+BrowserShortcutLauncherItemController*
 ChromeLauncherController::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::LauncherItem& item = model_->items()[index];
     if (item.type == ash::TYPE_BROWSER_SHORTCUT)
-      return i->second;
+      return static_cast<BrowserShortcutLauncherItemController*>(i->second);
   }
-  // LauncerItemController For Browser Shortcut must be existed. If it does not
-  // existe create it.
+  // Create a LauncherItemController for the Browser shortcut if it does not
+  // exist yet.
   ash::LauncherID id = CreateBrowserShortcutLauncherItem();
   DCHECK(id_to_item_controller_map_[id]);
-  return id_to_item_controller_map_[id];
+  return static_cast<BrowserShortcutLauncherItemController*>(
+      id_to_item_controller_map_[id]);
 }
 
 ash::LauncherID ChromeLauncherController::CreateBrowserShortcutLauncherItem() {

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

@@ -360,15 +360,9 @@ class ChromeLauncherController : public ash::LauncherDelegate,
   // If |web_contents| has not loaded, returns "Net Tab".
   string16 GetAppListTitle(content::WebContents* web_contents) const;
 
-  // Returns true when the given |browser| is listed in the browser application
-  // list.
-  bool IsBrowserRepresentedInBrowserList(Browser* browser);
-
   // Returns the LauncherItemController of BrowserShortcut.
-  LauncherItemController* GetBrowserShortcutLauncherItemController();
-
-  // Updates the activation state of the Broswer item.
-  void UpdateBrowserItemStatus();
+  BrowserShortcutLauncherItemController*
+      GetBrowserShortcutLauncherItemController();
 
  protected:
   // Creates a new app shortcut item and controller on the launcher at |index|.