Reland "[omnibox] Fix ZeroSuggestProvider duplicate classifications bug"

This is a reland of ebc60412

Original change's description:
> [omnibox] Fix ZeroSuggestProvider duplicate classifications bug
> 
> The below CL introduces a bug, where ZeroSuggestProvider can create
> a match with duplicate text classifications. This triggers a DCHECK
> as described in the linked bug.
> 
> https://chromium-review.googlesource.com/c/chromium/src/+/1947765
> 
> This CL fixes it and adds a test.
> 
> Bug: 1030741
> Change-Id: If59fbd320d720e2a7ff8d2f66d9ee92e11bb9a68
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1951557
> Commit-Queue: Tommy Li <tommycli@chromium.org>
> Reviewed-by: manuk hovanesian <manukh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#722497}

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

components/omnibox/browser/autocomplete_match.cc

@@ -1183,13 +1183,17 @@ void AutocompleteMatch::TryAutocompleteWithTitle(
 
 #if DCHECK_IS_ON()
 void AutocompleteMatch::Validate() const {
-  ValidateClassifications(contents, contents_class);
-  ValidateClassifications(description, description_class);
+  std::string provider_name = provider ? provider->GetName() : "None";
+  ValidateClassifications(contents, contents_class, provider_name);
+  ValidateClassifications(description, description_class, provider_name);
 }
+#endif  // DCHECK_IS_ON()
 
+// static
 void AutocompleteMatch::ValidateClassifications(
     const base::string16& text,
-    const ACMatchClassifications& classifications) const {
+    const ACMatchClassifications& classifications,
+    const std::string& provider_name) {
   if (text.empty()) {
     DCHECK(classifications.empty());
     return;
@@ -1205,7 +1209,6 @@ void AutocompleteMatch::ValidateClassifications(
   // The classifications should always be sorted.
   size_t last_offset = classifications[0].offset;
   for (auto i(classifications.begin() + 1); i != classifications.end(); ++i) {
-    const char* provider_name = provider ? provider->GetName() : "None";
     DCHECK_GT(i->offset, last_offset)
         << " Classification for \"" << text << "\" with offset of " << i->offset
         << " is unsorted in relation to last offset of " << last_offset
@@ -1217,4 +1220,3 @@ void AutocompleteMatch::ValidateClassifications(
     last_offset = i->offset;
   }
 }
-#endif  // DCHECK_IS_ON()

components/omnibox/browser/autocomplete_match.h

@@ -643,12 +643,13 @@ struct AutocompleteMatch {
 #if DCHECK_IS_ON()
   // Does a data integrity check on this match.
   void Validate() const;
+#endif  // DCHECK_IS_ON()
 
   // Checks one text/classifications pair for valid values.
-  void ValidateClassifications(
+  static void ValidateClassifications(
       const base::string16& text,
-      const ACMatchClassifications& classifications) const;
-#endif  // DCHECK_IS_ON()
+      const ACMatchClassifications& classifications,
+      const std::string& provider_name = "");
 };
 
 typedef AutocompleteMatch::ACMatchClassification ACMatchClassification;

components/omnibox/browser/search_suggestion_parser.cc

@@ -149,12 +149,15 @@ operator=(const SuggestResult& rhs) = default;
 void SearchSuggestionParser::SuggestResult::ClassifyMatchContents(
     const bool allow_bolding_all,
     const base::string16& input_text) {
-  if (input_text.empty()) {
-    // In case of zero-suggest results, do not highlight matches.
-    match_contents_class_.push_back(
-        ACMatchClassification(0, ACMatchClassification::NONE));
+  // 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())
     return;
-  }
 
   base::string16 lookup_text = input_text;
   if (type_ == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
@@ -181,6 +184,7 @@ void SearchSuggestionParser::SuggestResult::ClassifyMatchContents(
     return;
   }
 
+  // Note we discard our existing match_contents_class_ with this call.
   match_contents_class_ = AutocompleteProvider::ClassifyAllMatchesInString(
       input_text, match_contents_, true);
 }
@@ -244,12 +248,15 @@ void
 SearchSuggestionParser::NavigationResult::CalculateAndClassifyMatchContents(
     const bool allow_bolding_nothing,
     const base::string16& input_text) {
-  if (input_text.empty()) {
-    // In case of zero-suggest results, do not highlight matches.
-    match_contents_class_.push_back(
-        ACMatchClassification(0, ACMatchClassification::NONE));
+  // Start with the trivial nothing-bolded classification.
+  DCHECK(url_.is_valid());
+  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())
     return;
-  }
 
   // Set contents to the formatted URL while ensuring the scheme and subdomain
   // are kept if the user text seems to include them. E.g., for the user text

components/omnibox/browser/search_suggestion_parser_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/json/json_reader.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/test_scheme_classifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -154,3 +155,31 @@ TEST(SearchSuggestionParserTest, ParseSuggestResults) {
     ASSERT_EQ("http://example.com/a.png", suggestion_result.image_url());
   }
 }
+
+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.
+  result.ClassifyMatchContents(true, base::string16());
+  AutocompleteMatch::ValidateClassifications(result.match_contents(),
+                                             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(false, base::ASCIIToUTF16("apple"));
+  AutocompleteMatch::ValidateClassifications(result.match_contents(),
+                                             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"));
+  AutocompleteMatch::ValidateClassifications(result.match_contents(),
+                                             result.match_contents_class());
+}
\ No newline at end of file