Changes the tab menu to use pin and unpin instead of a check. The mac

changes to use the right thing (pinned vs mini) will be done separately.

BUG=none
TEST=none

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

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

6 files changed, 28 insertions, 49 deletions

diff --git a/chrome/browser/cocoa/tab_controller.mm b/chrome/browser/cocoa/tab_controller.mm
index cb997d0..f2f76db 100644
--- a/chrome/browser/cocoa/tab_controller.mm
+++ b/chrome/browser/cocoa/tab_controller.mm
@@ -28,7 +28,8 @@ namespace TabControllerInternal {
 class MenuDelegate : public menus::SimpleMenuModel::Delegate {
  public:
   explicit MenuDelegate(id<TabControllerTarget> target, TabController* owner)
-      : target_(target), owner_(owner) { }
+      : target_(target),
+        owner_(owner) {}
 
   // Overridden from menus::SimpleMenuModel::Delegate
   virtual bool IsCommandIdChecked(int command_id) const { return false; }
@@ -46,20 +47,6 @@ class MenuDelegate : public menus::SimpleMenuModel::Delegate {
     [target_ commandDispatch:command forController:owner_];
   }
 
-  virtual bool IsLabelForCommandIdDynamic(int command_id) const {
-    return command_id == TabStripModel::CommandTogglePinned;
-  }
-  virtual string16 GetLabelForCommandId(int command_id) const {
-    // Display "Pin Tab" when the tab is not pinned and "Unpin Tab" when it is
-    // (this is not a checkmark menu item, per Apple's HIG).
-    if (command_id == TabStripModel::CommandTogglePinned) {
-      return l10n_util::GetStringUTF16(
-          [owner_ mini] ? IDS_TAB_CXMENU_UNPIN_TAB_MAC
-                          : IDS_TAB_CXMENU_PIN_TAB_MAC);
-    }
-    return string16();
-  }
-
  private:
   id<TabControllerTarget> target_;  // weak
   TabController* owner_;  // weak, owns me
@@ -138,7 +125,9 @@ class MenuDelegate : public menus::SimpleMenuModel::Delegate {
 - (NSMenu*)menu {
   contextMenuDelegate_.reset(
       new TabControllerInternal::MenuDelegate(target_, self));
-  contextMenuModel_.reset(new TabMenuModel(contextMenuDelegate_.get()));
+  // TODO(42339): this is wrong, it should use pinned, not mini.
+  contextMenuModel_.reset(new TabMenuModel(contextMenuDelegate_.get(),
+                                           [self mini]));
   contextMenuController_.reset(
       [[MenuController alloc] initWithModel:contextMenuModel_.get()
                      useWithPopUpButtonCell:NO]);
diff --git a/chrome/browser/gtk/tabs/tab_gtk.cc b/chrome/browser/gtk/tabs/tab_gtk.cc
index 130b17a..c5f7201 100644
--- a/chrome/browser/gtk/tabs/tab_gtk.cc
+++ b/chrome/browser/gtk/tabs/tab_gtk.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -39,7 +39,7 @@ class TabGtk::ContextMenuController : public menus::SimpleMenuModel::Delegate {
  public:
   explicit ContextMenuController(TabGtk* tab)
       : tab_(tab),
-        model_(this) {
+        model_(this, tab->delegate()->IsTabPinned(tab)) {
     menu_.reset(new MenuGtk(NULL, &model_));
   }
 
@@ -57,9 +57,7 @@ class TabGtk::ContextMenuController : public menus::SimpleMenuModel::Delegate {
  private:
   // Overridden from menus::SimpleMenuModel::Delegate:
   virtual bool IsCommandIdChecked(int command_id) const {
-    if (!tab_ || command_id != TabStripModel::CommandTogglePinned)
-      return false;
-    return tab_->delegate()->IsTabPinned(tab_);
+    return false;
   }
   virtual bool IsCommandIdEnabled(int command_id) const {
     return tab_ && tab_->delegate()->IsCommandEnabledForTab(
@@ -348,8 +346,7 @@ void TabGtk::SetBounds(const gfx::Rect& bounds) {
 // TabGtk, private:
 
 void TabGtk::ShowContextMenu() {
-  if (!menu_controller_.get())
-    menu_controller_.reset(new ContextMenuController(this));
+  menu_controller_.reset(new ContextMenuController(this));
 
   menu_controller_->RunMenu();
 }
diff --git a/chrome/browser/tab_menu_model.cc b/chrome/browser/tab_menu_model.cc
index d2b38ce..bb0ba34 100644
--- a/chrome/browser/tab_menu_model.cc
+++ b/chrome/browser/tab_menu_model.cc
@@ -4,30 +4,24 @@
 
 #include "chrome/browser/tab_menu_model.h"
 
-#include "chrome/browser/defaults.h"
 #include "chrome/browser/tabs/tab_strip_model.h"
 #include "grit/generated_resources.h"
 
-TabMenuModel::TabMenuModel(menus::SimpleMenuModel::Delegate* delegate)
+TabMenuModel::TabMenuModel(menus::SimpleMenuModel::Delegate* delegate,
+                           bool is_pinned)
     : menus::SimpleMenuModel(delegate) {
-  Build();
+  Build(is_pinned);
 }
 
-void TabMenuModel::Build() {
+void TabMenuModel::Build(bool is_pinned) {
   AddItemWithStringId(TabStripModel::CommandNewTab, IDS_TAB_CXMENU_NEWTAB);
   AddSeparator();
   AddItemWithStringId(TabStripModel::CommandReload, IDS_TAB_CXMENU_RELOAD);
   AddItemWithStringId(TabStripModel::CommandDuplicate,
                       IDS_TAB_CXMENU_DUPLICATE);
-  // On Mac the HIG prefers "pin/unpin" to a checkmark. The Mac code will fix up
-  // the actual string based on the tab's state via the delegate.
-#if defined(OS_MACOSX)
-  AddItemWithStringId(TabStripModel::CommandTogglePinned,
-                      IDS_TAB_CXMENU_PIN_TAB);
-#else
-  AddCheckItemWithStringId(TabStripModel::CommandTogglePinned,
-                           IDS_TAB_CXMENU_PIN_TAB);
-#endif
+  AddItemWithStringId(
+      TabStripModel::CommandTogglePinned,
+      is_pinned ? IDS_TAB_CXMENU_UNPIN_TAB : IDS_TAB_CXMENU_PIN_TAB);
   AddSeparator();
   AddItemWithStringId(TabStripModel::CommandCloseTab,
                       IDS_TAB_CXMENU_CLOSETAB);
diff --git a/chrome/browser/tab_menu_model.h b/chrome/browser/tab_menu_model.h
index c98731d..dca49af 100644
--- a/chrome/browser/tab_menu_model.h
+++ b/chrome/browser/tab_menu_model.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved. Use of this
+// Copyright (c) 2010 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.
 
@@ -10,14 +10,16 @@
 class Browser;
 
 // A menu model that builds the contents of the tab context menu. This menu has
-// only one level (no submenus).
+// only one level (no submenus). TabMenuModel caches local state from the
+// tab (such as the pinned state). To make sure the menu reflects the real state
+// of the tab a new TabMenuModel should be created each time the menu is shown.
 class TabMenuModel : public menus::SimpleMenuModel {
  public:
-  explicit TabMenuModel(menus::SimpleMenuModel::Delegate* delegate);
+  TabMenuModel(menus::SimpleMenuModel::Delegate* delegate, bool is_pinned);
   virtual ~TabMenuModel() {}
 
  private:
-  void Build();
+  void Build(bool is_pinned);
 
   DISALLOW_COPY_AND_ASSIGN(TabMenuModel);
 };
diff --git a/chrome/browser/tab_menu_model_unittest.cc b/chrome/browser/tab_menu_model_unittest.cc
index 3e0fdd3..eaaa72e 100644
--- a/chrome/browser/tab_menu_model_unittest.cc
+++ b/chrome/browser/tab_menu_model_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved. Use of this
+// Copyright (c) 2010 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.
 
@@ -9,12 +9,11 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
-
 class TabMenuModelTest : public PlatformTest, public MenuModelTest {
 };
 
 TEST_F(TabMenuModelTest, Basics) {
-  TabMenuModel model(&delegate_);
+  TabMenuModel model(&delegate_, true);
 
   // Verify it has items. The number varies by platform, so we don't check
   // the exact number.
diff --git a/chrome/browser/views/tabs/tab.cc b/chrome/browser/views/tabs/tab.cc
index 3f3da3f..eb80e89 100644
--- a/chrome/browser/views/tabs/tab.cc
+++ b/chrome/browser/views/tabs/tab.cc
@@ -29,7 +29,8 @@ static const SkScalar kTabBottomCurveWidth = 3;
 class Tab::TabContextMenuContents : public menus::SimpleMenuModel::Delegate {
  public:
   explicit TabContextMenuContents(Tab* tab)
-      : ALLOW_THIS_IN_INITIALIZER_LIST(model_(this)),
+      : ALLOW_THIS_IN_INITIALIZER_LIST(
+          model_(this, tab->delegate()->IsTabPinned(tab))),
         tab_(tab),
         last_command_(TabStripModel::CommandFirst) {
     Build();
@@ -51,9 +52,7 @@ class Tab::TabContextMenuContents : public menus::SimpleMenuModel::Delegate {
 
   // Overridden from menus::SimpleMenuModel::Delegate:
   virtual bool IsCommandIdChecked(int command_id) const {
-    if (!tab_ || command_id != TabStripModel::CommandTogglePinned)
-      return false;
-    return tab_->delegate()->IsTabPinned(tab_);
+    return false;
   }
   virtual bool IsCommandIdEnabled(int command_id) const {
     return tab_ && tab_->delegate()->IsCommandEnabledForTab(
@@ -202,8 +201,7 @@ bool Tab::GetAccessibleRole(AccessibilityTypes::Role* role) {
 void Tab::ShowContextMenu(views::View* source,
                           const gfx::Point& p,
                           bool is_mouse_gesture) {
-  if (!context_menu_contents_.get())
-    context_menu_contents_.reset(new TabContextMenuContents(this));
+  context_menu_contents_.reset(new TabContextMenuContents(this));
   context_menu_contents_->RunMenuAt(p);
 }