[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;
   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;

[Peter Kasting, 2017/04/12 23:48:24]

Nit: Could do

  AutocompleteMatch match(nullptr, 500, false,
                          AutocompleteMatchType::HISTORY_TITLE);

...and ditch the explicit type/relevance setting below.

[Kevin Bailey, 2017/04/18 19:45:13]

Done.

+  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);
+  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 to trigger focus.
+  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);
+  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

[Peter Kasting, 2017/04/12 23:48:24]

Nit: Be clearer (since "the above" covers a lot); e.g. "Refocus the omnibox."

[Kevin Bailey, 2017/04/18 19:45:13]

Done.

+  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.

[Peter Kasting, 2017/04/12 23:48:24]

Nit: This really goes above the declaration of start/end (since getting the
selection bounds is part of checking the selection bounds) and should have a
blank line above it (to distinguish between the block that's about refocusing
and this one).

[Kevin Bailey, 2017/04/18 19:45:13]

Done.

+  EXPECT_NE(0U, start);

[Peter Kasting, 2017/04/12 23:48:25]

Nit: It's not immediately obvious why this should be nonzero.  I think what we
really want is to ensure the cursor position/selection is preserved across
blur/focus.  Maybe we should explicitly set the cursor position to some value
before blurring and then check that it's that value after refocusing.

[Kevin Bailey, 2017/04/18 19:45:13]

Done.

+  // Blur or it dchecks when rendering. The above queues messages for the
+  // task which get handled after the test is finished. Blurring quiesces
+  // them.

[Peter Kasting, 2017/04/12 23:48:24]

OK... this doesn't really make things clearer than your explanation to me, which
left me still not really understanding the problem :).  Also this comment is
still pretty ambiguous: "it dchecks" -> what dchecks?  "the task" -> which task?
 "get handler after the test is finished" -> why does that cause a dcheck? 
"Blurring quiesces them" -> why and how?

I would be picking on this less if I truly trusted that this is the right
solution.  But I don't; AFAIK no other tests blur on exit like this, so it seems
extremely unusual.  I'd imagine either we should spin the message loop here, or
we should ensure the problematic messages don't get queued, or we should ensure
running them as this test shuts down doesn't cause problems, or we should ensure
they don't get run after the test finishes.

[Kevin Bailey, 2017/04/13 00:07:12]

The part about this that bothers me is that the new test isn't doing anything
crazy (that I can see); just blur, re-focus. Regarding blurring, someone might
just as well ask, "None of the other tests spin the message loop." It feels more
like the test is forcing the Omnibox machinery through states that it doesn't
expect and that a correct sequence wouldn't have this problem (but I'm at a bit
of a loss as to what's wrong with it.)

[Peter Kasting, 2017/04/13 00:12:15]

Yes, I agree with your instinct.  Spinning the message loop still kinda makes me
uncomfortable.  Would focusing alone (no blur cycles) cause problems?  If so
don't other tests focus the omnibox, and what do they do to handle this?

What are the precise DCHECKs and how are they triggering?

[Kevin Bailey, 2017/04/17 16:39:11]

(For the reader, since this was discussed off-line) the 'contents' field of a
match hadn't been filled in yet, which didn't matter in the other tests, but
this one was sometimes getting to the point where it would attempt to render an
empty (invalid) match.

Fixed, albeit forced, by manually filling in the contents.

+  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.
   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());
 }