Multi-tab: Adding new Notification when tab selection changes.

chrome/browser/tabs/tab_strip_model_unittest.cc

Index: chrome/browser/tabs/tab_strip_model_unittest.cc
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index 6643bbc5c07b81055fa6612add24ed8bcc20e5f0..4358659fb99bd19ce1117673acc139d5ee5c1ac0 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -270,7 +270,13 @@ class TabStripModelTest : public RenderViewHostTestHarness {
 
 class MockTabStripModelObserver : public TabStripModelObserver {
  public:
-  MockTabStripModelObserver() : empty_(true) {}
+  MockTabStripModelObserver() : empty_(true),
+                                log_tab_selection_changed_(false),
+                                model_(NULL) {}
+  explicit MockTabStripModelObserver(TabStripModel* model) :
+      empty_(true),
+      log_tab_selection_changed_(false),
+      model_(model) {}
   ~MockTabStripModelObserver() {
     STLDeleteContainerPointers(states_.begin(), states_.end());
   }
@@ -279,6 +285,7 @@ class MockTabStripModelObserver : public TabStripModelObserver {
     INSERT,
     CLOSE,
     DETACH,
+    ACTIVATE,
     SELECT,
     MOVE,
     CHANGE,
@@ -348,11 +355,22 @@ class MockTabStripModelObserver : public TabStripModelObserver {
                                 TabContentsWrapper* new_contents,
                                 int index,
                                 bool user_gesture) {
-    State* s = new State(new_contents, index, SELECT);
+    State* s = new State(new_contents, index, ACTIVATE);
     s->src_contents = old_contents;
     s->user_gesture = user_gesture;
     states_.push_back(s);
   }
+  virtual void TabSelectionChanged(const TabStripSelectionModel& old_model) {
+    if (!log_tab_selection_changed())
+      return;
+
+    State* s = new State(model()->GetActiveTabContents(),
+                         model()->active_index(),
+                         SELECT);
+    s->src_contents = model()->GetTabContentsAt(old_model.active());
+    s->src_index = old_model.active();
+    states_.push_back(s);
+  }
   virtual void TabMoved(
       TabContentsWrapper* contents, int from_index, int to_index) {
     State* s = new State(contents, to_index, MOVE);
@@ -392,12 +410,22 @@ class MockTabStripModelObserver : public TabStripModelObserver {
     states_.clear();
   }
 
+  void set_log_tab_selection_changed(bool flag) {
+    log_tab_selection_changed_ = flag;
+  }
+
   bool empty() const { return empty_; }
+  bool log_tab_selection_changed() const { return log_tab_selection_changed_; }
+  TabStripModel* model() { return model_; }
 
  private:
   std::vector<State*> states_;
 
   bool empty_;
+  // TODO(dpapad): Remove this variable and update TestBasicAPI so that it takes
+  // into account TabSelectionChanged notifications.
+  bool log_tab_selection_changed_;
+  TabStripModel* model_;
 
   DISALLOW_COPY_AND_ASSIGN(MockTabStripModelObserver);
 };
@@ -405,7 +433,7 @@ class MockTabStripModelObserver : public TabStripModelObserver {
 TEST_F(TabStripModelTest, TestBasicAPI) {
   TabStripDummyDelegate delegate(NULL);
   TabStripModel tabstrip(&delegate, profile());
-  MockTabStripModelObserver observer;
+  MockTabStripModelObserver observer(&tabstrip);
   tabstrip.AddObserver(&observer);
 
   EXPECT_TRUE(tabstrip.empty());
@@ -428,7 +456,7 @@ TEST_F(TabStripModelTest, TestBasicAPI) {
     State s1(contents1, 0, MockTabStripModelObserver::INSERT);
     s1.foreground = true;
     EXPECT_TRUE(observer.StateEquals(0, s1));
-    State s2(contents1, 0, MockTabStripModelObserver::SELECT);
+    State s2(contents1, 0, MockTabStripModelObserver::ACTIVATE);
     s2.src_contents = NULL;
     EXPECT_TRUE(observer.StateEquals(1, s2));
     observer.ClearStates();
@@ -444,7 +472,7 @@ TEST_F(TabStripModelTest, TestBasicAPI) {
     State s1(contents2, 1, MockTabStripModelObserver::INSERT);
     s1.foreground = true;
     EXPECT_TRUE(observer.StateEquals(0, s1));
-    State s2(contents2, 1, MockTabStripModelObserver::SELECT);
+    State s2(contents2, 1, MockTabStripModelObserver::ACTIVATE);
     s2.src_contents = contents1;
     EXPECT_TRUE(observer.StateEquals(1, s2));
     observer.ClearStates();
@@ -467,7 +495,7 @@ TEST_F(TabStripModelTest, TestBasicAPI) {
   {
     tabstrip.ActivateTabAt(2, true);
     EXPECT_EQ(1, observer.GetStateCount());
-    State s1(contents3, 2, MockTabStripModelObserver::SELECT);
+    State s1(contents3, 2, MockTabStripModelObserver::ACTIVATE);
     s1.src_contents = contents2;
     s1.user_gesture = true;
     EXPECT_TRUE(observer.StateEquals(0, s1));
@@ -483,14 +511,14 @@ TEST_F(TabStripModelTest, TestBasicAPI) {
     EXPECT_EQ(4, observer.GetStateCount());
     State s1(detached, 2, MockTabStripModelObserver::DETACH);
     EXPECT_TRUE(observer.StateEquals(0, s1));
-    State s2(contents2, 1, MockTabStripModelObserver::SELECT);
+    State s2(contents2, 1, MockTabStripModelObserver::ACTIVATE);
     s2.src_contents = contents3;
     s2.user_gesture = false;
     EXPECT_TRUE(observer.StateEquals(1, s2));
     State s3(detached, 2, MockTabStripModelObserver::INSERT);
     s3.foreground = true;
     EXPECT_TRUE(observer.StateEquals(2, s3));
-    State s4(detached, 2, MockTabStripModelObserver::SELECT);
+    State s4(detached, 2, MockTabStripModelObserver::ACTIVATE);
     s4.src_contents = contents2;
     s4.user_gesture = false;
     EXPECT_TRUE(observer.StateEquals(3, s4));
@@ -515,7 +543,7 @@ TEST_F(TabStripModelTest, TestBasicAPI) {
     EXPECT_TRUE(observer.StateEquals(0, s1));
     State s2(contents3, 2, MockTabStripModelObserver::DETACH);
     EXPECT_TRUE(observer.StateEquals(1, s2));
-    State s3(contents2, 1, MockTabStripModelObserver::SELECT);
+    State s3(contents2, 1, MockTabStripModelObserver::ACTIVATE);
     s3.src_contents = contents3;
     s3.user_gesture = false;
     EXPECT_TRUE(observer.StateEquals(2, s3));
@@ -1689,7 +1717,7 @@ TEST_F(TabStripModelTest, NavigationForgettingDoesntAffectNewTab) {
 TEST_F(TabStripModelTest, FastShutdown) {
   TabStripDummyDelegate delegate(NULL);
   TabStripModel tabstrip(&delegate, profile());
-  MockTabStripModelObserver observer;
+  MockTabStripModelObserver observer(&tabstrip);
   tabstrip.AddObserver(&observer);
 
   EXPECT_TRUE(tabstrip.empty());
@@ -1750,7 +1778,7 @@ TEST_F(TabStripModelTest, FastShutdown) {
 TEST_F(TabStripModelTest, Apps) {
   TabStripDummyDelegate delegate(NULL);
   TabStripModel tabstrip(&delegate, profile());
-  MockTabStripModelObserver observer;
+  MockTabStripModelObserver observer(&tabstrip);
   tabstrip.AddObserver(&observer);
 
   EXPECT_TRUE(tabstrip.empty());
@@ -1876,7 +1904,7 @@ TEST_F(TabStripModelTest, Apps) {
 TEST_F(TabStripModelTest, Pinning) {
   TabStripDummyDelegate delegate(NULL);
   TabStripModel tabstrip(&delegate, profile());
-  MockTabStripModelObserver observer;
+  MockTabStripModelObserver observer(&tabstrip);
   tabstrip.AddObserver(&observer);
 
   EXPECT_TRUE(tabstrip.empty());
@@ -2062,7 +2090,7 @@ TEST_F(TabStripModelTest, ReplaceSendsSelected) {
   EXPECT_TRUE(tabstrip_observer.StateEquals(0, state));
 
   // And the second for selected.
-  state = State(new_contents, 0, MockTabStripModelObserver::SELECT);
+  state = State(new_contents, 0, MockTabStripModelObserver::ACTIVATE);
   state.src_contents = first_contents;
   EXPECT_TRUE(tabstrip_observer.StateEquals(1, state));
 
@@ -2181,9 +2209,71 @@ TEST_F(TabStripModelTest, CloseSelectedTabs) {
   strip.CloseAllTabs();
 }
 
+TEST_F(TabStripModelTest, MultipleSelection) {
+  TabStripDummyDelegate delegate(NULL);
+  TabStripModel strip(&delegate, profile());
+  MockTabStripModelObserver observer(&strip);
+  TabContentsWrapper* contents0 = CreateTabContents();
+  TabContentsWrapper* contents1 = CreateTabContents();
+  TabContentsWrapper* contents2 = CreateTabContents();
+  TabContentsWrapper* contents3 = CreateTabContents();
+  strip.AppendTabContents(contents0, false);
+  strip.AppendTabContents(contents1, false);
+  strip.AppendTabContents(contents2, false);
+  strip.AppendTabContents(contents3, false);
+  observer.set_log_tab_selection_changed(true);
+  strip.AddObserver(&observer);
+
+  // Selection and active tab change.
+  strip.ActivateTabAt(3, true);
+  ASSERT_EQ(2, observer.GetStateCount());
+  ASSERT_EQ(observer.GetStateAt(0)->action,
+            MockTabStripModelObserver::ACTIVATE);
+  MockTabStripModelObserver::State s1(contents3, 3,
+      MockTabStripModelObserver::SELECT);
+  EXPECT_TRUE(observer.StateEquals(1, s1));
+
+  // Adding all tabs to selection, active tab is now at 0.
+  strip.ExtendSelectionTo(0);
+  ASSERT_EQ(4, observer.GetStateCount());
+  ASSERT_EQ(observer.GetStateAt(2)->action,
+            MockTabStripModelObserver::ACTIVATE);
+  MockTabStripModelObserver::State s2(contents0, 0,
+      MockTabStripModelObserver::SELECT);
+  s2.src_contents = contents3;
+  s2.src_index = 3;
+  EXPECT_TRUE(observer.StateEquals(3, s2));
+
+  // Closing one of the selected tabs, not the active one.
+  strip.CloseTabContentsAt(1, TabStripModel::CLOSE_NONE);
+  EXPECT_EQ(3, strip.count());
+  ASSERT_EQ(6, observer.GetStateCount());
+  ASSERT_EQ(observer.GetStateAt(4)->action,

[dpapad, 2011/06/10 17:23:55]

It seems that closing a selected tab does not result in a TabSelectionChanged
notification being sent, that needs to be fixed, right?

[sky, 2011/06/10 17:56:04]

Yes, I think its a good idea to force the notification to be sent even though
old == new.

[dpapad, 2011/06/10 18:21:37]

No, sending a TabSelectionChanged notification does not depend on "old == new"
but on 
"if (!selection_model().Equals(old_model)) {...}" (see tab_strip_model.cc:1221) 
which is true in this case. So I assume that currently
NotifyIfActiveOrSelectionChagned() is not being called from anywhere within
CloseTabContentsAt(). Adding such a call should solve this problem without
changing the if conditions.

[sky, 2011/06/10 19:25:24]

Maybe I'm confused. Lets say you have {0} active and [1-4] selected. If you
detach tab 4 then old_model becomes {0}, [1-3]. This is the same as the new
model, right?

[dpapad, 2011/06/13 21:36:18]

You are right that the selection seems to not have changed (because we store the
old_model after calling DecrementFrom()), but this is not the only condition
that causes the TabStripSelectionModel::Equals() to return false and therefore
trigger the notification. See my other comment in TabStripModel for more detail.

+            MockTabStripModelObserver::CLOSE);
+  ASSERT_EQ(observer.GetStateAt(5)->action,
+            MockTabStripModelObserver::DETACH);
+
+  // Active tab is at 0, deselecting all but the active tab.
+  strip.ToggleSelectionAt(1);
+  strip.ToggleSelectionAt(2);
+  ASSERT_EQ(8, observer.GetStateCount());
+  ASSERT_EQ(observer.GetStateAt(6)->action,
+            MockTabStripModelObserver::SELECT);
+  ASSERT_EQ(observer.GetStateAt(7)->action,
+            MockTabStripModelObserver::SELECT);
+
+  // Attempting to deselect the only selected tab, it is ignored (no
+  // notifications being sent) and tab at 0 remains selected and active.
+  strip.ToggleSelectionAt(0);
+  ASSERT_EQ(8, observer.GetStateCount());
+
+  strip.RemoveObserver(&observer);
+  strip.CloseAllTabs();
+}
+
 // Verifies that if we change the selection from a multi selection to a single
 // selection, but not in a way that changes the selected_index that
-// ActiveTabChanged is still invoked.
+// TabSelectionChanged is invoked.
 TEST_F(TabStripModelTest, MultipleToSingle) {
   TabStripDummyDelegate delegate(NULL);
   TabStripModel strip(&delegate, profile());
@@ -2194,7 +2284,8 @@ TEST_F(TabStripModelTest, MultipleToSingle) {
   strip.ToggleSelectionAt(0);
   strip.ToggleSelectionAt(1);
 
-  MockTabStripModelObserver observer;
+  MockTabStripModelObserver observer(&strip);
+  observer.set_log_tab_selection_changed(true);
   strip.AddObserver(&observer);
   // This changes the selection (0 is no longer selected) but the selected_index
   // still remains at 1.
@@ -2203,7 +2294,8 @@ TEST_F(TabStripModelTest, MultipleToSingle) {
   MockTabStripModelObserver::State s(
       contents2, 1, MockTabStripModelObserver::SELECT);
   s.src_contents = contents2;
-  s.user_gesture = true;
+  s.src_index = 1;
+  s.user_gesture = false;
   EXPECT_TRUE(observer.StateEquals(0, s));
   strip.RemoveObserver(&observer);
   strip.CloseAllTabs();