Change logic for recording redirect pattern histograms.

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc

Index: components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
diff --git a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
index b9c7d1ca9b46b691aed64c8d89a51f06f1b137f2..0c88440349f56f1ae95f9736eb2f30ba7c339a63 100644
--- a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
+++ b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
@@ -41,6 +41,17 @@ bool ShouldMeasurePerformanceForPageLoad() {
   return rate == 1 || (rate > 0 && base::RandDouble() < rate);
 }
 
+// Macro for UMA reporting of histograms which track data to understand

[engedy, 2017/03/14 10:10:20]

Phrasing nit: 

// Records histograms about the length of redirect chains, and about the pattern
of whether each URL in the chain matched the activation list.

[melandory, 2017/03/14 15:51:55]

Done.

+// different redirect patterns.
+#define REPORT_REDIRECT_PATTERN_FOR_SUFFIX(suffix, hits_pattern, chain_size)   \
+  do {                                                                         \
+    UMA_HISTOGRAM_ENUMERATION(                                                 \
+        "SubresourceFilter.PageLoad.RedirectChainMatchPattern." suffix,        \
+        hits_pattern, 0x10);                                                   \
+    UMA_HISTOGRAM_COUNTS(                                                      \
+        "SubresourceFilter.PageLoad.RedirectChainLength." suffix, chain_size); \
+  } while (0)
+
 }  // namespace
 
 // static
@@ -120,18 +131,8 @@ void ContentSubresourceFilterDriverFactory::
         const std::vector<GURL>& redirect_urls,
         safe_browsing::SBThreatType threat_type,
         safe_browsing::ThreatPatternType threat_type_metadata) {
-  bool is_phishing_interstitial =
-      (threat_type == safe_browsing::SB_THREAT_TYPE_URL_PHISHING);
-  bool is_soc_engineering_ads_interstitial =
-      threat_type_metadata ==
-      safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS;
-
-  if (is_phishing_interstitial) {
-    if (is_soc_engineering_ads_interstitial) {
-      AddActivationListMatch(url, ActivationList::SOCIAL_ENG_ADS_INTERSTITIAL);
-    }
-    AddActivationListMatch(url, ActivationList::PHISHING_INTERSTITIAL);

[engedy, 2017/03/14 10:10:20]

When |is_phishing_interstitial| and |is_soc_engineering_ads_interstitial| were
both `true`, we used to activate when the current activation list was either
SOCIAL_ENG_ADS_INTERSTITIAL or PHISHING_INTERSTITIAL. Not sure what would be the
best way to preserve this behavior with the new helper function.

[melandory, 2017/03/14 15:51:55]

I think it's better to make the activation calculation logic to be  aware that
soc.eng. falls to phishing. It's not very clean, but it seems that we have a bit
different problem, in a future we need to support the activation on several
lists.

-  }
+  AddActivationListMatch(
+      url, GetListForThreatTypeAndMetadata(threat_type, threat_type_metadata));
 }
 
 void ContentSubresourceFilterDriverFactory::AddHostOfURLToWhitelistSet(
@@ -163,8 +164,8 @@ ContentSubresourceFilterDriverFactory::ComputeActivationDecisionForMainFrameURL(
       // AddActivationListMatch to ensure the activation list only has relevant
       // entries.
       DCHECK(url.SchemeIsHTTPOrHTTPS() ||
-             !DidURLMatchCurrentActivationList(url));
-      return DidURLMatchCurrentActivationList(url)
+             !DidURLMatchActivationList(url, GetCurrentActivationList()));
+      return DidURLMatchActivationList(url, GetCurrentActivationList())
                  ? ActivationDecision::ACTIVATED
                  : ActivationDecision::ACTIVATION_LIST_NOT_MATCHED;
     default:
@@ -321,54 +322,78 @@ void ContentSubresourceFilterDriverFactory::ReadyToCommitNavigationInternal(
   ActivateForFrameHostIfNeeded(render_frame_host, url);
 }
 
-bool ContentSubresourceFilterDriverFactory::DidURLMatchCurrentActivationList(
-    const GURL& url) const {
+bool ContentSubresourceFilterDriverFactory::DidURLMatchActivationList(
+    const GURL& url,
+    ActivationList activation_list) const {
   auto match_types =
       activation_list_matches_.find(DistillURLToHostAndPath(url));
   return match_types != activation_list_matches_.end() &&
-         match_types->second.find(GetCurrentActivationList()) !=
-             match_types->second.end();
+         match_types->second.find(activation_list) != match_types->second.end();
 }
 
 void ContentSubresourceFilterDriverFactory::AddActivationListMatch(
     const GURL& url,
     ActivationList match_type) {
+  if (match_type == ActivationList::NONE)
+    return;
   if (url.has_host() && url.SchemeIsHTTPOrHTTPS())
     activation_list_matches_[DistillURLToHostAndPath(url)].insert(match_type);
 }
 
-void ContentSubresourceFilterDriverFactory::RecordRedirectChainMatchPattern()
-    const {
+int ContentSubresourceFilterDriverFactory::CalculateHitPatternForActivationList(
+    ActivationList activation_list) const {
   int hits_pattern = 0;
   const int kInitialURLHitMask = 0x4;
   const int kRedirectURLHitMask = 0x2;
   const int kFinalURLHitMask = 0x1;
   if (navigation_chain_.size() > 1) {
-    if (DidURLMatchCurrentActivationList(navigation_chain_.back()))
+    if (DidURLMatchActivationList(navigation_chain_.back(), activation_list))
       hits_pattern |= kFinalURLHitMask;
-    if (DidURLMatchCurrentActivationList(navigation_chain_.front()))
+    if (DidURLMatchActivationList(navigation_chain_.front(), activation_list))
       hits_pattern |= kInitialURLHitMask;
 
     // Examine redirects.
     for (size_t i = 1; i < navigation_chain_.size() - 1; ++i) {
-      if (DidURLMatchCurrentActivationList(navigation_chain_[i])) {
+      if (DidURLMatchActivationList(navigation_chain_[i], activation_list)) {
         hits_pattern |= kRedirectURLHitMask;
         break;
       }
     }
   } else {
     if (navigation_chain_.size() &&
-        DidURLMatchCurrentActivationList(navigation_chain_.front())) {
+        DidURLMatchActivationList(navigation_chain_.front(), activation_list)) {
       hits_pattern = 0x8;  // One url hit.
     }
   }
+  return hits_pattern;
+}
+
+void ContentSubresourceFilterDriverFactory::RecordRedirectChainMatchPattern()
+    const {
+  RecordRedirectChainMatchPatternForList(
+      ActivationList::SOCIAL_ENG_ADS_INTERSTITIAL);
+  RecordRedirectChainMatchPatternForList(ActivationList::PHISHING_INTERSTITIAL);
+}
+
+void ContentSubresourceFilterDriverFactory::
+    RecordRedirectChainMatchPatternForList(
+        ActivationList activation_list) const {
+  int hits_pattern = CalculateHitPatternForActivationList(activation_list);
   if (!hits_pattern)
     return;
-  UMA_HISTOGRAM_ENUMERATION(
-      "SubresourceFilter.PageLoad.RedirectChainMatchPattern", hits_pattern,
-      0x10 /* max value */);
-  UMA_HISTOGRAM_COUNTS("SubresourceFilter.PageLoad.RedirectChainLength",
-                       navigation_chain_.size());
+  size_t chain_size = navigation_chain_.size();
+  switch (activation_list) {
+    case ActivationList::SOCIAL_ENG_ADS_INTERSTITIAL:
+      REPORT_REDIRECT_PATTERN_FOR_SUFFIX("SocialEngineeringAdsInterstitial",
+                                         hits_pattern, chain_size);
+      break;
+    case ActivationList::PHISHING_INTERSTITIAL:
+      REPORT_REDIRECT_PATTERN_FOR_SUFFIX("PhishingInterstital", hits_pattern,
+                                         chain_size);
+      break;
+    default:

[engedy, 2017/03/14 10:10:21]

nit: NOTREACHED();

[melandory, 2017/03/14 15:51:55]

Done.

+      break;
+  }
 }
 
 }  // namespace subresource_filter