[omnibox] Narrow condition for resetting selection

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

 // Copyright (c) 2013 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 "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
 
 #include <stddef.h>
 
 #include "base/command_line.h"
 #include "base/macros.h"
@@ skipping 283 matching lines @@
   // Pressing tab to focus the omnibox should select all text.
   while (!ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX)) {
     ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_TAB,
                                                 false, false, false, false));
   }
   EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
   EXPECT_TRUE(omnibox_view->IsSelectAll());
 }
 
 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);
 
   // Populate suggestions for the omnibox popup.
   AutocompleteController* autocomplete_controller =
       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(),
       metrics::OmniboxEventProto::INVALID_SPEC, false, false, true, true, false,
       TestSchemeClassifier());
   results.AppendMatches(input, matches);
   results.SortAndCull(
       input, TemplateURLServiceFactory::GetForProfile(browser()->profile()));
 
   // The omnibox popup should open with suggestions displayed.
   omnibox_view->model()->popup_model()->OnResultChanged();
   EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen());
 
   // The omnibox text should be selected.
   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);
   omnibox_view_views->OnMousePressed(pressed);
   EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen());
 
   // Simulate a mouse drag of the omnibox text, and the omnibox should close.
   ui::MouseEvent dragged(ui::ET_MOUSE_DRAGGED, point, point,
                          ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON, 0);
   omnibox_view_views->OnMouseDragged(dragged);
 
   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.
   OmniboxViewViews* view = BrowserView::GetBrowserViewForBrowser(browser())->
       toolbar()->location_bar()->omnibox_view();
   ASSERT_TRUE(view);
   EXPECT_FALSE(view->GetRenderText()->subpixel_rendering_suppressed());
 }
 
 // Tests if executing a command hides touch editing handles.
@@ skipping 23 matching lines @@
 IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, FocusedTextInputClient) {
   chrome::FocusLocationBar(browser());
   OmniboxView* view = NULL;
   ASSERT_NO_FATAL_FAILURE(GetOmniboxViewForBrowser(browser(), &view));
   OmniboxViewViews* omnibox_view_views = static_cast<OmniboxViewViews*>(view);
   ui::InputMethod* input_method =
       omnibox_view_views->GetWidget()->GetInputMethod();
   EXPECT_EQ(static_cast<ui::TextInputClient*>(omnibox_view_views),
             input_method->GetTextInputClient());
 }