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 ea3977e2999eb562da80c0ac1da047155999f80e..40ad80044a114e3a18ce4dc762d6fdb34230e621 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -158,11 +158,11 @@ void TabStripModel::InsertTabContentsAt(int index,
 
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                     TabInsertedAt(contents, index, active));
-
-  if (active) {
+  TabStripSelectionModel old_model;
+  old_model.Copy(selection_model_);
+  if (active)
     selection_model_.SetSelectedIndex(index);
-    NotifyTabSelectedIfChanged(selected_contents, index, false);
-  }
+  NotifyIfActiveOrSelectionChanged(old_model, false);

[sky, 2011/06/06 21:01:14]

Can 165 be moved into the if block?

[dpapad, 2011/06/06 22:17:11]

Done. Putting it in the if block seems reasonable.

 }
 
 TabContentsWrapper* TabStripModel::ReplaceTabContentsAt(
@@ -220,22 +220,21 @@ TabContentsWrapper* TabStripModel::DetachTabContentsAt(int index) {
     // a second pass.
     FOR_EACH_OBSERVER(TabStripModelObserver, observers_, TabStripEmpty());
   } else {
-    int old_active = active_index();
+    TabStripSelectionModel old_model;
+    old_model.Copy(selection_model_);

[sky, 2011/06/06 21:01:14]

Don't you want to copy the model after decrementing? In the insert code you copy
after incrementing the model.

[dpapad, 2011/06/06 22:17:11]

Copying the model after decrementing, results in no tab being active when the
active tab is closed.

[sky, 2011/06/06 22:42:16]

Did you update 226 as well?

[dpapad, 2011/06/07 20:19:56]

I moved lines 223-224 right after 225 and the behavior was as described.

     selection_model_.DecrementFrom(index);
-    if (index == old_active) {
+    if (index == old_model.active()) {
       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(old_model, false);

[sky, 2011/06/06 21:01:14]

Won't this end up passing the wrong contents to the delegate since the remove
contents is no longer in the model?

[dpapad, 2011/06/06 22:17:11]

It will pass indices that might no longer exist to the observers. Is that
necessarily wrong? For example what should it pass if the last tab is closed
while being active?

[sky, 2011/06/06 22:42:16]

The problem is NotifyIfActiveOrSelectionChanges digs out the previously selected
tabcontents using the old selection model with the updated vector. It's roughly
something like:

contents_data_.erase(index_to_remove);
TabContents* old_tab = GetTabContentsAt(index_to_remove)
ActiveTabChangedAt(old_tab);

GetTabContentsAt is invoked after the remove and is going to return the wrong
thing.
I think you need to special case delete code and not use the general notify
methods.

[dpapad, 2011/06/07 20:19:56]

Done. I changed the notify functions I had added and reused the existing
function NotifyTabSelectedIfChanged, now named as NotifyIfActiveTabChanged, see
latest patch. Storing the old active TabCotnentsWrapper* solves the problem you
just described and it seems to work both for code that is adding and removing
tabs.

     }
   }
   return removed_contents;
@@ -243,7 +242,8 @@ 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;
+  TabStripSelectionModel old_model;
+  old_model.Copy(selection_model_);
   TabContentsWrapper* old_contents =
       (active_index() == TabStripSelectionModel::kUnselectedIndex) ?
       NULL : GetSelectedTabContents();
@@ -253,11 +253,7 @@ void TabStripModel::ActivateTabAt(int index, bool user_gesture) {
     FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                       TabDeselected(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_model, user_gesture);
 }
 
 void TabStripModel::MoveTabContentsAt(int index,
@@ -569,15 +565,16 @@ int TabStripModel::ConstrainInsertionIndex(int index, bool mini_tab) {
 
 void TabStripModel::ExtendSelectionTo(int index) {
   DCHECK(ContainsIndex(index));
-  int old_active = active_index();
+  TabStripSelectionModel old_selection_model;
+  old_selection_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_selection_model, true);
 }
 
 void TabStripModel::ToggleSelectionAt(int index) {
   DCHECK(ContainsIndex(index));
-  int old_active = active_index();
+  TabStripSelectionModel old_selection_model;
+  old_selection_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 +590,14 @@ void TabStripModel::ToggleSelectionAt(int index) {
     selection_model_.set_anchor(index);
     selection_model_.set_active(index);
   }
-  NotifyActiveTabChanged(old_active);
+  NotifyIfActiveOrSelectionChanged(old_selection_model, true);
 }
 
 void TabStripModel::AddSelectionFromAnchorTo(int index) {
-  int old_active = active_index();
+  TabStripSelectionModel old_selection_model;
+  old_selection_model.Copy(selection_model());
   selection_model_.AddSelectionFromAnchorTo(index);
-  NotifyActiveTabChanged(old_active);
+  NotifyIfActiveOrSelectionChanged(old_selection_model, true);
 }
 
 bool TabStripModel::IsTabSelected(int index) const {
@@ -610,10 +608,10 @@ bool TabStripModel::IsTabSelected(int index) const {
 void TabStripModel::SetSelectionFromModel(
     const TabStripSelectionModel& source) {
   DCHECK_NE(TabStripSelectionModel::kUnselectedIndex, source.active());
-  int old_active_index = active_index();
+  TabStripSelectionModel old_selection_model;
+  old_selection_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_selection_model, true);
 }
 
 void TabStripModel::AddTabContents(TabContentsWrapper* contents,
@@ -1225,7 +1223,8 @@ void TabStripModel::NotifyTabSelectedIfChanged(TabContentsWrapper* old_contents,
                                      active_index(), user_gesture));
 }
 
-void TabStripModel::NotifyActiveTabChanged(int old_active_index) {
+void TabStripModel::NotifyActiveTabChanged(int old_active_index,
+                                           bool user_gesture) {
   TabContentsWrapper* old_tab =
       old_active_index == TabStripSelectionModel::kUnselectedIndex ?
       NULL : GetTabContentsAt(old_active_index);
@@ -1233,7 +1232,22 @@ void TabStripModel::NotifyActiveTabChanged(int old_active_index) {
       active_index() == TabStripSelectionModel::kUnselectedIndex ?
       NULL : GetTabContentsAt(active_index());
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-                    ActiveTabChanged(old_tab, new_tab, active_index(), true));
+                    ActiveTabChanged(old_tab, new_tab, active_index(),
+                                     user_gesture));
+}
+
+void TabStripModel::NotifySelectionChanged(
+    const TabStripSelectionModel& old_selection_model) {
+  FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
+                    TabSelectionChanged(old_selection_model));
+}
+
+void TabStripModel::NotifyIfActiveOrSelectionChanged(
+    const TabStripSelectionModel& old_selection_model, bool user_gesture) {
+  if (old_selection_model.active() != active_index())
+    NotifyActiveTabChanged(old_selection_model.active(), user_gesture);
+  if (!selection_model().Equals(old_selection_model))
+    NotifySelectionChanged(old_selection_model);
 }
 
 void TabStripModel::SelectRelativeTab(bool next) {