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..0f98be1c8c5fe56eb877f953259c23b0f3187418 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -569,15 +569,20 @@ 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);
+  if (old_selection_model.active() != active_index())

[dpapad, 2011/06/02 18:40:33]

Instead of repeatedly checking this I would prefer to put this check in
NotifyActiveTabChanged (and maybe rename it to NotifyIfActiveTabChanged or just
update the comments).

[sky, 2011/06/02 20:27:58]

Yes. In fact how about a single SelectionChanged method that replaces 575-579.
It would compare active index, and if necessary call NotifyActiveTabChanged as
well as NotifySelectionChanged.
How about a single method you call when ever the selection might have changed
(replace 575-

[dpapad, 2011/06/02 23:29:50]

Done.

+    NotifyActiveTabChanged(old_selection_model.active());
+
   // This may not have resulted in a change, but we assume it did.
-  NotifyActiveTabChanged(old_active);
+  NotifySelectionChanged(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 +598,18 @@ void TabStripModel::ToggleSelectionAt(int index) {
     selection_model_.set_anchor(index);
     selection_model_.set_active(index);
   }
-  NotifyActiveTabChanged(old_active);
+  if (old_selection_model.active() != active_index())
+    NotifyActiveTabChanged(old_selection_model.active());
+  NotifySelectionChanged(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);
+  if (old_selection_model.active() != active_index())
+    NotifyActiveTabChanged(old_selection_model.active());
+  NotifySelectionChanged(old_selection_model);
 }
 
 bool TabStripModel::IsTabSelected(int index) const {
@@ -610,10 +620,13 @@ 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);
+  if (old_selection_model.active() != active_index())
+    NotifyActiveTabChanged(old_selection_model.active());
+  NotifySelectionChanged(old_selection_model);
 }
 
 void TabStripModel::AddTabContents(TabContentsWrapper* contents,
@@ -1236,6 +1249,12 @@ void TabStripModel::NotifyActiveTabChanged(int old_active_index) {
                     ActiveTabChanged(old_tab, new_tab, active_index(), true));
 }
 
+void TabStripModel::NotifySelectionChanged(
+    const TabStripSelectionModel& model) {

[dpapad, 2011/06/02 18:40:33]

Also how about adding a check here to determine if selected_indices_ changed
before sending notifications? 

[sky, 2011/06/02 20:27:58]

Sure, but make sure you include the anchor/active index in that comparison.

[dpapad, 2011/06/02 23:29:50]

Done.

+  FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
+                    TabSelectionChanged(model));
+}
+
 void TabStripModel::SelectRelativeTab(bool next) {
   // This may happen during automated testing or if a user somehow buffers
   // many key accelerators.