Added notion of currently active app / browser

chrome/browser/ui/views/ash/launcher/launcher_updater.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_ASH_LAUNCHER_LAUNCHER_UPDATER_H_
 #define CHROME_BROWSER_UI_VIEWS_ASH_LAUNCHER_LAUNCHER_UPDATER_H_
 #pragma once
 
 #include <map>
 #include <string>
@@ skipping 14 matching lines @@
 }
 
 namespace aura {
 class Window;
 }
 
 // LauncherUpdater is responsible for keeping the launcher representation of a
 // window up to date as the tab strip changes.
 class LauncherUpdater : public TabStripModelObserver {
  public:
+  // This API is to be used as part of testing only.
+  class TestApi {

[sky, 2012/03/13 17:14:42]

I hate seeing the first public thing in a class a set of test only methods, and
this code gets compiled into the release build. Can you instead add _test
methods instead, or if you really like the class approach put it in a separate
file that is only build for tests?

[DaveMoore, 2012/03/13 23:33:56]

I understand the issue about it being the first thing, I could put it lower but
the style guide says that nested classes come first. Wouldn't _test methods get
compiled for release as well, or is there some magic that keeps them from being
compiled too? An extra class w/ no virtual methods isn't any worse than some
methods is it?

I could put it in a separate file and make it a friend class. I like that it
lives in the same place as the code it deeply interacts with so it's more likely
to be kept in sync.

This is not the first instance of this pattern in our code (and they haven't all
come from me). Would you expect all of them to be changed as well?

On 2012/03/13 17:14:42, sky wrote:

+   public:
+    TestApi(LauncherUpdater* launcher_updater)
+        : launcher_updater_(launcher_updater) {}
+    virtual ~TestApi() {}
+
+    // Returns the launcher id for |tab| if it's an app otherwise returns the
+    // id for the browser itself.
+    ash::LauncherID GetLauncherID(TabContentsWrapper* tab) {
+      return launcher_updater_->GetLauncherID(tab);
+    }
+
+    // Returns the launcher id for the browser window.
+    ash::LauncherID item_id() const { return launcher_updater_->item_id_; }
+
+   private:
+    LauncherUpdater* launcher_updater_;
+  };
+
   enum Type {
     TYPE_APP,
     TYPE_PANEL,
     TYPE_TABBED
   };
 
   LauncherUpdater(aura::Window* window,
                   TabStripModel* tab_model,
                   ChromeLauncherDelegate* launcher_delegate,
                   Type type,
                   const std::string& app_id);
   virtual ~LauncherUpdater();
 
   // Sets up this LauncherUpdater.
   void Init();
 
   // Creates and returns a new LauncherUpdater for |browser|. This returns
   // NULL if a LauncherUpdater is not needed for the specified browser.
   static LauncherUpdater* Create(Browser* browser);
 
   aura::Window* window() { return window_; }
 
   TabStripModel* tab_model() { return tab_model_; }
 
   Type type() const { return type_; }
 
   TabContentsWrapper* GetTab(ash::LauncherID id);
 
   // TabStripModel overrides:
+  void ActivationChanged(TabContentsWrapper* tab, bool active);

[sky, 2012/03/13 17:14:42]

Don't group this with TabStripModel overrides.

Also, since this is similarly named to ActivateTabChanged and it takes
TabContentsWrapper I thought this meant the activation state of the tab changed.
Since LauncherUpdater knows about the model, can you remove the
TabContentsWrapper argument and instead name this something like
BrowserActivationStateChanged.

[DaveMoore, 2012/03/13 23:33:56]

Done.

   virtual void ActiveTabChanged(TabContentsWrapper* old_contents,
                                 TabContentsWrapper* new_contents,
                                 int index,
                                 bool user_gesture) OVERRIDE;
   virtual void TabChangedAt(
       TabContentsWrapper* tab,
       int index,
       TabStripModelObserver::TabChangeType change_type) OVERRIDE;
   virtual void TabInsertedAt(TabContentsWrapper* contents,
                              int index,
@@ skipping 39 matching lines @@
   void RegisterAppItem(ash::LauncherID id, TabContentsWrapper* tab);
 
   // Creates a tabbed launcher item.
   void CreateTabbedItem();
 
   // Returns true if this LauncherUpdater created the launcher item with the
   // specified id. Returns true if it did. If the id corresponds to an app tab,
   // |tab| is set to the TabContentsWrapper for the app tab.
   bool ContainsID(ash::LauncherID id, TabContentsWrapper** tab);
 
+  // Returns the launcher id for |tab| if it's an app otherwise returns the
+  // id for the browser itself.
+  ash::LauncherID GetLauncherID(TabContentsWrapper* tab);
+
   ash::LauncherModel* launcher_model();
 
   // Browser window we're in.
   aura::Window* window_;
 
   TabStripModel* tab_model_;
 
   ChromeLauncherDelegate* launcher_delegate_;
 
   // Whether this corresponds to an app or tabbed browser.
   const Type type_;
 
   const std::string app_id_;
 
   // This is one of three possible values:
   // . If type_ == TYPE_APP, this is the ID of the app item.
   // . If type_ == TYPE_TABBED and all the tabs are app tabs this is -1.
   // . Otherwise this is the id of the TYPE_TABBED item.
   ash::LauncherID item_id_;
 
   // Used for any app tabs.
   AppTabMap app_map_;
 
   DISALLOW_COPY_AND_ASSIGN(LauncherUpdater);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_ASH_LAUNCHER_LAUNCHER_UPDATER_H_