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..ed18152f02f325b8e63cee24faacc6701d5ea775 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -158,10 +158,12 @@ 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);
+    NotifyIfActiveTabChanged(selected_contents, active_index(), false);

[sky, 2011/06/07 20:32:44]

You always end up invoking both of these methods at the same time. How about
combining them into a single method?

[dpapad, 2011/06/07 23:20:09]

Done. Combined to a single method named NotifyIfActiveOrSelectionChanged.

+    NotifyIfSelectionChanged(old_model);
   }
 }
 
@@ -220,22 +222,22 @@ 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/07 20:32:44]

I'm still confused by this. You've notified the observers the tab has been
removed, which means they should have shifted things down around already so that
using the selection model before its been adjusted doesn't makes sense.
Shouldn't you copy the old_model *after* 227? Compare this with
InsertTabContentsAt which does what I'm saying. And yes, you'll need to update
the if to make it work correctly.

[dpapad, 2011/06/07 23:20:09]

Done.

     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);
       }
+      NotifyIfActiveTabChanged(removed_contents, active_index(), false);
+      NotifyIfSelectionChanged(old_model);
     }
   }
   return removed_contents;
@@ -243,21 +245,12 @@ 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));
-  }
+  NotifyIfActiveTabChanged(old_contents, active_index(), user_gesture);
+  NotifyIfSelectionChanged(old_model);
 }
 
 void TabStripModel::MoveTabContentsAt(int index,
@@ -569,15 +562,19 @@ 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);
+  NotifyIfActiveTabChanged(old_contents, active_index(), true);
+  NotifyIfSelectionChanged(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 +590,17 @@ void TabStripModel::ToggleSelectionAt(int index) {
     selection_model_.set_anchor(index);
     selection_model_.set_active(index);
   }
-  NotifyActiveTabChanged(old_active);
+  NotifyIfActiveTabChanged(old_contents, active_index(), true);
+  NotifyIfSelectionChanged(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);
+  NotifyIfActiveTabChanged(old_contents, active_index(), true);
+  NotifyIfSelectionChanged(old_model);
 }
 
 bool TabStripModel::IsTabSelected(int index) const {
@@ -610,10 +611,12 @@ 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);
+  NotifyIfActiveTabChanged(old_contents, active_index(), true);
+  NotifyIfSelectionChanged(old_model);
 }
 
 void TabStripModel::AddTabContents(TabContentsWrapper* contents,
@@ -1207,9 +1210,9 @@ TabContentsWrapper* TabStripModel::GetContentsAt(int index) const {
   return contents_data_.at(index)->contents;
 }
 
-void TabStripModel::NotifyTabSelectedIfChanged(TabContentsWrapper* old_contents,
-                                               int to_index,
-                                               bool user_gesture) {
+void TabStripModel::NotifyIfActiveTabChanged(TabContentsWrapper* old_contents,
+                                             int to_index,

[sky, 2011/06/07 20:32:44]

You always pass in to_index = to active_index(). Could you remove the parameter
and use active_index() here? Especially since 1228 uses active_index already.

[dpapad, 2011/06/07 23:20:09]

Done.

+                                             bool user_gesture) {
   TabContentsWrapper* new_contents = GetContentsAt(to_index);
   if (old_contents == new_contents)
     return;
@@ -1225,15 +1228,12 @@ void TabStripModel::NotifyTabSelectedIfChanged(TabContentsWrapper* old_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::NotifyIfSelectionChanged(
+    const TabStripSelectionModel& old_selection_model) {
+  if (!selection_model().Equals(old_selection_model)) {
+    FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
+                      TabSelectionChanged(old_selection_model));
+  }
 }
 
 void TabStripModel::SelectRelativeTab(bool next) {