[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);
+
+  // 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(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;
+  matches.push_back(match);
+  const AutocompleteInput input(
+      base::ASCIIToUTF16("autocomplete-result"), 19, "autocomplete-result",
+      GURL("http://autocomplete-result/"),
+      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());
+
+#if 0
+  // Simulate a mouse click to trigger focus. Click well right of text.
+  gfx::Point point(omnibox_view_views->origin());
+  point = gfx::Point(point.x() + 100, point.y() + 1);
+  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);
+  omnibox_view_views->OnMouseReleased(pressed);
+#else
+//  omnibox_view_views->SetUserText(base::ASCIIToUTF16("autocomplete-result"));
+  omnibox_view_views->SetWindowTextAndCaretPos(

[Kevin Bailey, 2017/04/18 13:40:09]

If I replace 'false, false' with 'true, false', it dchecks because 'user_text_'
isn't set. If I uncomment setting it, it dchecks in a stack trace that I can
recognize very little of.

[Peter Kasting, 2017/04/18 20:47:49]

Sorry, but there's too much functionality under these to know offhand what might
be going wrong without you giving me actual stacks.

Note that trying to do this stuff without removing all the code above at the
beginning of the test (as I suggested) is highly likely to screw things up,
since that code pokes at the underlying data structures in ways that won't leave
the internal state consistent (it calls the notification methods for
autocompletion runs without actually running autocompletion or setting the
omnibox text).

I'd move your #if 0 up to cover most of the preceding code and then see what
your stacks look like.

Separately, calling SetUserText() alone, without SetWindowTextAndCaretPos(), is
probably even better at doing what you want here.

[Kevin Bailey, 2017/04/19 13:12:51]

By "most", I assume "OnResultChanged()", since we need the results.

+      base::ASCIIToUTF16("autocomplete-result"),
+      19, false, false);
+#endif
+  EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen());
+
+  // Save cursor position, before blur.
+  size_t prev_start, end;
+  omnibox_view->GetSelectionBounds(&prev_start, &end);
+
+#if 0
+  omnibox_view_views->OnBlur();
+#else
+  ClickBrowserWindowCenter();
+#endif
+  EXPECT_FALSE(omnibox_view->model()->popup_model()->IsOpen());
+
+  // Redo the above.
+#if 0
+  omnibox_view_views->OnFocus();
+#else
+  chrome::FocusLocationBar(browser());
+  omnibox_view_views->UpdatePopup();

[Kevin Bailey, 2017/04/18 13:40:09]

With this 'UpdatePopup()', the test fails because the cursor is at 0. Without
it, the cursor is at 11, and the box no doubt contains 'about:blank'.

[Peter Kasting, 2017/04/18 20:47:49]

Part of the problem here is that FocusLocationBar() will also select all (which
I didn't realize).

However, you could call LocationBarView::FocusLocation() to avoid this.

As for UpdatePopup(), do you even care about reopening the popup?  You just care
about preserving the cursor position on refocusing, right?  In which case that
call can just be dropped.

In either case, you also need to remove all the mucking with |results| and
calling of OnResultChanged() below, since it has the same problem as above (it
goofs with the internal methods instead of just letting the normal control flow
pathways handle this).  Either you don't want this at all, or else it would be
done for you by UpdatePopup().

[Kevin Bailey, 2017/04/19 13:12:51]

This part is actually working fine, although I trimmed it as you requested.

+#endif
+  results.AppendMatches(input, matches);
+  results.SortAndCull(
+      input, TemplateURLServiceFactory::GetForProfile(browser()->profile()));
+  omnibox_view->model()->popup_model()->OnResultChanged();
+  EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen());
+
+  // Make sure cursor is restored.
+  size_t start = 0;
+  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());
 }