Omnibox: Steady State Elisions - Stop abusing user_input_in_progress.

Currently, when Steady State Elisions is on, and the user unelides the
URL, we put the full URL into the user text.

This was convenient, but has some unintended consequences. Namely:

 1) user_input_in_progress() as a boolean state becomes confusing, as
    sometimes it means the user has edited the URL, and sometimes it
    means the the user has merely unelided the URL.

 2) Consequently, in many places in the code, we check if user input in
    progress AND if the user has meaningfully edited the URL away from
    the full URL text.

 3) That's not obvious to non-omnibox owners, so it creates bugs like
    the one linked below.

This CL changes how unelision works so it only updates the View's text
and leaves the model's user-text alone. It no longer sets
user_input_in_progress to true for mere unelision.

This allows us to clean up fair number of callsites and puts the nail
in the coffin for the below linked bug too.

Bug: 921777
Change-Id: I9d113649f50c0bfafa1d112002add577e05928f7
Reviewed-on: https://chromium-review.googlesource.com/c/1427244Reviewed-by: manuk hovanesian <manukh@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625358}

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

@@ -1230,15 +1230,16 @@ void OmniboxViewViews::OnBlur() {
   // Save the user's existing selection to restore it later.
   saved_selection_for_focus_change_ = GetSelectedRange();
 
-  // Revert the URL if the user has not made any changes. If steady-state
-  // elisions is on, this will also re-elide the URL.
+  // If the view is showing text that's not user-text, revert the text to the
+  // permanent display text. This usually occurs if Steady State Elisions is on
+  // and the user has unelided, but not edited the URL.
   //
   // Because merely Alt-Tabbing to another window and back should not change the
   // Omnibox state, we only revert the text only if the Omnibox is blurred in
   // favor of some other View in the same Widget.
   if (GetWidget() && GetWidget()->IsActive() &&
-      model()->user_input_in_progress() &&
-      text() == model()->GetPermanentDisplayText()) {
+      !model()->user_input_in_progress() &&
+      text() != model()->GetPermanentDisplayText()) {
     RevertAll();
   }
 

chrome/browser/ui/views/omnibox/omnibox_view_views.h

@@ -103,6 +103,10 @@ class OmniboxViewViews : public OmniboxView,
   using OmniboxView::SetUserText;
   void SetUserText(const base::string16& text,
                    bool update_popup) override;
+  void SetWindowTextAndCaretPos(const base::string16& text,
+                                size_t caret_pos,
+                                bool update_popup,
+                                bool notify_text_changed) override;
   void EnterKeywordModeForDefaultSearchProvider() override;
   bool IsSelectAll() const override;
   void GetSelectionBounds(base::string16::size_type* start,
@@ -192,10 +196,6 @@ class OmniboxViewViews : public OmniboxView,
   bool MaybeUnfocusTabButton();
 
   // OmniboxView:
-  void SetWindowTextAndCaretPos(const base::string16& text,
-                                size_t caret_pos,
-                                bool update_popup,
-                                bool notify_text_changed) override;
   void SetCaretPos(size_t caret_pos) override;
   void UpdatePopup() override;
   void ApplyCaretVisibility() override;

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

@@ -386,8 +386,7 @@ TEST_F(OmniboxViewViewsTest, SelectWithShift_863543) {
   location_bar_model()->set_url(GURL("http://www.example.com/?query=1"));
   const base::string16 text =
       base::ASCIIToUTF16("http://www.example.com/?query=1");
-  static_cast<OmniboxView*>(omnibox_view())
-      ->SetWindowTextAndCaretPos(text, 23U, false, false);
+  omnibox_view()->SetWindowTextAndCaretPos(text, 23U, false, false);
 
   ui::KeyEvent shift_up_pressed(ui::ET_KEY_PRESSED, ui::VKEY_UP,
                                 ui::EF_SHIFT_DOWN);
@@ -399,8 +398,7 @@ TEST_F(OmniboxViewViewsTest, SelectWithShift_863543) {
   EXPECT_EQ(0U, end);
   omnibox_view()->CheckUpdatePopupNotCalled();
 
-  static_cast<OmniboxView*>(omnibox_view())
-      ->SetWindowTextAndCaretPos(text, 18U, false, false);
+  omnibox_view()->SetWindowTextAndCaretPos(text, 18U, false, false);
 
   ui::KeyEvent shift_down_pressed(ui::ET_KEY_PRESSED, ui::VKEY_DOWN,
                                   ui::EF_SHIFT_DOWN);
@@ -431,8 +429,7 @@ TEST_F(OmniboxViewViewsTest, OnBlur) {
   omnibox_textfield()->OnFocus();
   const base::string16 kContentsRtl =
       base::WideToUTF16(L"\x05e8\x05e2.\x05e7\x05d5\x05dd/0123/abcd");
-  static_cast<OmniboxView*>(omnibox_view())
-      ->SetWindowTextAndCaretPos(kContentsRtl, 0, false, false);
+  omnibox_view()->SetWindowTextAndCaretPos(kContentsRtl, 0, false, false);
   EXPECT_EQ(gfx::NO_ELIDE, render_text->elide_behavior());
   // NOTE: Technically (depending on the font), this expectation could fail if
   // the entire domain fits in 60 pixels. However, 60px is so small it should
@@ -497,14 +494,28 @@ TEST_F(OmniboxViewViewsTest, Emphasis) {
 }
 
 TEST_F(OmniboxViewViewsTest, RevertOnBlur) {
-  location_bar_model()->set_url(GURL("https://permanent-text.com/"));
+  location_bar_model()->set_url(GURL("https://example.com/"));
   omnibox_view()->model()->ResetDisplayTexts();
   omnibox_view()->RevertAll();
 
-  EXPECT_EQ(base::ASCIIToUTF16("https://permanent-text.com/"),
-            omnibox_view()->text());
+  EXPECT_EQ(base::ASCIIToUTF16("https://example.com/"), omnibox_view()->text());
+  EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
+
+  // Set the view text without updating the model's user text. This usually
+  // occurs when the omnibox unapplies Steady State Elisions to temporarily show
+  // the full URL to the user.
+  omnibox_view()->SetWindowTextAndCaretPos(base::ASCIIToUTF16("view text"), 0,
+                                           false, false);
+  EXPECT_EQ(base::ASCIIToUTF16("view text"), omnibox_view()->text());
+  EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
+
+  // Expect that on blur, we revert to the original text and are not in user
+  // input mode.
+  omnibox_textfield()->OnBlur();
+  EXPECT_EQ(base::ASCIIToUTF16("https://example.com/"), omnibox_view()->text());
   EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
 
+  // Now set user text, which is reflected into the model as well.
   omnibox_view()->SetUserText(base::ASCIIToUTF16("user text"));
   EXPECT_EQ(base::ASCIIToUTF16("user text"), omnibox_view()->text());
   EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
@@ -513,16 +524,6 @@ TEST_F(OmniboxViewViewsTest, RevertOnBlur) {
   omnibox_textfield()->OnBlur();
   EXPECT_EQ(base::ASCIIToUTF16("user text"), omnibox_view()->text());
   EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
-
-  // Expect that on blur, if the text is the same as the
-  // https://permanent-text.com, exit user input mode.
-  omnibox_view()->SetUserText(
-      base::ASCIIToUTF16("https://permanent-text.com/"));
-  EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
-  omnibox_textfield()->OnBlur();
-  EXPECT_EQ(base::ASCIIToUTF16("https://permanent-text.com/"),
-            omnibox_view()->text());
-  EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
 }
 
 TEST_F(OmniboxViewViewsTest, RevertOnEscape) {
@@ -696,13 +697,7 @@ class OmniboxViewViewsSteadyStateElisionsTest : public OmniboxViewViewsTest {
 
   void ExpectFullUrlDisplayed() {
     EXPECT_EQ(base::UTF8ToUTF16(kFullUrl.spec()), omnibox_view()->text());
-    EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
-
-    // We test the user text stored in the model has been updated as well. The
-    // model user text is used to populate the text in the Omnibox after some
-    // state transitions, such as the ZeroSuggest popup opening.
-    EXPECT_EQ(base::UTF8ToUTF16(kFullUrl.spec()),
-              omnibox_view()->model()->GetUserTextForTesting());
+    EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
   }
 
   bool IsElidedUrlDisplayed() {

components/omnibox/browser/omnibox_edit_model.cc

@@ -232,12 +232,6 @@ AutocompleteMatch OmniboxEditModel::CurrentMatch(
 bool OmniboxEditModel::ResetDisplayTexts() {
   const base::string16 old_display_text = GetPermanentDisplayText();
 
-  // Track if the user has modified the text. This is different from
-  // |user_input_in_progress_| because we care if the user has actually
-  // modified the text, while |user_input_in_progress_| may be true even if
-  // the user has merely made a partial selection.
-  bool user_has_modified_text = view_->GetText() != old_display_text;
-
   LocationBarModel* location_bar_model = controller()->GetLocationBarModel();
   url_for_editing_ = location_bar_model->GetFormattedFullURL();
 
@@ -266,7 +260,7 @@ bool OmniboxEditModel::ResetDisplayTexts() {
   // URL" (which sounds as if it might be persistent) from seeing just that URL
   // forever afterwards.
   return (GetPermanentDisplayText() != old_display_text) &&
-         (!has_focus() || (!user_has_modified_text && !PopupIsOpen()));
+         (!has_focus() || (!user_input_in_progress_ && !PopupIsOpen()));
 }
 
 GURL OmniboxEditModel::PermanentURL() const {
@@ -275,9 +269,6 @@ GURL OmniboxEditModel::PermanentURL() const {
 }
 
 base::string16 OmniboxEditModel::GetPermanentDisplayText() const {
-  if (user_input_in_progress_)
-    return url_for_editing_;
-
   return display_text_;
 }
 
@@ -308,7 +299,6 @@ bool OmniboxEditModel::Unelide(bool exit_query_in_omnibox) {
       location_bar_model->GetDisplaySearchTerms(nullptr))
     return false;
 
-  SetUserText(url_for_editing_);
   view_->SetWindowTextAndCaretPos(url_for_editing_, 0, false, false);
 
   // Select all in reverse to ensure the beginning of the URL is shown.
@@ -366,7 +356,7 @@ void OmniboxEditModel::AdjustTextForCopy(int sel_min,
 
   // If the user has not modified the display text and is copying the whole
   // display text, copy the omnibox contents as a hyperlink to the current page.
-  if (!user_input_in_progress_ && *text == GetPermanentDisplayText()) {
+  if (!user_input_in_progress_ && *text == display_text_) {
     *url_from_text = PermanentURL();
     *write_url = true;
 
@@ -1230,7 +1220,7 @@ void OmniboxEditModel::OnPopupDataChanged(
     view_->OnInlineAutocompleteTextCleared();
 
   const base::string16& user_text =
-      user_input_in_progress_ ? user_text_ : GetPermanentDisplayText();
+      user_input_in_progress_ ? user_text_ : view_->GetText();
   if (keyword_state_changed && is_keyword_selected()) {
     // If we reach here, the user most likely entered keyword mode by inserting
     // a space between a keyword name and a search string (as pressing space or

components/omnibox/browser/omnibox_edit_model_unittest.cc

@@ -229,6 +229,32 @@ TEST_F(OmniboxEditModelTest, InlineAutocompleteText) {
   EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
 }
 
+// iOS doesn't use elisions in the Omnibox textfield.
+#if !defined(OS_IOS)
+TEST_F(OmniboxEditModelTest, RespectUnelisionInZeroSuggest) {
+  location_bar_model()->set_url(GURL("https://www.example.com/"));
+  location_bar_model()->set_url_for_display(base::ASCIIToUTF16("example.com"));
+
+  EXPECT_TRUE(model()->ResetDisplayTexts());
+  model()->Revert();
+
+  // Set up view with unelided text.
+  EXPECT_EQ(base::ASCIIToUTF16("example.com"), view()->GetText());
+  EXPECT_TRUE(model()->Unelide(false /* exit_query_in_omnibox */));
+  EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
+  EXPECT_FALSE(model()->user_input_in_progress());
+  EXPECT_TRUE(view()->IsSelectAll());
+
+  // Test that we don't clobber the unelided text with inline autocomplete text.
+  EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
+  model()->OnPopupDataChanged(base::string16(), nullptr, base::string16(),
+                              false);
+  EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
+  EXPECT_FALSE(model()->user_input_in_progress());
+  EXPECT_TRUE(view()->IsSelectAll());
+}
+#endif  // !defined(OS_IOS)
+
 // This verifies the fix for a bug where calling OpenMatch() with a valid
 // alternate nav URL would fail a DCHECK if the input began with "http://".
 // The failure was due to erroneously trying to strip the scheme from the
@@ -295,6 +321,7 @@ TEST_F(OmniboxEditModelTest, DisplayText) {
   // permanent display text. Unelision should return false.
   EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"),
             model()->GetPermanentDisplayText());
+  EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
   EXPECT_FALSE(model()->Unelide(false /* exit_query_in_omnibox */));
   EXPECT_FALSE(model()->user_input_in_progress());
   EXPECT_FALSE(view()->IsSelectAll());
@@ -302,8 +329,9 @@ TEST_F(OmniboxEditModelTest, DisplayText) {
   // Verify we can unelide and show the full URL properly.
   EXPECT_EQ(base::ASCIIToUTF16("example.com"),
             model()->GetPermanentDisplayText());
+  EXPECT_EQ(base::ASCIIToUTF16("example.com"), view()->GetText());
   EXPECT_TRUE(model()->Unelide(false /* exit_query_in_omnibox */));
-  EXPECT_TRUE(model()->user_input_in_progress());
+  EXPECT_FALSE(model()->user_input_in_progress());
   EXPECT_TRUE(view()->IsSelectAll());
 #endif
 
@@ -332,7 +360,7 @@ TEST_F(OmniboxEditModelTest, DisplayAndExitQueryInOmnibox) {
   // Verify we can exit Query in Omnibox mode properly.
   EXPECT_TRUE(model()->Unelide(true /* exit_query_in_omnibox */));
   EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
-  EXPECT_TRUE(model()->user_input_in_progress());
+  EXPECT_FALSE(model()->user_input_in_progress());
   EXPECT_TRUE(view()->IsSelectAll());
   EXPECT_TRUE(model()->CurrentTextIsURL());
 

components/omnibox/browser/test_omnibox_view.cc

@@ -54,7 +54,12 @@ 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);
+
+  // Just like the Views control, only change the selection if the text has
+  // actually changed.
+  if (text_changed)
+    selection_ = gfx::Range(text_.size(), user_text_length);
+
   return text_changed;
 }