Remove unneeded answer properties on AutocompleteMatch.

BUG=

Change-Id: I11d351d4681554741a961cd2d54c9de102192bd8
Reviewed-on: https://chromium-review.googlesource.com/c/1345273Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611014}

chrome/browser/autocomplete/search_provider_unittest.cc

@@ -3527,8 +3527,8 @@ TEST_F(SearchProviderTest, AnswersCache) {
   AutocompleteResult result;
   ACMatches matches;
   AutocompleteMatch match1;
-  match1.answer_contents = base::ASCIIToUTF16("m1");
-  match1.answer_type = base::ASCIIToUTF16("2334");
+  match1.answer = SuggestionAnswer();
+  match1.answer->set_type(2334);
   match1.fill_into_edit = base::ASCIIToUTF16("weather los angeles");
 
   AutocompleteMatch non_answer_match1;
@@ -3544,7 +3544,7 @@ TEST_F(SearchProviderTest, AnswersCache) {
   // Without scored results, no answers will be retrieved.
   AnswersQueryData answer = provider_->FindAnswersPrefetchData();
   EXPECT_TRUE(answer.full_query_text.empty());
-  EXPECT_TRUE(answer.query_type.empty());
+  EXPECT_EQ(-1, answer.query_type);
 
   // Inject a scored result, which will trigger answer retrieval.
   base::string16 query = base::ASCIIToUTF16("weather los angeles");
@@ -3557,7 +3557,7 @@ TEST_F(SearchProviderTest, AnswersCache) {
   provider_->transformed_default_history_results_.push_back(suggest_result);
   answer = provider_->FindAnswersPrefetchData();
   EXPECT_EQ(base::ASCIIToUTF16("weather los angeles"), answer.full_query_text);
-  EXPECT_EQ(base::ASCIIToUTF16("2334"), answer.query_type);
+  EXPECT_EQ(2334, answer.query_type);
 }
 
 TEST_F(SearchProviderTest, RemoveExtraAnswers) {
@@ -3571,14 +3571,8 @@ TEST_F(SearchProviderTest, RemoveExtraAnswers) {
   ACMatches matches;
   AutocompleteMatch match1, match2, match3, match4, match5;
   match1.answer = answer1;
-  match1.answer_contents = base::ASCIIToUTF16("the answer");
-  match1.answer_type = base::ASCIIToUTF16("42");
   match3.answer = answer2;
-  match3.answer_contents = base::ASCIIToUTF16("not to play");
-  match3.answer_type = base::ASCIIToUTF16("1983");
   match5.answer = answer3;
-  match5.answer_contents = base::ASCIIToUTF16("a person");
-  match5.answer_type = base::ASCIIToUTF16("423");
 
   matches.push_back(match1);
   matches.push_back(match2);
@@ -3587,20 +3581,11 @@ TEST_F(SearchProviderTest, RemoveExtraAnswers) {
   matches.push_back(match5);
 
   SearchProvider::RemoveExtraAnswers(&matches);
-  EXPECT_EQ(base::ASCIIToUTF16("the answer"), matches[0].answer_contents);
-  EXPECT_EQ(base::ASCIIToUTF16("42"), matches[0].answer_type);
+  EXPECT_EQ(42, matches[0].answer->type());
   EXPECT_TRUE(answer1.Equals(*matches[0].answer));
-  EXPECT_TRUE(matches[1].answer_contents.empty());
-  EXPECT_TRUE(matches[1].answer_type.empty());
   EXPECT_FALSE(matches[1].answer);
-  EXPECT_TRUE(matches[2].answer_contents.empty());
-  EXPECT_TRUE(matches[2].answer_type.empty());
   EXPECT_FALSE(matches[2].answer);
-  EXPECT_TRUE(matches[3].answer_contents.empty());
-  EXPECT_TRUE(matches[3].answer_type.empty());
   EXPECT_FALSE(matches[3].answer);
-  EXPECT_TRUE(matches[4].answer_contents.empty());
-  EXPECT_TRUE(matches[4].answer_type.empty());
   EXPECT_FALSE(matches[4].answer);
 }
 

components/omnibox/browser/answers_cache.cc

@@ -7,12 +7,9 @@
 #include "base/i18n/case_conversion.h"
 #include "base/strings/string_util.h"
 
-AnswersQueryData::AnswersQueryData() {
-}
-AnswersQueryData::AnswersQueryData(const base::string16& text,
-                                   const base::string16& type)
-    : full_query_text(text), query_type(type) {
-}
+AnswersQueryData::AnswersQueryData() : query_type(-1) {}
+AnswersQueryData::AnswersQueryData(const base::string16& text, int type)
+    : full_query_text(text), query_type(type) {}
 
 AnswersCache::AnswersCache(size_t max_entries) : max_entries_(max_entries) {
 }
@@ -36,7 +33,7 @@ AnswersQueryData AnswersCache::GetTopAnswerEntry(const base::string16& query) {
 }
 
 void AnswersCache::UpdateRecentAnswers(const base::string16& full_query_text,
-                                       const base::string16& query_type) {
+                                       int query_type) {
   // If this entry is already part of the cache, just update recency.
   for (auto it = cache_.begin(); it != cache_.end(); ++it) {
     if (full_query_text == it->full_query_text &&

components/omnibox/browser/answers_cache.h

@@ -14,10 +14,9 @@
 
 struct AnswersQueryData {
   AnswersQueryData();
-  AnswersQueryData(const base::string16& full_query_text,
-                   const base::string16& query_type);
+  AnswersQueryData(const base::string16& full_query_text, int query_type);
   base::string16 full_query_text;
-  base::string16 query_type;
+  int query_type;
 };
 
 // Cache for the most-recently seen answer for Answers in Suggest.
@@ -32,7 +31,7 @@ class AnswersCache {
 
   // Registers a query that received an answer suggestion.
   void UpdateRecentAnswers(const base::string16& full_query_text,
-                           const base::string16& query_type);
+                           int query_type);
 
   // Signals if cache is empty.
   bool empty() const { return cache_.empty(); }

components/omnibox/browser/answers_cache_unittest.cc

@@ -14,8 +14,7 @@ TEST(AnswersCacheTest, CacheStartsEmpty) {
 
 TEST(AnswersCacheTest, UpdatePopulatesCache) {
   AnswersCache cache(1);
-  cache.UpdateRecentAnswers(base::ASCIIToUTF16("weather los angeles"),
-                            base::ASCIIToUTF16("2334"));
+  cache.UpdateRecentAnswers(base::ASCIIToUTF16("weather los angeles"), 2334);
   EXPECT_FALSE(cache.empty());
 }
 
@@ -23,7 +22,7 @@ TEST(AnswersCacheTest, GetWillRetrieveMatchingInfo) {
   AnswersCache cache(1);
 
   base::string16 full_query_text = base::ASCIIToUTF16("weather los angeles");
-  base::string16 query_type = base::ASCIIToUTF16("2334");
+  int query_type = 2334;
   cache.UpdateRecentAnswers(full_query_text, query_type);
 
   // Partial prefixes retrieve info.
@@ -33,7 +32,8 @@ TEST(AnswersCacheTest, GetWillRetrieveMatchingInfo) {
 
   // Mismatched prefix retrieves empty data.
   data = cache.GetTopAnswerEntry(full_query_text.substr(1, 8));
-  EXPECT_TRUE(data.full_query_text.empty() && data.query_type.empty());
+  EXPECT_TRUE(data.full_query_text.empty());
+  EXPECT_EQ(-1, data.query_type);
 
   // Full match retrieves info.
   data = cache.GetTopAnswerEntry(full_query_text);
@@ -46,7 +46,7 @@ TEST(AnswersCacheTest, MatchMostRecent) {
 
   base::string16 query_weather_la = base::ASCIIToUTF16("weather los angeles");
   base::string16 query_weather_lv = base::ASCIIToUTF16("weather las vegas");
-  base::string16 query_type = base::ASCIIToUTF16("2334");
+  int query_type = 2334;
 
   cache.UpdateRecentAnswers(query_weather_lv, query_type);
   cache.UpdateRecentAnswers(query_weather_la, query_type);
@@ -70,7 +70,7 @@ TEST(AnswersCacheTest, LeastRecentItemIsEvicted) {
   base::string16 query_weather_la = base::ASCIIToUTF16("weather los angeles");
   base::string16 query_weather_lv = base::ASCIIToUTF16("weather las vegas");
   base::string16 query_weather_lb = base::ASCIIToUTF16("weather long beach");
-  base::string16 query_type = base::ASCIIToUTF16("2334");
+  int query_type = 2334;
 
   cache.UpdateRecentAnswers(query_weather_lb, query_type);
   cache.UpdateRecentAnswers(query_weather_lv, query_type);
@@ -96,7 +96,7 @@ TEST(AnswersCacheTest, DuplicateEntries) {
   base::string16 query_weather_lv = base::ASCIIToUTF16("weather las vegas");
   base::string16 query_weather_lb = base::ASCIIToUTF16("weather long beach");
   base::string16 query_weather_l = base::ASCIIToUTF16("weather l");
-  base::string16 query_type = base::ASCIIToUTF16("2334");
+  int query_type = 2334;
 
   cache.UpdateRecentAnswers(query_weather_lb, query_type);
   cache.UpdateRecentAnswers(query_weather_lv, query_type);

components/omnibox/browser/autocomplete_match.cc

@@ -133,8 +133,6 @@ AutocompleteMatch::AutocompleteMatch(const AutocompleteMatch& match)
       description(match.description),
       description_class(match.description_class),
       swap_contents_and_description(match.swap_contents_and_description),
-      answer_contents(match.answer_contents),
-      answer_type(match.answer_type),
       answer(match.answer),
       transition(match.transition),
       type(match.type),
@@ -179,8 +177,6 @@ AutocompleteMatch& AutocompleteMatch::operator=(
   description = match.description;
   description_class = match.description_class;
   swap_contents_and_description = match.swap_contents_and_description;
-  answer_contents = match.answer_contents;
-  answer_type = match.answer_type;
   answer = match.answer;
   transition = match.transition;
   type = match.type;
@@ -751,8 +747,6 @@ size_t AutocompleteMatch::EstimateMemoryUsage() const {
   res += base::trace_event::EstimateMemoryUsage(contents_class);
   res += base::trace_event::EstimateMemoryUsage(description);
   res += base::trace_event::EstimateMemoryUsage(description_class);
-  res += base::trace_event::EstimateMemoryUsage(answer_contents);
-  res += base::trace_event::EstimateMemoryUsage(answer_type);
   res += sizeof(int);
   if (answer)
     res += base::trace_event::EstimateMemoryUsage(answer.value());

components/omnibox/browser/autocomplete_match.h

@@ -437,11 +437,7 @@ struct AutocompleteMatch {
   // before displaying.
   bool swap_contents_and_description;
 
-  // TODO(jdonnelly): Remove the first two properties once the downstream
-  // clients are using the SuggestionAnswer.
   // A rich-format version of the display for the dropdown.
-  base::string16 answer_contents;
-  base::string16 answer_type;
   base::Optional<SuggestionAnswer> answer;
 
   // The transition type to use when the user opens this match.  By default

components/omnibox/browser/base_search_provider.cc

@@ -266,8 +266,6 @@ AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
   match.image_url = suggestion.image_url();
   match.contents = suggestion.match_contents();
   match.contents_class = suggestion.match_contents_class();
-  match.answer_contents = suggestion.answer_contents();
-  match.answer_type = suggestion.answer_type();
   match.answer = suggestion.answer();
   match.subtype_identifier = suggestion.subtype_identifier();
   if (suggestion.type() == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
@@ -490,8 +488,6 @@ void BaseSearchProvider::AddMatchToMap(
     const AutocompleteMatch& less_relevant_match =
         more_relevant_match.duplicate_matches.back();
     if (less_relevant_match.answer && !more_relevant_match.answer) {
-      more_relevant_match.answer_type = less_relevant_match.answer_type;
-      more_relevant_match.answer_contents = less_relevant_match.answer_contents;
       more_relevant_match.answer = less_relevant_match.answer;
     }
   }

components/omnibox/browser/base_search_provider_unittest.cc

@@ -97,8 +97,6 @@ TEST_F(BaseSearchProviderTest, PreserveAnswersWhenDeduplicating) {
 
   TestBaseSearchProvider::MatchMap map;
   base::string16 query = base::ASCIIToUTF16("weather los angeles");
-  base::string16 answer_contents = base::ASCIIToUTF16("some answer content");
-  base::string16 answer_type = base::ASCIIToUTF16("2334");
   SuggestionAnswer answer;
   answer.set_type(2334);
 
@@ -121,7 +119,7 @@ TEST_F(BaseSearchProviderTest, PreserveAnswersWhenDeduplicating) {
       /*subtype_identifier=*/0, /*from_keyword_provider=*/false,
       /*relevance=*/850, /*relevance_from_server=*/true,
       /*input_text=*/query);
-  less_relevant.SetAnswer(answer_contents, answer_type, answer);
+  less_relevant.SetAnswer(answer);
   provider_->AddMatchToMap(
       less_relevant, std::string(), TemplateURLRef::NO_SUGGESTION_CHOSEN,
       false, false, &map);
@@ -131,22 +129,16 @@ TEST_F(BaseSearchProviderTest, PreserveAnswersWhenDeduplicating) {
   ASSERT_EQ(1U, match.duplicate_matches.size());
   AutocompleteMatch duplicate = match.duplicate_matches[0];
 
-  EXPECT_EQ(answer_contents, match.answer_contents);
-  EXPECT_EQ(answer_type, match.answer_type);
   EXPECT_TRUE(answer.Equals(*match.answer));
   EXPECT_EQ(AutocompleteMatchType::SEARCH_HISTORY, match.type);
   EXPECT_EQ(1300, match.relevance);
 
-  EXPECT_EQ(answer_contents, duplicate.answer_contents);
-  EXPECT_EQ(answer_type, duplicate.answer_type);
   EXPECT_TRUE(answer.Equals(*duplicate.answer));
   EXPECT_EQ(AutocompleteMatchType::SEARCH_SUGGEST, duplicate.type);
   EXPECT_EQ(850, duplicate.relevance);
 
   // Ensure answers are not copied over existing answers.
   map.clear();
-  base::string16 answer_contents2 = base::ASCIIToUTF16("different answer");
-  base::string16 answer_type2 = base::ASCIIToUTF16("8242");
   SuggestionAnswer answer2;
   answer2.set_type(8242);
   more_relevant = SearchSuggestionParser::SuggestResult(
@@ -154,7 +146,7 @@ TEST_F(BaseSearchProviderTest, PreserveAnswersWhenDeduplicating) {
       /*subtype_identifier=*/0, /*from_keyword_provider=*/false,
       /*relevance=*/1300, /*relevance_from_server=*/true,
       /*input_text=*/query);
-  more_relevant.SetAnswer(answer_contents2, answer_type2, answer2);
+  more_relevant.SetAnswer(answer2);
   provider_->AddMatchToMap(
       more_relevant, std::string(), TemplateURLRef::NO_SUGGESTION_CHOSEN,
       false, false, &map);
@@ -166,14 +158,10 @@ TEST_F(BaseSearchProviderTest, PreserveAnswersWhenDeduplicating) {
   ASSERT_EQ(1U, match.duplicate_matches.size());
   duplicate = match.duplicate_matches[0];
 
-  EXPECT_EQ(answer_contents2, match.answer_contents);
-  EXPECT_EQ(answer_type2, match.answer_type);
   EXPECT_TRUE(answer2.Equals(*match.answer));
   EXPECT_EQ(AutocompleteMatchType::SEARCH_HISTORY, match.type);
   EXPECT_EQ(1300, match.relevance);
 
-  EXPECT_EQ(answer_contents, duplicate.answer_contents);
-  EXPECT_EQ(answer_type, duplicate.answer_type);
   EXPECT_TRUE(answer.Equals(*duplicate.answer));
   EXPECT_EQ(AutocompleteMatchType::SEARCH_SUGGEST, duplicate.type);
   EXPECT_EQ(850, duplicate.relevance);

components/omnibox/browser/search_provider.cc

@@ -19,6 +19,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/rand_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/trace_event/trace_event.h"
@@ -155,14 +156,14 @@ void SearchProvider::RegisterDisplayedAnswers(
   // only be in the second slot if AutocompleteController ranked a local search
   // history or a verbatim item higher than the answer.
   auto match = result.begin();
-  if (match->answer_contents.empty() && result.size() > 1)
+  if (!match->answer && result.size() > 1)
     ++match;
-  if (match->answer_contents.empty() || match->answer_type.empty() ||
-      match->fill_into_edit.empty())
+  if (!match->answer || match->fill_into_edit.empty())
     return;
 
   // Valid answer encountered, cache it for further queries.
-  answers_cache_.UpdateRecentAnswers(match->fill_into_edit, match->answer_type);
+  answers_cache_.UpdateRecentAnswers(match->fill_into_edit,
+                                     match->answer->type());
 }
 
 // static
@@ -882,7 +883,7 @@ std::unique_ptr<network::SimpleURLLoader> SearchProvider::CreateSuggestLoader(
     search_term_args.prefetch_query =
         base::UTF16ToUTF8(prefetch_data_.full_query_text);
     search_term_args.prefetch_query_type =
-        base::UTF16ToUTF8(prefetch_data_.query_type);
+        base::NumberToString(prefetch_data_.query_type);
   }
 
   // Append a specific suggest client if it is in ChromeOS app_list launcher
@@ -993,8 +994,6 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
     // Verbatim results don't get suggestions and hence, answers.
     // Scan previous matches if the last answer-bearing suggestion matches
     // verbatim, and if so, copy over answer contents.
-    base::string16 answer_contents;
-    base::string16 answer_type;
     SuggestionAnswer answer;
     bool has_answer = false;
     base::string16 trimmed_verbatim_lower =
@@ -1002,8 +1001,6 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
     for (auto it = matches_.begin(); it != matches_.end(); ++it) {
       if (it->answer &&
           base::i18n::ToLower(it->fill_into_edit) == trimmed_verbatim_lower) {
-        answer_contents = it->answer_contents;
-        answer_type = it->answer_type;
         answer = *it->answer;
         has_answer = true;
         break;
@@ -1017,7 +1014,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
         verbatim_relevance, relevance_from_server,
         /*input_text=*/trimmed_verbatim);
     if (has_answer)
-      verbatim.SetAnswer(answer_contents, answer_type, answer);
+      verbatim.SetAnswer(answer);
     AddMatchToMap(verbatim, std::string(), did_not_accept_default_suggestion,
                   false, keyword_url != nullptr, &map);
   }
@@ -1123,8 +1120,6 @@ void SearchProvider::RemoveExtraAnswers(ACMatches* matches) {
       if (!answer_seen) {
         answer_seen = true;
       } else {
-        it->answer_contents.clear();
-        it->answer_type.clear();
         it->answer.reset();
       }
     }

components/omnibox/browser/search_suggestion_parser.cc

@@ -187,11 +187,7 @@ void SearchSuggestionParser::SuggestResult::ClassifyMatchContents(
 }
 
 void SearchSuggestionParser::SuggestResult::SetAnswer(
-    const base::string16& answer_contents,
-    const base::string16& answer_type,
     const SuggestionAnswer& answer) {
-  answer_contents_ = answer_contents;
-  answer_type_ = answer_type;
   answer_ = answer;
 }
 
@@ -517,8 +513,6 @@ bool SearchSuggestionParser::ParseSuggestResults(
       }
 
       base::string16 match_contents_prefix;
-      base::string16 answer_contents;
-      base::string16 answer_type;
       SuggestionAnswer answer;
       bool answer_parsed_successfully = false;
       std::string image_dominant_color;
@@ -540,6 +534,7 @@ bool SearchSuggestionParser::ParseSuggestResults(
 
           // Extract the Answer, if provided.
           const base::DictionaryValue* answer_json = nullptr;
+          base::string16 answer_type;
           if (suggestion_detail->GetDictionary("ansa", &answer_json) &&
               suggestion_detail->GetString("ansb", &answer_type)) {
             if (SuggestionAnswer::ParseAnswer(answer_json, answer_type,
@@ -547,11 +542,6 @@ bool SearchSuggestionParser::ParseSuggestResults(
               base::UmaHistogramSparse("Omnibox.AnswerParseType",
                                        answer.type());
               answer_parsed_successfully = true;
-              std::string contents;
-              base::JSONWriter::Write(*answer_json, &contents);
-              answer_contents = base::UTF8ToUTF16(contents);
-            } else {
-              answer_type = base::string16();
             }
             UMA_HISTOGRAM_BOOLEAN("Omnibox.AnswerParseSuccess",
                                   answer_parsed_successfully);
@@ -576,10 +566,8 @@ bool SearchSuggestionParser::ParseSuggestResults(
           relevances != nullptr,
           should_prefetch,
           trimmed_input));
-      if (answer_parsed_successfully) {
-        results->suggest_results.back().SetAnswer(answer_contents, answer_type,
-                                                  answer);
-      }
+      if (answer_parsed_successfully)
+        results->suggest_results.back().SetAnswer(answer);
     }
   }
   results->relevances_from_server = relevances != nullptr;

components/omnibox/browser/search_suggestion_parser.h

@@ -160,11 +160,7 @@ class SearchSuggestionParser {
       return additional_query_params_;
     }
 
-    void SetAnswer(const base::string16& answer_contents,
-                   const base::string16& answer_type,
-                   const SuggestionAnswer& answer);
-    const base::string16& answer_contents() const { return answer_contents_; }
-    const base::string16& answer_type() const { return answer_type_; }
+    void SetAnswer(const SuggestionAnswer& answer);
     const base::Optional<SuggestionAnswer>& answer() const { return answer_; }
 
     const std::string& image_dominant_color() const {
@@ -203,14 +199,6 @@ class SearchSuggestionParser {
     // Optional additional parameters to be added to the search URL.
     std::string additional_query_params_;
 
-    // TODO(jdonnelly): Remove the following two properties once the downstream
-    // clients are using the SuggestionAnswer.
-    // Optional formatted Answers result.
-    base::string16 answer_contents_;
-
-    // Type of optional formatted Answers result.
-    base::string16 answer_type_;
-
     // Optional short answer to the input that produced this suggestion.
     base::Optional<SuggestionAnswer> answer_;