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

chrome/browser/tabs/tab_strip_model.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/tabs/tab_strip_model.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/command_line.h"
@@ skipping 551 matching lines @@
   return count();
 }
 
 int TabStripModel::ConstrainInsertionIndex(int index, bool mini_tab) {
   return mini_tab ? std::min(std::max(0, index), IndexOfFirstNonMiniTab()) :
       std::min(count(), std::max(index, IndexOfFirstNonMiniTab()));
 }
 
 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
       // unselect it.
       return;
     }
     selection_model_.RemoveIndexFromSelection(index);
     selection_model_.set_anchor(index);
     if (selection_model_.active() == TabStripSelectionModel::kUnselectedIndex)
       selection_model_.set_active(selection_model_.selected_indices()[0]);
   } else {
     selection_model_.AddIndexToSelection(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 {
   DCHECK(ContainsIndex(index));
   return selection_model_.IsSelected(index);
 }
 
 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,
                                    int index,
                                    PageTransition::Type transition,
                                    int add_types) {
   // If the newly-opened tab is part of the same task as the parent tab, we want
   // to inherit the parent's "group" attribute, so that if this tab is then
   // closed we'll jump back to the parent tab.
   bool inherit_group = (add_types & ADD_INHERIT_GROUP) == ADD_INHERIT_GROUP;
@@ skipping 602 matching lines @@
   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::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.
   if (contents_data_.empty())
     return;
 
   int index = active_index();
   int delta = next ? 1 : -1;
   index = (index + count() + delta) % count();
   ActivateTabAt(index, true);
@@ skipping 62 matching lines @@
 void TabStripModel::ForgetOpenersAndGroupsReferencing(
     const NavigationController* tab) {
   for (TabContentsDataVector::const_iterator i = contents_data_.begin();
        i != contents_data_.end(); ++i) {
     if ((*i)->group == tab)
       (*i)->group = NULL;
     if ((*i)->opener == tab)
       (*i)->opener = NULL;
   }
 }