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 140 matching lines @@
     }
     data->opener = &selected_contents->controller();
   }
 
   contents_data_.insert(contents_data_.begin() + index, data);
 
   selection_model_.IncrementFrom(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);
   }
 }
 
 TabContentsWrapper* TabStripModel::ReplaceTabContentsAt(
     int index,
     TabContentsWrapper* new_contents) {
   DCHECK(ContainsIndex(index));
   TabContentsWrapper* old_contents = GetContentsAt(index);
 
   ForgetOpenersAndGroupsReferencing(&(old_contents->controller()));
@@ skipping 38 matching lines @@
   ForgetOpenersAndGroupsReferencing(&(removed_contents->controller()));
   if (empty())
     closing_all_ = true;
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
       TabDetachedAt(removed_contents, index));
   if (empty()) {
     // TabDetachedAt() might unregister observers, so send |TabStripEmtpy()| in
     // 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;
 }
 
 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,
                                       int to_position,
                                       bool select_after_move) {
   DCHECK(ContainsIndex(index));
   if (index == to_position)
     return;
 
   int first_non_mini_tab = IndexOfFirstNonMiniTab();
@@ skipping 291 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();
+  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
       // 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);
+  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 {
   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();
+  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,
                                    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 573 matching lines @@
   // and detach it.
   delete contents;
 }
 
 TabContentsWrapper* TabStripModel::GetContentsAt(int index) const {
   CHECK(ContainsIndex(index)) <<
       "Failed to find: " << index << " in: " << count() << " entries.";
   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;
 
   TabContentsWrapper* last_selected_contents = old_contents;
   if (last_selected_contents) {
     FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                       TabDeactivated(last_selected_contents));
   }
 
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                     ActiveTabChanged(last_selected_contents, new_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) {
   // 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;
@@ skipping 64 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;
   }
 }