[omnibox] Clean up OmniboxEditModel::OnPopupDataChanged API

The API currently has an unused pointer argument that's used as a bool.
It just makes for a confusing read, and I'm trying to build on top of
it to fix some polish issues for on-focus suggestions (ZeroSuggest).

This CL turns that pointer-as-boolean argument into an explicit bool
argument that's properly labeled. No behavior changes.

Bug: 996516, 963173
Change-Id: I02767d26849dae035feeb54173ebee2fbc66ea03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1862256
Auto-Submit: Tommy Li <tommycli@chromium.org>
Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706092}

components/omnibox/browser/omnibox_controller.cc

@@ -60,7 +60,8 @@ void OmniboxController::OnResultChanged(bool default_match_changed) {
       InvalidateCurrentMatch();
       if (popup_)
         popup_->OnResultChanged();
-      omnibox_edit_model_->OnPopupDataChanged(base::string16(), nullptr,
+      omnibox_edit_model_->OnPopupDataChanged(base::string16(),
+                                              /*is_temporary_text=*/false,
                                               base::string16(), false);
     }
   } else if (popup_) {

components/omnibox/browser/omnibox_edit_model.cc

@@ -1212,13 +1212,11 @@ void OmniboxEditModel::OnUpOrDownKeyPressed(int count) {
   // force it to open immediately.
 }
 
-void OmniboxEditModel::OnPopupDataChanged(
-    const base::string16& text,
-    GURL* destination_for_temporary_text_change,
-    const base::string16& keyword,
-    bool is_keyword_hint) {
-  if (!original_user_text_with_keyword_.empty() &&
-      !destination_for_temporary_text_change &&
+void OmniboxEditModel::OnPopupDataChanged(const base::string16& text,
+                                          bool is_temporary_text,
+                                          const base::string16& keyword,
+                                          bool is_keyword_hint) {
+  if (!original_user_text_with_keyword_.empty() && !is_temporary_text &&
       (keyword.empty() || is_keyword_hint)) {
     user_text_ = original_user_text_with_keyword_;
     original_user_text_with_keyword_.clear();
@@ -1249,7 +1247,7 @@ void OmniboxEditModel::OnPopupDataChanged(
   }
 
   // Handle changes to temporary text.
-  if (destination_for_temporary_text_change) {
+  if (is_temporary_text) {
     const bool save_original_selection = !has_temporary_text_;
     if (save_original_selection) {
       // Save the original selection and URL so it can be reverted later.
@@ -1456,7 +1454,8 @@ void OmniboxEditModel::OnCurrentMatchChanged() {
   // on.  Therefore, copy match.inline_autocompletion to a temp to preserve
   // its value across the entire call.
   const base::string16 inline_autocompletion(match.inline_autocompletion);
-  OnPopupDataChanged(inline_autocompletion, nullptr, keyword, is_keyword_hint);
+  OnPopupDataChanged(inline_autocompletion,
+                     /*is_temporary_text=*/false, keyword, is_keyword_hint);
 }
 
 // static

components/omnibox/browser/omnibox_edit_model.h

@@ -342,19 +342,19 @@ class OmniboxEditModel {
   // 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.  We distinguish by
-  //     checking if |destination_for_temporary_text_change| is NULL.
+  //     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.
   //   |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_).
-  void OnPopupDataChanged(
-      const base::string16& text,
-      GURL* destination_for_temporary_text_change,
-      const base::string16& keyword,
-      bool is_keyword_hint);
+  void OnPopupDataChanged(const base::string16& text,
+                          bool is_temporary_text,
+                          const base::string16& keyword,
+                          bool is_keyword_hint);
 
   // Called by the OmniboxView after something changes, with details about what
   // state changes occured.  Updates internal state, updates the popup if

components/omnibox/browser/omnibox_edit_model_unittest.cc

@@ -227,8 +227,9 @@ TEST_F(OmniboxEditModelTest, DISABLED_InlineAutocompleteText) {
   // Test if the model updates the inline autocomplete text in the view.
   EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
   model()->SetUserText(base::ASCIIToUTF16("he"));
-  model()->OnPopupDataChanged(base::ASCIIToUTF16("llo"), nullptr,
-                              base::string16(), false);
+  model()->OnPopupDataChanged(base::ASCIIToUTF16("llo"),
+                              /*is_temporary_text=*/false, base::string16(),
+                              false);
   EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
   EXPECT_EQ(base::ASCIIToUTF16("llo"), view()->inline_autocomplete_text());
 
@@ -238,8 +239,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_autocomplete_text());
-  model()->OnPopupDataChanged(base::ASCIIToUTF16("lo"), nullptr,
-                              base::string16(), false);
+  model()->OnPopupDataChanged(base::ASCIIToUTF16("lo"),
+                              /*is_temporary_text=*/false, base::string16(),
+                              false);
   EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
   EXPECT_EQ(base::ASCIIToUTF16("lo"), view()->inline_autocomplete_text());
 
@@ -248,8 +250,9 @@ TEST_F(OmniboxEditModelTest, DISABLED_InlineAutocompleteText) {
   EXPECT_EQ(base::string16(), view()->inline_autocomplete_text());
 
   model()->SetUserText(base::ASCIIToUTF16("he"));
-  model()->OnPopupDataChanged(base::ASCIIToUTF16("llo"), nullptr,
-                              base::string16(), false);
+  model()->OnPopupDataChanged(base::ASCIIToUTF16("llo"),
+                              /*is_temporary_text=*/false, base::string16(),
+                              false);
   EXPECT_EQ(base::ASCIIToUTF16("hello"), view()->GetText());
   EXPECT_EQ(base::ASCIIToUTF16("llo"), view()->inline_autocomplete_text());
 
@@ -276,8 +279,8 @@ TEST_F(OmniboxEditModelTest, RespectUnelisionInZeroSuggest) {
 
   // 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);
+  model()->OnPopupDataChanged(base::string16(), /*is_temporary_text=*/false,
+                              base::string16(), false);
   EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
   EXPECT_FALSE(model()->user_input_in_progress());
   EXPECT_TRUE(view()->IsSelectAll());
@@ -506,8 +509,9 @@ TEST_F(OmniboxEditModelTest, KeywordModePreservesTemporaryText) {
   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);
+  model()->OnPopupDataChanged(base::UTF8ToUTF16("match text"),
+                              /*is_temporary_text=*/true, base::string16(),
+                              false);
 
   // Entering keyword search mode should preserve temporary text as the user
   // text, and select all.

components/omnibox/browser/omnibox_popup_model.cc

@@ -135,7 +135,6 @@ void OmniboxPopupModel::SetSelectedLine(size_t line,
   // opened the popup and made it possible to get here should have also set a
   // selected line.
   CHECK(selected_line_ != kNoMatch);
-  GURL current_destination(result.match_at(selected_line_).destination_url);
   const size_t prev_selected_line = selected_line_;
   selected_line_state_ = NORMAL;
   selected_line_ = line;
@@ -153,11 +152,13 @@ void OmniboxPopupModel::SetSelectedLine(size_t line,
   match.GetKeywordUIState(service, &keyword, &is_keyword_hint);
 
   if (reset_to_default) {
-    edit_model_->OnPopupDataChanged(match.inline_autocompletion, nullptr,
-                                    keyword, is_keyword_hint);
+    edit_model_->OnPopupDataChanged(match.inline_autocompletion,
+                                    /*is_temporary_text=*/false, keyword,
+                                    is_keyword_hint);
   } else {
-    edit_model_->OnPopupDataChanged(match.fill_into_edit, &current_destination,
-                                    keyword, is_keyword_hint);
+    edit_model_->OnPopupDataChanged(match.fill_into_edit,
+                                    /*is_temporary_text=*/true, keyword,
+                                    is_keyword_hint);
   }
 }