Change logic for recording redirect pattern histograms.

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
 #include "base/time/time.h"
 #include "components/subresource_filter/content/browser/subresource_filter_client.h"
@@ skipping 23 matching lines @@
 // ThreadTicks is supported, otherwise returns false.
 bool ShouldMeasurePerformanceForPageLoad() {
   if (!base::ThreadTicks::IsSupported())
     return false;
   // TODO(pkalinnikov): Cache |rate| and other variation params in
   // ContentSubresourceFilterDriverFactory.
   const double rate = GetPerformanceMeasurementRate();
   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
 void ContentSubresourceFilterDriverFactory::CreateForWebContents(
     content::WebContents* web_contents,
     std::unique_ptr<SubresourceFilterClient> client) {
   if (FromWebContents(web_contents))
     return;
   web_contents->SetUserData(kWebContentsUserDataKey,
                             new ContentSubresourceFilterDriverFactory(
@@ skipping 59 matching lines @@
     const GURL& url) const {
   return whitelisted_hosts_.find(url.host()) != whitelisted_hosts_.end();
 }
 
 void ContentSubresourceFilterDriverFactory::
     OnMainResourceMatchedSafeBrowsingBlacklist(
         const GURL& url,
         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(
     const GURL& url) {
   if (url.has_host() && url.SchemeIsHTTPOrHTTPS())
     whitelisted_hosts_.insert(url.host());
 }
 
 ContentSubresourceFilterDriverFactory::ActivationDecision
 ContentSubresourceFilterDriverFactory::ComputeActivationDecisionForMainFrameURL(
@@ skipping 11 matching lines @@
     return ActivationDecision::URL_WHITELISTED;
 
   switch (scope) {
     case ActivationScope::ALL_SITES:
       return ActivationDecision::ACTIVATED;
     case ActivationScope::ACTIVATION_LIST:
       // The logic to ensure only http/https URLs are activated lives in
       // 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:
       return ActivationDecision::ACTIVATION_DISABLED;
   }
 }
 
 void ContentSubresourceFilterDriverFactory::ActivateForFrameHostIfNeeded(
     content::RenderFrameHost* render_frame_host,
     const GURL& url) {
@@ skipping 136 matching lines @@
     ResetActivationState();
     return;
   }
 
   activation_level_ = GetMaximumActivationLevel();
   measure_performance_ = activation_level_ != ActivationLevel::DISABLED &&
                          ShouldMeasurePerformanceForPageLoad();
   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