[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..fc1321cbd1a0724c531a2568ba18cb5ba71567b6 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,67 @@ 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());

[Peter Kasting, 2017/04/24 21:55:06]

It doesn't seem like testing this is valuable.  Also see next comment.

[Kevin Bailey, 2017/04/25 14:16:44]

I'll leave it in until below is resolved.

+
+  // Simulate a mouse click to trigger focus. Click well right of text.

[Peter Kasting, 2017/04/24 21:55:06]

(1) It seems like we should focus the location bar before we fake a zerosuggest
run (by putting results in, as we do above).
(2) I would think you could just FocusLocationBar() to focus the bar.

So I'd try to structure like:

  // Focus the omnibox, which in normal operation might trigger ZeroSuggest.
  FocusLocationBar();

  // Fake ZeroSuggest opening the popup.
  AutocompleteController* autocomplete_controller =
      omnibox_view->model()->popup_model()->autocomplete_controller();
  ...
  EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen());

  // Place the cursor in mid-text.
  SendKeyPressSync(VKEY_END);
  SendKeyPressSync(VKEY_LEFT);  // Or whatever key combo you want

  // Save cursor position to verify it's properly restored later.
  ...
  

[Kevin Bailey, 2017/04/25 14:16:43]

I don't know if I caught your intent. I'm assuming e.g. the
autocomplete_controller line corresponds to (left side)#360.

Adding a call to FocusLocationBar() before that makes no difference, I assume
because it's already focused (or is just focusing twice.)

Using a key instead of a mouse press causes it to pass even with the old code. I
believe part of the problem is that the assertion on (right side)#388 fails (the
text is still selected).

[Peter Kasting, 2017/04/26 01:35:40]

Yes.
OK.  Interesting.
That seems strange.  How can that be?  If the omnibox is really focused, sending
VKEY_END should place the caret at the end of the text.  Is there any way to
e.g. sleep for a second after each step to watch onscreen what the test is
doing, so you can see what's up?  Could calling OnResultChanged() somehow lose
focus, so FocusLocationBar() after rather than before that works?  This seems
really odd.  Does sending key presses to type characters cause the omnibox text
to change?

[Kevin Bailey, 2017/04/26 14:27:45]

Neither sleeping nor things like RunUntilIdle help. Strangely, what works
consistently is sending 2 'end' key events. It's as if the keys are doing
something like what clicking does: When you click the first time, it selects the
word. When you click the second time, it unselects it and places a cursor.

It's definitely focused. I had a test and print-out that verified it. However,
I've no clue if OnResultsChanged() is doing something else more subtle.

[Peter Kasting, 2017/04/26 17:28:34]

Wat.

What if you send VK_RIGHT instead of VK_END?  Maybe print out the selection
range after sending varieties of different cursor keys?

+  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);
+  EXPECT_TRUE(omnibox_view->model()->popup_model()->IsOpen());
+
+  // 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.