[omnibox] Fix SuggestResult::ClassifyMatchContents edge case

This function has a truly weird contract that I didn't completely
understand initially, so I caused a regression.

Regression here:
https://chromium-review.googlesource.com/c/chromium/src/+/1955787

The weird contract is - it recalculates the match's classifications,
except in the case where we pass in a flag to forbid bold-all, and the
reclassification would otherwise bold-all. Then the function is
supposed to leave the existing classifications alone.

It's weird, but it's necessary for flicker-free search queries as the
user types with punctuation.

This CL fixes it.

It also adds some tests to prevent future regressions, as well as to
explicitly document the edge case.

I'll make a followup CL to do the same fix and test for NavigationResult
separately.

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

components/omnibox/browser/search_suggestion_parser.cc

@@ -148,15 +148,14 @@ operator=(const SuggestResult& rhs) = default;
 void SearchSuggestionParser::SuggestResult::ClassifyMatchContents(
     const bool allow_bolding_all,
     const base::string16& input_text) {
-  // Start with the trivial nothing-bolded classification.
   DCHECK(!match_contents_.empty());
-  match_contents_class_.clear();
-  match_contents_class_.push_back(
-      ACMatchClassification(0, ACMatchClassification::NONE));
 
   // Leave trivial classification alone in the ZeroSuggest case.
-  if (input_text.empty())
+  if (input_text.empty()) {
+    match_contents_class_ = {
+        ACMatchClassification(0, ACMatchClassification::NONE)};
     return;
+  }
 
   base::string16 lookup_text = input_text;
   if (type_ == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {

components/omnibox/browser/search_suggestion_parser_unittest.cc

@@ -157,29 +157,43 @@ TEST(SearchSuggestionParserTest, ParseSuggestResults) {
 }
 
 TEST(SearchSuggestionParserTest, SuggestClassification) {
-  AutocompleteMatch::ACMatchClassification none_classification(
-      0, AutocompleteMatch::ACMatchClassification::NONE);
-
   SearchSuggestionParser::SuggestResult result(
       base::ASCIIToUTF16("foobar"), AutocompleteMatchType::SEARCH_SUGGEST, 0,
       false, 400, true, base::string16());
   AutocompleteMatch::ValidateClassifications(result.match_contents(),
                                              result.match_contents_class());
 
-  // Re-classify the match contents, as the ZeroSuggestProvider does.
+  // Nothing should be bolded for ZeroSuggest classified input.
   result.ClassifyMatchContents(true, base::string16());
   AutocompleteMatch::ValidateClassifications(result.match_contents(),
                                              result.match_contents_class());
+  const ACMatchClassifications kNone = {
+      {0, AutocompleteMatch::ACMatchClassification::NONE}};
+  EXPECT_EQ(kNone, result.match_contents_class());
 
-  // Make sure that searching text-not-found still gives valid classifications,
-  // if we don't allow the code to bold everything.
+  // Test a simple case of bolding half the text.
+  result.ClassifyMatchContents(false, base::ASCIIToUTF16("foo"));
+  AutocompleteMatch::ValidateClassifications(result.match_contents(),
+                                             result.match_contents_class());
+  const ACMatchClassifications kHalfBolded = {
+      {0, AutocompleteMatch::ACMatchClassification::NONE},
+      {3, AutocompleteMatch::ACMatchClassification::MATCH}};
+  EXPECT_EQ(kHalfBolded, result.match_contents_class());
+
+  // Test the edge case that if we forbid bolding all, and then reclassifying
+  // would otherwise bold-all, we leave the existing classifications alone.
+  // This is weird, but it's in the function contract, and is useful for
+  // flicker-free search suggestions as the user types.
   result.ClassifyMatchContents(false, base::ASCIIToUTF16("apple"));
   AutocompleteMatch::ValidateClassifications(result.match_contents(),
                                              result.match_contents_class());
+  EXPECT_EQ(kHalfBolded, result.match_contents_class());
 
-  // Make sure that searching text-not-found still gives valid classifications,
-  // if we don't allow the code to bold everything.
-  result.ClassifyMatchContents(true, base::ASCIIToUTF16("foobar"));
+  // And finally, test the case where we do allow bolding-all.
+  result.ClassifyMatchContents(true, base::ASCIIToUTF16("apple"));
   AutocompleteMatch::ValidateClassifications(result.match_contents(),
                                              result.match_contents_class());
+  const ACMatchClassifications kBoldAll = {
+      {0, AutocompleteMatch::ACMatchClassification::MATCH}};
+  EXPECT_EQ(kBoldAll, result.match_contents_class());
 }
\ No newline at end of file