[ImportantStorage] Blacklisted reason metrics

chrome/browser/android/preferences/important_sites_util.cc

Index: chrome/browser/android/preferences/important_sites_util.cc
diff --git a/chrome/browser/android/preferences/important_sites_util.cc b/chrome/browser/android/preferences/important_sites_util.cc
index 55aaf2cc9094679122c888aef400e695aefcebcb..2a9589ba492781a84dbe2036d96c7d2d42f11a7b 100644
--- a/chrome/browser/android/preferences/important_sites_util.cc
+++ b/chrome/browser/android/preferences/important_sites_util.cc
@@ -8,6 +8,7 @@
 #include <map>
 #include <set>
 
+#include "base/metrics/histogram_macros.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/engagement/site_engagement_score.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
@@ -18,6 +19,23 @@
 
 namespace {
 
+enum ReasonStatTypes {
+  DURABLE = 0,
+  NOTIFICATIONS,
+  ENGAGEMENT,
+  NOTIFICATIONS_AND_ENGAGEMENT,
+  DURABLE_AND_ENGAGEMENT,
+  NOTIFICATIONS_AND_DURABLE,
+  NOTIFICATIONS_AND_DURABLE_AND_ENGAGEMENT,
+  REASON_BOUNDARY
+};
+
+struct ImportantReason {
+  bool engagement = false;
+  bool notifications = false;
+  bool durable = false;
+};
+
 std::vector<std::pair<GURL, double>> GetSortedTopEngagementOrigins(
     const SiteEngagementService* site_engagement_service,
     const std::map<GURL, double>& engagement_map,
@@ -133,6 +151,99 @@ std::vector<std::string> ImportantSitesUtil::GetImportantRegisterableDomains(
   return final_list;
 }
 
+void ImportantSitesUtil::RecordMetricsForBlacklistedSites(
+    Profile* profile, std::vector<std::string> blacklisted_sites) {
+  SiteEngagementService* site_engagement_service =
+        SiteEngagementService::Get(profile);
+
+  std::map<std::string, ImportantReason> reason_map;
+
+  std::map<GURL, double> engagement_map =
+      site_engagement_service->GetScoreMap();
+
+  // Site engagement.
+  for (const auto& url_score_pair : engagement_map) {
+    if (url_score_pair.second <
+        SiteEngagementScore::GetMediumEngagementBoundary()) {
+      continue;
+    }
+    const std::string& host = url_score_pair.first.host();
+    for (const std::string& blacklisted_site : blacklisted_sites) {
+      if (host.find(blacklisted_site) != host.end()) {
+        reason_map[blacklisted_site].engagement |= true;
+        break;
+      }
+    }
+  }
+
+  // Durable.
+  ContentSettingsForOneType content_settings_list;
+  HostContentSettingsMapFactory::GetForProfile(profile)->GetSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_DURABLE_STORAGE,
+      content_settings::ResourceIdentifier(), &content_settings_list);
+  for (const ContentSettingPatternSource& site : content_settings_list) {
+    if (site.setting != CONTENT_SETTING_ALLOW)
+      continue;
+    GURL origin(site.primary_pattern.ToString());
+    if (!origin.is_valid())
+      continue;
+    const std::string& host = origin.host();
+    for (const std::string& blacklisted_site : blacklisted_sites) {
+      if (host.find(blacklisted_site) != host.end()) {
+        reason_map[blacklisted_site].durable |= true;
+        break;
+      }
+    }
+  }
+
+  // Notifications.
+  ContentSettingsForOneType content_settings_list;
+  HostContentSettingsMapFactory::GetForProfile(profile)->GetSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      content_settings::ResourceIdentifier(), &content_settings_list);
+  for (const ContentSettingPatternSource& site : content_settings_list) {
+    if (site.setting != CONTENT_SETTING_ALLOW)
+      continue;
+    GURL origin(site.primary_pattern.ToString());
+    if (!origin.is_valid())
+      continue;
+    const std::string& host = origin.host();
+    for (const std::string& blacklisted_site : blacklisted_sites) {

[Ted C, 2016/06/29 16:14:54]

would it not be possible to just gather the content settings for the particular
blacklisted sites?  Then loop over the black listed sites instead of the content
setting for all sites?

I would expect the blacklisted list to be quite small and the content settings
"might" not be.

[dmurph, 2016/06/29 19:20:59]

The main issue here is that we don't have GURLs to give it. We just have
registerable domains, which is eTLD+1. We can't really look up content settings
on that large of a granularity. 

[Ted C, 2016/06/29 19:44:19]

Hmm...that is unfortunate.  We actually have all of this information when we
build the list of important sites right?  I wonder if we should be pushing all
this information up from that point so we wouldn't need to do this
recalculation.

Does that seem feasible?  If not, I can approve this change but it just
seems...unfortunate.

[dmurph, 2016/06/29 21:59:26]

Unfortunately this seems really complicated, we would have to somehow hold onto
a handle in java to a c++ object... or maybe an integer that's used as a
bitmask? It seems fragile and complicated, so even though this is terrible (and
I know it), it's the least-bad in my mind.

On the bright side, if we go forward with important sites being a thing, we can
rework how we record stats by saving these things instead of generating them on
the fly.

[Ted C, 2016/06/29 23:53:48]

I think the bitmask would be the most reasonable approach.  I wouldn't
necessarily hold onto a C++ object from java, but you'd end up passing a list of
strings and a corresponding list of ints up to java in your call above.  But I'm
fine punting on that for now as I don't think it is as complicated as you think,
it is still complicated and a bunch of plumbing.

+      if (host.find(blacklisted_site) != host.end()) {
+        reason_map[blacklisted_site].notifications |= true;
+        break;
+      }
+    }
+  }
+
+  for (const auto& reason_pair : reason_map) {
+    const ImportantReason& reason = reason_pair.second;
+    if (reason.notifications && reason.durable && reason.engagement) {
+      UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason",

[Ted C, 2016/06/29 16:14:54]

i'm slightly worried that this won't scale as we add different options.  We
don't want to have an explosion of combinations for each new addition.

[dmurph, 2016/06/29 19:20:59]

Initially, we want all of this data to determine which signal was better. We're
not going to want a huge cross of everything if we add more sources, this is
purely for this initial dialog experiment. I can add a comment about that if
that helps.

[Ted C, 2016/06/29 19:44:19]

Ack...no comment needed

+                                NOTIFICATIONS_AND_DURABLE_AND_ENGAGEMENT,
+                                REASON_BOUNDARY);
+    } else if (reason.notifications && reason.durable) {
+      UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason",
+                                NOTIFICATIONS_AND_DURABLE, REASON_BOUNDARY);
+    } else if (reason.notifications && reason.engagement) {
+      UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason",
+                                NOTIFICATIONS_AND_ENGAGEMENT, REASON_BOUNDARY);
+    } else if (reason.durable && reason.engagement) {
+      UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason",
+                                DURABLE_AND_ENGAGEMENT, REASON_BOUNDARY);
+    } else if (reason.notifications) {
+      UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason",
+                                NOTIFICATIONS, REASON_BOUNDARY);
+    } else if (reason.durable) {
+      UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason",
+                                DURABLE, REASON_BOUNDARY);
+    } else if (reason.engagement) {
+      UMA_HISTOGRAM_ENUMERATION("Storage.BlacklistedImportantSites.Reason",
+                                ENGAGEMENT, REASON_BOUNDARY);
+    }
+  }
+}
+
 void ImportantSitesUtil::MarkOriginAsImportantForTesting(Profile* profile,
                                                          const GURL& origin) {
   // First get data from site engagement.