Yet more tab strip model changes. I'm hoping this is the last of

it. I'm now going with:

// . Mini-tab. Mini tabs are locked to the left side of the tab strip and
//   rendered differently (small tabs with only a favicon). The model makes
//   sure all mini-tabs are at the beginning of the tab strip. For example,
//   if a non-mini tab is added it is forced to be with non-mini tabs. Requests
//   to move tabs outside the range of the tab type are ignored. For example,
//   a request to move a mini-tab after non-mini-tabs is ignored.
//   You'll notice there is no explcit api for making a tab a mini-tab, rather
//   there are two tab types that are implicitly mini-tabs:
//   . App. Corresponds to an extension that wants an app tab. App tabs are
//     identified by TabContents::is_app().
//   . Pinned. Any tab can be pinned. A pinned tab is made phantom when closed.
//     Non-app tabs whose pinned state is changed are moved to be with other
//     mini-tabs or non-mini tabs.

I'm going with a more neutral name like mini-tabs so that we can
change the meaning if we need to without changing around all the UI
code. I'll convert the UI code next.

BUG=32845
TEST=none

Review URL: http://codereview.chromium.org/570043

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@38237 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 287 insertions, 79 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index b22880b..0a652d4 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -682,7 +682,7 @@ TabContents* Browser::AddRestoredTab(
                                          from_last_session);
 
   bool really_pin =
-      (pin && tab_index == tabstrip_model()->IndexOfFirstNonAppTab());
+      (pin && tab_index == tabstrip_model()->IndexOfFirstNonMiniTab());
   tabstrip_model_.InsertTabContentsAt(tab_index, new_tab, select, false);
   if (really_pin)
     tabstrip_model_.SetTabPinned(tab_index, true);
diff --git a/chrome/browser/chromeos/compact_navigation_bar.cc b/chrome/browser/chromeos/compact_navigation_bar.cc
index aeb36e2..6dcbeab 100644
--- a/chrome/browser/chromeos/compact_navigation_bar.cc
+++ b/chrome/browser/chromeos/compact_navigation_bar.cc
@@ -285,7 +285,7 @@ void CompactNavigationBar::AddTabWithURL(const GURL& url,
   switch (StatusAreaView::GetOpenTabsMode()) {
     case StatusAreaView::OPEN_TABS_ON_LEFT: {
       // Add the new tab at the first non-pinned location.
-      int index = browser->tabstrip_model()->IndexOfFirstNonAppTab();
+      int index = browser->tabstrip_model()->IndexOfFirstNonMiniTab();
       browser->AddTabWithURL(url, GURL(), transition,
                               true, index, true, NULL);
       break;
diff --git a/chrome/browser/gtk/tabs/dragged_tab_controller_gtk.cc b/chrome/browser/gtk/tabs/dragged_tab_controller_gtk.cc
index 0a602d1..6513c0c 100644
--- a/chrome/browser/gtk/tabs/dragged_tab_controller_gtk.cc
+++ b/chrome/browser/gtk/tabs/dragged_tab_controller_gtk.cc
@@ -302,7 +302,7 @@ void DraggedTabControllerGtk::MoveTab(const gfx::Point& screen_point) {
       if (!dragged_tab_->is_pinned()) {
         // If the dragged tab isn't pinned, don't allow the drag to a pinned
         // tab.
-        to_index = std::max(to_index, attached_model->IndexOfFirstNonAppTab());
+        to_index = std::max(to_index, attached_model->IndexOfFirstNonMiniTab());
       }
       if (from_index != to_index) {
         last_move_screen_x_ = screen_point.x();
@@ -345,7 +345,7 @@ void DraggedTabControllerGtk::StartPinTimerIfNecessary(
     return;
 
   TabStripModel* attached_model = attached_tabstrip_->model();
-  int pinned_count = attached_model->IndexOfFirstNonAppTab();
+  int pinned_count = attached_model->IndexOfFirstNonMiniTab();
   if (pinned_count > 0)
     return;
 
@@ -368,7 +368,7 @@ void DraggedTabControllerGtk::AdjustDragPointForPinnedTabs(
     int* from_index,
     gfx::Point* dragged_tab_point) {
   TabStripModel* attached_model = attached_tabstrip_->model();
-  int pinned_count = attached_model->IndexOfFirstNonAppTab();
+  int pinned_count = attached_model->IndexOfFirstNonMiniTab();
   if (pinned_count == 0)
     return;
 
@@ -579,7 +579,7 @@ gfx::Point DraggedTabControllerGtk::ConvertScreenPointToTabStripPoint(
 }
 
 int DraggedTabControllerGtk::GetPinnedThreshold() {
-  int pinned_count = attached_tabstrip_->model()->IndexOfFirstNonAppTab();
+  int pinned_count = attached_tabstrip_->model()->IndexOfFirstNonMiniTab();
   if (pinned_count == 0)
     return 0;
   if (!dragged_tab_->is_pinned()) {
diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index 28b3639..4730f76 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -106,13 +106,14 @@ void TabStripModel::AppendTabContents(TabContents* contents, bool foreground) {
 void TabStripModel::InsertTabContentsAt(int index,
                                         TabContents* contents,
                                         bool foreground,
-                                        bool inherit_group) {
+                                        bool inherit_group,
+                                        bool pinned) {
   // Make sure the index maintains that all app tab occurs before non-app tabs.
-  int first_non_app = IndexOfFirstNonAppTab();
-  if (contents->is_app())
-    index = std::min(first_non_app, index);
+  int first_non_mini_tab = IndexOfFirstNonMiniTab();
+  if (contents->is_app() || pinned)
+    index = std::min(first_non_mini_tab, index);
   else
-    index = std::max(first_non_app, index);
+    index = std::max(first_non_mini_tab, index);
 
   // In tab dragging situations, if the last tab in the window was detached
   // then the user aborted the drag, we will have the |closing_all_| member
@@ -125,6 +126,7 @@ void TabStripModel::InsertTabContentsAt(int index,
   // since the old contents and the new contents will be the same...
   TabContents* selected_contents = GetSelectedTabContents();
   TabContentsData* data = new TabContentsData(contents);
+  data->pinned = pinned;
   if (inherit_group && selected_contents) {
     if (foreground) {
       // Forget any existing relationships, we don't want to make things too
@@ -205,29 +207,15 @@ void TabStripModel::MoveTabContentsAt(int index, int to_position,
   if (index == to_position)
     return;
 
-  int first_non_app_tab = IndexOfFirstNonAppTab();
-  if ((index < first_non_app_tab && to_position >= first_non_app_tab) ||
-      (to_position < first_non_app_tab && index >= first_non_app_tab)) {
-    // This would result in app tabs mixed with non-app tabs. We don't allow
+  int first_non_mini_tab = IndexOfFirstNonMiniTab();
+  if ((index < first_non_mini_tab && to_position >= first_non_mini_tab) ||
+      (to_position < first_non_mini_tab && index >= first_non_mini_tab)) {
+    // This would result in mini tabs mixed with non-mini tabs. We don't allow
     // that.
     return;
   }
 
-  TabContentsData* moved_data = contents_data_.at(index);
-  contents_data_.erase(contents_data_.begin() + index);
-  contents_data_.insert(contents_data_.begin() + to_position, moved_data);
-
-  // if !select_after_move, keep the same tab selected as was selected before.
-  if (select_after_move || index == selected_index_) {
-    selected_index_ = to_position;
-  } else if (index < selected_index_ && to_position >= selected_index_) {
-    selected_index_--;
-  } else if (index > selected_index_ && to_position <= selected_index_) {
-    selected_index_++;
-  }
-
-  FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-                    TabMoved(moved_data->contents, index, to_position));
+  MoveTabContentsAtImpl(index, to_position, select_after_move);
 }
 
 TabContents* TabStripModel::GetSelectedTabContents() const {
@@ -398,25 +386,42 @@ void TabStripModel::SetTabBlocked(int index, bool blocked) {
 
 void TabStripModel::SetTabPinned(int index, bool pinned) {
   DCHECK(ContainsIndex(index));
-  int first_non_app_tab = IndexOfFirstNonAppTab();
-  if (index >= first_non_app_tab)
-    return;  // Only allow pinning of app tabs.
-
   if (contents_data_[index]->pinned == pinned)
     return;
 
-  contents_data_[index]->pinned = pinned;
+  if (IsAppTab(index)) {
+    // Changing the pinned state of an app tab doesn't effect it's mini-tab
+    // status.
+    contents_data_[index]->pinned = pinned;
+  } else {
+    // The tab is not an app tab, it's position may have to change as the
+    // mini-tab state is changing.
+    int non_mini_tab_index = IndexOfFirstNonMiniTab();
+    contents_data_[index]->pinned = pinned;
+    if (pinned && index != non_mini_tab_index) {
+      MoveTabContentsAtImpl(index, non_mini_tab_index, false);
+      return;  // Don't send TabPinnedStateChanged notification.
+    } else if (!pinned && index + 1 != non_mini_tab_index) {
+      MoveTabContentsAtImpl(index, non_mini_tab_index - 1, false);
+      return;  // Don't send TabPinnedStateChanged notification.
+    }
+  }
 
-  // Notify observers of new state.
+  // else: the tab was at the boundary and it's position doesn't need to
+  // change.
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-                      TabPinnedStateChanged(contents_data_[index]->contents,
-                                            index));
+                    TabPinnedStateChanged(contents_data_[index]->contents,
+                                          index));
 }
 
 bool TabStripModel::IsTabPinned(int index) const {
   return contents_data_[index]->pinned;
 }
 
+bool TabStripModel::IsMiniTab(int index) const {
+  return IsTabPinned(index) || IsAppTab(index);
+}
+
 bool TabStripModel::IsAppTab(int index) const {
   return GetTabContentsAt(index)->is_app();
 }
@@ -430,12 +435,12 @@ bool TabStripModel::IsTabBlocked(int index) const {
   return contents_data_[index]->blocked;
 }
 
-int TabStripModel::IndexOfFirstNonAppTab() const {
+int TabStripModel::IndexOfFirstNonMiniTab() const {
   for (size_t i = 0; i < contents_data_.size(); ++i) {
-    if (!contents_data_[i]->contents->is_app())
+    if (!contents_data_[i]->contents->is_app() && !contents_data_[i]->pinned)
       return static_cast<int>(i);
   }
-  // No app tabs.
+  // No mini-tabs.
   return count();
 }
 
@@ -477,6 +482,8 @@ void TabStripModel::AddTabContents(TabContents* contents,
     inherit_group = true;
   }
   InsertTabContentsAt(index, contents, foreground, inherit_group);
+  // Reset the index, just in case insert ended up moving it on us.
+  index = GetIndexOfTabContents(contents);
   if (inherit_group && transition == PageTransition::TYPED)
     contents_data_.at(index)->reset_group_on_select = true;
 
@@ -549,24 +556,23 @@ bool TabStripModel::IsContextMenuCommandEnabled(
         return false;
       }
     case CommandCloseOtherTabs:
-      // Close other doesn't effect app tabs.
-      return count() > IndexOfFirstNonAppTab();
+      // Close other doesn't effect mini-tabs.
+      return count() > IndexOfFirstNonMiniTab();
     case CommandCloseTabsToRight:
-      // Close doesn't effect app tabs.
-      return count() != IndexOfFirstNonAppTab() &&
+      // Close doesn't effect mini-tabs.
+      return count() != IndexOfFirstNonMiniTab() &&
           context_index < (count() - 1);
     case CommandCloseTabsOpenedBy: {
       int next_index = GetIndexOfNextTabContentsOpenedBy(
           &GetTabContentsAt(context_index)->controller(), context_index, true);
-      return next_index != kNoTab && !IsAppTab(next_index);
+      return next_index != kNoTab && !IsMiniTab(next_index);
     }
     case CommandDuplicate:
       return delegate_->CanDuplicateContentsAt(context_index);
     case CommandRestoreTab:
       return delegate_->CanRestoreTab();
     case CommandTogglePinned:
-      // Only app tabs can be pinned.
-      return IsAppTab(context_index);
+      return true;
     case CommandBookmarkAllTabs: {
       return delegate_->CanBookmarkAllTabs();
     }
@@ -601,7 +607,7 @@ void TabStripModel::ExecuteContextMenuCommand(
       TabContents* contents = GetTabContentsAt(context_index);
       std::vector<int> closing_tabs;
       for (int i = count() - 1; i >= 0; --i) {
-        if (GetTabContentsAt(i) != contents && !IsAppTab(i))
+        if (GetTabContentsAt(i) != contents && !IsMiniTab(i))
           closing_tabs.push_back(i);
       }
       InternalCloseTabs(closing_tabs, true);
@@ -611,7 +617,7 @@ void TabStripModel::ExecuteContextMenuCommand(
       UserMetrics::RecordAction("TabContextMenu_CloseTabsToRight", profile_);
       std::vector<int> closing_tabs;
       for (int i = count() - 1; i > context_index; --i) {
-        if (!IsAppTab(i))
+        if (!IsMiniTab(i))
           closing_tabs.push_back(i);
       }
       InternalCloseTabs(closing_tabs, true);
@@ -622,7 +628,7 @@ void TabStripModel::ExecuteContextMenuCommand(
       std::vector<int> closing_tabs = GetIndexesOpenedBy(context_index);
       for (std::vector<int>::iterator i = closing_tabs.begin();
            i != closing_tabs.end();) {
-        if (IsAppTab(*i))
+        if (IsMiniTab(*i))
           i = closing_tabs.erase(i);
         else
           ++i;
@@ -696,7 +702,9 @@ void TabStripModel::Observe(NotificationType type,
       for (int i = count() - 1; i >= 0; i--) {
         if (GetTabContentsAt(i)->app_extension() == extension) {
           // The extension an app tab was created from has been nuked. Delete
-          // the TabContents.
+          // the TabContents. Deleting a TabContents results in a notification
+          // of type TAB_CONTENTS_DESTROYED; we do the necessary cleanup in
+          // handling that notification.
           delete GetTabContentsAt(i);
         }
       }
@@ -848,8 +856,8 @@ int TabStripModel::IndexOfNextNonPhantomTab(int index,
 }
 
 bool TabStripModel::ShouldMakePhantomOnClose(int index) {
-  if (IsAppTab(index) && IsTabPinned(index) && !IsPhantomTab(index) &&
-      !closing_all_ && profile()) {
+  if (IsTabPinned(index) && !IsPhantomTab(index) && !closing_all_ &&
+      profile()) {
     ExtensionsService* extension_service = profile()->GetExtensionsService();
     if (!extension_service)
       return false;
@@ -887,6 +895,26 @@ void TabStripModel::MakePhantom(int index) {
                     TabReplacedAt(old_contents, new_contents, index));
 }
 
+
+void TabStripModel::MoveTabContentsAtImpl(int index, int to_position,
+                                          bool select_after_move) {
+  TabContentsData* moved_data = contents_data_.at(index);
+  contents_data_.erase(contents_data_.begin() + index);
+  contents_data_.insert(contents_data_.begin() + to_position, moved_data);
+
+  // if !select_after_move, keep the same tab selected as was selected before.
+  if (select_after_move || index == selected_index_) {
+    selected_index_ = to_position;
+  } else if (index < selected_index_ && to_position >= selected_index_) {
+    selected_index_--;
+  } else if (index > selected_index_ && to_position <= selected_index_) {
+    selected_index_++;
+  }
+
+  FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
+                    TabMoved(moved_data->contents, index, to_position));
+}
+
 // static
 bool TabStripModel::OpenerMatches(const TabContentsData* data,
                                   const NavigationController* opener,
diff --git a/chrome/browser/tabs/tab_strip_model.h b/chrome/browser/tabs/tab_strip_model.h
index 305c562..8c6f546 100644
--- a/chrome/browser/tabs/tab_strip_model.h
+++ b/chrome/browser/tabs/tab_strip_model.h
@@ -103,7 +103,8 @@ class TabStripModelObserver {
   virtual void TabReplacedAt(TabContents* old_contents,
                              TabContents* new_contents, int index) {}
 
-  // Invoked when the pinned state of a tab changes.
+  // Invoked when the mini state of a tab changes. This is not invoked if the
+  // tab ends up moving as a result of the mini state changing.
   virtual void TabPinnedStateChanged(TabContents* contents, int index) {}
 
   // Invoked when the blocked state of a tab changes.
@@ -229,16 +230,21 @@ class TabStripModelDelegate {
 //  tasks like adding new Tabs from just a URL, etc.
 //
 // Each tab may be any one of the following states:
-// . App. Corresponds to an extension that wants an app tab. App tabs are
-//   rendered differently than non-app tabs. The model makes sure all app tabs
-//   are at the beginning of the tab strip. Requests to insert a non-app tab
-//   before app tabs or an app tab after the app tabs is ignored (the UI
-//   disallows this). Similarly requests to move an app tab to be with non-app
-//   tabs is ignored.
-// . Pinned. Only app tabs can be pinned. A pinned tab is made phantom when
-///  closed.
-// . Phantom. Only app pinned tabs may be made phantom. When a tab that can be
-//   made phantom is closed the renderer is shutdown, a new
+// . Mini-tab. Mini tabs are locked to the left side of the tab strip and
+//   rendered differently (small tabs with only a favicon). The model makes
+//   sure all mini-tabs are at the beginning of the tab strip. For example,
+//   if a non-mini tab is added it is forced to be with non-mini tabs. Requests
+//   to move tabs outside the range of the tab type are ignored. For example,
+//   a request to move a mini-tab after non-mini-tabs is ignored.
+//   You'll notice there is no explcit api for making a tab a mini-tab, rather
+//   there are two tab types that are implicitly mini-tabs:
+//   . App. Corresponds to an extension that wants an app tab. App tabs are
+//     identified by TabContents::is_app().
+//   . Pinned. Any tab can be pinned. A pinned tab is made phantom when closed.
+//     Non-app tabs whose pinned state is changed are moved to be with other
+//     mini-tabs or non-mini tabs.
+// . Phantom. Only pinned tabs may be made phantom. When a tab that can be made
+//   phantom is closed the renderer is shutdown, a new
 //   TabContents/NavigationController is created that has not yet loaded the
 //   renderer and observers are notified via the TabReplacedAt method. When a
 //   phantom tab is selected the renderer is loaded and the tab is no longer
@@ -317,6 +323,15 @@ class TabStripModel : public NotificationObserver {
   // foreground inherit the group of the previously selected tab.
   void AppendTabContents(TabContents* contents, bool foreground);
 
+  // TODO(sky): convert callers over to new variant, and consider using a
+  // bitmask rather than bools.
+  void InsertTabContentsAt(int index,
+                           TabContents* contents,
+                           bool foreground,
+                           bool inherit_group) {
+    InsertTabContentsAt(index, contents, foreground, inherit_group, false);
+  }
+
   // Adds the specified TabContents in the specified location. If
   // |inherit_group| is true, the new contents is linked to the current tab's
   // group. This adjusts the index such that all app tabs occur before non-app
@@ -324,7 +339,8 @@ class TabStripModel : public NotificationObserver {
   void InsertTabContentsAt(int index,
                            TabContents* contents,
                            bool foreground,
-                           bool inherit_group);
+                           bool inherit_group,
+                           bool pinned);
 
   // Closes the TabContents at the specified index. This causes the TabContents
   // to be destroyed, but it may not happen immediately (e.g. if it's a
@@ -453,6 +469,10 @@ class TabStripModel : public NotificationObserver {
   // See description above class for details on pinned tabs.
   bool IsTabPinned(int index) const;
 
+  // Is the tab a mini-tab?
+  // See description above class for details on this.
+  bool IsMiniTab(int index) const;
+
   // Is the tab at |index| an app?
   // See description above class for details on app tabs.
   bool IsAppTab(int index) const;
@@ -465,9 +485,10 @@ class TabStripModel : public NotificationObserver {
   // Returns true if the tab at |index| is blocked by a tab modal dialog.
   bool IsTabBlocked(int index) const;
 
-  // Returns the index of the first tab that is not an app. This returns
-  // |count()| if all of the tabs are apps, and 0 if none of the tabs are apps.
-  int IndexOfFirstNonAppTab() const;
+  // Returns the index of the first tab that is not a mini-tab. This returns
+  // |count()| if all of the tabs are mini-tabs, and 0 if none of the tabs are
+  // mini-tabs.
+  int IndexOfFirstNonMiniTab() const;
 
   // Command level API /////////////////////////////////////////////////////////
 
@@ -599,6 +620,12 @@ class TabStripModel : public NotificationObserver {
   // Makes the tab a phantom tab.
   void MakePhantom(int index);
 
+  // Does the work of MoveTabContentsAt. This has no checks to make sure the
+  // position is valid, those are done in MoveTabContentsAt.
+  void MoveTabContentsAtImpl(int index,
+                             int to_position,
+                             bool select_after_move);
+
   // Returns true if the tab represented by the specified data has an opener
   // that matches the specified one. If |use_group| is true, then this will
   // fall back to check the group relationship as well.
@@ -662,7 +689,6 @@ class TabStripModel : public NotificationObserver {
     bool reset_group_on_select;
 
     // Is the tab pinned?
-    // TODO(sky): decide if we really want this, or call it phantomable.
     bool pinned;
 
     // Is the tab interaction blocked by a modal dialog?
diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.cc b/chrome/browser/tabs/tab_strip_model_order_controller.cc
index 36db14a7..ad957dc 100644
--- a/chrome/browser/tabs/tab_strip_model_order_controller.cc
+++ b/chrome/browser/tabs/tab_strip_model_order_controller.cc
@@ -27,7 +27,7 @@ int TabStripModelOrderController::DetermineInsertionIndex(
   if (!tab_count)
     return 0;
 
-  int first_non_app_tab = tabstrip_->IndexOfFirstNonAppTab();
+  int first_non_app_tab = tabstrip_->IndexOfFirstNonMiniTab();
   if (transition == PageTransition::LINK && tabstrip_->selected_index() != -1) {
     if (foreground) {
       // If the page was opened in the foreground by a link click in another
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index f577bc6..0975ff4 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -191,7 +191,6 @@ class MockTabStripModelObserver : public TabStripModelObserver {
           dst_index(a_dst_index),
           user_gesture(false),
           foreground(false),
-          pinned_state_changed(false),
           action(a_action) {
     }
 
@@ -201,7 +200,6 @@ class MockTabStripModelObserver : public TabStripModelObserver {
     int dst_index;
     bool user_gesture;
     bool foreground;
-    bool pinned_state_changed;
     TabStripModelObserverAction action;
   };
 
@@ -222,7 +220,6 @@ class MockTabStripModelObserver : public TabStripModelObserver {
     EXPECT_EQ(s->dst_index, state.dst_index);
     EXPECT_EQ(s->user_gesture, state.user_gesture);
     EXPECT_EQ(s->foreground, state.foreground);
-    EXPECT_EQ(s->pinned_state_changed, state.pinned_state_changed);
     EXPECT_EQ(s->action, state.action);
     return (s->src_contents == state.src_contents &&
             s->dst_contents == state.dst_contents &&
@@ -230,7 +227,6 @@ class MockTabStripModelObserver : public TabStripModelObserver {
             s->dst_index == state.dst_index &&
             s->user_gesture == state.user_gesture &&
             s->foreground == state.foreground &&
-            s->pinned_state_changed == state.pinned_state_changed &&
             s->action == state.action);
   }
 
@@ -1462,3 +1458,161 @@ TEST_F(TabStripModelTest, Apps) {
 
   tabstrip.CloseAllTabs();
 }
+
+// Tests various permutations of pinning tabs.
+TEST_F(TabStripModelTest, Pinning) {
+  TabStripDummyDelegate delegate(NULL);
+  TabStripModel tabstrip(&delegate, profile());
+  MockTabStripModelObserver observer;
+  tabstrip.AddObserver(&observer);
+
+  EXPECT_TRUE(tabstrip.empty());
+
+  typedef MockTabStripModelObserver::State State;
+
+  TabContents* contents1 = CreateTabContents();
+  TabContents* contents2 = CreateTabContents();
+  TabContents* contents3 = CreateTabContents();
+
+  SetID(contents1, 1);
+  SetID(contents2, 2);
+  SetID(contents3, 3);
+
+  // Note! The ordering of these tests is important, each subsequent test
+  // builds on the state established in the previous. This is important if you
+  // ever insert tests rather than append.
+
+  // Initial state, three tabs, first selected.
+  tabstrip.AppendTabContents(contents1, true);
+  tabstrip.AppendTabContents(contents2, false);
+  tabstrip.AppendTabContents(contents3, false);
+
+  observer.ClearStates();
+
+  // Pin the first tab, this shouldn't visually reorder anything.
+  {
+    tabstrip.SetTabPinned(0, true);
+
+    // As the order didn't change, we should get a pinned notification.
+    ASSERT_EQ(1, observer.GetStateCount());
+    State state(contents1, 0, MockTabStripModelObserver::PINNED);
+    EXPECT_TRUE(observer.StateEquals(0, state));
+
+    // And verify the state.
+    EXPECT_EQ("1p 2 3", GetPinnedState(tabstrip));
+
+    observer.ClearStates();
+  }
+
+  // Unpin the first tab.
+  {
+    tabstrip.SetTabPinned(0, false);
+
+    // As the order didn't change, we should get a pinned notification.
+    ASSERT_EQ(1, observer.GetStateCount());
+    State state(contents1, 0, MockTabStripModelObserver::PINNED);
+    EXPECT_TRUE(observer.StateEquals(0, state));
+
+    // And verify the state.
+    EXPECT_EQ("1 2 3", GetPinnedState(tabstrip));
+
+    observer.ClearStates();
+  }
+
+  // Pin the 3rd tab, which should move it to the front.
+  {
+    tabstrip.SetTabPinned(2, true);
+
+    // The pinning should have resulted in a move.
+    ASSERT_EQ(1, observer.GetStateCount());
+    State state(contents3, 0, MockTabStripModelObserver::MOVE);
+    state.src_index = 2;
+    EXPECT_TRUE(observer.StateEquals(0, state));
+
+    // And verify the state.
+    EXPECT_EQ("3p 1 2", GetPinnedState(tabstrip));
+
+    observer.ClearStates();
+  }
+
+  // Pin the tab "1", which shouldn't move anything.
+  {
+    tabstrip.SetTabPinned(1, true);
+
+    // As the order didn't change, we should get a pinned notification.
+    ASSERT_EQ(1, observer.GetStateCount());
+    State state(contents1, 1, MockTabStripModelObserver::PINNED);
+    EXPECT_TRUE(observer.StateEquals(0, state));
+
+    // And verify the state.
+    EXPECT_EQ("3p 1p 2", GetPinnedState(tabstrip));
+
+    observer.ClearStates();
+  }
+
+  // Try to move tab "2" to the front, it should be ignored.
+  {
+    tabstrip.MoveTabContentsAt(2, 0, false);
+
+    // As the order didn't change, we should get a pinned notification.
+    ASSERT_EQ(0, observer.GetStateCount());
+
+    // And verify the state.
+    EXPECT_EQ("3p 1p 2", GetPinnedState(tabstrip));
+
+    observer.ClearStates();
+  }
+
+  // Unpin tab "3", which implicitly moves it to the end.
+  {
+    tabstrip.SetTabPinned(0, false);
+
+    ASSERT_EQ(1, observer.GetStateCount());
+    State state(contents3, 1, MockTabStripModelObserver::MOVE);
+    state.src_index = 0;
+    EXPECT_TRUE(observer.StateEquals(0, state));
+
+    // And verify the state.
+    EXPECT_EQ("1p 3 2", GetPinnedState(tabstrip));
+
+    observer.ClearStates();
+  }
+
+  // Unpin tab "3", nothing should happen.
+  {
+    tabstrip.SetTabPinned(1, false);
+
+    ASSERT_EQ(0, observer.GetStateCount());
+
+    EXPECT_EQ("1p 3 2", GetPinnedState(tabstrip));
+
+    observer.ClearStates();
+  }
+
+  // Pin "3" and "1".
+  {
+    tabstrip.SetTabPinned(0, true);
+    tabstrip.SetTabPinned(1, true);
+
+    EXPECT_EQ("1p 3p 2", GetPinnedState(tabstrip));
+
+    observer.ClearStates();
+  }
+
+  TabContents* contents4 = CreateTabContents();
+  SetID(contents4, 4);
+
+  // Insert "4" between "1" and "3". As "1" and "4" are pinned, "4" should end
+  // up after them.
+  {
+    tabstrip.InsertTabContentsAt(1, contents4, false, false);
+
+    ASSERT_EQ(1, observer.GetStateCount());
+    State state(contents4, 2, MockTabStripModelObserver::INSERT);
+    EXPECT_TRUE(observer.StateEquals(0, state));
+
+    EXPECT_EQ("1p 3p 4 2", GetPinnedState(tabstrip));
+  }
+
+  tabstrip.CloseAllTabs();
+}
diff --git a/chrome/browser/views/tabs/dragged_tab_controller.cc b/chrome/browser/views/tabs/dragged_tab_controller.cc
index 61d3d6a..96dcaae 100644
--- a/chrome/browser/views/tabs/dragged_tab_controller.cc
+++ b/chrome/browser/views/tabs/dragged_tab_controller.cc
@@ -681,7 +681,7 @@ void DraggedTabController::MoveTab(const gfx::Point& screen_point) {
       if (!view_->pinned()) {
         // If the dragged tab isn't pinned, don't allow the drag to a pinned
         // tab.
-        to_index = std::max(to_index, attached_model->IndexOfFirstNonAppTab());
+        to_index = std::max(to_index, attached_model->IndexOfFirstNonMiniTab());
       }
       if (from_index != to_index) {
         last_move_screen_x_ = screen_point.x();
@@ -744,7 +744,7 @@ void DraggedTabController::StartPinTimerIfNecessary(
     return;
 
   TabStripModel* attached_model = attached_tabstrip_->model();
-  int app_count = attached_model->IndexOfFirstNonAppTab();
+  int app_count = attached_model->IndexOfFirstNonMiniTab();
   if (app_count > 0)
     return;
 
@@ -767,7 +767,7 @@ void DraggedTabController::AdjustDragPointForPinnedTabs(
     int* from_index,
     gfx::Point* dragged_tab_point) {
   TabStripModel* attached_model = attached_tabstrip_->model();
-  int pinned_count = attached_model->IndexOfFirstNonAppTab();
+  int pinned_count = attached_model->IndexOfFirstNonMiniTab();
   if (pinned_count == 0)
     return;
 
@@ -981,7 +981,7 @@ void DraggedTabController::Detach() {
 }
 
 int DraggedTabController::GetPinnedThreshold() {
-  int pinned_count = attached_tabstrip_->model()->IndexOfFirstNonAppTab();
+  int pinned_count = attached_tabstrip_->model()->IndexOfFirstNonMiniTab();
   if (pinned_count == 0)
     return 0;
   if (!view_->pinned()) {