Separate provider filtering and metrics from UI

This change moves filtering of the DoH provider list and reporting of
UMA metrics out of the UI code to dns_util.cc.  This simplifies the UI
code and enables sharing of functionality with the forthcoming Android
DoH control UI.

Change-Id: I66d0313d9e913ad1d34ac65a2278771261788b7d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2174779
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774500}

chrome/browser/BUILD.gn

@@ -2057,6 +2057,7 @@ static_library("browser") {
     "//components/contextual_search/content:browser",
     "//components/contextual_search/core:browser",
     "//components/cookie_config",
+    "//components/country_codes",
     "//components/crx_file",
     "//components/data_reduction_proxy/core/browser",
     "//components/data_use_measurement/core:ascriber",

chrome/browser/DEPS

@@ -77,6 +77,7 @@ include_rules = [
   "+components/contextual_search/content/common",
   "+components/contextual_search/core/browser",
   "+components/cookie_config",
+  "+components/country_codes",
   "+components/crash/content/app",
   "+components/crash/content/browser",
   "+components/crash/core/app",

chrome/browser/net/dns_over_https_browsertest.cc

@@ -13,7 +13,7 @@
 #include "content/public/common/content_features.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/test_navigation_observer.h"
-#include "net/dns/public/doh_provider_list.h"
+#include "net/dns/public/doh_provider_entry.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -32,10 +32,9 @@ struct DohParameter {
 
 std::vector<DohParameter> GetDohServerTestCases() {
   std::vector<DohParameter> doh_test_cases;
-  const auto& doh_providers = net::GetDohProviderList();
-  for (const auto& doh_provider : doh_providers) {
-    doh_test_cases.emplace_back(doh_provider.provider,
-                                doh_provider.dns_over_https_template, true);
+  for (const auto* entry : net::DohProviderEntry::GetList()) {
+    doh_test_cases.emplace_back(entry->provider, entry->dns_over_https_template,
+                                true);
   }
   // Negative test-case
   doh_test_cases.emplace_back("NegativeTestExampleCom",

chrome/browser/net/secure_dns_util.cc

@@ -8,12 +8,15 @@
 #include <string>
 
 #include "base/feature_list.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/string_split.h"
 #include "chrome/common/chrome_features.h"
+#include "components/country_codes/country_codes.h"
 #include "components/embedder_support/pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "net/dns/dns_config_overrides.h"
+#include "net/dns/public/doh_provider_entry.h"
 #include "net/dns/public/util.h"
 #include "net/third_party/uri_template/uri_template.h"
 #include "url/gurl.h"
@@ -26,6 +29,37 @@ namespace {
 
 const char kAlternateErrorPagesBackup[] = "alternate_error_pages.backup";
 
+void IncrementDropdownHistogram(net::DohProviderIdForHistogram id,
+                                const std::string& doh_template,
+                                base::StringPiece old_template,
+                                base::StringPiece new_template) {
+  if (doh_template == old_template) {
+    UMA_HISTOGRAM_ENUMERATION("Net.DNS.UI.DropdownSelectionEvent.Unselected",
+                              id);
+  } else if (doh_template == new_template) {
+    UMA_HISTOGRAM_ENUMERATION("Net.DNS.UI.DropdownSelectionEvent.Selected", id);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION("Net.DNS.UI.DropdownSelectionEvent.Ignored", id);
+  }
+}
+
+bool EntryIsForCountry(const net::DohProviderEntry* entry, int country_id) {
+  if (entry->display_globally) {
+    return true;
+  }
+  const auto& countries = entry->display_countries;
+  bool matches = std::any_of(countries.begin(), countries.end(),
+                             [country_id](const std::string& country_code) {
+                               return country_codes::CountryStringToCountryID(
+                                          country_code) == country_id;
+                             });
+  if (matches) {
+    DCHECK(!entry->ui_name.empty());
+    DCHECK(!entry->privacy_policy.empty());
+  }
+  return matches;
+}
+
 }  // namespace
 
 void RegisterProbesSettingBackupPref(PrefRegistrySimple* registry) {
@@ -68,6 +102,35 @@ void MigrateProbesSettingToOrFromBackup(PrefService* prefs) {
   }
 }
 
+net::DohProviderEntry::List ProvidersForCountry(
+    const net::DohProviderEntry::List& providers,
+    int country_id) {
+  net::DohProviderEntry::List local_providers;
+  std::copy_if(providers.begin(), providers.end(),
+               std::back_inserter(local_providers),
+               [country_id](const auto* entry) {
+                 return EntryIsForCountry(entry, country_id);
+               });
+  return local_providers;
+}
+
+std::vector<std::string> GetDisabledProviders() {
+  return SplitString(features::kDnsOverHttpsDisabledProvidersParam.Get(), ",",
+                     base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+}
+
+net::DohProviderEntry::List RemoveDisabledProviders(
+    const net::DohProviderEntry::List& providers,
+    const std::vector<string>& disabled_providers) {
+  net::DohProviderEntry::List filtered_providers;
+  std::copy_if(providers.begin(), providers.end(),
+               std::back_inserter(filtered_providers),
+               [&disabled_providers](const auto* entry) {
+                 return !base::Contains(disabled_providers, entry->provider);
+               });
+  return filtered_providers;
+}
+
 std::vector<base::StringPiece> SplitGroup(base::StringPiece group) {
   // Templates in a group are whitespace-separated.
   return SplitStringPiece(group, " ", base::TRIM_WHITESPACE,
@@ -83,6 +146,28 @@ bool IsValidGroup(base::StringPiece group) {
   });
 }
 
+void UpdateDropdownHistograms(
+    const std::vector<const net::DohProviderEntry*>& providers,
+    base::StringPiece old_template,
+    base::StringPiece new_template) {
+  for (const auto* entry : providers) {
+    IncrementDropdownHistogram(entry->provider_id_for_histogram.value(),
+                               entry->dns_over_https_template, old_template,
+                               new_template);
+  }
+  // An empty template indicates a custom provider.
+  IncrementDropdownHistogram(net::DohProviderIdForHistogram::kCustom,
+                             std::string(), old_template, new_template);
+}
+
+void UpdateValidationHistogram(bool valid) {
+  UMA_HISTOGRAM_BOOLEAN("Net.DNS.UI.ValidationAttemptSuccess", valid);
+}
+
+void UpdateProbeHistogram(bool success) {
+  UMA_HISTOGRAM_BOOLEAN("Net.DNS.UI.ProbeAttemptSuccess", success);
+}
+
 void ApplyTemplate(net::DnsConfigOverrides* overrides,
                    base::StringPiece server_template) {
   std::string server_method;

chrome/browser/net/secure_dns_util.h

@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/strings/string_piece.h"
+#include "net/dns/public/doh_provider_entry.h"
 
 namespace net {
 struct DnsConfigOverrides;
@@ -20,6 +21,22 @@ namespace chrome_browser_net {
 
 namespace secure_dns {
 
+// Returns the subset of |providers| that are marked for use in the specified
+// country.
+net::DohProviderEntry::List ProvidersForCountry(
+    const net::DohProviderEntry::List& providers,
+    int country_id);
+
+// Returns the names of providers that have been remotely disabled, for use with
+// RemoveDisabledProviders().
+std::vector<std::string> GetDisabledProviders();
+
+// Returns the subset of |providers| for which |DohProviderEntry::provider| is
+// not listed in |disabled_providers|.
+net::DohProviderEntry::List RemoveDisabledProviders(
+    const net::DohProviderEntry::List& providers,
+    const std::vector<std::string>& disabled_providers);
+
 // Implements the whitespace-delimited group syntax for DoH templates.
 std::vector<base::StringPiece> SplitGroup(base::StringPiece group);
 
@@ -28,6 +45,16 @@ std::vector<base::StringPiece> SplitGroup(base::StringPiece group);
 // stored preferences.
 bool IsValidGroup(base::StringPiece group);
 
+// When the selected template changes, call this function to update the
+// Selected, Unselected, and Ignored histograms for all the included providers,
+// and also for the custom provider option.  If the old or new selection is the
+// custom provider option, pass an empty string as the template.
+void UpdateDropdownHistograms(const net::DohProviderEntry::List& providers,
+                              base::StringPiece old_template,
+                              base::StringPiece new_template);
+void UpdateValidationHistogram(bool valid);
+void UpdateProbeHistogram(bool success);
+
 // Modifies |overrides| to use the DoH server specified by |server_template|.
 void ApplyTemplate(net::DnsConfigOverrides* overrides,
                    base::StringPiece server_template);

chrome/browser/net/secure_dns_util_unittest.cc

@@ -6,12 +6,15 @@
 
 #include <memory>
 
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/common/chrome_features.h"
+#include "components/country_codes/country_codes.h"
 #include "components/embedder_support/pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
 #include "net/dns/dns_config_overrides.h"
+#include "net/dns/public/doh_provider_entry.h"
 #include "testing/gmock/include/gmock/gmock-matchers.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -27,7 +30,7 @@ const char kAlternateErrorPagesBackup[] = "alternate_error_pages.backup";
 
 namespace secure_dns {
 
-class DNSUtilTest : public testing::Test {
+class SecureDnsUtilTest : public testing::Test {
  public:
   void SetUp() override { DisableRedesign(); }
 
@@ -43,7 +46,7 @@ class DNSUtilTest : public testing::Test {
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-TEST_F(DNSUtilTest, MigrateProbesPref) {
+TEST_F(SecureDnsUtilTest, MigrateProbesPref) {
   TestingPrefServiceSimple prefs;
   prefs.registry()->RegisterBooleanPref(
       embedder_support::kAlternateErrorPagesEnabled, true);
@@ -150,14 +153,14 @@ TEST_F(DNSUtilTest, MigrateProbesPref) {
   EXPECT_FALSE(backup_pref->HasUserSetting());
 }
 
-TEST(DNSUtil, SplitGroup) {
+TEST(SecureDnsUtil, SplitGroup) {
   EXPECT_THAT(SplitGroup("a"), ElementsAre("a"));
   EXPECT_THAT(SplitGroup("a b"), ElementsAre("a", "b"));
   EXPECT_THAT(SplitGroup("a \tb\nc"), ElementsAre("a", "b\nc"));
   EXPECT_THAT(SplitGroup(" \ta b\n"), ElementsAre("a", "b"));
 }
 
-TEST(DNSUtil, IsValidGroup) {
+TEST(SecureDnsUtil, IsValidGroup) {
   EXPECT_TRUE(IsValidGroup(""));
   EXPECT_TRUE(IsValidGroup("https://valid"));
   EXPECT_TRUE(IsValidGroup("https://valid https://valid2"));
@@ -168,7 +171,7 @@ TEST(DNSUtil, IsValidGroup) {
   EXPECT_FALSE(IsValidGroup("invalid invalid2"));
 }
 
-TEST(DNSUtil, ApplyDohTemplatePost) {
+TEST(SecureDnsUtil, ApplyDohTemplatePost) {
   std::string post_template("https://valid");
   net::DnsConfigOverrides overrides;
   ApplyTemplate(&overrides, post_template);
@@ -178,7 +181,7 @@ TEST(DNSUtil, ApplyDohTemplatePost) {
                   {post_template, true /* use_post */}))));
 }
 
-TEST(DNSUtil, ApplyDohTemplateGet) {
+TEST(SecureDnsUtil, ApplyDohTemplateGet) {
   std::string get_template("https://valid/{?dns}");
   net::DnsConfigOverrides overrides;
   ApplyTemplate(&overrides, get_template);
@@ -188,6 +191,105 @@ TEST(DNSUtil, ApplyDohTemplateGet) {
                   {get_template, false /* use_post */}))));
 }
 
+net::DohProviderEntry::List GetProvidersForTesting() {
+  static const auto global1 = net::DohProviderEntry::ConstructForTesting(
+      "Provider_Global1", net::DohProviderIdForHistogram(-1), {} /*ip_strs */,
+      {} /* dot_hostnames */, "https://global1.provider/dns-query{?dns}",
+      "Global Provider 1" /* ui_name */,
+      "https://global1.provider/privacy_policy/" /* privacy_policy */,
+      true /* display_globally */, {} /* display_countries */);
+  static const auto no_display = net::DohProviderEntry::ConstructForTesting(
+      "Provider_NoDisplay", net::DohProviderIdForHistogram(-2), {} /*ip_strs */,
+      {} /* dot_hostnames */, "https://nodisplay.provider/dns-query{?dns}",
+      "No Display Provider" /* ui_name */,
+      "https://nodisplay.provider/privacy_policy/" /* privacy_policy */,
+      false /* display_globally */, {} /* display_countries */);
+  static const auto ee_fr = net::DohProviderEntry::ConstructForTesting(
+      "Provider_EE_FR", net::DohProviderIdForHistogram(-3), {} /*ip_strs */,
+      {} /* dot_hostnames */, "https://ee.fr.provider/dns-query{?dns}",
+      "EE/FR Provider" /* ui_name */,
+      "https://ee.fr.provider/privacy_policy/" /* privacy_policy */,
+      false /* display_globally */, {"EE", "FR"} /* display_countries */);
+  static const auto fr = net::DohProviderEntry::ConstructForTesting(
+      "Provider_FR", net::DohProviderIdForHistogram(-4), {} /*ip_strs */,
+      {} /* dot_hostnames */, "https://fr.provider/dns-query{?dns}",
+      "FR Provider" /* ui_name */,
+      "https://fr.provider/privacy_policy/" /* privacy_policy */,
+      false /* display_globally */, {"FR"} /* display_countries */);
+  static const auto global2 = net::DohProviderEntry::ConstructForTesting(
+      "Provider_Global2", net::DohProviderIdForHistogram(-5), {} /*ip_strs */,
+      {} /* dot_hostnames */, "https://global2.provider/dns-query{?dns}",
+      "Global Provider 2" /* ui_name */,
+      "https://global2.provider/privacy_policy/" /* privacy_policy */,
+      true /* display_globally */, {} /* display_countries */);
+  return {&global1, &no_display, &ee_fr, &fr, &global2};
+}
+
+TEST(SecureDnsUtil, LocalProviders) {
+  const auto providers = GetProvidersForTesting();
+
+  const auto fr_providers = ProvidersForCountry(
+      providers, country_codes::CountryStringToCountryID("FR"));
+  EXPECT_THAT(fr_providers, ElementsAre(providers[0], providers[2],
+                                        providers[3], providers[4]));
+
+  const auto ee_providers = ProvidersForCountry(
+      providers, country_codes::CountryStringToCountryID("EE"));
+  EXPECT_THAT(ee_providers,
+              ElementsAre(providers[0], providers[2], providers[4]));
+
+  const auto us_providers = ProvidersForCountry(
+      providers, country_codes::CountryStringToCountryID("US"));
+  EXPECT_THAT(us_providers, ElementsAre(providers[0], providers[4]));
+
+  const auto unknown_providers =
+      ProvidersForCountry(providers, country_codes::kCountryIDUnknown);
+  EXPECT_THAT(unknown_providers, ElementsAre(providers[0], providers[4]));
+}
+
+TEST(SecureDnsUtil, DisabledProviders) {
+  base::test::ScopedFeatureList scoped_features;
+  scoped_features.InitAndEnableFeatureWithParameters(
+      features::kDnsOverHttps,
+      {{"DisabledProviders", "Provider_Global2, , Provider_EE_FR,Unexpected"}});
+  EXPECT_THAT(GetDisabledProviders(),
+              ElementsAre("Provider_Global2", "Provider_EE_FR", "Unexpected"));
+}
+
+TEST(SecureDnsUtil, RemoveDisabled) {
+  const auto providers = GetProvidersForTesting();
+  const auto filtered_providers = RemoveDisabledProviders(
+      providers, {"Provider_Global2", "", "Provider_EE_FR", "Unexpected"});
+  EXPECT_THAT(filtered_providers,
+              ElementsAre(providers[0], providers[1], providers[3]));
+}
+
+TEST(SecureDnsUtil, UpdateDropdownHistograms) {
+  base::HistogramTester histograms;
+
+  const auto providers = GetProvidersForTesting();
+  UpdateDropdownHistograms(providers, providers[4]->dns_over_https_template,
+                           providers[0]->dns_over_https_template);
+
+  const std::string kUmaBase = "Net.DNS.UI.DropdownSelectionEvent";
+  histograms.ExpectTotalCount(kUmaBase + ".Ignored", 4u);
+  histograms.ExpectTotalCount(kUmaBase + ".Selected", 1u);
+  histograms.ExpectTotalCount(kUmaBase + ".Unselected", 1u);
+}
+
+TEST(SecureDnsUtil, UpdateDropdownHistogramsCustom) {
+  base::HistogramTester histograms;
+
+  const auto providers = GetProvidersForTesting();
+  UpdateDropdownHistograms(providers, std::string(),
+                           providers[2]->dns_over_https_template);
+
+  const std::string kUmaBase = "Net.DNS.UI.DropdownSelectionEvent";
+  histograms.ExpectTotalCount(kUmaBase + ".Ignored", 4u);
+  histograms.ExpectTotalCount(kUmaBase + ".Selected", 1u);
+  histograms.ExpectTotalCount(kUmaBase + ".Unselected", 1u);
+}
+
 }  // namespace secure_dns
 
 }  // namespace chrome_browser_net

chrome/browser/resources/settings/privacy_page/secure_dns.js

@@ -189,7 +189,7 @@ Polymer({
     this.showRadioGroup_ =
         /** @type {boolean} */ (this.secureDnsToggle_.value);
     if (this.secureDnsRadio_ === SecureDnsMode.SECURE &&
-        this.$.secureResolverSelect.value === 'custom') {
+        !this.$.secureResolverSelect.value) {
       this.$.secureDnsInput.focus();
     }
     this.updateDnsPrefs_(
@@ -205,7 +205,7 @@ Polymer({
    */
   onRadioSelectionChanged_: function(event) {
     if (event.detail.value === SecureDnsMode.SECURE &&
-        this.$.secureResolverSelect.value === 'custom') {
+        !this.$.secureResolverSelect.value) {
       this.$.secureDnsInput.focus();
     }
     this.updateDnsPrefs_(event.detail.value);
@@ -230,7 +230,7 @@ Polymer({
         // was not specified, the custom entry may be invalid or may not
         // have passed validation yet, and we should not update either the
         // underlying mode or templates prefs.
-        if (this.$.secureResolverSelect.value === 'custom') {
+        if (!this.$.secureResolverSelect.value) {
           if (!templates) {
             return;
           }
@@ -276,7 +276,7 @@ Polymer({
     }
     this.updatePrivacyPolicyLine_();
 
-    if (this.$.secureResolverSelect.value === 'custom') {
+    if (!this.$.secureResolverSelect.value) {
       this.$.secureDnsInput.focus();
     }
 
@@ -362,8 +362,8 @@ Polymer({
     }
 
     // Otherwise, select the custom option.
-    this.$.secureResolverSelect.value = 'custom';
-    this.lastResolverOption_ = 'custom';
+    this.$.secureResolverSelect.value = '';
+    this.lastResolverOption_ = '';
 
     // Only update the custom input field if the templates are non-empty.
     // Otherwise, we may be clearing a previous value that the user wishes to
@@ -381,7 +381,7 @@ Polymer({
   updatePrivacyPolicyLine_: function() {
     // If the selected item is the custom provider option, hide the privacy
     // policy line.
-    if (this.$.secureResolverSelect.value === 'custom') {
+    if (!this.$.secureResolverSelect.value) {
       this.$.privacyPolicy.style.display = 'none';
       this.$.secureDnsInput.style.display = 'block';
       return;

chrome/browser/ui/webui/settings/settings_secure_dns_handler.cc

@@ -8,7 +8,6 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/net/secure_dns_config.h"
@@ -24,10 +23,12 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "net/dns/public/dns_over_https_server_config.h"
-#include "net/dns/public/doh_provider_list.h"
+#include "net/dns/public/doh_provider_entry.h"
 #include "net/dns/public/util.h"
 #include "ui/base/l10n/l10n_util.h"
 
+namespace secure_dns = chrome_browser_net::secure_dns;
+
 namespace settings {
 
 namespace {
@@ -57,11 +58,7 @@ std::unique_ptr<base::DictionaryValue> CreateSecureDnsSettingDict() {
 
 }  // namespace
 
-SecureDnsHandler::SecureDnsHandler()
-    : network_context_getter_(
-          base::BindRepeating(&SecureDnsHandler::GetNetworkContext,
-                              base::Unretained(this))) {}
-
+SecureDnsHandler::SecureDnsHandler() = default;
 SecureDnsHandler::~SecureDnsHandler() = default;
 
 void SecureDnsHandler::RegisterMessages() {
@@ -111,38 +108,14 @@ void SecureDnsHandler::OnJavascriptDisallowed() {
   pref_registrar_.RemoveAll();
 }
 
-base::Value SecureDnsHandler::GetSecureDnsResolverListForCountry(
-    int country_id,
-    const std::vector<net::DohProviderEntry>& providers) {
-  std::vector<std::string> disabled_providers =
-      SplitString(features::kDnsOverHttpsDisabledProvidersParam.Get(), ",",
-                  base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
-
+base::Value SecureDnsHandler::GetSecureDnsResolverList() {
   base::Value resolvers(base::Value::Type::LIST);
-  resolver_histogram_map_.clear();
-  // Add all non-disabled resolvers that should be displayed in |country_id|.
-  for (const auto& entry : providers) {
-    if (base::Contains(disabled_providers, entry.provider))
-      continue;
-
-    if (entry.display_globally ||
-        std::find_if(
-            entry.display_countries.begin(), entry.display_countries.end(),
-            [&country_id](const std::string& country_code) {
-              return country_codes::CountryCharsToCountryID(
-                         country_code[0], country_code[1]) == country_id;
-            }) != entry.display_countries.end()) {
-      DCHECK(!entry.ui_name.empty());
-      DCHECK(!entry.privacy_policy.empty());
-      base::Value dict(base::Value::Type::DICTIONARY);
-      dict.SetKey("name", base::Value(entry.ui_name));
-      dict.SetKey("value", base::Value(entry.dns_over_https_template));
-      dict.SetKey("policy", base::Value(entry.privacy_policy));
-      resolvers.Append(std::move(dict));
-      DCHECK(entry.provider_id_for_histogram.has_value());
-      resolver_histogram_map_.insert({entry.dns_over_https_template,
-                                      entry.provider_id_for_histogram.value()});
-    }
+  for (const auto* entry : providers_) {
+    base::Value dict(base::Value::Type::DICTIONARY);
+    dict.SetStringKey("name", entry->ui_name);
+    dict.SetStringKey("value", entry->dns_over_https_template);
+    dict.SetStringKey("policy", entry->privacy_policy);
+    resolvers.Append(std::move(dict));
   }
 
   // Randomize the order of the resolvers.
@@ -150,13 +123,10 @@ base::Value SecureDnsHandler::GetSecureDnsResolverListForCountry(
 
   // Add a custom option to the front of the list
   base::Value custom(base::Value::Type::DICTIONARY);
-  custom.SetKey("name",
-                base::Value(l10n_util::GetStringUTF8(IDS_SETTINGS_CUSTOM)));
-  custom.SetKey("value", base::Value("custom"));
-  custom.SetKey("policy", base::Value(std::string()));
+  custom.SetStringKey("name", l10n_util::GetStringUTF8(IDS_SETTINGS_CUSTOM));
+  custom.SetStringKey("value", std::string());  // Empty value means custom.
+  custom.SetStringKey("policy", std::string());
   resolvers.Insert(resolvers.GetList().begin(), std::move(custom));
-  resolver_histogram_map_.insert(
-      {"custom", net::DohProviderIdForHistogram::kCustom});
 
   return resolvers;
 }
@@ -176,15 +146,18 @@ network::mojom::NetworkContext* SecureDnsHandler::GetNetworkContext() {
       ->GetNetworkContext();
 }
 
+void SecureDnsHandler::SetProvidersForTesting(
+    net::DohProviderEntry::List providers) {
+  providers_ = std::move(providers);
+}
+
 void SecureDnsHandler::HandleGetSecureDnsResolverList(
     const base::ListValue* args) {
   AllowJavascript();
   std::string callback_id = args->GetList()[0].GetString();
 
-  ResolveJavascriptCallback(
-      base::Value(callback_id),
-      GetSecureDnsResolverListForCountry(country_codes::GetCurrentCountryID(),
-                                         net::GetDohProviderList()));
+  ResolveJavascriptCallback(base::Value(callback_id),
+                            GetSecureDnsResolverList());
 }
 
 void SecureDnsHandler::HandleGetSecureDnsSetting(const base::ListValue* args) {
@@ -203,14 +176,12 @@ void SecureDnsHandler::HandleParseCustomDnsEntry(const base::ListValue* args) {
 
   // Return all templates in the entry, or none if they are not all valid.
   base::Value templates(base::Value::Type::LIST);
-  if (chrome_browser_net::secure_dns::IsValidGroup(custom_entry)) {
-    for (base::StringPiece t :
-         chrome_browser_net::secure_dns::SplitGroup(custom_entry)) {
+  if (secure_dns::IsValidGroup(custom_entry)) {
+    for (base::StringPiece t : secure_dns::SplitGroup(custom_entry)) {
       templates.Append(t);
     }
   }
-  UMA_HISTOGRAM_BOOLEAN("Net.DNS.UI.ValidationAttemptSuccess",
-                        !templates.GetList().empty());
+  secure_dns::UpdateValidationHistogram(!templates.GetList().empty());
   ResolveJavascriptCallback(*callback_id, templates);
 }
 
@@ -236,7 +207,7 @@ void SecureDnsHandler::HandleProbeCustomDnsTemplate(
   overrides.attempts = 1;
   overrides.randomize_ports = false;
   overrides.secure_dns_mode = net::DnsConfig::SecureDnsMode::SECURE;
-  chrome_browser_net::secure_dns::ApplyTemplate(&overrides, server_template);
+  secure_dns::ApplyTemplate(&overrides, server_template);
   DCHECK(!runner_);
   runner_ = std::make_unique<chrome_browser_net::DnsProbeRunner>(
       overrides, network_context_getter_);
@@ -251,29 +222,15 @@ void SecureDnsHandler::HandleRecordUserDropdownInteraction(
   std::string new_provider;
   CHECK(args->GetString(0, &old_provider));
   CHECK(args->GetString(1, &new_provider));
-  DCHECK(resolver_histogram_map_.find(old_provider) !=
-         resolver_histogram_map_.end());
-  DCHECK(resolver_histogram_map_.find(new_provider) !=
-         resolver_histogram_map_.end());
-  for (auto& pair : resolver_histogram_map_) {
-    if (pair.first == old_provider) {
-      UMA_HISTOGRAM_ENUMERATION("Net.DNS.UI.DropdownSelectionEvent.Unselected",
-                                pair.second);
-    } else if (pair.first == new_provider) {
-      UMA_HISTOGRAM_ENUMERATION("Net.DNS.UI.DropdownSelectionEvent.Selected",
-                                pair.second);
-    } else {
-      UMA_HISTOGRAM_ENUMERATION("Net.DNS.UI.DropdownSelectionEvent.Ignored",
-                                pair.second);
-    }
-  }
+
+  secure_dns::UpdateDropdownHistograms(providers_, old_provider, new_provider);
 }
 
 void SecureDnsHandler::OnProbeComplete() {
   bool success =
       runner_->result() == chrome_browser_net::DnsProbeRunner::CORRECT;
   runner_.reset();
-  UMA_HISTOGRAM_BOOLEAN("Net.DNS.UI.ProbeAttemptSuccess", success);
+  secure_dns::UpdateProbeHistogram(success);
   ResolveJavascriptCallback(base::Value(probe_callback_id_),
                             base::Value(success));
   probe_callback_id_.clear();
@@ -284,4 +241,12 @@ void SecureDnsHandler::SendSecureDnsSettingUpdatesToJavascript() {
                     *CreateSecureDnsSettingDict());
 }
 
+// static
+net::DohProviderEntry::List SecureDnsHandler::GetFilteredProviders() {
+  const auto local_providers = secure_dns::ProvidersForCountry(
+      net::DohProviderEntry::GetList(), country_codes::GetCurrentCountryID());
+  return secure_dns::RemoveDisabledProviders(
+      local_providers, secure_dns::GetDisabledProviders());
+}
+
 }  // namespace settings

chrome/browser/ui/webui/settings/settings_secure_dns_handler.h

@@ -9,12 +9,13 @@
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/macros.h"
 #include "base/values.h"
 #include "chrome/browser/net/dns_probe_runner.h"
 #include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h"
 #include "components/prefs/pref_change_registrar.h"
-#include "net/dns/public/doh_provider_list.h"
+#include "net/dns/public/doh_provider_entry.h"
 #include "services/network/public/cpp/resolve_host_client_base.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 
@@ -35,13 +36,15 @@ class SecureDnsHandler : public SettingsPageUIHandler {
   // as a dictionary with the following keys: "name" (the text to display in the
   // UI), "value" (the DoH template for this provider), and "policy" (the URL of
   // the provider's privacy policy).
-  base::Value GetSecureDnsResolverListForCountry(
-      int country_id,
-      const std::vector<net::DohProviderEntry>& providers);
+  base::Value GetSecureDnsResolverList();
 
   void SetNetworkContextForTesting(
       network::mojom::NetworkContext* network_context);
 
+  // DohProviderEntry cannot be copied, so the entries referenced in |providers|
+  // must be long-lived.
+  void SetProvidersForTesting(net::DohProviderEntry::List providers);
+
  protected:
   // Retrieves all pre-approved secure resolvers and returns them to WebUI.
   void HandleGetSecureDnsResolverList(const base::ListValue* args);
@@ -66,10 +69,14 @@ class SecureDnsHandler : public SettingsPageUIHandler {
   network::mojom::NetworkContext* GetNetworkContext();
   void OnProbeComplete();
 
-  std::map<std::string, net::DohProviderIdForHistogram> resolver_histogram_map_;
+  static net::DohProviderEntry::List GetFilteredProviders();
+
+  net::DohProviderEntry::List providers_ = GetFilteredProviders();
   std::unique_ptr<chrome_browser_net::DnsProbeRunner> runner_;
   chrome_browser_net::DnsProbeRunner::NetworkContextGetter
-      network_context_getter_;
+      network_context_getter_ =
+          base::BindRepeating(&SecureDnsHandler::GetNetworkContext,
+                              base::Unretained(this));
   // ID of the Javascript callback for the current pending probe, or "" if
   // there is no probe currently in progress.
   std::string probe_callback_id_;

chrome/test/data/webui/settings/secure_dns_interactive_test.js

@@ -62,7 +62,7 @@ suite('SettingsSecureDnsInteractive', function() {
 
   /** @type {!Array<!ResolverOption>} */
   const resolverList = [
-    {name: 'Custom', value: 'custom', policy: ''},
+    {name: 'Custom', value: '', policy: ''},
     {
       name: 'resolver1',
       value: 'resolver1_template',
@@ -192,7 +192,7 @@ suite('SettingsSecureDnsInteractive', function() {
   test('SecureDnsDropdownCustom', function() {
     webUIListenerCallback('secure-dns-setting-changed', {
       mode: SecureDnsMode.SECURE,
-      templates: ['custom'],
+      templates: [''],
       managementMode: SecureDnsUiManagementMode.NO_OVERRIDE,
     });
     flush();
@@ -202,7 +202,7 @@ suite('SettingsSecureDnsInteractive', function() {
         'none', getComputedStyle(testElement.$$('#privacyPolicy')).display);
     assertEquals(
         'block', getComputedStyle(testElement.$$('#secureDnsInput')).display);
-    assertEquals('custom', testElement.$$('#secureDnsInput').value);
+    assertEquals('', testElement.$$('#secureDnsInput').value);
   });
 
   test('SecureDnsDropdownChangeInSecureMode', async function() {
@@ -242,11 +242,11 @@ suite('SettingsSecureDnsInteractive', function() {
 
     // Change to custom
     testBrowserProxy.reset();
-    dropdownMenu.value = 'custom';
+    dropdownMenu.value = '';
     dropdownMenu.dispatchEvent(new Event('change'));
     args = await testBrowserProxy.whenCalled('recordUserDropdownInteraction');
     assertEquals(resolverList[2].value, args[0]);
-    assertEquals('custom', args[1]);
+    assertEquals('', args[1]);
     assertEquals(0, dropdownMenu.selectedIndex);
     assertEquals(
         'none', getComputedStyle(testElement.$$('#privacyPolicy')).display);
@@ -266,7 +266,7 @@ suite('SettingsSecureDnsInteractive', function() {
     dropdownMenu.value = resolverList[1].value;
     dropdownMenu.dispatchEvent(new Event('change'));
     args = await testBrowserProxy.whenCalled('recordUserDropdownInteraction');
-    assertEquals('custom', args[0]);
+    assertEquals('', args[0]);
     assertEquals(resolverList[1].value, args[1]);
     assertEquals(
         SecureDnsMode.SECURE, testElement.prefs.dns_over_https.mode.value);
@@ -274,11 +274,11 @@ suite('SettingsSecureDnsInteractive', function() {
         resolverList[1].value,
         testElement.prefs.dns_over_https.templates.value);
     testBrowserProxy.reset();
-    dropdownMenu.value = 'custom';
+    dropdownMenu.value = '';
     dropdownMenu.dispatchEvent(new Event('change'));
     args = await testBrowserProxy.whenCalled('recordUserDropdownInteraction');
     assertEquals(resolverList[1].value, args[0]);
-    assertEquals('custom', args[1]);
+    assertEquals('', args[1]);
     assertEquals('some_input', secureDnsInput.value);
   });
 

components/country_codes/country_codes.cc

@@ -112,16 +112,12 @@ int GeoIDToCountryID(GEOID geo_id) {
 
 const char kCountryIDAtInstall[] = "countryid_at_install";
 
-#if !defined(OS_WIN) && !defined(OS_MACOSX)
-
 int CountryStringToCountryID(const std::string& country) {
   return (country.length() == 2)
              ? CountryCharsToCountryIDWithUpdate(country[0], country[1])
              : kCountryIDUnknown;
 }
 
-#endif
-
 int GetCountryIDFromPrefs(PrefService* prefs) {
   if (!prefs)
     return GetCurrentCountryID();

net/dns/dns_util.cc

@@ -21,7 +21,7 @@
 #include "net/base/address_list.h"
 #include "net/base/url_util.h"
 #include "net/dns/public/dns_protocol.h"
-#include "net/dns/public/doh_provider_list.h"
+#include "net/dns/public/doh_provider_entry.h"
 #include "net/dns/public/util.h"
 #include "net/third_party/uri_template/uri_template.h"
 #include "url/url_canon.h"
@@ -113,21 +113,21 @@ bool DNSDomainFromDot(const base::StringPiece& dotted,
   return true;
 }
 
-std::vector<const DohProviderEntry*> GetDohProviderEntriesFromNameservers(
+DohProviderEntry::List GetDohProviderEntriesFromNameservers(
     const std::vector<IPEndPoint>& dns_servers,
     const std::vector<std::string>& excluded_providers) {
-  const std::vector<DohProviderEntry>& providers = GetDohProviderList();
-  std::vector<const DohProviderEntry*> entries;
+  const DohProviderEntry::List& providers = DohProviderEntry::GetList();
+  DohProviderEntry::List entries;
 
   for (const auto& server : dns_servers) {
-    for (const auto& entry : providers) {
-      if (base::Contains(excluded_providers, entry.provider))
+    for (const auto* entry : providers) {
+      if (base::Contains(excluded_providers, entry->provider))
         continue;
 
       // DoH servers should only be added once.
-      if (base::Contains(entry.ip_addresses, server.address()) &&
-          !base::Contains(entries, &entry)) {
-        entries.push_back(&entry);
+      if (base::Contains(entry->ip_addresses, server.address()) &&
+          !base::Contains(entries, entry)) {
+        entries.push_back(entry);
       }
     }
   }
@@ -311,23 +311,21 @@ DnsQueryType AddressFamilyToDnsQueryType(AddressFamily address_family) {
 std::vector<DnsOverHttpsServerConfig> GetDohUpgradeServersFromDotHostname(
     const std::string& dot_server,
     const std::vector<std::string>& excluded_providers) {
-  const std::vector<DohProviderEntry>& entries = GetDohProviderList();
   std::vector<DnsOverHttpsServerConfig> doh_servers;
 
   if (dot_server.empty())
     return doh_servers;
 
-  for (const auto& entry : entries) {
-    if (base::Contains(excluded_providers, entry.provider))
+  for (const auto* entry : DohProviderEntry::GetList()) {
+    if (base::Contains(excluded_providers, entry->provider))
       continue;
 
-    if (base::Contains(entry.dns_over_tls_hostnames, dot_server)) {
+    if (base::Contains(entry->dns_over_tls_hostnames, dot_server)) {
       std::string server_method;
-      CHECK(dns_util::IsValidDohTemplate(entry.dns_over_https_template,
+      CHECK(dns_util::IsValidDohTemplate(entry->dns_over_https_template,
                                          &server_method));
-      doh_servers.push_back(DnsOverHttpsServerConfig(
-          entry.dns_over_https_template, server_method == "POST"));
-      break;
+      doh_servers.emplace_back(entry->dns_over_https_template,
+                               server_method == "POST");
     }
   }
   return doh_servers;
@@ -336,38 +334,35 @@ std::vector<DnsOverHttpsServerConfig> GetDohUpgradeServersFromDotHostname(
 std::vector<DnsOverHttpsServerConfig> GetDohUpgradeServersFromNameservers(
     const std::vector<IPEndPoint>& dns_servers,
     const std::vector<std::string>& excluded_providers) {
-  std::vector<const DohProviderEntry*> entries =
+  const auto entries =
       GetDohProviderEntriesFromNameservers(dns_servers, excluded_providers);
   std::vector<DnsOverHttpsServerConfig> doh_servers;
-  std::string server_method;
-  for (const auto* entry : entries) {
-    CHECK(dns_util::IsValidDohTemplate(entry->dns_over_https_template,
-                                       &server_method));
-    doh_servers.push_back(DnsOverHttpsServerConfig(
-        entry->dns_over_https_template, server_method == "POST"));
-  }
+  doh_servers.reserve(entries.size());
+  std::transform(entries.begin(), entries.end(),
+                 std::back_inserter(doh_servers), [](const auto* entry) {
+                   std::string server_method;
+                   CHECK(dns_util::IsValidDohTemplate(
+                       entry->dns_over_https_template, &server_method));
+                   return DnsOverHttpsServerConfig(
+                       entry->dns_over_https_template, server_method == "POST");
+                 });
   return doh_servers;
 }
 
 std::string GetDohProviderIdForHistogramFromDohConfig(
     const DnsOverHttpsServerConfig& doh_server) {
-  const std::vector<DohProviderEntry>& entries = GetDohProviderList();
-  for (const auto& entry : entries) {
-    if (doh_server.server_template == entry.dns_over_https_template) {
-      return entry.provider;
-    }
-  }
-  return "Other";
+  const auto& entries = DohProviderEntry::GetList();
+  const auto it =
+      std::find_if(entries.begin(), entries.end(), [&](const auto* entry) {
+        return entry->dns_over_https_template == doh_server.server_template;
+      });
+  return it != entries.end() ? (*it)->provider : "Other";
 }
 
 std::string GetDohProviderIdForHistogramFromNameserver(
     const IPEndPoint& nameserver) {
-  std::vector<const DohProviderEntry*> entries =
-      GetDohProviderEntriesFromNameservers({nameserver}, {});
-  if (entries.size() == 0)
-    return "Other";
-  else
-    return entries[0]->provider;
+  const auto entries = GetDohProviderEntriesFromNameservers({nameserver}, {});
+  return entries.empty() ? "Other" : entries[0]->provider;
 }
 
 std::string SecureDnsModeToString(

net/dns/dns_util_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/stl_util.h"
 #include "net/dns/public/dns_protocol.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -183,20 +184,24 @@ TEST_F(DNSUtilTest, GetDohUpgradeServersFromNameservers) {
 
   doh_servers = GetDohUpgradeServersFromNameservers(nameservers,
                                                     std::vector<std::string>());
-  EXPECT_EQ(3u, doh_servers.size());
-  EXPECT_EQ("https://chrome.cloudflare-dns.com/dns-query",
-            doh_servers[0].server_template);
-  EXPECT_EQ("https://doh.cleanbrowsing.org/doh/family-filter{?dns}",
-            doh_servers[1].server_template);
-  EXPECT_EQ("https://doh.cleanbrowsing.org/doh/security-filter{?dns}",
-            doh_servers[2].server_template);
+  EXPECT_THAT(
+      doh_servers,
+      testing::ElementsAre(
+          DnsOverHttpsServerConfig(
+              "https://chrome.cloudflare-dns.com/dns-query", true),
+          DnsOverHttpsServerConfig(
+              "https://doh.cleanbrowsing.org/doh/family-filter{?dns}", false),
+          DnsOverHttpsServerConfig(
+              "https://doh.cleanbrowsing.org/doh/security-filter{?dns}",
+              false)));
 
   doh_servers = GetDohUpgradeServersFromNameservers(
       nameservers, std::vector<std::string>(
                        {"CleanBrowsingSecure", "Cloudflare", "Unexpected"}));
-  EXPECT_EQ(1u, doh_servers.size());
-  EXPECT_EQ("https://doh.cleanbrowsing.org/doh/family-filter{?dns}",
-            doh_servers[0].server_template);
+  EXPECT_THAT(
+      doh_servers,
+      testing::ElementsAre(DnsOverHttpsServerConfig(
+          "https://doh.cleanbrowsing.org/doh/family-filter{?dns}", false)));
 }
 
 TEST_F(DNSUtilTest, GetDohProviderIdForHistogramFromDohConfig) {

net/dns/public/BUILD.gn

@@ -19,8 +19,8 @@ source_set("public") {
     "dns_protocol.h",
     "dns_query_type.cc",
     "dns_query_type.h",
-    "doh_provider_list.cc",
-    "doh_provider_list.h",
+    "doh_provider_entry.cc",
+    "doh_provider_entry.h",
     "resolve_error_info.cc",
     "resolve_error_info.h",
     "util.cc",
@@ -35,7 +35,7 @@ source_set("public") {
 source_set("tests") {
   testonly = true
   sources = [
-    "doh_provider_list_unittest.cc",
+    "doh_provider_entry_unittest.cc",
     "util_unittest.cc",
   ]
 

net/dns/public/doh_provider_list.h → net/dns/public/doh_provider_entry.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef NET_DNS_PUBLIC_DOH_PROVIDER_LIST_H_
-#define NET_DNS_PUBLIC_DOH_PROVIDER_LIST_H_
+#ifndef NET_DNS_PUBLIC_DOH_PROVIDER_ENTRY_H_
+#define NET_DNS_PUBLIC_DOH_PROVIDER_ENTRY_H_
 
 #include <set>
 #include <string>
@@ -41,37 +41,56 @@ enum class DohProviderIdForHistogram {
 // codes, if any, where the entry is eligible for being displayed in the
 // dropdown menu.
 struct NET_EXPORT DohProviderEntry {
-  DohProviderEntry(
+ public:
+  using List = std::vector<const DohProviderEntry*>;
+
+  std::string provider;
+  // A provider_id_for_histogram is required for entries that are intended to
+  // be visible in the UI.
+  base::Optional<DohProviderIdForHistogram> provider_id_for_histogram;
+  std::set<IPAddress> ip_addresses;
+  std::set<std::string> dns_over_tls_hostnames;
+  std::string dns_over_https_template;
+  std::string ui_name;
+  std::string privacy_policy;
+  bool display_globally;
+  std::set<std::string> display_countries;
+
+  // Returns the full list of DoH providers. A subset of this list may be used
+  // to support upgrade in automatic mode or to populate the dropdown menu for
+  // secure mode.
+  static const List& GetList();
+
+  static DohProviderEntry ConstructForTesting(
       std::string provider,
       base::Optional<DohProviderIdForHistogram> provider_id_for_histogram,
-      std::set<std::string> ip_strs,
+      std::set<base::StringPiece> ip_strs,
       std::set<std::string> dns_over_tls_hostnames,
       std::string dns_over_https_template,
       std::string ui_name,
       std::string privacy_policy,
       bool display_globally,
       std::set<std::string> display_countries);
-  DohProviderEntry(const DohProviderEntry& other);
+
+  // Entries are move-only.  This allows tests to construct a List but ensures
+  // that |const DohProviderEntry*| is a safe type for application code.
+  DohProviderEntry(DohProviderEntry&& other);
+  DohProviderEntry& operator=(DohProviderEntry&& other);
   ~DohProviderEntry();
 
-  const std::string provider;
-  // A provider_id_for_histogram is required for entries that are intended to
-  // be visible in the UI.
-  const base::Optional<DohProviderIdForHistogram> provider_id_for_histogram;
-  std::set<IPAddress> ip_addresses;
-  const std::set<std::string> dns_over_tls_hostnames;
-  const std::string dns_over_https_template;
-  const std::string ui_name;
-  const std::string privacy_policy;
-  bool display_globally;
-  std::set<std::string> display_countries;
+ private:
+  DohProviderEntry(
+      std::string provider,
+      base::Optional<DohProviderIdForHistogram> provider_id_for_histogram,
+      std::set<base::StringPiece> ip_strs,
+      std::set<std::string> dns_over_tls_hostnames,
+      std::string dns_over_https_template,
+      std::string ui_name,
+      std::string privacy_policy,
+      bool display_globally,
+      std::set<std::string> display_countries);
 };
 
-// Returns the full list of DoH providers. A subset of this list may be used
-// to support upgrade in automatic mode or to populate the dropdown menu for
-// secure mode.
-NET_EXPORT const std::vector<DohProviderEntry>& GetDohProviderList();
-
 }  // namespace net
 
-#endif  // NET_DNS_PUBLIC_DOH_PROVIDER_LIST_H_
+#endif  // NET_DNS_PUBLIC_DOH_PROVIDER_ENTRY_H_

net/dns/public/doh_provider_list_unittest.cc → net/dns/public/doh_provider_entry_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "net/dns/public/doh_provider_list.h"
+#include "net/dns/public/doh_provider_entry.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -10,7 +10,7 @@ namespace net {
 namespace {
 
 TEST(DohProviderListTest, GetDohProviderList) {
-  const std::vector<DohProviderEntry>& list = GetDohProviderList();
+  const DohProviderEntry::List& list = DohProviderEntry::GetList();
   EXPECT_FALSE(list.empty());
 }