Reduce calling count of UpdateAppState()

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

Index: chrome/browser/ui/ash/launcher/browser_status_monitor.cc
diff --git a/chrome/browser/ui/ash/launcher/browser_status_monitor.cc b/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
index c9965ed1b419acd26631e71fa2b4b000c4a1f1a7..42e4db866049859b0c33ce978547b17ed764be57 100644
--- a/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
+++ b/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
@@ -6,6 +6,7 @@
 
 #include "ash/shell.h"
 #include "ash/wm/window_util.h"
+#include "base/stl_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"
@@ -15,12 +16,35 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "content/public/browser/web_contents.h"
-#include "content/public/browser/web_contents_view.h"
 #include "ui/aura/client/activation_client.h"
 #include "ui/aura/root_window.h"
 #include "ui/aura/window.h"
 #include "ui/gfx/screen.h"
 
+BrowserStatusMonitor::LocalWebContentsObserver::LocalWebContentsObserver(
+    content::WebContents* contents,
+    BrowserStatusMonitor* monitor)
+    : content::WebContentsObserver(contents),
+      monitor_(monitor) {
+}
+
+BrowserStatusMonitor::LocalWebContentsObserver::~LocalWebContentsObserver() {
+}
+
+void BrowserStatusMonitor::LocalWebContentsObserver::DidNavigateMainFrame(
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
+  ChromeLauncherController::AppState state =
+      ChromeLauncherController::APP_STATE_ACTIVE;
+  if (browser->window()->IsActive() &&
+      browser->tab_strip_model()->GetActiveWebContents() == web_contents())
+    state = ChromeLauncherController::APP_STATE_WINDOW_ACTIVE;
+
+  monitor_->UpdateAppItemState(web_contents(), state);
+  monitor_->UpdateBrowserItemState();
+}
+
 BrowserStatusMonitor::BrowserStatusMonitor(
     ChromeLauncherController* launcher_controller)
     : launcher_controller_(launcher_controller),
@@ -62,21 +86,53 @@ BrowserStatusMonitor::~BrowserStatusMonitor() {
        i != browser_list->end(); ++i) {
     OnBrowserRemoved(*i);
   }
+
+  STLDeleteContainerPairSecondPointers(webcontents_to_observer_map_.begin(),
+                                       webcontents_to_observer_map_.end());
+}
+
+void BrowserStatusMonitor::UpdateAppItemState(
+    content::WebContents* contents,
+    ChromeLauncherController::AppState app_state) {
+  DCHECK(contents);
+  launcher_controller_->UpdateAppState(contents, app_state);
+}
+
+void BrowserStatusMonitor::UpdateBrowserItemState() {
+  launcher_controller_->GetBrowserShortcutLauncherItemController()->
+      UpdateBrowserItemState();
 }
 
 void BrowserStatusMonitor::OnWindowActivated(aura::Window* gained_active,
                                              aura::Window* lost_active) {
-  Browser* browser = chrome::FindBrowserWithWindow(lost_active);
-  if (browser) {
-    UpdateAppAndBrowserState(
-        browser->tab_strip_model()->GetActiveWebContents());
+  Browser* browser = NULL;
+  content::WebContents* contents = NULL;
+  // Update active webcontents's app item state of |lost_active|, if existed.
+  if (lost_active) {
+    browser = chrome::FindBrowserWithWindow(lost_active);
+    if (browser)
+      contents = browser->tab_strip_model()->GetActiveWebContents();
+    if (contents) {
+      UpdateAppItemState(
+          contents,
+          ChromeLauncherController::APP_STATE_INACTIVE);
+    }
   }
 
-  browser = chrome::FindBrowserWithWindow(gained_active);
-  if (browser) {
-    UpdateAppAndBrowserState(
-        browser->tab_strip_model()->GetActiveWebContents());
+  // Update active webcontents's app item state of |gained_active|, if existed.
+  if (gained_active) {
+    browser = chrome::FindBrowserWithWindow(gained_active);
+    if (browser)
+      contents = browser->tab_strip_model()->GetActiveWebContents();
+    if (contents) {

[Mr4D (OOO till 08-26), 2013/09/18 01:43:49]

since contents was already set in 114 - you could come here even though browser
is NULL.

[simonhong_, 2013/09/18 13:38:38]

Done.

+      UpdateAppItemState(
+          contents,
+          ChromeLauncherController::APP_STATE_WINDOW_ACTIVE);
+    }
   }
+
+  if (contents)

[Mr4D (OOO till 08-26), 2013/09/18 01:43:49]

I am not sure which contents you are referring to here? I suppose either content
from 114 or contents from 126?  It cannot be both.

[simonhong_, 2013/09/18 13:38:38]

My purpose was updating browser item state only if one of them is not NULL or
both are not NULL.
Fixed.

+    UpdateBrowserItemState();
 }
 
 void BrowserStatusMonitor::OnWindowDestroyed(aura::Window* window) {
@@ -145,77 +201,65 @@ void BrowserStatusMonitor::ActiveTabChanged(content::WebContents* old_contents,
                                             int index,
                                             int reason) {
   Browser* browser = NULL;
-  if (old_contents)
-    browser = chrome::FindBrowserWithWebContents(old_contents);
+  // Use |new_contents|. |old_contents| could be NULL.

[Mr4D (OOO till 08-26), 2013/09/18 01:43:49]

Just checking: Is there a guarantee that new_contents is not NULL? Maybe you
want to add a DCHECK?

[simonhong_, 2013/09/18 13:38:38]

I checked that |new_contents| is set to WebContents of current active index in
TabStripModel::NotifyIfActiveTabChanged().
So I think it will be always not NULL.
DCHECK() is added instead of if ().

+  if (new_contents)
+    browser = chrome::FindBrowserWithWebContents(new_contents);
 
   if (browser && browser->host_desktop_type() != chrome::HOST_DESKTOP_TYPE_ASH)
     return;
 
+  ChromeLauncherController::AppState state =
+      ChromeLauncherController::APP_STATE_INACTIVE;
+
   // Update immediately on a tab change.
   if (browser &&

[Mr4D (OOO till 08-26), 2013/09/18 01:43:49]

maybe you want to test also for old_contents == NULL here?

[simonhong_, 2013/09/18 13:38:38]

Done.

       (TabStripModel::kNoTab !=
-           browser->tab_strip_model()->GetIndexOfWebContents(old_contents))) {
-    launcher_controller_->UpdateAppState(
-        old_contents,
-        ChromeLauncherController::APP_STATE_INACTIVE);
-  }
+           browser->tab_strip_model()->GetIndexOfWebContents(old_contents)))
+    launcher_controller_->UpdateAppState(old_contents, state);
 
-  UpdateAppAndBrowserState(new_contents);
+  if (new_contents) {
+    state = browser->window()->IsActive() ?
+        ChromeLauncherController::APP_STATE_WINDOW_ACTIVE :
+        ChromeLauncherController::APP_STATE_ACTIVE;
+    UpdateAppItemState(new_contents, state);
+    UpdateBrowserItemState();
+  }
 }
 
 void BrowserStatusMonitor::TabInsertedAt(content::WebContents* contents,
                                          int index,
                                          bool foreground) {
-  UpdateAppAndBrowserState(contents);
-}
+  if (webcontents_to_observer_map_.find(contents) ==
+          webcontents_to_observer_map_.end()) {
+    webcontents_to_observer_map_[contents] =
+        new LocalWebContentsObserver(contents, this);
+  }
 

[Mr4D (OOO till 08-26), 2013/09/18 01:43:49]

same what I commented on in 264 is valid here. You might have a browser with an
app running. Then you insert a new content tab which will raise the browser item
from APP_STATE_INACTIVE to something more active.

[simonhong_, 2013/09/18 13:38:38]

When an app tab is running, browser item state is already running state.
When a new tab inserted into this browser, its state is updated to running
state. If this tab becomes active, the new tab's item and browser item state
will be updated by ActiveTabChanged().

-void BrowserStatusMonitor::TabDetachedAt(content::WebContents* contents,
-                                         int index) {
+  // There are three cases for tab inserting.
+  //   1) New tab is inserted as an active tab.
+  //   2) Tab is dragged and dropped.
+  //   3) New tab is inserted not as an active tab.
+  // For 1) and 2), This app tab's item status will be updated active state by
+  // ActiveTabChanged().
+  // For 3), This app tab's item status is running state.
+  // Because of this reason, |APP_STATE_INACTIVE| is set to all tab inserting.

[Mr4D (OOO till 08-26), 2013/09/18 01:43:49]

Instead of the comment above: what about 
// An inserted tab is not active - ActiveTabChanged will be called to activate.
We initialize therefore with |APP_STATE_INACTIVE|.

[simonhong_, 2013/09/18 13:38:38]

Done.

   launcher_controller_->UpdateAppState(
-      contents, ChromeLauncherController::APP_STATE_REMOVED);
-  UpdateBrowserItemState();
+      contents,
+      ChromeLauncherController::APP_STATE_INACTIVE);
+  // We don't need to call UpdateBrowserItemState(). For 1) and 2), browser item
+  // will be updated by following ActiveTabChanged(). For 3), browser item will
+  // be by other tab's state change.

[Mr4D (OOO till 08-26), 2013/09/18 01:43:49]

And why should any tab's state change if it only get inserted without getting
activated?

[simonhong_, 2013/09/18 13:38:38]

If we don't update the state of inserted tab, all inactive tab's item state is
closed state on startup. They should be updated to running state.
I found one case on startup phase that tab is inserted and not become an active
tab.
When user select the startup option("Continue where I left off" or "Open a
specific page or set of pages"), many tabs can be created on startup.
In this case ActiveTabChanged() is called only once just for active tab with
old_contents(NULL). That means if we update all inactive tabs state when it is
inserted, its state will be closed state.

 }
 
-void BrowserStatusMonitor::TabChangedAt(
-    content::WebContents* contents,
-    int index,
-    TabStripModelObserver::TabChangeType change_type) {
-  UpdateAppAndBrowserState(contents);
-}
+void BrowserStatusMonitor::TabClosingAt(TabStripModel* tab_strip_mode,
+                                        content::WebContents* contents,
+                                        int index) {
+  DCHECK(webcontents_to_observer_map_.find(contents) !=
+          webcontents_to_observer_map_.end());
+  delete webcontents_to_observer_map_[contents];
+  webcontents_to_observer_map_.erase(contents);
 
-void BrowserStatusMonitor::TabReplacedAt(TabStripModel* tab_strip_model,
-                                         content::WebContents* old_contents,
-                                         content::WebContents* new_contents,
-                                         int index) {
   launcher_controller_->UpdateAppState(
-      old_contents,
+      contents,
       ChromeLauncherController::APP_STATE_REMOVED);

[Mr4D (OOO till 08-26), 2013/09/18 01:43:49]

You can remove a browser tab which is not an app and all which is then left is
an app. When that happens the browserItemState should go inactive and thus the
Browser Item State needs updating.

Why did you remove it?

[simonhong_, 2013/09/18 13:38:38]

When a tab is closing, ActiveTabChanged() is also called after TabClosingAt() if
it was not the only one tab in the tab strip.
Browser item and next active app tab state can be updated by ActiveTabChanged().
Also when a tab is the only one, browser item state can be updated by
OnBrowserRemoved().

-  UpdateAppAndBrowserState(new_contents);
-}
-
-void BrowserStatusMonitor::UpdateAppAndBrowserState(
-    content::WebContents* contents) {
-  if (contents) {
-    ChromeLauncherController::AppState app_state =
-        ChromeLauncherController::APP_STATE_INACTIVE;
-
-    Browser* browser = chrome::FindBrowserWithWebContents(contents);
-    DCHECK(browser);
-    if (browser->host_desktop_type() != chrome::HOST_DESKTOP_TYPE_ASH)
-      return;
-    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();
-}
-
-void BrowserStatusMonitor::UpdateBrowserItemState() {
-  launcher_controller_->GetBrowserShortcutLauncherItemController()->
-      UpdateBrowserItemState();
 }