[omnibox] [rich-autocomplete] Un-overload OEM::OnPopupDataChanged param.

OmniboxEditModel::OnPopupDataChanged params include:
- |text| representing either the temporary or suffix autocompletion
texts.
- |is_temporary_text| to distinguish the 2 cases.
- |prefix_autocompletion|

The overloaded use of |text| is confusing, especially now that
autocompletion consists of more than 1 string field
(prefix_autocompletion and we expect to add split_autocompletion in the
future).

This CL replaces |text| with |temporary_text| and
|inline_autocompletion|.

Bug: 1062446
Change-Id: I5aa4535db1f9d4a85dced041f920d2f90afe0bda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2427025
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812106}

components/omnibox/browser/omnibox_controller.cc

@@ -61,7 +61,7 @@ void OmniboxController::OnResultChanged(AutocompleteController* controller,
       omnibox_edit_model_->OnPopupDataChanged(
           base::string16(),
           /*is_temporary_text=*/false, base::string16(), base::string16(),
-          false, base::string16());
+          base::string16(), false, base::string16());
     }
   } else if (popup_) {
     popup_->OnResultChanged();

components/omnibox/browser/omnibox_edit_model.cc

@@ -1293,8 +1293,9 @@ bool OmniboxEditModel::MaybeStartQueryForPopup() {
 }
 
 void OmniboxEditModel::OnPopupDataChanged(
-    const base::string16& text,
+    const base::string16& temporary_text,
     bool is_temporary_text,
+    const base::string16& inline_autocompletion,
     const base::string16& prefix_autocompletion,
     const base::string16& keyword,
     bool is_keyword_hint,
@@ -1350,13 +1351,13 @@ void OmniboxEditModel::OnPopupDataChanged(
 
     const AutocompleteMatch& match = CurrentMatch(nullptr);
     view_->OnTemporaryTextMaybeChanged(
-        MaybeStripKeyword(text), match,
+        MaybeStripKeyword(temporary_text), match,
         save_original_selection && original_user_text_with_keyword_.empty(),
         true);
     return;
   }
 
-  inline_autocompletion_ = text;
+  inline_autocompletion_ = inline_autocompletion;
   prefix_autocompletion_ = prefix_autocompletion;
   if (inline_autocompletion_.empty() && prefix_autocompletion_.empty())
     view_->OnInlineAutocompleteTextCleared();
@@ -1545,9 +1546,10 @@ void OmniboxEditModel::OnCurrentMatchChanged() {
   const base::string16 prefix_autocompletion(match.prefix_autocompletion);
   const base::string16 fill_into_edit_additional_text(
       match.fill_into_edit_additional_text);
-  OnPopupDataChanged(inline_autocompletion,
-                     /*is_temporary_text=*/false, prefix_autocompletion,
-                     keyword, is_keyword_hint, fill_into_edit_additional_text);
+  OnPopupDataChanged(base::string16(),
+                     /*is_temporary_text=*/false, inline_autocompletion,
+                     prefix_autocompletion, keyword, is_keyword_hint,
+                     fill_into_edit_additional_text);
 }
 
 // static

components/omnibox/browser/omnibox_edit_model.h

@@ -356,23 +356,28 @@ class OmniboxEditModel {
 
   // Called when any relevant data changes.  This rolls together several
   // separate pieces of data into one call so we can update all the UI
-  // efficiently:
-  //   |text| is either the new temporary text from the user manually selecting
-  //     a different match, or the inline autocomplete text.
-  //   |is_temporary_text| is true if |text| contains the temporary text for
-  //     a match, and is false if |text| contains the inline autocomplete text.
-  //   |prefix_autocompletion| is the prefix autocomplete text.
+  // efficiently. Specifically, it's invoked for temporary text, autocompletion,
+  // and keyword changes.
+  //   |temporary_text| is the new temporary text from the user selecting a
+  //   different match. This will be empty when selecting a suggestion
+  //   without a |fill_into_edit| (e.g. FOCUSED_BUTTON_HEADER) and when
+  //   |is_temporary_test| is false.
+  //   |is_temporary_text| is true if invoked because of a temporary text change
+  //     or false if |temporary_text| should be ignored.
+  //   |inline_autocompletion| and |prefix_autocompletion| are the autocomplete
+  //     texts.
   //   |destination_for_temporary_text_change| is NULL (if temporary text should
-  //     not change) or the pre-change destination URL (if temporary text should
-  //     change) so we can save it off to restore later.
-  //   |keyword| is the keyword to show a hint for if |is_keyword_hint| is true,
-  //     or the currently selected keyword if |is_keyword_hint| is false (see
-  //     comments on keyword_ and is_keyword_hint_).
+  //     not change) or the pre-change destination URL (if temporary text
+  //     should change) so we can save it off to restore later.
+  //   |keyword| is the keyword to show a hint for if |is_keyword_hint| is true
+  //     or the currently selected keyword if |is_keyword_hint| is false
+  //     (see comments on keyword_ and is_keyword_hint_).
   //   |additional_text| is additional omnibox text to be displayed adjacent to
-  //   the omnibox view.
+  //     the omnibox view.
   // Virtual to allow testing.
-  virtual void OnPopupDataChanged(const base::string16& text,
+  virtual void OnPopupDataChanged(const base::string16& temporary_text,
                                   bool is_temporary_text,
+                                  const base::string16& inline_autocompletion,
                                   const base::string16& prefix_autocompletion,
                                   const base::string16& keyword,
                                   bool is_keyword_hint,

components/omnibox/browser/omnibox_edit_model_unittest.cc

@@ -218,8 +218,9 @@ TEST_F(OmniboxEditModelTest, DISABLED_InlineAutocompleteText) {
   // Test if the model updates the inline autocomplete text in the view.
   EXPECT_EQ(base::string16(), view()->inline_autocompletion());
   model()->SetUserText(base::ASCIIToUTF16("he"));
-  model()->OnPopupDataChanged(base::ASCIIToUTF16("llo"),
-                              /*is_temporary_text=*/false, base::string16(),
+  model()->OnPopupDataChanged(base::string16(),
+                              /*is_temporary_text=*/false,
+                              base::ASCIIToUTF16("llo"), base::string16(),
                               base::string16(), false, base::string16());
   EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
   EXPECT_EQ(base::ASCIIToUTF16("llo"), view()->inline_autocompletion());
@@ -230,8 +231,9 @@ TEST_F(OmniboxEditModelTest, DISABLED_InlineAutocompleteText) {
       &text_before, &text_after, 3, 3, false, true, false, false};
   model()->OnAfterPossibleChange(state_changes, true);
   EXPECT_EQ(base::string16(), view()->inline_autocompletion());
-  model()->OnPopupDataChanged(base::ASCIIToUTF16("lo"),
-                              /*is_temporary_text=*/false, base::string16(),
+  model()->OnPopupDataChanged(base::string16(),
+                              /*is_temporary_text=*/false,
+                              base::ASCIIToUTF16("lo"), base::string16(),
                               base::string16(), false, base::string16());
   EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
   EXPECT_EQ(base::ASCIIToUTF16("lo"), view()->inline_autocompletion());
@@ -241,8 +243,9 @@ TEST_F(OmniboxEditModelTest, DISABLED_InlineAutocompleteText) {
   EXPECT_EQ(base::string16(), view()->inline_autocompletion());
 
   model()->SetUserText(base::ASCIIToUTF16("he"));
-  model()->OnPopupDataChanged(base::ASCIIToUTF16("llo"),
-                              /*is_temporary_text=*/false, base::string16(),
+  model()->OnPopupDataChanged(base::string16(),
+                              /*is_temporary_text=*/false,
+                              base::ASCIIToUTF16("llo"), base::string16(),
                               base::string16(), false, base::string16());
   EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
   EXPECT_EQ(base::ASCIIToUTF16("llo"), view()->inline_autocompletion());
@@ -272,8 +275,8 @@ TEST_F(OmniboxEditModelTest, RespectUnelisionInZeroSuggest) {
   EXPECT_EQ(base::string16(), view()->inline_autocompletion());
   model()->StartZeroSuggestRequest();
   model()->OnPopupDataChanged(base::string16(), /*is_temporary_text=*/false,
-                              base::string16(), base::string16(), false,
-                              base::string16());
+                              base::string16(), base::string16(),
+                              base::string16(), false, base::string16());
   EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
   EXPECT_FALSE(model()->user_input_in_progress());
   EXPECT_TRUE(view()->IsSelectAll());
@@ -293,7 +296,8 @@ TEST_F(OmniboxEditModelTest, RevertZeroSuggestTemporaryText) {
   model()->StartZeroSuggestRequest();
   model()->OnPopupDataChanged(base::ASCIIToUTF16("fake_temporary_text"),
                               /*is_temporary_text=*/true, base::string16(),
-                              base::string16(), false, base::string16());
+                              base::string16(), base::string16(), false,
+                              base::string16());
 
   // Test that reverting brings back the original input text.
   EXPECT_TRUE(model()->OnEscapeKeyPressed());
@@ -519,7 +523,8 @@ TEST_F(OmniboxEditModelTest, KeywordModePreservesTemporaryText) {
   // OnPopupDataChanged() is called when the user focuses a suggestion.
   model()->OnPopupDataChanged(base::UTF8ToUTF16("match text"),
                               /*is_temporary_text=*/true, base::string16(),
-                              base::string16(), false, base::string16());
+                              base::string16(), base::string16(), false,
+                              base::string16());
 
   // Entering keyword search mode should preserve temporary text as the user
   // text, and select all.
@@ -550,7 +555,8 @@ TEST_F(OmniboxEditModelTest, CtrlEnterNavigatesToDesiredTLDTemporaryText) {
   model()->StartAutocomplete(false, false);
   model()->OnPopupDataChanged(base::ASCIIToUTF16("foobar"),
                               /*is_temporary_text=*/true, base::string16(),
-                              base::string16(), false, base::string16());
+                              base::string16(), base::string16(), false,
+                              base::string16());
 
   model()->OnControlKeyChanged(true);
   model()->AcceptInput(WindowOpenDisposition::UNKNOWN);

components/omnibox/browser/omnibox_popup_model.cc

@@ -176,8 +176,8 @@ void OmniboxPopupModel::SetSelection(Selection new_selection,
     // If the new selection is a Header, the temporary text is an empty string.
     edit_model_->OnPopupDataChanged(base::string16(),
                                     /*is_temporary_text=*/true,
-                                    base::string16(), keyword, is_keyword_hint,
-                                    base::string16());
+                                    base::string16(), base::string16(), keyword,
+                                    is_keyword_hint, base::string16());
   } else if (old_selection.line != selection_.line ||
              (old_selection.IsButtonFocused() &&
               !new_selection.IsButtonFocused() &&
@@ -187,14 +187,15 @@ void OmniboxPopupModel::SetSelection(Selection new_selection,
     // Updating the edit model for every state change breaks keyword mode.
     if (reset_to_default) {
       edit_model_->OnPopupDataChanged(
-          match.inline_autocompletion,
-          /*is_temporary_text=*/false, match.prefix_autocompletion, keyword,
-          is_keyword_hint, match.fill_into_edit_additional_text);
+          base::string16(),
+          /*is_temporary_text=*/false, match.inline_autocompletion,
+          match.prefix_autocompletion, keyword, is_keyword_hint,
+          match.fill_into_edit_additional_text);
     } else {
       edit_model_->OnPopupDataChanged(
           match.fill_into_edit,
-          /*is_temporary_text=*/true, base::UTF8ToUTF16(""), keyword,
-          is_keyword_hint, match.fill_into_edit_additional_text);
+          /*is_temporary_text=*/true, base::string16(), base::string16(),
+          keyword, is_keyword_hint, match.fill_into_edit_additional_text);
     }
   }
 }

components/omnibox/browser/omnibox_popup_model_unittest.cc

@@ -50,16 +50,17 @@ class TestOmniboxEditModel : public OmniboxEditModel {
   const base::string16& text() const { return text_; }
   bool is_temporary_text() const { return is_temporary_text_; }
 
-  void OnPopupDataChanged(const base::string16& text,
+  void OnPopupDataChanged(const base::string16& temporary_text,
                           bool is_temporary_text,
+                          const base::string16& inline_autocompletion,
                           const base::string16& prefix_autocompletion,
                           const base::string16& keyword,
                           bool is_keyword_hint,
                           const base::string16& additional_text) override {
-    OmniboxEditModel::OnPopupDataChanged(text, is_temporary_text,
-                                         prefix_autocompletion, keyword,
-                                         is_keyword_hint, additional_text);
-    text_ = text;
+    OmniboxEditModel::OnPopupDataChanged(
+        temporary_text, is_temporary_text, inline_autocompletion,
+        prefix_autocompletion, keyword, is_keyword_hint, additional_text);
+    text_ = is_temporary_text ? temporary_text : inline_autocompletion;
     is_temporary_text_ = is_temporary_text;
   }
 
@@ -599,8 +600,9 @@ TEST_F(OmniboxPopupModelTest, PopupInlineAutocompleteAndTemporaryText) {
   popup_model()->OnResultChanged();
 
   // Simulate OmniboxController updating the popup, then check initial state.
-  model()->OnPopupDataChanged(base::UTF8ToUTF16("1"),
-                              /*is_temporary_text=*/false, base::string16(),
+  model()->OnPopupDataChanged(base::string16(),
+                              /*is_temporary_text=*/false,
+                              base::UTF8ToUTF16("1"), base::string16(),
                               base::string16(), false, base::string16());
   EXPECT_EQ(Selection(0, OmniboxPopupModel::NORMAL),
             model()->popup_model()->selection());