Cleanups and trivial improvements for omnibox tests.

Changing the click tracking overlay view makes it clearer, when the test fails,
if the overlay received a click or not.

Touch events won't be supported on Mac for the foreseeable future, so #if those
out entirely.

Extract duplicated line from an #if.

Bug: none
Change-Id: I0eac1154459da170a9c6924a20cc290361a219ab
Reviewed-on: https://chromium-review.googlesource.com/c/1338860
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608977}

chrome/browser/ui/views/omnibox/omnibox_popup_contents_view_browsertest.cc

@@ -40,19 +40,20 @@ namespace {
 // A View that positions itself over another View to intercept clicks.
 class ClickTrackingOverlayView : public views::View {
  public:
-  ClickTrackingOverlayView(views::View* over, gfx::Point* last_click)
-      : last_click_(last_click) {
+  explicit ClickTrackingOverlayView(views::View* over) {
     SetBoundsRect(over->bounds());
     over->parent()->AddChildView(this);
   }
 
   // views::View:
   void OnMouseEvent(ui::MouseEvent* event) override {
-    *last_click_ = event->location();
+    last_click_ = event->location();
   }
 
+  base::Optional<gfx::Point> last_click() const { return last_click_; }
+
  private:
-  gfx::Point* last_click_;
+  base::Optional<gfx::Point> last_click_;
 };
 
 // Helper to wait for theme changes. The wait is triggered when an instance of
@@ -273,11 +274,12 @@ IN_PROC_BROWSER_TEST_F(OmniboxPopupContentsViewTest, MAYBE_ClickOmnibox) {
     const gfx::Point expected_point = result->GetLocalBounds().CenterPoint();
     EXPECT_NE(gfx::Point(), expected_point);
 
-    gfx::Point click;
-    ClickTrackingOverlayView overlay(result, &click);
+    ClickTrackingOverlayView overlay(result);
     generator.MoveMouseTo(result->GetBoundsInScreen().CenterPoint());
     generator.ClickLeftButton();
-    EXPECT_EQ(expected_point, click);
+    auto click = overlay.last_click();
+    ASSERT_TRUE(click.has_value());
+    ASSERT_EQ(expected_point, click.value());
   }
 
   // Select the text, so that we can test whether a click is received (which

chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc

@@ -226,11 +226,10 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnClick) {
                                 click_location, click_location));
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
-  EXPECT_FALSE(omnibox_view->IsSelectAll());
 #else
   EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
-  EXPECT_FALSE(omnibox_view->IsSelectAll());
 #endif  // OS_LINUX && !OS_CHROMEOS
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
 }
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
@@ -282,13 +281,10 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectionClipboard) {
 }
 #endif  // OS_LINUX && !OS_CHROMEOS
 
-// MacOS does not support touch.
-#if defined(OS_MACOSX)
-#define MAYBE_SelectAllOnTap DISABLED_SelectAllOnTap
-#else
-#define MAYBE_SelectAllOnTap SelectAllOnTap
-#endif
-IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, MAYBE_SelectAllOnTap) {
+// No touch on desktop Mac. Tracked in http://crbug.com/445520.
+#if !defined(OS_MACOSX) || defined(USE_AURA)
+
+IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnTap) {
   OmniboxView* omnibox_view = NULL;
   ASSERT_NO_FATAL_FAILURE(GetOmniboxViewForBrowser(browser(), &omnibox_view));
   omnibox_view->SetUserText(base::ASCIIToUTF16("http://www.google.com/"));
@@ -333,6 +329,32 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, MAYBE_SelectAllOnTap) {
   EXPECT_FALSE(omnibox_view->IsSelectAll());
 }
 
+// Tests if executing a command hides touch editing handles.
+IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest,
+                       DeactivateTouchEditingOnExecuteCommand) {
+  OmniboxView* view = NULL;
+  ASSERT_NO_FATAL_FAILURE(GetOmniboxViewForBrowser(browser(), &view));
+  OmniboxViewViews* omnibox_view_views = static_cast<OmniboxViewViews*>(view);
+  views::TextfieldTestApi textfield_test_api(omnibox_view_views);
+
+  // Put a URL on the clipboard.
+  SetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE, "http://www.example.com/");
+
+  // Tap to activate touch editing.
+  gfx::Point omnibox_center =
+      omnibox_view_views->GetBoundsInScreen().CenterPoint();
+  Tap(omnibox_center, omnibox_center);
+  EXPECT_TRUE(textfield_test_api.touch_selection_controller());
+
+  // Execute a command and check if it deactivate touch editing. Paste & Go is
+  // chosen since it is specific to Omnibox and its execution wouldn't be
+  // delegated to the base Textfield class.
+  omnibox_view_views->ExecuteCommand(IDS_PASTE_AND_GO, ui::EF_NONE);
+  EXPECT_FALSE(textfield_test_api.touch_selection_controller());
+}
+
+#endif  // !defined(OS_MACOSX) || defined(USE_AURA)
+
 IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnTabToFocus) {
   OmniboxView* omnibox_view = NULL;
   ASSERT_NO_FATAL_FAILURE(GetOmniboxViewForBrowser(browser(), &omnibox_view));
@@ -468,38 +490,6 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, BackgroundIsOpaque) {
   EXPECT_FALSE(view->GetRenderText()->subpixel_rendering_suppressed());
 }
 
-// MacOS does not support touch.
-#if defined(OS_MACOSX)
-#define MAYBE_DeactivateTouchEditingOnExecuteCommand \
-  DISABLED_DeactivateTouchEditingOnExecuteCommand
-#else
-#define MAYBE_DeactivateTouchEditingOnExecuteCommand \
-  DeactivateTouchEditingOnExecuteCommand
-#endif
-// Tests if executing a command hides touch editing handles.
-IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest,
-                       MAYBE_DeactivateTouchEditingOnExecuteCommand) {
-  OmniboxView* view = NULL;
-  ASSERT_NO_FATAL_FAILURE(GetOmniboxViewForBrowser(browser(), &view));
-  OmniboxViewViews* omnibox_view_views = static_cast<OmniboxViewViews*>(view);
-  views::TextfieldTestApi textfield_test_api(omnibox_view_views);
-
-  // Put a URL on the clipboard.
-  SetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE, "http://www.example.com/");
-
-  // Tap to activate touch editing.
-  gfx::Point omnibox_center =
-      omnibox_view_views->GetBoundsInScreen().CenterPoint();
-  Tap(omnibox_center, omnibox_center);
-  EXPECT_TRUE(textfield_test_api.touch_selection_controller());
-
-  // Execute a command and check if it deactivate touch editing. Paste & Go is
-  // chosen since it is specific to Omnibox and its execution wouldn't be
-  // delegated to the base Textfield class.
-  omnibox_view_views->ExecuteCommand(IDS_PASTE_AND_GO, ui::EF_NONE);
-  EXPECT_FALSE(textfield_test_api.touch_selection_controller());
-}
-
 IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, FocusedTextInputClient) {
   chrome::FocusLocationBar(browser());
   OmniboxView* view = NULL;