Change logic for recording redirect pattern histograms.

components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.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/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
 #include "base/test/histogram_tester.h"
 #include "components/safe_browsing_db/util.h"
 #include "components/subresource_filter/content/browser/subresource_filter_client.h"
 #include "components/subresource_filter/content/common/subresource_filter_messages.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features_test_support.h"
+#include "components/subresource_filter/core/common/activation_list.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/public/test/web_contents_tester.h"
 #include "net/base/net_errors.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ skipping 28 matching lines @@
   F0M1L1,            // B or C, or both and D are Safe Browsing matches.
   F1M0L0,            // A is Safe Browsing match
   F1M0L1,            // A and D are Safe Browsing matches.
   F1M1L0,            // B and/or C and A are Safe Browsing matches.
   F1M1L1,            // B and/or C and A and D are Safe Browsing matches.
   NO_REDIRECTS_HIT,  // Redirect chain consists of single URL, aka no redirects
                      // has happened, and this URL was a Safe Browsing hit.
   NUM_HIT_PATTERNS,
 };
 
+std::string GetSuffixForList(const ActivationList& type) {
+  switch (type) {
+    case ActivationList::SOCIAL_ENG_ADS_INTERSTITIAL:
+      return ".SocialEngineeringAdsInterstitial";
+    case ActivationList::PHISHING_INTERSTITIAL:
+      return ".PhishingInterstital";
+    case ActivationList::NONE:
+      return std::string();
+  }
+  return std::string();
+}
+
 struct ActivationListTestData {
   ActivationDecision expected_activation_decision;
   const char* const activation_list;
   safe_browsing::SBThreatType threat_type;
   safe_browsing::ThreatPatternType threat_type_metadata;
 };
 
 const ActivationListTestData kActivationListTestData[] = {
     {ActivationDecision::ACTIVATION_LIST_NOT_MATCHED, "",
      safe_browsing::SB_THREAT_TYPE_URL_PHISHING,
@@ skipping 42 matching lines @@
      subresource_filter::kActivationListPhishingInterstitial,
      safe_browsing::SB_THREAT_TYPE_SAFE,
      safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS},
     {ActivationDecision::ACTIVATED,
      subresource_filter::kActivationListPhishingInterstitial,
      safe_browsing::SB_THREAT_TYPE_URL_PHISHING,
      safe_browsing::ThreatPatternType::NONE},
     {ActivationDecision::ACTIVATED,
      subresource_filter::kActivationListSocialEngineeringAdsInterstitial,
      safe_browsing::SB_THREAT_TYPE_URL_PHISHING,
      safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS},

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

Could you please add:

    {ActivationDecision::ACTIVATED,
     subresource_filter::kActivationListPhishingInterstitial,
     safe_browsing::SB_THREAT_TYPE_URL_PHISHING,
     safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS},

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

Done.

 };
 
 struct ActivationScopeTestData {
   ActivationDecision expected_activation_decision;
   bool url_matches_activation_list;
   const char* const activation_scope;
 };
 
 const ActivationScopeTestData kActivationScopeTestData[] = {
     {ActivationDecision::ACTIVATED, false /* url_matches_activation_list */,
@@ skipping 124 matching lines @@
 
     navigation_simulator->Commit();
     ExpectActivationSignalForFrame(main_rfh(), expected_activation);
     EXPECT_EQ(expected_activation_decision,
               factory()->GetActivationDecisionForLastCommittedPageLoad());
 
     // Re-create a subframe now that the frame has navigated.
     content::RenderFrameHostTester* rfh_tester =
         content::RenderFrameHostTester::For(main_rfh());
     rfh_tester->AppendChild(kSubframeName);
+    ActivationList activation_list =
+        GetListForThreatTypeAndMetadata(threat_type, threat_type_metadata);
 
+    const std::string suffix(GetSuffixForList(activation_list));
     if (expected_pattern != EMPTY) {
-      EXPECT_THAT(tester.GetAllSamples(kMatchesPatternHistogramName),
+      EXPECT_THAT(tester.GetAllSamples(kMatchesPatternHistogramName + suffix),
                   ::testing::ElementsAre(base::Bucket(expected_pattern, 1)));
       EXPECT_THAT(
-          tester.GetAllSamples(kNavigationChainSize),
+          tester.GetAllSamples(kNavigationChainSize + suffix),
           ::testing::ElementsAre(base::Bucket(navigation_chain.size(), 1)));
-
     } else {
-      EXPECT_THAT(tester.GetAllSamples(kMatchesPatternHistogramName),
+      EXPECT_THAT(tester.GetAllSamples(kMatchesPatternHistogramName + suffix),
                   ::testing::IsEmpty());
-      EXPECT_THAT(tester.GetAllSamples(kNavigationChainSize),
+      EXPECT_THAT(tester.GetAllSamples(kNavigationChainSize + suffix),
                   ::testing::IsEmpty());
     }
   }
 
   void NavigateAndCommitSubframe(const GURL& url, bool expected_activation) {
     EXPECT_CALL(*client(), ToggleNotificationVisibility(::testing::_)).Times(0);
 
     content::NavigationSimulator::NavigateAndCommitFromDocument(
         url, GetSubframeRFH());
     ExpectActivationSignalForFrame(GetSubframeRFH(), expected_activation);
@@ skipping 126 matching lines @@
 TEST_F(ContentSubresourceFilterDriverFactoryTest,
        ActivateForFrameHostDisabledFeature) {
   // Activation scope is set to NONE => no activation should happen even if URL
   // which is visited was a SB hit.
   base::FieldTrialList field_trial_list(nullptr);
   testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
       base::FeatureList::OVERRIDE_DISABLE_FEATURE, kActivationLevelEnabled,
       kActivationScopeAllSites,
       kActivationListSocialEngineeringAdsInterstitial);
   const GURL url(kExampleUrlWithParams);
-  NavigateAndExpectActivation({true}, {url}, EMPTY,
+  NavigateAndExpectActivation({true}, {url}, NO_REDIRECTS_HIT,
                               ActivationDecision::ACTIVATION_DISABLED);
   factory()->AddHostOfURLToWhitelistSet(url);
-  NavigateAndExpectActivation({true}, {url}, EMPTY,
+  NavigateAndExpectActivation({true}, {url}, NO_REDIRECTS_HIT,
                               ActivationDecision::ACTIVATION_DISABLED);
 }
 
 TEST_F(ContentSubresourceFilterDriverFactoryTest, NoActivationWhenNoMatch) {
   base::FieldTrialList field_trial_list(nullptr);
   testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
       base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
       kActivationScopeActivationList,
       kActivationListSocialEngineeringAdsInterstitial);
   NavigateAndExpectActivation({false}, {GURL(kExampleUrl)}, EMPTY,
@@ skipping 38 matching lines @@
   base::FieldTrialList field_trial_list(nullptr);
   testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
       base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
       kActivationScopeAllSites);
   const GURL url(kExampleUrl);
   NavigateAndExpectActivation({false}, {url}, EMPTY,
                               ActivationDecision::ACTIVATED);
   EmulateFailedNavigationAndExpectNoActivation(url);
 }
 
 TEST_F(ContentSubresourceFilterDriverFactoryTest, RedirectPatternTest) {

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

Could you please add a TODO here to refactor this test to no longer require the
current activation list to be matching (or set altogether)?

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

Done.

   base::FieldTrialList field_trial_list(nullptr);
   testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
       base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
       kActivationScopeActivationList,
       kActivationListSocialEngineeringAdsInterstitial);
   struct RedirectRedirectChainMatchPatternTestData {
     std::vector<bool> blacklisted_urls;
     std::vector<GURL> navigation_chain;
     RedirectChainMatchPattern hit_expected_pattern;
     ActivationDecision expected_activation_decision;
@@ skipping 166 matching lines @@
   // list for which the Safe Browsing hit has happened.
   const ActivationListTestData& test_data = GetParam();
   base::FieldTrialList field_trial_list(nullptr);
   testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
       base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
       kActivationScopeActivationList, test_data.activation_list);
 
   const GURL test_url("https://example.com/nonsoceng?q=engsocnon");
   std::vector<GURL> navigation_chain;
 
-  const bool expected_activation =
-      test_data.expected_activation_decision == ActivationDecision::ACTIVATED;
+  ActivationList effective_list = GetListForThreatTypeAndMetadata(
+      test_data.threat_type, test_data.threat_type_metadata);
   NavigateAndExpectActivation(
       {false, false, false, true},
       {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC), test_url}, test_data.threat_type,
       test_data.threat_type_metadata, content::Referrer(),
-      ui::PAGE_TRANSITION_LINK, expected_activation ? F0M0L1 : EMPTY,
+      ui::PAGE_TRANSITION_LINK,
+      effective_list != ActivationList::NONE ? F0M0L1 : EMPTY,
       test_data.expected_activation_decision);
 };
 
 TEST_P(ContentSubresourceFilterDriverFactoryActivationScopeTest,
        ActivateForScopeType) {
   const ActivationScopeTestData& test_data = GetParam();
   base::FieldTrialList field_trial_list(nullptr);
   testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
       base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
       test_data.activation_scope,
@@ skipping 60 matching lines @@
     ActivationScopeTest,
     ContentSubresourceFilterDriverFactoryActivationScopeTest,
     ::testing::ValuesIn(kActivationScopeTestData));
 
 INSTANTIATE_TEST_CASE_P(
     ActivationLevelTest,
     ContentSubresourceFilterDriverFactoryActivationLevelTest,
     ::testing::ValuesIn(kActivationLevelTestData));
 
 }  // namespace subresource_filter