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..3accf5c96c9032f35487af65b8bf695cce3d00d2 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));

[dpapad, 2011/06/03 17:49:02]

Should the |active| flag from TabInsertedAt be removed now? If the newly
inserted tab becomes active a new notification will follow. It looks like
TabInsertedAt and ActiveTabChanged step on each other's responsibilities.

[sky, 2011/06/03 18:17:40]

I'm not completely sure. You'll have to look at what all consumers do. If you
remove it, it would mean consumers don't have an easy way to know selection is
changing as the result of an insert. That said, I don't know if anyone actually
cares about that cast.

-
-  if (active) {
+  TabStripSelectionModel old_model;
+  old_model.Copy(selection_model_);
+  if (active)
     selection_model_.SetSelectedIndex(index);
-    NotifyTabSelectedIfChanged(selected_contents, index, false);
-  }
+  NotifyIfActiveOrSelectionChanged(old_model);
 }
 
 TabContentsWrapper* TabStripModel::ReplaceTabContentsAt(
@@ -220,21 +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_);
     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);
+        NotifyIfActiveOrSelectionChanged(old_model);

[dpapad, 2011/06/03 17:49:02]

Isn't the |user_gesture| flag info being lost with this change? Same holds for
below.

[sky, 2011/06/03 18:17:40]

What do you mean? For whatever reason Detach doesn't take a user_gesture field,
so we have to assume it's false.

[dpapad, 2011/06/03 18:35:11]

As of now NotifyIfActiveOrSelectionChanged() calls NotifyActiveTabChanged()
which always calls
FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                  ActiveTabChanged(old_tab, new_tab, active_index(), true));
with the user_gesture hardcoded to "true". I think it should be a parameter of
both NotifyActiveTabChanged() and NotifyIfActiveOrSelectionChanged() if we plan
to use it in cases where |user_gesture| can be both true/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);
       }
     }
   }
@@ -569,15 +569,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);
 }
 
 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 +594,14 @@ void TabStripModel::ToggleSelectionAt(int index) {
     selection_model_.set_anchor(index);
     selection_model_.set_active(index);
   }
-  NotifyActiveTabChanged(old_active);
+  NotifyIfActiveOrSelectionChanged(old_selection_model);
 }
 
 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);
 }
 
 bool TabStripModel::IsTabSelected(int index) const {
@@ -610,10 +612,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);
 }
 
 void TabStripModel::AddTabContents(TabContentsWrapper* contents,
@@ -1236,6 +1238,20 @@ void TabStripModel::NotifyActiveTabChanged(int old_active_index) {
                     ActiveTabChanged(old_tab, new_tab, active_index(), true));
 }
 
+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) {
+  if (old_selection_model.active() != active_index())
+    NotifyActiveTabChanged(old_selection_model.active());
+  if (!selection_model().Equals(old_selection_model))
+    NotifySelectionChanged(old_selection_model);
+}
+
 void TabStripModel::SelectRelativeTab(bool next) {
   // This may happen during automated testing or if a user somehow buffers
   // many key accelerators.