Makes app tabs always pinned and unpinable.

BUG=none
TEST=none

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

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

3 files changed, 18 insertions, 11 deletions

diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index 22cea3b..ba697dc 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -185,8 +185,9 @@ void TabStripModel::InsertTabContentsAt(int index,
                                         TabContents* contents,
                                         int add_types) {
   bool foreground = add_types & ADD_SELECTED;
-  index = ConstrainInsertionIndex(index, contents->is_app() ||
-                                  add_types & ADD_PINNED);
+  // Force app tabs to be pinned.
+  bool pin = contents->is_app() || add_types & ADD_PINNED;
+  index = ConstrainInsertionIndex(index, pin);
 
   // 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
@@ -199,7 +200,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 = (add_types & ADD_PINNED) == ADD_PINNED;
+  data->pinned = pin;
   if ((add_types & ADD_INHERIT_GROUP) && selected_contents) {
     if (foreground) {
       // Forget any existing relationships, we don't want to make things too
@@ -484,6 +485,11 @@ void TabStripModel::SetTabPinned(int index, bool pinned) {
     return;
 
   if (IsAppTab(index)) {
+    if (!pinned) {
+      // App tabs should always be pinned.
+      NOTREACHED();
+      return;
+    }
     // Changing the pinned state of an app tab doesn't effect it's mini-tab
     // status.
     contents_data_[index]->pinned = pinned;
@@ -694,7 +700,7 @@ bool TabStripModel::IsContextMenuCommandEnabled(
     case CommandRestoreTab:
       return delegate_->CanRestoreTab();
     case CommandTogglePinned:
-      return true;
+      return !IsAppTab(context_index);
     case CommandBookmarkAllTabs:
       return delegate_->CanBookmarkAllTabs();
     case CommandUseVerticalTabs:
diff --git a/chrome/browser/tabs/tab_strip_model.h b/chrome/browser/tabs/tab_strip_model.h
index 5e43d85..d8eba2e 100644
--- a/chrome/browser/tabs/tab_strip_model.h
+++ b/chrome/browser/tabs/tab_strip_model.h
@@ -257,7 +257,8 @@ class TabStripModelDelegate {
 //   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().
+//     identified by TabContents::is_app(). App tabs are always pinneded (you
+//     can't unpin them).
 //   . 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.
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index 90f3012..c864f27 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -1536,7 +1536,7 @@ TEST_F(TabStripModelTest, Apps) {
     EXPECT_TRUE(observer.StateEquals(0, state));
 
     // And verify the state.
-    EXPECT_EQ("1a 3", GetPinnedState(tabstrip));
+    EXPECT_EQ("1ap 3", GetPinnedState(tabstrip));
 
     observer.ClearStates();
   }
@@ -1550,7 +1550,7 @@ TEST_F(TabStripModelTest, Apps) {
     EXPECT_TRUE(observer.StateEquals(0, state));
 
     // And verify the state.
-    EXPECT_EQ("1a 2a 3", GetPinnedState(tabstrip));
+    EXPECT_EQ("1ap 2ap 3", GetPinnedState(tabstrip));
 
     observer.ClearStates();
   }
@@ -1562,7 +1562,7 @@ TEST_F(TabStripModelTest, Apps) {
     ASSERT_EQ(0, observer.GetStateCount());
 
     // And verify the state didn't change.
-    EXPECT_EQ("1a 2a 3", GetPinnedState(tabstrip));
+    EXPECT_EQ("1ap 2ap 3", GetPinnedState(tabstrip));
 
     observer.ClearStates();
   }
@@ -1574,7 +1574,7 @@ TEST_F(TabStripModelTest, Apps) {
     ASSERT_EQ(0, observer.GetStateCount());
 
     // And verify the state didn't change.
-    EXPECT_EQ("1a 2a 3", GetPinnedState(tabstrip));
+    EXPECT_EQ("1ap 2ap 3", GetPinnedState(tabstrip));
 
     observer.ClearStates();
   }
@@ -1589,7 +1589,7 @@ TEST_F(TabStripModelTest, Apps) {
     EXPECT_TRUE(observer.StateEquals(0, state));
 
     // And verify the state didn't change.
-    EXPECT_EQ("2a 1a 3", GetPinnedState(tabstrip));
+    EXPECT_EQ("2ap 1ap 3", GetPinnedState(tabstrip));
 
     observer.ClearStates();
   }
@@ -1606,7 +1606,7 @@ TEST_F(TabStripModelTest, Apps) {
     EXPECT_TRUE(observer.StateEquals(0, state));
 
     // And verify the state didn't change.
-    EXPECT_EQ("2a 1a 3", GetPinnedState(tabstrip));
+    EXPECT_EQ("2ap 1ap 3", GetPinnedState(tabstrip));
 
     observer.ClearStates();
   }