[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..0c9ce3a8019ae6b36ca33880c78e8922260706b1 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
@@ -350,6 +350,68 @@ 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 = NULL;

[Peter Kasting, 2017/04/10 23:04:13]

Nit: nullptr_t

[Kevin Bailey, 2017/04/11 14:12:42]

Fixed above too.

+  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;
+  match.destination_url = GURL("http://autocomplete-result/");
+  match.allowed_to_be_default_match = true;
+  match.type = AutocompleteMatchType::HISTORY_TITLE;
+  match.relevance = 500;

[Peter Kasting, 2017/04/10 23:04:13]

Do things like the type and relevance matter?

[Kevin Bailey, 2017/04/11 14:12:42]

I copy-pasted the previous test, since it was mostly what I wanted, and I didn't
know what the code cared about. Would you like me to factor the common code, or
would you genuinely like to shrink it?

[Peter Kasting, 2017/04/11 20:04:39]

I would likely shrink rather than refactor.  Maybe both places can be shrunk,
and maybe at that point they're still similar, and the code would be better
factoring these bits out, and so we can do both.  I don't know.  I mostly want
to ensure that tests do as little as possible beyond what they need in order to
test what they're trying to test.

[Kevin Bailey, 2017/04/12 14:57:03]

It needs the type and relevance, but only needed one result. I assume there's
not especially much left to factor.

+  matches.push_back(match);
+  match.destination_url = GURL("http://autocomplete-result2/");
+  matches.push_back(match);

[Peter Kasting, 2017/04/10 23:04:13]

Do we need two suggestions instead of just the one here?  Why?

[Kevin Bailey, 2017/04/11 14:12:41]

Same as above.

+  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());
+
+  // Simulate a mouse click before dragging the mouse.

[Peter Kasting, 2017/04/10 23:04:13]

(a) I don't understand the "why" of this
(b) I don't see any drag

[Kevin Bailey, 2017/04/11 14:12:41]

Stale. All I want is a mouse click and release. Comment fixed.

+  gfx::Point point(omnibox_view_views->x(), omnibox_view_views->y());

[Peter Kasting, 2017/04/10 23:04:13]

Nit: (omnibox_view_views->origin())?

[Kevin Bailey, 2017/04/11 14:12:41]

Fixed above too.

+  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());
+
+  omnibox_view_views->OnBlur();
+  EXPECT_FALSE(omnibox_view->model()->popup_model()->IsOpen());
+
+  // Redo the above
+  omnibox_view_views->OnFocus();
+  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());
+  size_t start = 0, end;
+  omnibox_view->GetSelectionBounds(&start, &end);
+  // Make sure cursor isn't homed.
+  EXPECT_NE(static_cast<size_t>(0), start);

[Peter Kasting, 2017/04/10 23:04:13]

Nit: Just say 0U

[Kevin Bailey, 2017/04/11 14:12:41]

Done.

+  // Blur or it dchecks when rendering.

[Peter Kasting, 2017/04/10 23:04:13]

What dchecks?  Why?

[Kevin Bailey, 2017/04/11 14:12:42]

There seem to be stale messages in the work queue which, when executed, refer to
text fields (or RenderText's, not sure) which no longer exist. (There is garbage
in various fields, that change from run to run.) I could try emptying the queue
prior to shut down, but it lives until leaving this test routine, so it seems
like a test artifact.

[Peter Kasting, 2017/04/11 20:04:39]

OK.  I'm not sure I fully understand the problem yet.  At the least, I'd suggest
having a much more detailed explanation here of exactly what the problem is and
why this solution is both useful and correct.  As it is, this comment basically
says "don't remove this line", which is more valuable than having no comment,
but leaves the reader unsure exactly what's going on.

[Kevin Bailey, 2017/04/12 14:57:03]

Done.

+  omnibox_view_views->OnBlur();
+}
+
 IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, BackgroundIsOpaque) {
   // The omnibox text should be rendered on an opaque background. Otherwise, we
   // can't use subpixel rendering.