Fix for issue 27210, show bookmarks bar menu is out of sync with the browser's status.

 * Update the states of menu before showing menu like Window's menu does.
 * Don't fire activate signal when the state is chagned via API.
 * Eliminated UpdateStateData and replace it with the NativeMenuGtk and the position
   stored in the gtk widget.
 * Fixed StateAreaView to check bookmark bar status.

BUG=27210
TEST=Run linux builds (gtk/views/chromeos) check if the bookmark bar menu's status is in sync with the bookmark bar.

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

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

3 files changed, 43 insertions, 27 deletions

diff --git a/chrome/browser/chromeos/status_area_view.cc b/chrome/browser/chromeos/status_area_view.cc
index f880ab5..dedbf5b 100755
--- a/chrome/browser/chromeos/status_area_view.cc
+++ b/chrome/browser/chromeos/status_area_view.cc
@@ -23,6 +23,7 @@
 #include "chrome/browser/views/frame/browser_view.h"
 #endif
 #include "chrome/browser/profile.h"
+#include "chrome/common/pref_names.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
@@ -48,8 +49,7 @@ class OptionsMenuModel : public views::SimpleMenuModel,
     CREATE_NEW_WINDOW = StatusAreaView::OPEN_TABS_ON_RIGHT + 1,
   };
 
-  explicit OptionsMenuModel(Browser* browser,
-                            views::SimpleMenuModel::Delegate* delegate)
+  explicit OptionsMenuModel(Browser* browser)
       : SimpleMenuModel(this),
         browser_(browser) {
 #if defined(TOOLKIT_VIEWS)
@@ -235,7 +235,7 @@ void StatusAreaView::CreateAppMenu() {
   if (app_menu_contents_.get())
     return;
 
-  options_menu_contents_.reset(new OptionsMenuModel(browser_, this));
+  options_menu_contents_.reset(new OptionsMenuModel(browser_));
 
   app_menu_contents_.reset(new views::SimpleMenuModel(this));
   app_menu_contents_->AddItemWithStringId(IDC_NEW_TAB, IDS_NEW_TAB);
@@ -277,6 +277,8 @@ void StatusAreaView::CreateAppMenu() {
 }
 
 bool StatusAreaView::IsCommandIdChecked(int command_id) const {
+  if (command_id == IDC_SHOW_BOOKMARK_BAR)
+    return browser_->profile()->GetPrefs()->GetBoolean(prefs::kShowBookmarkBar);
   return false;
 }
 
diff --git a/views/controls/menu/native_menu_gtk.cc b/views/controls/menu/native_menu_gtk.cc
index a50b9b8..ea18243 100644
--- a/views/controls/menu/native_menu_gtk.cc
+++ b/views/controls/menu/native_menu_gtk.cc
@@ -16,13 +16,8 @@
 #include "views/controls/menu/menu_2.h"
 
 namespace {
-// Data passed to the UpdateStateCallback from gtk_container_foreach.
-struct UpdateStateData {
-  // The model to retrieve state from.
-  views::Menu2Model* model;
-  // The index within said model.
-  int index;
-};
+
+const char kPositionString[] = "position";
 
 // Data passed to the MenuPositionFunc from gtk_menu_popup
 struct Position {
@@ -68,7 +63,8 @@ namespace views {
 NativeMenuGtk::NativeMenuGtk(Menu2Model* model)
     : model_(model),
       menu_(NULL),
-      menu_shown_(false) {
+      menu_shown_(false),
+      suppress_activate_signal_(false) {
 }
 
 NativeMenuGtk::~NativeMenuGtk() {
@@ -79,6 +75,7 @@ NativeMenuGtk::~NativeMenuGtk() {
 // NativeMenuGtk, MenuWrapper implementation:
 
 void NativeMenuGtk::RunMenuAt(const gfx::Point& point, int alignment) {
+  UpdateStates();
   Position position = { point, static_cast<Menu2::Alignment>(alignment) };
   // TODO(beng): value of '1' will not work for context menus!
   gtk_menu_popup(GTK_MENU(menu_), NULL, NULL, MenuPositionFunc, &position, 1,
@@ -116,8 +113,7 @@ void NativeMenuGtk::Rebuild() {
 }
 
 void NativeMenuGtk::UpdateStates() {
-  UpdateStateData data = { model_, 0 };
-  gtk_container_foreach(GTK_CONTAINER(menu_), &UpdateStateCallback, &data);
+  gtk_container_foreach(GTK_CONTAINER(menu_), &UpdateStateCallback, this);
 }
 
 gfx::NativeMenu NativeMenuGtk::GetNativeMenu() const {
@@ -195,7 +191,7 @@ void NativeMenuGtk::AddMenuItemAt(int index,
   if (model_->GetAcceleratorAt(index, &accelerator)) {
     // TODO(beng): accelerators w/gtk_widget_add_accelerator.
   }
-  g_object_set_data(G_OBJECT(menu_item), "position",
+  g_object_set_data(G_OBJECT(menu_item), kPositionString,
                              reinterpret_cast<void*>(index));
   g_signal_connect(G_OBJECT(menu_item), "activate", G_CALLBACK(CallActivate),
                    this);
@@ -203,13 +199,22 @@ void NativeMenuGtk::AddMenuItemAt(int index,
   gtk_menu_append(menu_, menu_item);
 }
 
-// static
-void NativeMenuGtk::UpdateStateCallback(GtkWidget* menu_item, gpointer data) {
-  UpdateStateData* usd = reinterpret_cast<UpdateStateData*>(data);
-  gtk_widget_set_sensitive(menu_item, usd->model->IsEnabledAt(usd->index));
+void NativeMenuGtk::ResetMenu() {
+  if (menu_)
+    gtk_widget_destroy(menu_);
+  menu_ = gtk_menu_new();
+}
+
+void NativeMenuGtk::UpdateMenuItemState(GtkWidget* menu_item) {
+  int index = GPOINTER_TO_INT(g_object_get_data(G_OBJECT(menu_item),
+                                                kPositionString));
+
+  gtk_widget_set_sensitive(menu_item, model_->IsEnabledAt(index));
   if (GTK_IS_CHECK_MENU_ITEM(menu_item)) {
+    suppress_activate_signal_ = true;
     gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(menu_item),
-                                   usd->model->IsItemCheckedAt(usd->index));
+                                   model_->IsItemCheckedAt(index));
+    suppress_activate_signal_ = false;
   }
   // Recurse into submenus, too.
   if (GTK_IS_MENU_ITEM(menu_item)) {
@@ -221,13 +226,12 @@ void NativeMenuGtk::UpdateStateCallback(GtkWidget* menu_item, gpointer data) {
         submenu->UpdateStates();
     }
   }
-  ++usd->index;
 }
 
-void NativeMenuGtk::ResetMenu() {
-  if (menu_)
-    gtk_widget_destroy(menu_);
-  menu_ = gtk_menu_new();
+// static
+void NativeMenuGtk::UpdateStateCallback(GtkWidget* menu_item, gpointer data) {
+  NativeMenuGtk* menu = reinterpret_cast<NativeMenuGtk*>(data);
+  menu->UpdateMenuItemState(menu_item);
 }
 
 // static
@@ -249,8 +253,10 @@ void NativeMenuGtk::MenuPositionFunc(GtkMenu* menu,
 }
 
 void NativeMenuGtk::OnActivate(GtkMenuItem* menu_item) {
+  if (suppress_activate_signal_)
+    return;
   int position = GPOINTER_TO_INT(g_object_get_data(G_OBJECT(menu_item),
-                                                   "position"));
+                                                   kPositionString));
   if (model_->IsEnabledAt(position) &&
       MenuTypeCanExecute(model_->GetTypeAt(position))) {
     model_->ActivatedAt(position);
diff --git a/views/controls/menu/native_menu_gtk.h b/views/controls/menu/native_menu_gtk.h
index 768fada..92c500d 100644
--- a/views/controls/menu/native_menu_gtk.h
+++ b/views/controls/menu/native_menu_gtk.h
@@ -33,10 +33,13 @@ class NativeMenuGtk : public MenuWrapper {
   void AddSeparatorAt(int index);
   void AddMenuItemAt(int index, GtkRadioMenuItem** last_radio_item);
 
-  static void UpdateStateCallback(GtkWidget* menu_item, gpointer data);
-
   void ResetMenu();
 
+  // Updates the menu item's state.
+  void UpdateMenuItemState(GtkWidget* menu_item);
+
+  static void UpdateStateCallback(GtkWidget* menu_item, gpointer data);
+
   // Callback for gtk_menu_popup to position the menu.
   static void MenuPositionFunc(GtkMenu* menu, int* x, int* y, gboolean* push_in,
                                void* data);
@@ -53,6 +56,11 @@ class NativeMenuGtk : public MenuWrapper {
 
   bool menu_shown_;
 
+  // A flag used to avoid misfiring ActivateAt call on the menu model.
+  // This is necessary as GTK menu fires an activate signal even when the
+  // state is changed by |UpdateStates()| API.
+  bool suppress_activate_signal_;
+
   DISALLOW_COPY_AND_ASSIGN(NativeMenuGtk);
 };