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

chrome/browser/tabs/tab_strip_model.cc

Index: chrome/browser/tabs/tab_strip_model.cc
diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index de8df7ff8a0a1c3618d3afb7c5f844aec280eaec..4667a7f5f662fbea79f8741683dcb5c052129b2d 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -158,10 +158,11 @@ void TabStripModel::InsertTabContentsAt(int index,
 
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                     TabInsertedAt(contents, index, active));
-
+  TabStripSelectionModel old_model;
+  old_model.Copy(selection_model_);
   if (active) {
     selection_model_.SetSelectedIndex(index);
-    NotifyTabSelectedIfChanged(selected_contents, index, false);
+    NotifyIfActiveOrSelectionChanged(selected_contents, false, old_model);
   }
 }
 
@@ -220,22 +221,21 @@ TabContentsWrapper* TabStripModel::DetachTabContentsAt(int index) {
     // a second pass.
     FOR_EACH_OBSERVER(TabStripModelObserver, observers_, TabStripEmpty());
   } else {
-    int old_active = active_index();
     selection_model_.DecrementFrom(index);
-    if (index == old_active) {
+    TabStripSelectionModel old_model;
+    old_model.Copy(selection_model_);
+    if (selection_model_.active() == TabStripSelectionModel::kUnselectedIndex) {

[dpapad, 2011/06/08 18:22:51]

It turns out that this condition is not correct, caused the
TabStripModelTest.InsertBefore to hang (fixed now).

In the process of fixing it, I noticed a very puzzling behavior:

Could you take a very quick look at TabStripSelectionModel::SetSelectedIndex and
SetSelectionFromAnchorTo. These two functions call each other, specifically if
you invoke SetSelectedIndex(-1) the program enters an infinite loop!! I suppose
that this is a clear logic error.

[sky, 2011/06/08 19:25:07]

Ick! I wanted SetSelectionFromAnchorTo to behave like SetSelectedIndex if there
is no anchor. That's why it calls out like that. And then I wanted
SetSelectedIndex to share the code in SetSelectionFromAnchorTo for the actual
work. But that obviously is problematic as you found.

Seems best to make SetSelectedIndex not use SetSelectionFromAnchorTo at all.

       if (!selection_model_.empty()) {
         // A selected tab was removed, but there is still something selected.
         // Move the active and anchor to the first selected index.
         selection_model_.set_active(selection_model_.selected_indices()[0]);
         selection_model_.set_anchor(selection_model_.active());
-        NotifyTabSelectedIfChanged(removed_contents, active_index(), false);
       } else {
         // The active tab was removed and nothing is selected. Reset the
         // selection and send out notification.
         selection_model_.SetSelectedIndex(next_selected_index);
-        NotifyTabSelectedIfChanged(removed_contents, next_selected_index,
-                                   false);
       }
+      NotifyIfActiveOrSelectionChanged(removed_contents, false, old_model);
     }
   }
   return removed_contents;
@@ -243,21 +243,11 @@ TabContentsWrapper* TabStripModel::DetachTabContentsAt(int index) {
 
 void TabStripModel::ActivateTabAt(int index, bool user_gesture) {
   DCHECK(ContainsIndex(index));
-  bool had_multi = selection_model_.selected_indices().size() > 1;
-  TabContentsWrapper* old_contents =
-      (active_index() == TabStripSelectionModel::kUnselectedIndex) ?
-      NULL : GetSelectedTabContents();
+  TabContentsWrapper* old_contents = GetSelectedTabContents();
+  TabStripSelectionModel old_model;
+  old_model.Copy(selection_model_);
   selection_model_.SetSelectedIndex(index);
-  TabContentsWrapper* new_contents = GetContentsAt(index);
-  if (old_contents != new_contents && old_contents) {
-    FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-                      TabDeactivated(old_contents));
-  }
-  if (old_contents != new_contents || had_multi) {
-    FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-                      ActiveTabChanged(old_contents, new_contents,
-                                       active_index(), user_gesture));
-  }
+  NotifyIfActiveOrSelectionChanged(old_contents, user_gesture, old_model);
 }
 
 void TabStripModel::MoveTabContentsAt(int index,
@@ -569,15 +559,18 @@ int TabStripModel::ConstrainInsertionIndex(int index, bool mini_tab) {
 
 void TabStripModel::ExtendSelectionTo(int index) {
   DCHECK(ContainsIndex(index));
-  int old_active = active_index();
+  TabContentsWrapper* old_contents = GetSelectedTabContents();
+  TabStripSelectionModel old_model;
+  old_model.Copy(selection_model());
   selection_model_.SetSelectionFromAnchorTo(index);
-  // This may not have resulted in a change, but we assume it did.
-  NotifyActiveTabChanged(old_active);
+  NotifyIfActiveOrSelectionChanged(old_contents, false, old_model);
 }
 
 void TabStripModel::ToggleSelectionAt(int index) {
   DCHECK(ContainsIndex(index));
-  int old_active = active_index();
+  TabContentsWrapper* old_contents = GetSelectedTabContents();
+  TabStripSelectionModel old_model;
+  old_model.Copy(selection_model());
   if (selection_model_.IsSelected(index)) {
     if (selection_model_.size() == 1) {
       // One tab must be selected and this tab is currently selected so we can't
@@ -593,13 +586,15 @@ void TabStripModel::ToggleSelectionAt(int index) {
     selection_model_.set_anchor(index);
     selection_model_.set_active(index);
   }
-  NotifyActiveTabChanged(old_active);
+  NotifyIfActiveOrSelectionChanged(old_contents, false, old_model);
 }
 
 void TabStripModel::AddSelectionFromAnchorTo(int index) {
-  int old_active = active_index();
+  TabContentsWrapper* old_contents = GetSelectedTabContents();
+  TabStripSelectionModel old_model;
+  old_model.Copy(selection_model());
   selection_model_.AddSelectionFromAnchorTo(index);
-  NotifyActiveTabChanged(old_active);
+  NotifyIfActiveOrSelectionChanged(old_contents, false, old_model);
 }
 
 bool TabStripModel::IsTabSelected(int index) const {
@@ -610,10 +605,11 @@ bool TabStripModel::IsTabSelected(int index) const {
 void TabStripModel::SetSelectionFromModel(
     const TabStripSelectionModel& source) {
   DCHECK_NE(TabStripSelectionModel::kUnselectedIndex, source.active());
-  int old_active_index = active_index();
+  TabContentsWrapper* old_contents = GetSelectedTabContents();
+  TabStripSelectionModel old_model;
+  old_model.Copy(selection_model());
   selection_model_.Copy(source);
-  // This may not have resulted in a change, but we assume it did.
-  NotifyActiveTabChanged(old_active_index);
+  NotifyIfActiveOrSelectionChanged(old_contents, false, old_model);
 }
 
 void TabStripModel::AddTabContents(TabContentsWrapper* contents,
@@ -1207,33 +1203,25 @@ TabContentsWrapper* TabStripModel::GetContentsAt(int index) const {
   return contents_data_.at(index)->contents;
 }
 
-void TabStripModel::NotifyTabSelectedIfChanged(TabContentsWrapper* old_contents,
-                                               int to_index,
-                                               bool user_gesture) {
-  TabContentsWrapper* new_contents = GetContentsAt(to_index);
-  if (old_contents == new_contents)
-    return;
-
-  TabContentsWrapper* last_selected_contents = old_contents;
-  if (last_selected_contents) {
+void TabStripModel::NotifyIfActiveOrSelectionChanged(
+    TabContentsWrapper* old_contents,
+    bool user_gesture,
+    const TabStripSelectionModel& old_model) {
+  TabContentsWrapper* new_contents = GetContentsAt(active_index());
+  if (old_contents != new_contents) {
+    TabContentsWrapper* last_selected_contents = old_contents;
+    if (last_selected_contents) {
+      FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
+                        TabDeactivated(last_selected_contents));
+    }
     FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-                      TabDeactivated(last_selected_contents));
+                      ActiveTabChanged(last_selected_contents, new_contents,
+                                       active_index(), user_gesture));
+  }
+  if (!selection_model().Equals(old_model)) {
+    FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
+                      TabSelectionChanged(old_model));
   }
-
-  FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-                    ActiveTabChanged(last_selected_contents, new_contents,
-                                     active_index(), user_gesture));
-}
-
-void TabStripModel::NotifyActiveTabChanged(int old_active_index) {
-  TabContentsWrapper* old_tab =
-      old_active_index == TabStripSelectionModel::kUnselectedIndex ?
-      NULL : GetTabContentsAt(old_active_index);
-  TabContentsWrapper* new_tab =
-      active_index() == TabStripSelectionModel::kUnselectedIndex ?
-      NULL : GetTabContentsAt(active_index());
-  FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-                    ActiveTabChanged(old_tab, new_tab, active_index(), true));
 }
 
 void TabStripModel::SelectRelativeTab(bool next) {