[Autofill] Added sorting of MatchingPatterns by score.

This CL adds sorting of MatchingPatterns in PatternProvider.

Bug: 1141654
Change-Id: I1ec0bc1bba489fe2217b55fb3711cee63749aac4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2493381
Commit-Queue: Christoph Schwering <schwering@google.com>
Reviewed-by: Matthias Körber <koerber@google.com>
Cr-Commit-Position: refs/heads/master@{#820797}

components/autofill/core/browser/pattern_provider/pattern_provider.cc

@@ -12,6 +12,7 @@
 #include "base/feature_list.h"
 #include "base/no_destructor.h"
 #include "components/autofill/core/browser/autofill_type.h"
+#include "components/autofill/core/browser/form_parsing/autofill_parsing_utils.h"
 #include "components/autofill/core/browser/pattern_provider/pattern_configuration_parser.h"
 #include "components/autofill/core/common/autofill_features.h"
 
@@ -19,10 +20,68 @@ namespace autofill {
 
 namespace {
 const char* kSourceCodeLanguage = "en";
+
+// Adds the English patterns, restricted to MatchFieldType MATCH_NAME, to
+// every other language.
+void EnrichPatternsWithEnVersion(
+    PatternProvider::Map* type_and_lang_to_patterns) {
+  DCHECK(type_and_lang_to_patterns);
+  for (auto& p : *type_and_lang_to_patterns) {
+    std::map<std::string, std::vector<MatchingPattern>>& lang_to_patterns =
+        p.second;
+
+    auto it = lang_to_patterns.find(kSourceCodeLanguage);
+    if (it == lang_to_patterns.end())
+      continue;
+    std::vector<MatchingPattern> en_patterns = it->second;
+    for (MatchingPattern& en_pattern : en_patterns) {
+      en_pattern.match_field_attributes = MATCH_NAME;
+    }
+
+    for (auto& q : lang_to_patterns) {
+      const std::string& page_language = q.first;
+      std::vector<MatchingPattern>& patterns = q.second;
+
+      if (page_language != kSourceCodeLanguage) {
+        patterns.insert(patterns.end(), en_patterns.begin(), en_patterns.end());
+      }
+    }
+  }
+}
+
+// Sorts patterns in descending order by their score.
+void SortPatternsByScore(PatternProvider::Map* type_and_lang_to_patterns) {
+  for (auto& p : *type_and_lang_to_patterns) {
+    std::map<std::string, std::vector<MatchingPattern>>& lang_to_patterns =
+        p.second;
+    for (auto& q : lang_to_patterns) {
+      std::vector<MatchingPattern>& patterns = q.second;
+      std::sort(patterns.begin(), patterns.end(),
+                [](const MatchingPattern& mp1, const MatchingPattern& mp2) {
+                  return mp1.positive_score > mp2.positive_score;
+                });
+    }
+  }
+}
 }
 
 PatternProvider* PatternProvider::g_pattern_provider = nullptr;
 
+// static
+PatternProvider& PatternProvider::GetInstance() {
+  if (!g_pattern_provider) {
+    static base::NoDestructor<PatternProvider> instance;
+    g_pattern_provider = instance.get();
+    field_type_parsing::PopulateFromResourceBundle();
+  }
+  return *g_pattern_provider;
+}
+
+// static
+void PatternProvider::ResetPatternProvider() {
+  g_pattern_provider = nullptr;
+}
+
 PatternProvider::PatternProvider() = default;
 PatternProvider::~PatternProvider() = default;
 
@@ -35,7 +94,8 @@ void PatternProvider::SetPatterns(PatternProvider::Map patterns,
       (overwrite_equal_version && pattern_version_ == version)) {
     patterns_ = patterns;
     pattern_version_ = version;
-    EnrichPatternsWithEnVersion();
+    EnrichPatternsWithEnVersion(&patterns_);
+    SortPatternsByScore(&patterns_);
   }
 }
 
@@ -77,45 +137,6 @@ const std::vector<MatchingPattern> PatternProvider::GetMatchPatterns(
   return GetMatchPatterns(pattern_name, page_language);
 }
 
-// static
-PatternProvider& PatternProvider::GetInstance() {
-  if (!g_pattern_provider) {
-    static base::NoDestructor<PatternProvider> instance;
-    g_pattern_provider = instance.get();
-    field_type_parsing::PopulateFromResourceBundle();
-  }
-  return *g_pattern_provider;
-}
-
-// static
-void PatternProvider::ResetPatternProvider() {
-  g_pattern_provider = nullptr;
-}
-
-void PatternProvider::EnrichPatternsWithEnVersion() {
-  for (auto& p : patterns_) {
-    std::map<std::string, std::vector<MatchingPattern>>& lg_to_patterns =
-        p.second;
-
-    auto it = lg_to_patterns.find(kSourceCodeLanguage);
-    if (it == lg_to_patterns.end())
-      continue;
-    std::vector<MatchingPattern> en_patterns = it->second;
-    for (MatchingPattern& en_pattern : en_patterns) {
-      en_pattern.match_field_attributes = MATCH_NAME;
-    }
-
-    for (auto& q : lg_to_patterns) {
-      const std::string& page_language = q.first;
-      std::vector<MatchingPattern>& patterns = q.second;
-
-      if (page_language != kSourceCodeLanguage) {
-        patterns.insert(patterns.end(), en_patterns.begin(), en_patterns.end());
-      }
-    }
-  }
-}
-
 const std::vector<MatchingPattern> PatternProvider::GetAllPatternsByType(
     ServerFieldType type) const {
   std::string type_str = AutofillType(type).ToString();
@@ -130,20 +151,16 @@ const std::vector<MatchingPattern> PatternProvider::GetAllPatternsByType(
   const std::map<std::string, std::vector<MatchingPattern>>& type_patterns =
       it->second;
 
-  size_t en_size = [&type_patterns]() -> size_t {
-    auto jt = type_patterns.find(kSourceCodeLanguage);
-    return jt != type_patterns.end() ? jt->second.size() : 0;
-  }();
-
   std::vector<MatchingPattern> all_language_patterns;
   for (const auto& p : type_patterns) {
     const std::string& page_language = p.first;
     const std::vector<MatchingPattern>& language_patterns = p.second;
-    DCHECK(language_patterns.size() >= en_size);
-    auto end = language_patterns.end() -
-               (page_language == kSourceCodeLanguage ? 0 : en_size);
-    all_language_patterns.insert(all_language_patterns.end(),
-                                 language_patterns.begin(), end);
+    for (const MatchingPattern& mp : language_patterns) {
+      if (page_language == kSourceCodeLanguage ||
+          mp.language != kSourceCodeLanguage) {
+        all_language_patterns.push_back(mp);
+      }
+    }
   }
   return all_language_patterns;
 }

components/autofill/core/browser/pattern_provider/pattern_provider.h

@@ -72,12 +72,6 @@ class PatternProvider {
   const Map& patterns() const { return patterns_; }
 
  private:
-  void SortPatternsByScore(std::vector<MatchingPattern>& patterns);
-
-  // Adds the English patterns, restricted to MatchFieldType MATCH_NAME, to
-  // every other language.
-  void EnrichPatternsWithEnVersion();
-
   FRIEND_TEST_ALL_PREFIXES(AutofillPatternProviderPipelineTest,
                            TestParsingEquivalent);
   FRIEND_TEST_ALL_PREFIXES(AutofillPatternProviderPipelineTest,

components/autofill/core/browser/pattern_provider/pattern_provider_unittest.cc

@@ -12,6 +12,8 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "components/autofill/core/browser/autofill_type.h"
+#include "components/autofill/core/browser/field_types.h"
+#include "components/autofill/core/browser/form_parsing/autofill_parsing_utils.h"
 #include "components/autofill/core/browser/pattern_provider/pattern_configuration_parser.h"
 #include "components/autofill/core/browser/pattern_provider/pattern_provider.h"
 #include "components/autofill/core/browser/pattern_provider/test_pattern_provider.h"
@@ -51,15 +53,23 @@ MatchingPattern GetCompanyPatternDe() {
 class UnitTestPatternProvider : public PatternProvider {
  public:
   UnitTestPatternProvider();
+  UnitTestPatternProvider(const std::vector<MatchingPattern>& de_patterns,
+                          const std::vector<MatchingPattern>& en_patterns);
   ~UnitTestPatternProvider();
 };
 
-UnitTestPatternProvider::UnitTestPatternProvider() {
+UnitTestPatternProvider::UnitTestPatternProvider()
+    : UnitTestPatternProvider({GetCompanyPatternDe()},
+                              {GetCompanyPatternEn()}) {}
+
+UnitTestPatternProvider::UnitTestPatternProvider(
+    const std::vector<MatchingPattern>& de_patterns,
+    const std::vector<MatchingPattern>& en_patterns) {
   PatternProvider::SetPatternProviderForTesting(this);
   Map patterns;
   auto& company_patterns = patterns[AutofillType(COMPANY_NAME).ToString()];
-  company_patterns["en"].push_back(GetCompanyPatternEn());
-  company_patterns["de"].push_back(GetCompanyPatternDe());
+  company_patterns["de"] = de_patterns;
+  company_patterns["en"] = en_patterns;
   SetPatterns(patterns, base::Version(), true);
 }
 
@@ -211,4 +221,31 @@ TEST(AutofillPatternProvider, EnrichPatternsWithEnVersion) {
   }
 }
 
+TEST(AutofillPatternProvider, SortPatternsByScore) {
+  base::test::ScopedFeatureList feature;
+  feature.InitWithFeatures(
+      // enabled
+      {features::kAutofillUsePageLanguageToSelectFieldParsingPatterns,
+       features::kAutofillApplyNegativePatternsForFieldTypeDetectionHeuristics},
+      // disabled
+      {});
+  std::vector<MatchingPattern> de_input_patterns;
+  de_input_patterns.push_back(GetCompanyPatternDe());
+  de_input_patterns.push_back(GetCompanyPatternDe());
+  de_input_patterns.push_back(GetCompanyPatternDe());
+  de_input_patterns.push_back(GetCompanyPatternDe());
+  de_input_patterns[0].positive_score = 3.0;
+  de_input_patterns[1].positive_score = 1.0;
+  de_input_patterns[2].positive_score = 5.0;
+  de_input_patterns[3].positive_score = 3.0;
+  UnitTestPatternProvider p(de_input_patterns, {});
+  const std::vector<MatchingPattern>& de_patterns =
+      p.GetMatchPatterns(COMPANY_NAME, "de");
+  ASSERT_EQ(de_patterns.size(), de_input_patterns.size());
+  EXPECT_EQ(de_patterns[0].positive_score, 5.0);
+  EXPECT_EQ(de_patterns[1].positive_score, 3.0);
+  EXPECT_EQ(de_patterns[2].positive_score, 3.0);
+  EXPECT_EQ(de_patterns[3].positive_score, 1.0);
+}
+
 }  // namespace autofill