Omnibox: Break up OmniboxViewTest.PreserveDisplayTextOnFocusSearch

This test breaks up OmniboxViewTest.PreserveDisplayTextOnFocusSearch,
which was a browser test, into three smaller unit tests.

OmniboxViewTest.PreserveDisplayTextOnFocusSearch is flaky, and the
whole set of OmniboxViewTest.* browser tests have been flaky for at
least a year.

Bug: 751031
Change-Id: I6e7a8fc88f55fee706cdfc1ee0617ce4a27f5103
Reviewed-on: https://chromium-review.googlesource.com/1232525Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593246}

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

@@ -940,60 +940,6 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, FocusSearchLongUrl) {
   ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
 }
 
-// Make sure the display text is preserved when calling FocusSearch() when the
-// display text is not the permanent text.
-IN_PROC_BROWSER_TEST_F(OmniboxViewTest, PreserveDisplayTextOnFocusSearch) {
-  OmniboxView* omnibox_view = NULL;
-  ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));
-
-  OmniboxPopupModel* popup_model = omnibox_view->model()->popup_model();
-  ASSERT_TRUE(popup_model);
-
-  // Input something that can match history items.
-  omnibox_view->SetUserText(ASCIIToUTF16("site.com/p"));
-  ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
-  EXPECT_TRUE(popup_model->IsOpen());
-  EXPECT_EQ(ASCIIToUTF16("site.com/path/1"), omnibox_view->GetText());
-  base::string16::size_type start, end;
-  omnibox_view->GetSelectionBounds(&start, &end);
-  EXPECT_EQ(10U, std::min(start, end));
-  EXPECT_EQ(15U, std::max(start, end));
-
-  // Calling FocusSearch() with an inline autocompletion should preserve the
-  // autocompleted text, and should select all.
-  chrome::FocusSearch(browser());
-  ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
-  EXPECT_EQ(ASCIIToUTF16("site.com/path/1"), omnibox_view->GetText());
-  omnibox_view->GetSelectionBounds(&start, &end);
-  EXPECT_EQ(0U, std::min(start, end));
-  EXPECT_EQ(15U, std::max(start, end));
-
-  // Press backspace twice and the omnibox should be empty.
-  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACK, 0));
-  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACK, 0));
-  EXPECT_EQ(base::string16(), omnibox_view->GetText());
-  omnibox_view->GetSelectionBounds(&start, &end);
-  EXPECT_EQ(0U, start);
-  EXPECT_EQ(0U, end);
-
-  // Calling FocusSearch() with temporary text showing should preserve the
-  // suggested text, and should select all.
-  omnibox_view->SetUserText(ASCIIToUTF16("site.com/p"));
-  ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
-  EXPECT_TRUE(popup_model->IsOpen());
-  omnibox_view->model()->OnUpOrDownKeyPressed(1);
-  EXPECT_EQ(ASCIIToUTF16("www.site.com/path/2"), omnibox_view->GetText());
-  omnibox_view->GetSelectionBounds(&start, &end);
-  EXPECT_EQ(start, end);
-
-  chrome::FocusSearch(browser());
-  ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
-  EXPECT_EQ(ASCIIToUTF16("www.site.com/path/2"), omnibox_view->GetText());
-  omnibox_view->GetSelectionBounds(&start, &end);
-  EXPECT_EQ(0U, std::min(start, end));
-  EXPECT_EQ(19U, std::max(start, end));
-}
-
 IN_PROC_BROWSER_TEST_F(OmniboxViewTest, AcceptKeywordByTypingQuestionMark) {
   OmniboxView* omnibox_view = NULL;
   ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));

chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc

@@ -473,6 +473,27 @@ TEST_F(OmniboxViewViewsTest, RevertOnBlur) {
   EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
 }
 
+TEST_F(OmniboxViewViewsTest, BackspaceExitsKeywordMode) {
+  omnibox_view()->SetUserText(base::UTF8ToUTF16("user text"));
+  omnibox_view()->model()->EnterKeywordModeForDefaultSearchProvider(
+      KeywordModeEntryMethod::KEYBOARD_SHORTCUT);
+
+  ASSERT_EQ(base::UTF8ToUTF16("user text"), omnibox_view()->GetText());
+  ASSERT_TRUE(omnibox_view()->IsSelectAll());
+  ASSERT_FALSE(omnibox_view()->model()->keyword().empty());
+
+  // First backspace should clear the user text but not exit keyword mode.
+  ui::KeyEvent backspace(ui::ET_KEY_PRESSED, ui::VKEY_BACK, 0);
+  omnibox_textfield()->OnKeyEvent(&backspace);
+  EXPECT_TRUE(omnibox_view()->GetText().empty());
+  EXPECT_FALSE(omnibox_view()->model()->keyword().empty());
+
+  // Second backspace should exit keyword mode.
+  omnibox_textfield()->OnKeyEvent(&backspace);
+  EXPECT_TRUE(omnibox_view()->GetText().empty());
+  EXPECT_TRUE(omnibox_view()->model()->keyword().empty());
+}
+
 class OmniboxViewViewsSteadyStateElisionsTest : public OmniboxViewViewsTest {
  public:
   OmniboxViewViewsSteadyStateElisionsTest()

components/omnibox/browser/omnibox_controller.cc

@@ -76,9 +76,11 @@ void OmniboxController::InvalidateCurrentMatch() {
 }
 
 void OmniboxController::ClearPopupKeywordMode() const {
-  if (popup_->IsOpen() &&
-      popup_->selected_line_state() == OmniboxPopupModel::KEYWORD)
+  // |popup_| can be nullptr in tests.
+  if (popup_ && popup_->IsOpen() &&
+      popup_->selected_line_state() == OmniboxPopupModel::KEYWORD) {
     popup_->SetSelectedLineState(OmniboxPopupModel::NORMAL);
+  }
 }
 
 void OmniboxController::SetRichSuggestionBitmap(int result_index,

components/omnibox/browser/omnibox_edit_model.cc

@@ -834,9 +834,10 @@ void OmniboxEditModel::ClearKeyword() {
   // While we're always in keyword mode upon reaching here, sometimes we've just
   // toggled in via space or tab, and sometimes we're on a non-toggled line
   // (usually because the user has typed a search string).  Keep track of the
-  // difference, as we'll need it below.
+  // difference, as we'll need it below. popup_model() may be nullptr in tests.
   bool was_toggled_into_keyword_mode =
-    popup_model()->selected_line_state() == OmniboxPopupModel::KEYWORD;
+      popup_model() &&
+      popup_model()->selected_line_state() == OmniboxPopupModel::KEYWORD;
 
   omnibox_controller_->ClearPopupKeywordMode();
 
@@ -1331,10 +1332,6 @@ void OmniboxEditModel::InternalSetUserText(const base::string16& text) {
   view_->OnInlineAutocompleteTextCleared();
 }
 
-void OmniboxEditModel::ClearPopupKeywordMode() const {
-  omnibox_controller_->ClearPopupKeywordMode();
-}
-
 base::string16 OmniboxEditModel::MaybeStripKeyword(
     const base::string16& text) const {
   return is_keyword_selected() ?

components/omnibox/browser/omnibox_edit_model.h

@@ -415,9 +415,6 @@ class OmniboxEditModel {
   // Called whenever user_text_ should change.
   void InternalSetUserText(const base::string16& text);
 
-  // Turns off keyword mode for the current match.
-  void ClearPopupKeywordMode() const;
-
   // Conversion between user text and display text. User text is the text the
   // user has input. Display text is the text being shown in the edit. The
   // two are different if a keyword is selected.

components/omnibox/browser/omnibox_edit_model_unittest.cc

@@ -28,19 +28,20 @@ class OmniboxEditModelTest : public testing::Test {
   void SetUp() override {
     controller_ = std::make_unique<TestOmniboxEditController>();
     view_ = std::make_unique<TestOmniboxView>(controller_.get());
-    model_ =
-        std::make_unique<TestOmniboxEditModel>(view_.get(), controller_.get());
+    view_->SetModel(
+        std::make_unique<TestOmniboxEditModel>(view_.get(), controller_.get()));
   }
 
-  const TestOmniboxView& view() { return *view_; }
+  TestOmniboxView* view() { return view_.get(); }
   TestToolbarModel* toolbar_model() { return controller_->GetToolbarModel(); }
-  TestOmniboxEditModel* model() { return model_.get(); }
+  TestOmniboxEditModel* model() {
+    return static_cast<TestOmniboxEditModel*>(view_->model());
+  }
 
  private:
   base::test::ScopedTaskEnvironment task_environment_;
   std::unique_ptr<TestOmniboxEditController> controller_;
   std::unique_ptr<TestOmniboxView> view_;
-  std::unique_ptr<TestOmniboxEditModel> model_;
 };
 
 // Tests various permutations of AutocompleteModel::AdjustTextForCopy.
@@ -165,37 +166,37 @@ TEST_F(OmniboxEditModelTest, AdjustTextForCopy) {
 
 TEST_F(OmniboxEditModelTest, InlineAutocompleteText) {
   // Test if the model updates the inline autocomplete text in the view.
-  EXPECT_EQ(base::string16(), view().inline_autocomplete_text());
+  EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
   model()->SetUserText(base::ASCIIToUTF16("he"));
   model()->OnPopupDataChanged(base::ASCIIToUTF16("llo"), nullptr,
                               base::string16(), false);
-  EXPECT_EQ(base::ASCIIToUTF16("hello"), view().GetText());
-  EXPECT_EQ(base::ASCIIToUTF16("llo"), view().inline_autocomplete_text());
+  EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
+  EXPECT_EQ(base::ASCIIToUTF16("llo"), view()->inline_autocomplete_text());
 
   base::string16 text_before = base::ASCIIToUTF16("he");
   base::string16 text_after = base::ASCIIToUTF16("hel");
   OmniboxView::StateChanges state_changes{
       &text_before, &text_after, 3, 3, false, true, false, false};
   model()->OnAfterPossibleChange(state_changes, true);
-  EXPECT_EQ(base::string16(), view().inline_autocomplete_text());
+  EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
   model()->OnPopupDataChanged(base::ASCIIToUTF16("lo"), nullptr,
                               base::string16(), false);
-  EXPECT_EQ(base::ASCIIToUTF16("hello"), view().GetText());
-  EXPECT_EQ(base::ASCIIToUTF16("lo"), view().inline_autocomplete_text());
+  EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
+  EXPECT_EQ(base::ASCIIToUTF16("lo"), view()->inline_autocomplete_text());
 
   model()->Revert();
-  EXPECT_EQ(base::string16(), view().GetText());
-  EXPECT_EQ(base::string16(), view().inline_autocomplete_text());
+  EXPECT_EQ(base::string16(), view()->GetText());
+  EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
 
   model()->SetUserText(base::ASCIIToUTF16("he"));
   model()->OnPopupDataChanged(base::ASCIIToUTF16("llo"), nullptr,
                               base::string16(), false);
-  EXPECT_EQ(base::ASCIIToUTF16("hello"), view().GetText());
-  EXPECT_EQ(base::ASCIIToUTF16("llo"), view().inline_autocomplete_text());
+  EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
+  EXPECT_EQ(base::ASCIIToUTF16("llo"), view()->inline_autocomplete_text());
 
   model()->AcceptTemporaryTextAsUserText();
-  EXPECT_EQ(base::ASCIIToUTF16("hello"), view().GetText());
-  EXPECT_EQ(base::string16(), view().inline_autocomplete_text());
+  EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
+  EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
 }
 
 // This verifies the fix for a bug where calling OpenMatch() with a valid
@@ -354,3 +355,48 @@ TEST_F(OmniboxEditModelTest, ConsumeCtrlKeyOnCtrlAction) {
   EXPECT_EQ(model()->control_key_state_,
             TestOmniboxEditModel::DOWN_AND_CONSUMED);
 }
+
+TEST_F(OmniboxEditModelTest, KeywordModePreservesInlineAutocompleteText) {
+  // Set the edit model into an inline autocompletion state.
+  view()->SetUserText(base::UTF8ToUTF16("user"));
+  view()->OnInlineAutocompleteTextMaybeChanged(base::UTF8ToUTF16("user text"),
+                                               4);
+
+  // Entering keyword search mode should preserve the full display text as the
+  // user text, and select all.
+  model()->EnterKeywordModeForDefaultSearchProvider(
+      KeywordModeEntryMethod::KEYBOARD_SHORTCUT);
+  EXPECT_EQ(base::UTF8ToUTF16("user text"), model()->GetUserTextForTesting());
+  EXPECT_EQ(base::UTF8ToUTF16("user text"), view()->GetText());
+  EXPECT_TRUE(view()->IsSelectAll());
+
+  // Deleting the user text and exiting keyword mode should clear everything.
+  view()->SetUserText(base::string16());
+  model()->ClearKeyword();
+  {
+    EXPECT_TRUE(view()->GetText().empty());
+    EXPECT_TRUE(model()->GetUserTextForTesting().empty());
+    size_t start = 0, end = 0;
+    view()->GetSelectionBounds(&start, &end);
+    EXPECT_EQ(0U, start);
+    EXPECT_EQ(0U, end);
+  }
+}
+
+TEST_F(OmniboxEditModelTest, KeywordModePreservesTemporaryText) {
+  // Set the edit model into a temporary text state.
+  view()->SetUserText(base::UTF8ToUTF16("user text"));
+  GURL destination_url("http://example.com");
+
+  // OnPopupDataChanged() is called when the user focuses a suggestion.
+  model()->OnPopupDataChanged(base::UTF8ToUTF16("match text"), &destination_url,
+                              base::string16(), false);
+
+  // Entering keyword search mode should preserve temporary text as the user
+  // text, and select all.
+  model()->EnterKeywordModeForDefaultSearchProvider(
+      KeywordModeEntryMethod::KEYBOARD_SHORTCUT);
+  EXPECT_EQ(base::UTF8ToUTF16("match text"), model()->GetUserTextForTesting());
+  EXPECT_EQ(base::UTF8ToUTF16("match text"), view()->GetText());
+  EXPECT_TRUE(view()->IsSelectAll());
+}

components/omnibox/browser/omnibox_view_unittest.cc

@@ -40,9 +40,8 @@ class OmniboxViewTest : public testing::Test {
   OmniboxViewTest() {
     controller_ = std::make_unique<TestOmniboxEditController>();
     view_ = std::make_unique<TestOmniboxView>(controller_.get());
-
-    model_ = new TestOmniboxEditModel(view_.get(), controller_.get());
-    view_->SetModel(model_);
+    view_->SetModel(
+        std::make_unique<TestOmniboxEditModel>(view_.get(), controller_.get()));
 
     bookmark_model_ = bookmarks::TestBookmarkClient::CreateModel();
     client()->SetBookmarkModel(bookmark_model_.get());
@@ -54,7 +53,9 @@ class OmniboxViewTest : public testing::Test {
 
   TestOmniboxView* view() { return view_.get(); }
 
-  TestOmniboxEditModel* model() { return model_; }
+  TestOmniboxEditModel* model() {
+    return static_cast<TestOmniboxEditModel*>(view_->model());
+  }
 
   TestOmniboxClient* client() {
     return static_cast<TestOmniboxClient*>(model()->client());
@@ -67,7 +68,6 @@ class OmniboxViewTest : public testing::Test {
   std::unique_ptr<ui::test::MaterialDesignControllerTestAPI> material_design_;
   std::unique_ptr<TestOmniboxEditController> controller_;
   std::unique_ptr<TestOmniboxView> view_;
-  TestOmniboxEditModel* model_;
   std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_;
 };
 

components/omnibox/browser/test_omnibox_view.cc

@@ -4,25 +4,40 @@
 
 #include "components/omnibox/browser/test_omnibox_view.h"
 
+#include <algorithm>
+
 #include "ui/gfx/native_widget_types.h"
 
+void TestOmniboxView::SetModel(std::unique_ptr<OmniboxEditModel> model) {
+  model_ = std::move(model);
+}
+
 base::string16 TestOmniboxView::GetText() const {
   return text_;
 }
 
-void TestOmniboxView::SetUserText(const base::string16& text,
-                                  bool update_popup) {
-  text_ = text;
-}
 void TestOmniboxView::SetWindowTextAndCaretPos(const base::string16& text,
                                                size_t caret_pos,
                                                bool update_popup,
                                                bool notify_text_changed) {
   text_ = text;
+  selection_ = gfx::Range(caret_pos);
 }
 
 bool TestOmniboxView::IsSelectAll() const {
-  return false;
+  return selection_.EqualsIgnoringDirection(gfx::Range(0, text_.size()));
+}
+
+void TestOmniboxView::GetSelectionBounds(size_t* start, size_t* end) const {
+  *start = selection_.start();
+  *end = selection_.end();
+}
+
+void TestOmniboxView::SelectAll(bool reversed) {
+  if (reversed)
+    selection_ = gfx::Range(text_.size(), 0);
+  else
+    selection_ = gfx::Range(0, text_.size());
 }
 
 void TestOmniboxView::OnTemporaryTextMaybeChanged(
@@ -39,6 +54,7 @@ bool TestOmniboxView::OnInlineAutocompleteTextMaybeChanged(
   const bool text_changed = text_ != display_text;
   text_ = display_text;
   inline_autocomplete_text_ = display_text.substr(user_text_length);
+  selection_ = gfx::Range(text_.size(), user_text_length);
   return text_changed;
 }
 
@@ -73,7 +89,3 @@ bool TestOmniboxView::IsImeComposing() const {
 int TestOmniboxView::GetOmniboxTextLength() const {
   return 0;
 }
-
-void TestOmniboxView::SetModel(OmniboxEditModel* model) {
-  model_.reset(model);
-}

components/omnibox/browser/test_omnibox_view.h

@@ -11,6 +11,7 @@
 #include "base/strings/string16.h"
 #include "components/omnibox/browser/omnibox_edit_model.h"
 #include "components/omnibox/browser/omnibox_view.h"
+#include "ui/gfx/range/range.h"
 
 struct AutocompleteMatch;
 class OmniboxEditController;
@@ -21,6 +22,8 @@ class TestOmniboxView : public OmniboxView {
   explicit TestOmniboxView(OmniboxEditController* controller)
       : OmniboxView(controller, nullptr) {}
 
+  void SetModel(std::unique_ptr<OmniboxEditModel> model);
+
   const base::string16& inline_autocomplete_text() const {
     return inline_autocomplete_text_;
   }
@@ -34,7 +37,6 @@ class TestOmniboxView : public OmniboxView {
                  size_t selected_line,
                  base::TimeTicks match_selection_timestamp) override {}
   base::string16 GetText() const override;
-  void SetUserText(const base::string16& text, bool update_popup) override;
   void SetWindowTextAndCaretPos(const base::string16& text,
                                 size_t caret_pos,
                                 bool update_popup,
@@ -42,8 +44,8 @@ class TestOmniboxView : public OmniboxView {
   void SetCaretPos(size_t caret_pos) override {}
   void EnterKeywordModeForDefaultSearchProvider() override {}
   bool IsSelectAll() const override;
-  void GetSelectionBounds(size_t* start, size_t* end) const override {}
-  void SelectAll(bool reversed) override {}
+  void GetSelectionBounds(size_t* start, size_t* end) const override;
+  void SelectAll(bool reversed) override;
   void RevertAll() override {}
   void UpdatePopup() override {}
   void SetFocus() override {}
@@ -67,11 +69,11 @@ class TestOmniboxView : public OmniboxView {
   void EmphasizeURLComponents() override {}
   void SetEmphasis(bool emphasize, const gfx::Range& range) override {}
   void UpdateSchemeStyle(const gfx::Range& range) override {}
-  void SetModel(OmniboxEditModel* model);
 
  private:
   base::string16 text_;
   base::string16 inline_autocomplete_text_;
+  gfx::Range selection_;
 
   DISALLOW_COPY_AND_ASSIGN(TestOmniboxView);
 };