Added notion of currently active app / browser

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

Index: chrome/browser/ui/views/ash/launcher/launcher_updater.h
diff --git a/chrome/browser/ui/views/ash/launcher/launcher_updater.h b/chrome/browser/ui/views/ash/launcher/launcher_updater.h
index 2e0d4c2570d04bcd594dc91b274fb0a52dc25583..0bc061cff40a39dbaf5a499ff4a65fec4edb9924 100644
--- a/chrome/browser/ui/views/ash/launcher/launcher_updater.h
+++ b/chrome/browser/ui/views/ash/launcher/launcher_updater.h
@@ -32,6 +32,26 @@ class Window;
 // 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,
@@ -61,6 +81,7 @@ class LauncherUpdater : public TabStripModelObserver {
   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,
@@ -120,6 +141,10 @@ class LauncherUpdater : public TabStripModelObserver {
   // |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.