Update Flaky ToolbarActionViewInteractiveUITests

chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc

 // Copyright 2015 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.
 
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ skipping 21 matching lines @@
 namespace {
 
 AppMenuButton* GetAppButtonFromBrowser(Browser* browser) {
   return BrowserView::GetBrowserViewForBrowser(browser)
       ->toolbar()
       ->app_menu_button();
 }
 
 // Tests clicking on an overflowed toolbar action. This is called when the app
 // menu is open, and handles actually clicking on the action.
-// |button| specifies the mouse button to click with.
-void TestOverflowedToolbarAction(Browser* browser,
-                                 ui_controls::MouseButton button) {
+// |button| specifies the mouse button to click with. Returning the targetted

[Devlin, 2016/08/25 16:42:12]

s/Returning/Returns

[Devlin, 2016/08/25 16:42:12]

typo: targetted

[jonross, 2016/08/25 18:29:27]

Done.

[jonross, 2016/08/25 18:29:27]

Done.

+// ToolbarActionView.
+ToolbarActionView* TestOverflowedToolbarAction(
+    Browser* browser,
+    ui_controls::MouseButton button) {
   // A bunch of plumbing to safely get at the overflowed toolbar action.
   AppMenuButton* app_menu_button = GetAppButtonFromBrowser(browser);
   EXPECT_TRUE(app_menu_button->IsMenuShowing());
   AppMenu* app_menu = app_menu_button->app_menu_for_testing();
-  ASSERT_TRUE(app_menu);
+  EXPECT_TRUE(app_menu);

[Devlin, 2016/08/25 16:42:12]

The reason these were asserts is because if they're false, we segfault.  I think
they should remain asserts. :)

[jonross, 2016/08/25 18:29:27]

I removed these as they interfere with return values.

I've restored them for the early segfaults, and switched to an optional out
param

   ExtensionToolbarMenuView* menu_view =
       app_menu->extension_toolbar_for_testing();
-  ASSERT_TRUE(menu_view);
+  EXPECT_TRUE(menu_view);
   BrowserActionsContainer* overflow_container =
       menu_view->container_for_testing();
-  ASSERT_TRUE(overflow_container);
+  EXPECT_TRUE(overflow_container);
   ToolbarActionView* action_view =
       overflow_container->GetToolbarActionViewAt(0);
   EXPECT_TRUE(action_view->visible());
 
   // Click on the toolbar action to activate it.
   gfx::Point action_view_loc =
       ui_test_utils::GetCenterInScreenCoordinates(action_view);
   ui_controls::SendMouseMove(action_view_loc.x(), action_view_loc.y());
   EXPECT_TRUE(ui_test_utils::SendMouseEventsSync(
       button, ui_controls::DOWN | ui_controls::UP));
+  return action_view;
 }
 
 // Tests the context menu of an overflowed action.
-void TestWhileContextMenuOpen(bool* did_test_while_menu_open,
-                              Browser* browser,
+void TestWhileContextMenuOpen(Browser* browser,
                               ToolbarActionView* context_menu_action) {
-  *did_test_while_menu_open = true;
-
   views::MenuItemView* menu_root = context_menu_action->menu_for_testing();
   ASSERT_TRUE(menu_root);
   ASSERT_TRUE(menu_root->GetSubmenu());
   EXPECT_TRUE(menu_root->GetSubmenu()->IsShowing());
   views::MenuItemView* first_menu_item =
       menu_root->GetSubmenu()->GetMenuItemAt(0);
   ASSERT_TRUE(first_menu_item);
 
   // Make sure we're showing the right context menu.
   EXPECT_EQ(base::UTF8ToUTF16("Browser Action Popup"),
@@ skipping 28 matching lines @@
                                       &action_view_loc_in_menu_item_bounds);
   // Regression test for crbug.com/538414: The first menu item is overlapping
   // the second row action button. With crbug.com/538414, the click would go to
   // the menu button, instead of the menu item.
   EXPECT_TRUE(
       first_menu_item->HitTestPoint(action_view_loc_in_menu_item_bounds));
 
   // Click on the first menu item (which shares bounds, but overlaps, the second
   // row action).
   ui_controls::SendMouseMove(action_view_loc.x(), action_view_loc.y());
-  ui_controls::SendMouseEventsNotifyWhenDone(
-      ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP, base::Closure());
-
+  EXPECT_TRUE(ui_test_utils::SendMouseEventsSync(
+      ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP));
   // Test resumes in the main test body.
 }
 
-// Posts a task to test the context menu.
-void OnContextMenuWillShow(bool* did_test_while_menu_open,
-                           Browser* browser,
-                           ToolbarActionView* toolbar_action_view) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&TestWhileContextMenuOpen, did_test_while_menu_open,
-                            browser, toolbar_action_view));
-}
-
 }  // namespace
 
 class ToolbarActionViewInteractiveUITest : public ExtensionBrowserTest {
  protected:
   ToolbarActionViewInteractiveUITest();
   ~ToolbarActionViewInteractiveUITest() override;
 
   // ExtensionBrowserTest:
   void SetUpCommandLine(base::CommandLine* command_line) override;
   void TearDownOnMainThread() override;
@@ skipping 60 matching lines @@
   // The app menu should no longer be showing.
   EXPECT_FALSE(app_menu_button->IsMenuShowing());
 
   // And the extension should have been activated.
   listener.WaitUntilSatisfied();
 }
 
 // TODO(jonross): determine cause of new flake, and restore previous MAYBE
 // conditions. Temporarily disabling due to number of flakes (crbug.com/639010)
 // Tests the context menus of overflowed extension actions.
+
+#if defined(USE_OZONE)
+// ozone bringup - http://crbug.com/401304
+#define MAYBE_TestContextMenuOnOverflowedAction \
+  DISABLED_TestContextMenuOnOverflowedAction
+#else
+#define MAYBE_TestContextMenuOnOverflowedAction \
+  TestContextMenuOnOverflowedAction
+#endif
 IN_PROC_BROWSER_TEST_F(ToolbarActionViewInteractiveUITest,
-                       DISABLED_TestContextMenuOnOverflowedAction) {
+                       MAYBE_TestContextMenuOnOverflowedAction) {
   views::MenuController::TurnOffMenuSelectionHoldForTest();
 
   // Load an extension that has a home page (important for the context menu's
   // first item being enabled).
   ASSERT_TRUE(LoadExtension(
       test_data_dir_.AppendASCII("ui").AppendASCII("browser_action_popup")));
   base::RunLoop().RunUntilIdle();  // Ensure the extension is fully loaded.
 
   // Aaaannnnd... Load a bunch of other extensions so that the overflow menu
   // is spread across multiple rows.
   for (int i = 0; i < 15; ++i) {
     scoped_refptr<const extensions::Extension> extension =
         extensions::extension_action_test_util::CreateActionExtension(
             base::IntToString(i),
             extensions::extension_action_test_util::BROWSER_ACTION);
     extension_service()->AddExtension(extension.get());
   }
 
   ASSERT_EQ(16u, browser()
                      ->window()
                      ->GetToolbarActionsBar()
                      ->toolbar_actions_unordered()
                      .size());
 
   // Reduce visible count to 0 so that all actions are overflowed.
   ToolbarActionsModel::Get(profile())->SetVisibleIconCount(0);
 
-  // Set a callback for the context menu showing.
-  bool did_test_while_menu_open = false;
-  base::Callback<void(ToolbarActionView*)> context_menu_callback(
-      base::Bind(&OnContextMenuWillShow, &did_test_while_menu_open, browser()));
-  ToolbarActionView::set_context_menu_callback_for_testing(
-      &context_menu_callback);
-
   AppMenuButton* app_menu_button = GetAppButtonFromBrowser(browser());
   // Click on the app button, and then right-click on the first toolbar action.
   gfx::Point app_button_loc =
       ui_test_utils::GetCenterInScreenCoordinates(app_menu_button);
   ui_controls::SendMouseMove(app_button_loc.x(), app_button_loc.y());
   EXPECT_TRUE(ui_test_utils::SendMouseEventsSync(
       ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP));
-
-  base::RunLoop().RunUntilIdle();
-  TestOverflowedToolbarAction(browser(), ui_controls::RIGHT);
   base::RunLoop().RunUntilIdle();
 
-  // Test is continued first in TestOverflowedToolbarAction() to right click on
-  // the action, followed by OnContextMenuWillShow() and
-  // TestWhileContextMenuOpen().
+  // Right clicks on the action view, this should trigger the context menu.
+  ToolbarActionView* action_view =
+      TestOverflowedToolbarAction(browser(), ui_controls::RIGHT);
+  base::RunLoop().RunUntilIdle();
 
-  // Make sure we did all the expected tests.
-  EXPECT_TRUE(did_test_while_menu_open);
+  // Ensure that the menu actually opened.
+  EXPECT_TRUE(action_view->IsMenuRunning());
 
+  // Triggers the action within the context menu. This should load the extension
+  // webpage, and close the menu.
+  TestWhileContextMenuOpen(browser(), action_view);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(action_view->IsMenuRunning());
   // We should have navigated to the extension's home page, which is google.com.
   EXPECT_EQ(
       GURL("https://www.google.com/"),
       browser()->tab_strip_model()->GetActiveWebContents()->GetVisibleURL());
 }
 
 // Tests that clicking on the toolbar action a second time when the action is
 // already open results in closing the popup, and doesn't re-open it.
 IN_PROC_BROWSER_TEST_F(ToolbarActionViewInteractiveUITest,
                        DoubleClickToolbarActionToClose) {
@@ skipping 47 matching lines @@
   EXPECT_FALSE(view_controller->is_showing_popup());
   EXPECT_EQ(nullptr, toolbar_actions_bar->popup_owner());
 
   // Releasing the mouse shouldn't result in the popup being shown again.
   EXPECT_TRUE(
       ui_test_utils::SendMouseEventsSync(ui_controls::LEFT, ui_controls::UP));
   EXPECT_FALSE(view_controller->is_showing_popup());
   EXPECT_EQ(nullptr, toolbar_actions_bar->popup_owner());
 }
 
-#if defined(USE_OZONE) || defined(OS_WIN)
+#if defined(USE_OZONE)
 // ozone bringup - http://crbug.com/401304
-// flaky on Windows - http://crbug.com/638692
 #define MAYBE_ActivateOverflowedToolbarActionWithKeyboard \
   DISABLED_ActivateOverflowedToolbarActionWithKeyboard
 #else
 #define MAYBE_ActivateOverflowedToolbarActionWithKeyboard \
   ActivateOverflowedToolbarActionWithKeyboard
 #endif
 IN_PROC_BROWSER_TEST_F(ToolbarActionViewInteractiveUITest,
                        MAYBE_ActivateOverflowedToolbarActionWithKeyboard) {
   views::MenuController::TurnOffMenuSelectionHoldForTest();
   // Load an extension with an action.
@@ skipping 20 matching lines @@
 
   EXPECT_TRUE(app_menu_button->IsMenuShowing());
   gfx::NativeWindow native_window =
       views::MenuController::GetActiveInstance()->owner()->GetNativeWindow();
   // Send a key down event followed by the return key.
   // The key down event targets the toolbar action in the app menu.
   ui_controls::SendKeyPress(native_window, ui::VKEY_DOWN, false, false, false,
                             false);
   // The triggering of the action and subsequent widget destruction occurs on
   // the message loop. Wait for this all to complete.
-  base::RunLoop loop;
-  ui_controls::SendKeyPressNotifyWhenDone(native_window, ui::VKEY_RETURN, false,
-                                          false, false, false,
-                                          loop.QuitClosure());
-  loop.Run();
+  EXPECT_TRUE(ui_test_utils::SendKeyPressToWindowSync(
+      native_window, ui::VKEY_RETURN, false, false, false, false));
+  base::RunLoop().RunUntilIdle();
 
   // The menu should be closed.
   EXPECT_FALSE(app_menu_button->IsMenuShowing());
   // And the extension should have been activated.
   EXPECT_TRUE(listener.WaitUntilSatisfied());
 }