Content settings breakouts

This CL adds new breakouts to the existing ContentSettings.Exceptions
histograms, for cookies, popups, and subresource-filter settings.

Bug: None
Change-Id: I2967cd82ba9dd23b73a466f40b980f303ed48373
Reviewed-on: https://chromium-review.googlesource.com/c/1277548Reviewed-by: Robert Kaplow (sloooow) <rkaplow@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601571}

components/content_settings/core/browser/content_settings_pref.cc

@@ -247,9 +247,6 @@ void ContentSettingsPref::ReadContentSettingsFromPref() {
     settings = all_settings_dictionary;
   }
 
-  size_t cookies_block_exception_count = 0;
-  size_t cookies_allow_exception_count = 0;
-  size_t cookies_session_only_exception_count = 0;
   for (base::DictionaryValue::Iterator i(*settings); !i.IsAtEnd();
        i.Advance()) {
     const std::string& pattern_str(i.key());
@@ -299,35 +296,7 @@ void ContentSettingsPref::ReadContentSettingsFromPref() {
       value_map_.SetValue(pattern_pair.first, pattern_pair.second,
                           content_type_, ResourceIdentifier(), last_modified,
                           value->DeepCopy());
-      if (content_type_ == CONTENT_SETTINGS_TYPE_COOKIES) {
-        ContentSetting s = ValueToContentSetting(value);
-        switch (s) {
-          case CONTENT_SETTING_ALLOW :
-            ++cookies_allow_exception_count;
-            break;
-          case CONTENT_SETTING_BLOCK :
-            ++cookies_block_exception_count;
-            break;
-          case CONTENT_SETTING_SESSION_ONLY :
-            ++cookies_session_only_exception_count;
-            break;
-          default:
-            NOTREACHED();
-            break;
-        }
-      }
     }
-
-  }
-
-  if (content_type_ == CONTENT_SETTINGS_TYPE_COOKIES) {
-    UMA_HISTOGRAM_COUNTS_1M("ContentSettings.NumberOfBlockCookiesExceptions",
-                            cookies_block_exception_count);
-    UMA_HISTOGRAM_COUNTS_1M("ContentSettings.NumberOfAllowCookiesExceptions",
-                            cookies_allow_exception_count);
-    UMA_HISTOGRAM_COUNTS_1M(
-        "ContentSettings.NumberOfSessionOnlyCookiesExceptions",
-        cookies_session_only_exception_count);
   }
 }
 

components/content_settings/core/browser/host_content_settings_map.cc

@@ -11,8 +11,10 @@
 #include <utility>
 
 #include "base/command_line.h"
+#include "base/containers/flat_map.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -195,6 +197,43 @@ enum class FlashPermissions {
   kMaxValue = kRepeated,
 };
 
+// Returns whether per-content setting exception information should be
+// collected. All content settings for which this method returns true here be
+// content settings, not website settings (i.e. their value should be a
+// ContentSetting).
+//
+// This method should be kept in sync with histograms.xml, as every type here
+// is an affected histogram under the "ContentSetting" suffix.
+bool ShouldCollectFineGrainedExceptionHistograms(ContentSettingsType type) {
+  switch (type) {
+    case CONTENT_SETTINGS_TYPE_COOKIES:
+    case CONTENT_SETTINGS_TYPE_POPUPS:
+    case CONTENT_SETTINGS_TYPE_ADS:
+      return true;
+    default:
+      return false;
+  }
+}
+
+const char* ContentSettingToString(ContentSetting setting) {
+  switch (setting) {
+    case CONTENT_SETTING_ALLOW:
+      return "Allow";
+    case CONTENT_SETTING_BLOCK:
+      return "Block";
+    case CONTENT_SETTING_ASK:
+      return "Ask";
+    case CONTENT_SETTING_SESSION_ONLY:
+      return "SessionOnly";
+    case CONTENT_SETTING_DETECT_IMPORTANT_CONTENT:
+      return "DetectImportantContent";
+    case CONTENT_SETTING_DEFAULT:
+    case CONTENT_SETTING_NUM_SETTINGS:
+      NOTREACHED();
+      return nullptr;
+  }
+}
+
 }  // namespace
 
 HostContentSettingsMap::HostContentSettingsMap(
@@ -586,6 +625,8 @@ void HostContentSettingsMap::SetClockForTesting(base::Clock* clock) {
 }
 
 void HostContentSettingsMap::RecordExceptionMetrics() {
+  auto* content_setting_registry =
+      content_settings::ContentSettingsRegistry::GetInstance();
   for (const content_settings::WebsiteSettingsInfo* info :
        *content_settings::WebsiteSettingsRegistry::GetInstance()) {
     ContentSettingsType content_type = info->type();
@@ -594,6 +635,9 @@ void HostContentSettingsMap::RecordExceptionMetrics() {
     ContentSettingsForOneType settings;
     GetSettingsForOneType(content_type, std::string(), &settings);
     size_t num_exceptions = 0;
+    base::flat_map<ContentSetting, size_t> num_exceptions_with_setting;
+    const content_settings::ContentSettingsInfo* content_info =
+        content_setting_registry->Get(content_type);
     for (const ContentSettingPatternSource& setting_entry : settings) {
       // Skip default settings.
       if (setting_entry.primary_pattern == ContentSettingsPattern::Wildcard() &&
@@ -624,17 +668,34 @@ void HostContentSettingsMap::RecordExceptionMetrics() {
         }
       }
 
-      if (setting_entry.source == "preference")
+      if (setting_entry.source == "preference") {
+        // |content_info| will be non-nullptr iff |content_type| is a content
+        // setting rather than a website setting.
+        if (content_info)
+          ++num_exceptions_with_setting[setting_entry.GetContentSetting()];
         ++num_exceptions;
+      }
     }
 
     std::string histogram_name =
         "ContentSettings.Exceptions." + type_name;
-
-    base::HistogramBase* histogram_pointer = base::Histogram::FactoryGet(
-        histogram_name, 1, 1000, 30,
-        base::HistogramBase::kUmaTargetedHistogramFlag);
-    histogram_pointer->Add(num_exceptions);
+    base::UmaHistogramCustomCounts(histogram_name, num_exceptions, 1, 1000, 30);
+
+    // For some ContentSettingTypes, collect exception histograms broken out by
+    // ContentSetting.
+    if (ShouldCollectFineGrainedExceptionHistograms(content_type)) {
+      DCHECK(content_info);
+      for (int setting = 0; setting < CONTENT_SETTING_NUM_SETTINGS; ++setting) {
+        ContentSetting content_setting = IntToContentSetting(setting);
+        if (!content_info->IsSettingValid(content_setting))
+          continue;
+        std::string histogram_with_suffix =
+            histogram_name + "." + ContentSettingToString(content_setting);
+        base::UmaHistogramCustomCounts(
+            histogram_with_suffix, num_exceptions_with_setting[content_setting],
+            1, 1000, 30);
+      }
+    }
   }
 }
 

tools/metrics/histograms/histograms.xml

@@ -14917,6 +14917,9 @@ uploading your change for review.
 </histogram>
 
 <histogram name="ContentSettings.NumberOfAllowCookiesExceptions">
+  <obsolete>
+    Deprecated Oct 2018 in favor of ContentSettings.Exceptions.cookies.Allow
+  </obsolete>
   <owner>battre@chromium.org</owner>
   <summary>
     The number of user defined cookies setting exceptions for allowing cookies
@@ -14925,6 +14928,9 @@ uploading your change for review.
 </histogram>
 
 <histogram name="ContentSettings.NumberOfBlockCookiesExceptions">
+  <obsolete>
+    Deprecated Oct 2018 in favor of ContentSettings.Exceptions.cookies.Block
+  </obsolete>
   <owner>battre@chromium.org</owner>
   <summary>
     The number of user defined cookies setting exceptions for blocking cookies
@@ -14941,6 +14947,10 @@ uploading your change for review.
 </histogram>
 
 <histogram name="ContentSettings.NumberOfSessionOnlyCookiesExceptions">
+  <obsolete>
+    Deprecated Oct 2018 in favor of
+    ContentSettings.Exceptions.cookies.SessionOnly
+  </obsolete>
   <owner>battre@chromium.org</owner>
   <summary>
     The number of user defined cookies setting exceptions for 'clearing cookies
@@ -124152,6 +124162,17 @@ uploading your change for review.
   <affected-histogram name="PLT.LoadType"/>
 </histogram_suffixes>
 
+<histogram_suffixes name="ContentSetting" separator=".">
+  <suffix name="Allow" label="Allow"/>
+  <suffix name="Ask" label="Ask"/>
+  <suffix name="Block" label="Block"/>
+  <suffix name="DetectImportantContent" label="Detect important content"/>
+  <suffix name="SessionOnly" label="Session only"/>
+  <affected-histogram name="ContentSettings.Exceptions.cookies"/>
+  <affected-histogram name="ContentSettings.Exceptions.popups"/>
+  <affected-histogram name="ContentSettings.Exceptions.subresource-filter"/>
+</histogram_suffixes>
+
 <histogram_suffixes name="ContentSettingsType" separator=".">
   <suffix name="app-banner" label="App banner exceptions"/>
   <suffix name="auto-select-certificate" label="Auto-select cert exceptions"/>