[omnibox] Narrow condition for resetting selection

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

Index: chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
index 91b12dd6237d52870b183983672973778281b1cf..76374003629933524efe9430ac580a1ad7833af8 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
@@ -301,7 +301,7 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnTabToFocus) {
 }
 
 IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, CloseOmniboxPopupOnTextDrag) {
-  OmniboxView* omnibox_view = NULL;
+  OmniboxView* omnibox_view = nullptr;
   ASSERT_NO_FATAL_FAILURE(GetOmniboxViewForBrowser(browser(), &omnibox_view));
   OmniboxViewViews* omnibox_view_views =
       static_cast<OmniboxViewViews*>(omnibox_view);
@@ -311,13 +311,13 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, CloseOmniboxPopupOnTextDrag) {
       omnibox_view->model()->popup_model()->autocomplete_controller();
   AutocompleteResult& results = autocomplete_controller->result_;
   ACMatches matches;
-  AutocompleteMatch match;
+  AutocompleteMatch match(nullptr, 500, false,
+                          AutocompleteMatchType::HISTORY_TITLE);
+  match.contents = base::ASCIIToUTF16("http://autocomplete-result/");
+  match.contents_class.push_back(
+      ACMatchClassification(0, ACMatchClassification::URL));
   match.destination_url = GURL("http://autocomplete-result/");
   match.allowed_to_be_default_match = true;
-  match.type = AutocompleteMatchType::HISTORY_TITLE;
-  match.relevance = 500;
-  matches.push_back(match);
-  match.destination_url = GURL("http://autocomplete-result2/");
   matches.push_back(match);
   const AutocompleteInput input(
       base::ASCIIToUTF16("a"), base::string16::npos, std::string(), GURL(),
@@ -335,7 +335,7 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, CloseOmniboxPopupOnTextDrag) {
   EXPECT_TRUE(omnibox_view->IsSelectAll());
 
   // Simulate a mouse click before dragging the mouse.
-  gfx::Point point(omnibox_view_views->x(), omnibox_view_views->y());
+  gfx::Point point(omnibox_view_views->origin());
   ui::MouseEvent pressed(ui::ET_MOUSE_PRESSED, point, point,
                          ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON,
                          ui::EF_LEFT_MOUSE_BUTTON);
@@ -350,6 +350,32 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, CloseOmniboxPopupOnTextDrag) {
   EXPECT_FALSE(omnibox_view->model()->popup_model()->IsOpen());
 }
 
+IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, MaintainCursorAfterFocusCycle) {
+  OmniboxView* omnibox_view = nullptr;
+  ASSERT_NO_FATAL_FAILURE(GetOmniboxViewForBrowser(browser(), &omnibox_view));
+  OmniboxViewViews* omnibox_view_views =
+      static_cast<OmniboxViewViews*>(omnibox_view);
+
+  omnibox_view_views->SetUserText(base::ASCIIToUTF16("autocomplete-result"),
+                                  true);
+  EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen());

[Peter Kasting, 2017/04/20 04:14:38]

I was thinking about why you even care whether the popup is open, and I realized
I don't think this test regression-tests your change at all.  It basically
checks that CloseOmniboxPopup() doesn't nuke cursor position.  But that was true
before the change.

To test this, you need to actually trigger zerosuggest.  This may be hard to do
in a test unless you can inject providers or args somehow -- I dunno how? 
Another way might be to have a unittest that actually creates an omnibox view,
autocomplete controller, and fake autocomplete provider that always returns
matches when run when the input is from focus.  Then, basically, do the body of
this test.

I'm not sure anybody does something like this today for you to copy from.

It may be that your original test, which mucked with the popup without actually
triggering user input, got closer to testing this stuff, albeit in a rather
artificial sort of way instead of by actually using zerosuggest.  Which means I
may have spent several days of your time leading you on a complete wild goose
chase.  Which would be... very embarrassing.  :/

I would try and verify that your original test fails without the rest of your
patch, and this one passes.  That would be confirmation that we want to do
something like what it does, and not this.

Although really we might want to expand your original test to also check the
case where there are no zerosuggest results (which should preserve selection
both before and after your patch) and the case where the permanent text changes
while the popup is open (which should reset selection before and after your
patch).

[Kevin Bailey, 2017/04/20 15:13:04]

The last one that differentiates the fix is patchset 10, which manufactures
results and simulates a click (ie. before SetUserText) Should I cheer up the end
of it - use FocusAppMenu() and lose the second set of results mucking - or do we
want an omnibus test? I'm leaning 'not for this CL', but if the existing tests
don't have enough coverage, we should work on improving them.

[Peter Kasting, 2017/04/20 17:16:07]

I think it's OK to clean up that patch and land it and then think about how to
get better test coverage here.  The omnibox _is_ historically poorly tested
(blame me, I didn't really believe in testing ten years ago when I was writing
it), and while there are scattered low-level and very-high-level tests, it's
really difficult to test something "in the middle", which is what you want here,
and what we'd want for a lot of different subtle OmniboxView polish fixes.

[Kevin Bailey, 2017/04/20 19:21:38]

Ok, brought back as little (I hope) code as necessary to trigger old error.

+
+  // Save cursor position, before blur.
+  size_t prev_start, end;
+  omnibox_view->GetSelectionBounds(&prev_start, &end);
+
+  chrome::FocusAppMenu(browser());
+  EXPECT_FALSE(omnibox_view->model()->popup_model()->IsOpen());
+
+  // Re-focus.
+  chrome::FocusLocationBar(browser());
+
+  // Make sure cursor is restored.
+  size_t start;
+  omnibox_view->GetSelectionBounds(&start, &end);
+  EXPECT_EQ(prev_start, start);
+}
+
 IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, BackgroundIsOpaque) {
   // The omnibox text should be rendered on an opaque background. Otherwise, we
   // can't use subpixel rendering.