Handling browser item status in BrowserShortcutLauncherItemController

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 06b01786a24db345b750b5516f4c18d9b7c64080..058dcd3da9b1d71f9e88d88bed3c69d462af750c 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 "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"
@@ -71,6 +72,7 @@ void BrowserStatusMonitor::OnWindowActivated(aura::Window* gained_active,
     active_contents = browser->tab_strip_model()->GetActiveWebContents();
     if (active_contents)
       UpdateAppState(active_contents);
+    GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();

[Mr4D (OOO till 08-26), 2013/09/04 15:12:33]

Coincidentally the "UpdateAppState" goes hand in hand with
"GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus()" - so why
not calling UpdateBrowserItemStatus from "UpdateAppState"? and remove the many
calls you have here?

[simonhong_, 2013/09/04 17:08:31]

Yes, it can call by UpdateAppState() and it has same effect.
But, I thought the code that update browser item status should exist in BSLIC
because BSLIC handles launcher browser item status.

[Mr4D (OOO till 08-26), 2013/09/05 23:30:13]

You misunderstood - I was wondering why UpdateAppState(..) isn't at the same
time calling the UpdateBrowserItemStatus. This was maybe a bad example, but when
you look into line 153, 160, 167, 175, 186 you will notice that the two calls
always come in pairs...

[simonhong_, 2013/09/06 00:40:02]

Ah.. your concern is why UpdateAppState() and UpdateBrowserItemStautus() aren't
always called both.

First, they are called in pair when an event is occurred from TabStripModel.
In line 75, 83, If browser has active web contents, both are called in pair.
I think 105 is not needed. When browser is created browser item and app item
status can be updated by TabInsertedAt().
In line 115, When browser is removed, app's item status is already updated by
TabDetachedAt(). So in here, only browser item state update is needed.

[Mr4D (OOO till 08-26), 2013/09/06 16:11:12]

For line 75 / 83: An if .. else .. could resolve that.

For all others: You can remove them by adding the line after line 204 and
include a bracket around 190, throwing in another call). Then rename the
function UpdateAppState to UpdateAppAndBrowserState.

Thanks for removing 105 again! (somehow I didn't realize why I didn't have it
there before).

[simonhong_, 2013/09/06 18:52:57]

Done.

   }
 
   browser = chrome::FindBrowserWithWindow(gained_active);
@@ -78,6 +80,7 @@ void BrowserStatusMonitor::OnWindowActivated(aura::Window* gained_active,
     active_contents = browser->tab_strip_model()->GetActiveWebContents();
     if (active_contents)
       UpdateAppState(active_contents);
+    GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
   }
 }
 
@@ -99,6 +102,7 @@ void BrowserStatusMonitor::OnBrowserAdded(Browser* browser) {
       launcher_controller_->LockV1AppWithID(app_id);
     }
   }
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::OnBrowserRemoved(Browser* browser) {
@@ -108,7 +112,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();
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::OnDisplayBoundsChanged(
@@ -146,18 +150,21 @@ void BrowserStatusMonitor::ActiveTabChanged(content::WebContents* old_contents,
     UpdateAppState(old_contents);
 
   UpdateAppState(new_contents);
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::TabInsertedAt(content::WebContents* contents,
                                          int index,
                                          bool foreground) {
   UpdateAppState(contents);
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::TabDetachedAt(content::WebContents* contents,
                                          int index) {
   launcher_controller_->UpdateAppState(
       contents, ChromeLauncherController::APP_STATE_REMOVED);
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::TabChangedAt(
@@ -165,6 +172,7 @@ void BrowserStatusMonitor::TabChangedAt(
     int index,
     TabStripModelObserver::TabChangeType change_type) {
   UpdateAppState(contents);
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::TabReplacedAt(TabStripModel* tab_strip_model,
@@ -175,6 +183,7 @@ void BrowserStatusMonitor::TabReplacedAt(TabStripModel* tab_strip_model,
       old_contents,
       ChromeLauncherController::APP_STATE_REMOVED);
   UpdateAppState(new_contents);
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::UpdateAppState(content::WebContents* contents) {
@@ -194,3 +203,8 @@ void BrowserStatusMonitor::UpdateAppState(content::WebContents* contents) {
 
   launcher_controller_->UpdateAppState(contents, app_state);
 }
+
+BrowserShortcutLauncherItemController*
+BrowserStatusMonitor::GetBrowserShortcutLauncherItemController() {

[Mr4D (OOO till 08-26), 2013/09/04 15:12:33]

You need this function only to get access to the "UpdateBrowserItemStatus"
function - so why not getting rid of this function and add a
"UpdateBrowserItemStatus()" instead which gets the CLC and calls the function?

[simonhong_, 2013/09/04 17:08:31]

Same reason as above comments.
This class is just monitoring class of browser status.
So I thought locating UpdateBrowserItemStatus() in BSLIC looks better.
If you want to call UpdateBrowserItemStatus() in UpdateAppState(), I'll follow
you.

[Mr4D (OOO till 08-26), 2013/09/05 23:30:13]

You have misunderstood:

void BrowserStatusMonitor::UpdateBrowserItemStatus() {
 
launcher_controller_->GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
}

No one else requires a pointer to BrowserShortcutLauncherItemController.

[simonhong_, 2013/09/06 00:40:02]

Ah.. I made ugly code..
Done.

+  return launcher_controller_->GetBrowserShortcutLauncherItemController();
+}