[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..d770efdea1ffc46f74e05c015dde08d26fc949b8 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,88 @@ 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);
+
+  // 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.