Wait calling OnMenuOpened until the domui menu is ready to accept input.

 * Call OnMenuOpened when the window is mapped.
   This is necessary as DOMUI menu doesn't show the popup window until page is renderred.
 * Moved listners to Menu2 as Menu2 manages MenuListeners.

BUG=chromium-os:7642
TEST=TestMenuKeyboardAccess,TestAltMenuKeyboardAccess passes with this fix.

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

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

6 files changed, 58 insertions, 38 deletions

diff --git a/chrome/browser/chromeos/views/domui_menu_widget.cc b/chrome/browser/chromeos/views/domui_menu_widget.cc
index aefc2f9..72b6ea6 100644
--- a/chrome/browser/chromeos/views/domui_menu_widget.cc
+++ b/chrome/browser/chromeos/views/domui_menu_widget.cc
@@ -223,6 +223,7 @@ void DOMUIMenuWidget::EnableInput(bool select_item) {
   if (select_item) {
     ExecuteJavascript(L"selectItem()");
   }
+  domui_menu_->InputIsReady();
 }
 
 void DOMUIMenuWidget::ExecuteJavascript(const std::wstring& script) {
diff --git a/chrome/browser/chromeos/views/native_menu_domui.cc b/chrome/browser/chromeos/views/native_menu_domui.cc
index 27682ba..5d117a2 100644
--- a/chrome/browser/chromeos/views/native_menu_domui.cc
+++ b/chrome/browser/chromeos/views/native_menu_domui.cc
@@ -76,7 +76,8 @@ NativeMenuDOMUI::NativeMenuDOMUI(menus::MenuModel* menu_model, bool root)
       activated_menu_(NULL),
       activated_index_(-1),
       menu_action_(MENU_ACTION_NONE),
-      menu_url_(StringPrintf("chrome://%s", chrome::kChromeUIMenu)) {
+      menu_url_(StringPrintf("chrome://%s", chrome::kChromeUIMenu)),
+      on_menu_opened_called_(false) {
   menu_widget_ = new DOMUIMenuWidget(this, root);
   // Set the initial location off the screen not to show small
   // window with dropshadow.
@@ -119,9 +120,9 @@ void NativeMenuDOMUI::RunMenuAt(const gfx::Point& point, int alignment) {
       MenuLocator::CreateContextMenuLocator(point) :
       MenuLocator::CreateDropDownMenuLocator(point);
   ShowAt(locator);
-  FOR_EACH_OBSERVER(views::MenuListener, listeners_, OnMenuOpened());
   DCHECK(!menu_shown_);
   menu_shown_ = true;
+  on_menu_opened_called_ = false;
 
   // TODO(oshima): A menu must be deleted when parent window is
   // closed. Menu2 doesn't know about the parent window, however, so
@@ -326,6 +327,13 @@ Profile* NativeMenuDOMUI::GetProfile() {
   return browser->GetProfile();
 }
 
+void NativeMenuDOMUI::InputIsReady() {
+  if (!on_menu_opened_called_) {
+    on_menu_opened_called_ = true;
+    FOR_EACH_OBSERVER(views::MenuListener, listeners_, OnMenuOpened());
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // NativeMenuDOMUI, private:
 
diff --git a/chrome/browser/chromeos/views/native_menu_domui.h b/chrome/browser/chromeos/views/native_menu_domui.h
index d000985..2c5e216a 100644
--- a/chrome/browser/chromeos/views/native_menu_domui.h
+++ b/chrome/browser/chromeos/views/native_menu_domui.h
@@ -78,6 +78,10 @@ class NativeMenuDOMUI : public views::MenuWrapper,
   // Returns the profile to create DOMView.
   Profile* GetProfile();
 
+  // Called when the menu is ready to accept input.
+  // Used in interactive_ui_test to wait for menu opened.
+  void InputIsReady();
+
   // Sets/Gets the url for the domui menu.
   void set_menu_url(const GURL& url) { menu_url_ = url; }
   const GURL& menu_url() const { return menu_url_; }
@@ -133,6 +137,9 @@ class NativeMenuDOMUI : public views::MenuWrapper,
   // (e.g. chrome://wrench-menu for wrench menu).
   GURL menu_url_;
 
+  // A guard flag to avoid calling MenuListener::OnMenuOpened twice.
+  bool on_menu_opened_called_;
+
   DISALLOW_COPY_AND_ASSIGN(NativeMenuDOMUI);
 };
 
diff --git a/chrome/browser/views/toolbar_view.cc b/chrome/browser/views/toolbar_view.cc
index 48e7f69..7007545 100644
--- a/chrome/browser/views/toolbar_view.cc
+++ b/chrome/browser/views/toolbar_view.cc
@@ -10,10 +10,6 @@
 #include "chrome/browser/accessibility/browser_accessibility_state.h"
 #include "chrome/browser/browser.h"
 #include "chrome/browser/browser_window.h"
-#if defined(OS_CHROMEOS)
-#include "chrome/browser/chromeos/cros/cros_library.h"
-#include "chrome/browser/chromeos/cros/update_library.h"
-#endif
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/themes/browser_theme_provider.h"
@@ -22,12 +18,6 @@
 #include "chrome/browser/views/browser_actions_container.h"
 #include "chrome/browser/views/event_utils.h"
 #include "chrome/browser/views/frame/browser_view.h"
-#if !defined(OS_CHROMEOS)
-#include "chrome/browser/views/wrench_menu.h"
-#else
-#include "views/controls/menu/menu_2.h"
-#include "chrome/browser/chromeos/dom_ui/wrench_menu_ui.h"
-#endif
 #include "chrome/browser/wrench_menu_model.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/common/pref_names.h"
@@ -43,6 +33,15 @@
 #include "views/window/non_client_view.h"
 #include "views/window/window.h"
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/cros/cros_library.h"
+#include "chrome/browser/chromeos/cros/update_library.h"
+#include "chrome/browser/chromeos/dom_ui/wrench_menu_ui.h"
+#include "views/controls/menu/menu_2.h"
+#else
+#include "chrome/browser/views/wrench_menu.h"
+#endif
+
 // The space between items is 4 px in general.
 const int ToolbarView::kStandardSpacing = 4;
 // The top of the toolbar has an edge we have to skip over in addition to the 4
@@ -121,6 +120,10 @@ void ToolbarView::Init(Profile* profile) {
   forward_menu_model_.reset(new BackForwardMenuModel(
       browser_, BackForwardMenuModel::FORWARD_MENU));
   wrench_menu_model_.reset(new WrenchMenuModel(this, browser_));
+#if defined(OS_CHROMEOS)
+  wrench_menu_.reset(
+      chromeos::WrenchMenuUI::CreateMenu2(wrench_menu_model_.get()));
+#endif
 
   back_ = new views::ButtonDropDown(this, back_menu_model_.get());
   back_->set_triggerable_event_flags(views::Event::EF_LEFT_BUTTON_DOWN |
@@ -227,10 +230,19 @@ bool ToolbarView::IsAppMenuFocused() {
 }
 
 void ToolbarView::AddMenuListener(views::MenuListener* listener) {
+#if defined(OS_CHROMEOS)
+  DCHECK(wrench_menu_.get());
+  wrench_menu_->AddMenuListener(listener);
+#else
   menu_listeners_.push_back(listener);
+#endif
 }
 
 void ToolbarView::RemoveMenuListener(views::MenuListener* listener) {
+#if defined(OS_CHROMEOS)
+  DCHECK(wrench_menu_.get());
+  wrench_menu_->RemoveMenuListener(listener);
+#else
   for (std::vector<views::MenuListener*>::iterator i(menu_listeners_.begin());
        i != menu_listeners_.end(); ++i) {
     if (*i == listener) {
@@ -238,6 +250,7 @@ void ToolbarView::RemoveMenuListener(views::MenuListener* listener) {
       return;
     }
   }
+#endif
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -268,24 +281,21 @@ void ToolbarView::RunMenu(views::View* source, const gfx::Point& /* pt */) {
   bool destroyed_flag = false;
   destroyed_flag_ = &destroyed_flag;
 #if defined(OS_CHROMEOS)
-  wrench_menu_.reset(
-      chromeos::WrenchMenuUI::CreateMenu2(wrench_menu_model_.get()));
-#else
-  wrench_menu_ = new WrenchMenu(browser_);
-  wrench_menu_->Init(wrench_menu_model_.get());
-#endif
-
-  for (size_t i = 0; i < menu_listeners_.size(); ++i)
-    menu_listeners_[i]->OnMenuOpened();
-
-#if defined(OS_CHROMEOS)
   gfx::Point screen_loc;
   views::View::ConvertPointToScreen(app_menu_, &screen_loc);
   gfx::Rect bounds(screen_loc, app_menu_->size());
+
   // TODO(oshima): Support RTL.
   wrench_menu_->RunMenuAt(gfx::Point(bounds.right(), bounds.bottom()),
                           views::Menu2::ALIGN_TOPRIGHT);
-#else
+
+#else // for all other views
+  wrench_menu_ = new WrenchMenu(browser_);
+  wrench_menu_->Init(wrench_menu_model_.get());
+
+  for (size_t i = 0; i < menu_listeners_.size(); ++i)
+    menu_listeners_[i]->OnMenuOpened();
+
   wrench_menu_->RunMenu(app_menu_);
 #endif
 
diff --git a/chrome/browser/views/toolbar_view.h b/chrome/browser/views/toolbar_view.h
index f79ec25..396a392 100644
--- a/chrome/browser/views/toolbar_view.h
+++ b/chrome/browser/views/toolbar_view.h
@@ -198,16 +198,19 @@ class ToolbarView : public AccessibleToolbarView,
   // The contents of the wrench menu.
   scoped_ptr<menus::SimpleMenuModel> wrench_menu_model_;
 
-  // Wrench menu.
 #if defined(OS_CHROMEOS)
+  // Wrench menu.
   scoped_ptr<views::Menu2> wrench_menu_;
+
+  // MenuLister is managed by Menu2 on chromeos.
+
 #else
+  // Wrench menu.
   scoped_refptr<WrenchMenu> wrench_menu_;
-#endif
 
   // Vector of listeners to receive callbacks when the menu opens.
   std::vector<views::MenuListener*> menu_listeners_;
-
+#endif
   // The animation that makes the update reminder pulse.
   scoped_ptr<SlideAnimation> update_reminder_animation_;
 
diff --git a/chrome/test/interactive_ui/keyboard_access_uitest.cc b/chrome/test/interactive_ui/keyboard_access_uitest.cc
index ddaaacb..849571e 100644
--- a/chrome/test/interactive_ui/keyboard_access_uitest.cc
+++ b/chrome/test/interactive_ui/keyboard_access_uitest.cc
@@ -105,20 +105,11 @@ void KeyboardAccessTest::TestMenuKeyboardAccess(bool alternate_key_sequence,
   ASSERT_EQ(1, tab_index);
 }
 
-#if defined(OS_CHROMEOS)
-// crosbug.com/7642.
-#define MAYBE_TestMenuKeyboardAccess DISABLED_TestMenuKeyboardAccess
-#define MAYBE_TestAltMenuKeyboardAccess DISABLED_TestAltMenuKeyboardAccess
-#else
-#define MAYBE_TestMenuKeyboardAccess TestMenuKeyboardAccess
-#define MAYBE_TestAltMenuKeyboardAccess TestAltMenuKeyboardAccess
-#endif
-
-TEST_F(KeyboardAccessTest, MAYBE_TestMenuKeyboardAccess) {
+TEST_F(KeyboardAccessTest, TestMenuKeyboardAccess) {
   TestMenuKeyboardAccess(false, 0);
 }
 
-TEST_F(KeyboardAccessTest, MAYBE_TestAltMenuKeyboardAccess) {
+TEST_F(KeyboardAccessTest, TestAltMenuKeyboardAccess) {
   TestMenuKeyboardAccess(true, 0);
 }