ash: Select omnibox text on mouse up instead of down.

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

Index: chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
index 956490d00cf6f30cece7dc7c1c29b54f9f8bead1..edab0826f976588e3f80358fc15a6d35bbd60cf8 100644
--- a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
+++ b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
@@ -36,16 +36,15 @@
 #include "net/base/mock_host_resolver.h"
 #include "ui/base/events.h"
 #include "ui/base/keycodes/keyboard_codes.h"
+#include "ui/gfx/point.h"
 
 #if defined(TOOLKIT_GTK)
 #include <gdk/gdk.h>
 #include <gtk/gtk.h>
 #endif
 
-#if defined(TOOLKIT_VIEWS)
-#include "ui/views/controls/textfield/native_textfield_views.h"
-#include "ui/views/events/event.h"
-#include "ui/views/widget/widget.h"
+#if defined(USE_AURA)
+#include "chrome/browser/ui/views/frame/browser_view.h"
 #endif
 
 using base::Time;
@@ -1270,6 +1269,45 @@ class OmniboxViewTest : public InProcessBrowserTest,
     EXPECT_EQ(old_text, omnibox_view->GetText());
   }
 
+#if defined(USE_AURA)
+  BrowserView* GetBrowserView() {
+    return static_cast<BrowserView*>(browser()->window());
+  }
+
+  views::View* GetFocusView() {
+    return GetBrowserView()->GetViewByID(location_bar_focus_view_id_);
+  }
+
+  // Move the mouse to the center of the browser window and left-click.
+  void ClickBrowserWindowCenter() {
+    ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(
+                    GetBrowserView()->GetScreenBounds().CenterPoint()));
+    ASSERT_TRUE(ui_test_utils::SendMouseEventsSync(
+                    ui_controls::LEFT, ui_controls::DOWN));
+    ASSERT_TRUE(ui_test_utils::SendMouseEventsSync(
+                    ui_controls::LEFT, ui_controls::UP));
+  }
+
+  // Press and release the mouse in the focus view at an offset from its origin.
+  // If |release_offset| differs from |press_offset|, the mouse will be moved
+  // between the press and release.
+  void ClickFocusViewOrigin(ui_controls::MouseButton button,
+                            const gfx::Point& press_offset,
+                            const gfx::Point& release_offset) {
+    gfx::Point focus_view_origin = GetFocusView()->GetScreenBounds().origin();
+    gfx::Point press_point = focus_view_origin;
+    press_point.Offset(press_offset.x(), press_offset.y());
+    ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(press_point));
+    ASSERT_TRUE(ui_test_utils::SendMouseEventsSync(button, ui_controls::DOWN));
+
+    gfx::Point release_point = focus_view_origin;
+    release_point.Offset(release_offset.x(), release_offset.y());
+    if (release_point != press_point)
+      ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(release_point));
+    ASSERT_TRUE(ui_test_utils::SendMouseEventsSync(button, ui_controls::UP));
+  }
+#endif  // defined(USE_AURA)
+
  private:
   ViewID location_bar_focus_view_id_;
 };
@@ -1481,4 +1519,52 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, PasteReplacingAll) {
   // Inline autocomplete shouldn't be triggered.
   ASSERT_EQ(ASCIIToUTF16("abc"), omnibox_view->GetText());
 }
-#endif
+#endif  // defined(TOOLKIT_GTK)
+
+#if defined(USE_AURA)

[Daniel Erat, 2012/05/16 21:06:02]

I'd prefer to run these for TOOLKIT_VIEWS, but I'm getting failures from the
Windows bot:

E:\b\build\slave\win\build\src\chrome\browser\ui\omnibox\omnibox_view_browsertest.cc(1538):
error: Value of: omnibox_view->IsSelectAll()
  Actual: false
Expected: true
E:\b\build\slave\win\build\src\chrome\browser\ui\omnibox\omnibox_view_browsertest.cc(1546):
error: Value of: omnibox_view->IsSelectAll()
  Actual: false
Expected: true
E:\b\build\slave\win\build\src\chrome\browser\ui\omnibox\omnibox_view_browsertest.cc(1553):
error: Value of: omnibox_view->IsSelectAll()
  Actual: true
Expected: false
E:\b\build\slave\win\build\src\chrome\browser\ui\omnibox\omnibox_view_browsertest.cc(1561):
error: Value of: omnibox_view->IsSelectAll()
  Actual: true
Expected: false
[0516/133950:ERROR:test_launcher.cc(384)] Test timeout (45000 ms) exceeded for
OmniboxViewTest.SelectAllOnClick

Windows isn't using views::Textfield for the omnibox, right?

[oshima, 2012/05/16 21:28:31]

I think so. I have no idea why this doesn't work on native win version. peter
may know.

[Peter Kasting, 2012/05/16 21:42:25]

Right, it uses a subclass of a native CRichEditCtrl.

+IN_PROC_BROWSER_TEST_F(OmniboxViewTest, SelectAllOnClick) {
+  OmniboxView* omnibox_view = NULL;
+  ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));
+  omnibox_view->SetUserText(ASCIIToUTF16("http://www.google.com/"));
+  const gfx::Point kClickOffset(2, 2);
+
+  // Take the focus away from the omnibox.
+  ClickBrowserWindowCenter();
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(GetFocusView()->HasFocus());
+
+  // Click in the omnibox.  All of its text should be selected.
+  ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset);

[sky, 2012/05/16 21:39:56]

You should wrap functions you call that ASSERT with ASSERT_NO_FATAL_FAILURE.

[Daniel Erat, 2012/05/16 22:03:10]

Done.

+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(GetFocusView()->HasFocus());
+
+  // Ensure that all of the text is selected and then take the focus away.  The
+  // selection should persist.
+  omnibox_view->SelectAll(false);
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  ClickBrowserWindowCenter();
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(GetFocusView()->HasFocus());
+
+  // Clicking in the omnibox while some of its text is already selected should
+  // have the effect of unselecting the text, rather than re-selecting all of
+  // it.
+  ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset);
+  EXPECT_FALSE(omnibox_view->IsSelectAll());

[Peter Kasting, 2012/05/16 21:42:25]

This seems wrong.  If we create a selection or something in the omnibox, but
then click out of it, when we click back in to give it focus again we should
select all again.  This is what Windows does.

Now if the omnibox already has focus, and we click, then this test would be
correct.  (But make sure you don't effectively generate a double-click, which
will do its own thing.)

[Daniel Erat, 2012/05/16 22:03:10]

Done.

+  EXPECT_TRUE(GetFocusView()->HasFocus());
+
+  // Take the focus away and click in the omnibox again, but drag a bit before
+  // releasing.  We should focus the omnibox but not select all of its text.
+  ClickBrowserWindowCenter();
+  const gfx::Point kReleaseOffset(kClickOffset.x() + 10, kClickOffset.y());
+  ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kReleaseOffset);
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(GetFocusView()->HasFocus());
+
+  // Middle-clicking shouldn't select all the text either.
+  ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset);
+  ClickBrowserWindowCenter();
+  ClickFocusViewOrigin(ui_controls::MIDDLE, kClickOffset, kClickOffset);
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+}
+#endif  // defined(USE_AURA)