Handling browser item status in BrowserShortcutLauncherItemController

chrome/browser/ui/ash/launcher/browser_status_monitor.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/browser_status_monitor.h"
 
 #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"
 #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 "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
 #include "ui/aura/client/activation_client.h"
@@ skipping 45 matching lines @@
 
 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);
+    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);
   if (browser) {
     active_contents = browser->tab_strip_model()->GetActiveWebContents();
     if (active_contents)
       UpdateAppState(active_contents);
+    GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
   }
 }
 
 void BrowserStatusMonitor::OnWindowDestroyed(aura::Window* window) {
   // Remove RootWindow and its ActivationClient from observed list.
   observed_root_windows_.Remove(window);
   observed_activation_clients_.Remove(aura::client::GetActivationClient(
       static_cast<aura::RootWindow*>(window)));
 }
 
 void BrowserStatusMonitor::OnBrowserAdded(Browser* browser) {
   browser->tab_strip_model()->AddObserver(this);
 
   if (browser->is_type_popup() && browser->is_app()) {
     std::string app_id =
         web_app::GetExtensionIdFromApplicationName(browser->app_name());
     if (!app_id.empty()) {
       browser_to_app_id_map_[browser] = app_id;
       launcher_controller_->LockV1AppWithID(app_id);
     }
   }
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::OnBrowserRemoved(Browser* browser) {
   browser->tab_strip_model()->RemoveObserver(this);
 
   if (browser_to_app_id_map_.find(browser) != browser_to_app_id_map_.end()) {
     launcher_controller_->UnlockV1AppWithID(browser_to_app_id_map_[browser]);
     browser_to_app_id_map_.erase(browser);
   }
-  launcher_controller_->UpdateBrowserItemStatus();
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::OnDisplayBoundsChanged(
     const gfx::Display& display) {
   // Do nothing here.
 }
 
 void BrowserStatusMonitor::OnDisplayAdded(const gfx::Display& new_display) {
   // Add a new RootWindow and its ActivationClient to observed list.
   aura::RootWindow* root_window = ash::Shell::GetInstance()->
@@ skipping 17 matching lines @@
   if (old_contents)
     browser = chrome::FindBrowserWithWebContents(old_contents);
 
   // Update immediately on a tab change.
   if (browser &&
       (TabStripModel::kNoTab !=
            browser->tab_strip_model()->GetIndexOfWebContents(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(
     content::WebContents* contents,
     int index,
     TabStripModelObserver::TabChangeType change_type) {
   UpdateAppState(contents);
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::TabReplacedAt(TabStripModel* tab_strip_model,
                                          content::WebContents* old_contents,
                                          content::WebContents* new_contents,
                                          int index) {
   launcher_controller_->UpdateAppState(
       old_contents,
       ChromeLauncherController::APP_STATE_REMOVED);
   UpdateAppState(new_contents);
+  GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
 }
 
 void BrowserStatusMonitor::UpdateAppState(content::WebContents* contents) {
   if (!contents)
     return;
 
   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;
   }
 
   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();
+}