Add the SocEng as a type for checking in CheckUrlForSubresourceFilter.

components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc

Index: components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
diff --git a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
index b230b216a937540c3ab06ec71df1758cda36272e..761de390c3933f5a87078e050066e3d9d4dced4c 100644
--- a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
+++ b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
@@ -48,26 +48,16 @@ const char kUrlD[] = "https://example_d.com";
 const char kSubframeName[] = "Child";
 const char kDisallowedUrl[] = "https://example.com/disallowed.html";
 
-const char kMatchesPatternHistogramName[] =
-    "SubresourceFilter.PageLoad.RedirectChainMatchPattern.";
+const char kMatchesHistogramName[] = "SubresourceFilter.PageLoad.Match.";

[engedy, 2017/04/20 11:16:11]

How about `SubresourecFilter.PageLoad.FinalURLMatch.`?

[melandory, 2017/04/25 13:48:13]

Done.

 const char kNavigationChainSize[] =
     "SubresourceFilter.PageLoad.RedirectChainLength.";
 
-// Human readable representation of expected redirect chain match patterns.
-// The explanations for the buckets given for the following redirect chain:
-// A->B->C->D, where A is initial URL and D is a final URL.
-enum RedirectChainMatchPattern {
-  EMPTY,             // No histograms were recorded.
-  F0M0L1,            // D is a Safe Browsing match.
-  F0M1L0,            // B or C, or both are Safe Browsing matches.
-  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,
+// Enum helps to setup test cases. In future, when checking first URL in the
+// redirect chain is implemented, the enum will be expanded.
+enum RecordHistograms {

[engedy, 2017/04/20 11:16:11]

naming nit: RecordedHistograms

[melandory, 2017/04/25 13:48:13]

Done.

+  EMPTY,
+  MATCH,
+  NUM_MATCH_PATTERNS,

[engedy, 2017/04/20 11:16:11]

nit: Never used.

[melandory, 2017/04/25 13:48:13]

Done.

 };
 
 std::string GetSuffixForList(const ActivationList& type) {
@@ -151,6 +141,10 @@ const ActivationListTestData kActivationListTestData[] = {
      subresource_filter::kActivationListPhishingInterstitial,
      safe_browsing::SB_THREAT_TYPE_URL_PHISHING,
      safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS},
+    {ActivationDecision::ACTIVATED,

[engedy, 2017/04/20 11:16:11]

Could you please add corresponding NOT_MATCHED test cases for this activation
list as well?

+     subresource_filter::kActivationListSubresourceFilter,
+     safe_browsing::SB_THREAT_TYPE_SUBRESOURCE_FILTER,
+     safe_browsing::ThreatPatternType::NONE},
 };
 
 struct ActivationScopeTestData {
@@ -289,7 +283,7 @@ class ContentSubresourceFilterDriverFactoryTest
       safe_browsing::ThreatPatternType threat_type_metadata,
       const content::Referrer& referrer,
       ui::PageTransition transition,
-      RedirectChainMatchPattern expected_pattern,
+      RecordHistograms expected_pattern,

[engedy, 2017/04/20 11:16:11]

nit: expected_histograms

[melandory, 2017/04/25 13:48:13]

Done.

       ActivationDecision expected_activation_decision) {
     const bool expected_activation =
         expected_activation_decision == ActivationDecision::ACTIVATED;
@@ -333,15 +327,13 @@ class ContentSubresourceFilterDriverFactoryTest
 
     const std::string suffix(GetSuffixForList(activation_list));
     size_t all_pattern =
-        tester.GetTotalCountsForPrefix(kMatchesPatternHistogramName).size();
+        tester.GetTotalCountsForPrefix(kMatchesHistogramName).size();
     size_t all_chain_size =
         tester.GetTotalCountsForPrefix(kNavigationChainSize).size();
     if (expected_pattern != EMPTY) {
-      EXPECT_THAT(tester.GetAllSamples(kMatchesPatternHistogramName + suffix),
-                  ::testing::ElementsAre(base::Bucket(expected_pattern, 1)));
-      EXPECT_THAT(
-          tester.GetAllSamples(kNavigationChainSize + suffix),
-          ::testing::ElementsAre(base::Bucket(navigation_chain.size(), 1)));
+      tester.ExpectTotalCount(kMatchesHistogramName + suffix, 1);

[engedy, 2017/04/20 11:16:11]

Let's also check that the correct value is recorded here!

[melandory, 2017/04/25 13:48:13]

Done.

+      tester.ExpectUniqueSample(kNavigationChainSize + suffix,
+                                navigation_chain.size(), 1);
       // Check that we recorded only what is needed.
       EXPECT_EQ(1u, all_pattern);
       EXPECT_EQ(1u, all_chain_size);
@@ -383,7 +375,7 @@ class ContentSubresourceFilterDriverFactoryTest
       safe_browsing::ThreatPatternType threat_type_metadata,
       const content::Referrer& referrer,
       ui::PageTransition transition,
-      RedirectChainMatchPattern expected_pattern,
+      RecordHistograms expected_pattern,
       ActivationDecision expected_activation_decision) {
     const bool expected_activation =
         expected_activation_decision == ActivationDecision::ACTIVATED;
@@ -397,7 +389,7 @@ class ContentSubresourceFilterDriverFactoryTest
   void NavigateAndExpectActivation(
       const std::vector<bool>& blacklisted_urls,
       const std::vector<GURL>& navigation_chain,
-      RedirectChainMatchPattern expected_pattern,
+      RecordHistograms expected_pattern,
       ActivationDecision expected_activation_decision) {
     NavigateAndExpectActivation(
         blacklisted_urls, navigation_chain,
@@ -422,7 +414,7 @@ class ContentSubresourceFilterDriverFactoryTest
 
   void EmulateInPageNavigation(
       const std::vector<bool>& blacklisted_urls,
-      RedirectChainMatchPattern expected_pattern,
+      RecordHistograms expected_pattern,
       ActivationDecision expected_activation_decision) {
     // This test examines the navigation with the following sequence of events:
     //   DidStartProvisional(main, "example.com")
@@ -522,10 +514,10 @@ TEST_F(ContentSubresourceFilterDriverFactoryTest,
       kActivationListSocialEngineeringAdsInterstitial);
   factory()->set_configuration_for_testing(GetActiveConfiguration());
   const GURL url(kExampleUrlWithParams);
-  NavigateAndExpectActivation({true}, {url}, NO_REDIRECTS_HIT,
+  NavigateAndExpectActivation({true}, {url}, MATCH,
                               ActivationDecision::ACTIVATION_DISABLED);
   factory()->AddHostOfURLToWhitelistSet(url);
-  NavigateAndExpectActivation({true}, {url}, NO_REDIRECTS_HIT,
+  NavigateAndExpectActivation({true}, {url}, MATCH,
                               ActivationDecision::ACTIVATION_DISABLED);
 }
 
@@ -560,8 +552,7 @@ TEST_F(ContentSubresourceFilterDriverFactoryTest,
       kActivationScopeActivationList,
       kActivationListSocialEngineeringAdsInterstitial);
   factory()->set_configuration_for_testing(GetActiveConfiguration());
-  EmulateInPageNavigation({true}, NO_REDIRECTS_HIT,
-                          ActivationDecision::ACTIVATED);
+  EmulateInPageNavigation({true}, MATCH, ActivationDecision::ACTIVATED);
 }
 
 TEST_F(ContentSubresourceFilterDriverFactoryTest,
@@ -573,8 +564,7 @@ TEST_F(ContentSubresourceFilterDriverFactoryTest,
       kActivationListSocialEngineeringAdsInterstitial,
       "1" /* performance_measurement_rate */);
   factory()->set_configuration_for_testing(GetActiveConfiguration());
-  EmulateInPageNavigation({true}, NO_REDIRECTS_HIT,
-                          ActivationDecision::ACTIVATED);
+  EmulateInPageNavigation({true}, MATCH, ActivationDecision::ACTIVATED);
 }
 
 TEST_F(ContentSubresourceFilterDriverFactoryTest, FailedNavigation) {
@@ -598,32 +588,32 @@ TEST_F(ContentSubresourceFilterDriverFactoryTest, RedirectPatternTest) {
       kActivationScopeActivationList,
       kActivationListSocialEngineeringAdsInterstitial);
   factory()->set_configuration_for_testing(GetActiveConfiguration());
-  struct RedirectRedirectChainMatchPatternTestData {
+  struct RedirectRecordHistogramsTestData {
     std::vector<bool> blacklisted_urls;
     std::vector<GURL> navigation_chain;
-    RedirectChainMatchPattern hit_expected_pattern;
+    RecordHistograms hit_expected_pattern;
     ActivationDecision expected_activation_decision;
-  } kRedirectRedirectChainMatchPatternTestData[] = {
+  } kRedirectRecordHistogramsTestData[] = {
       {{false},
        {GURL(kUrlA)},
        EMPTY,
        ActivationDecision::ACTIVATION_LIST_NOT_MATCHED},
-      {{true}, {GURL(kUrlA)}, NO_REDIRECTS_HIT, ActivationDecision::ACTIVATED},
+      {{true}, {GURL(kUrlA)}, MATCH, ActivationDecision::ACTIVATED},
       {{false, false},
        {GURL(kUrlA), GURL(kUrlB)},
        EMPTY,
        ActivationDecision::ACTIVATION_LIST_NOT_MATCHED},
       {{false, true},
        {GURL(kUrlA), GURL(kUrlB)},
-       F0M0L1,
+       MATCH,
        ActivationDecision::ACTIVATED},
       {{true, false},
        {GURL(kUrlA), GURL(kUrlB)},
-       F1M0L0,
+       EMPTY,
        ActivationDecision::ACTIVATION_LIST_NOT_MATCHED},
       {{true, true},
        {GURL(kUrlA), GURL(kUrlB)},
-       F1M0L1,
+       MATCH,
        ActivationDecision::ACTIVATED},
       {{false, false, false},
        {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC)},
@@ -631,41 +621,40 @@ TEST_F(ContentSubresourceFilterDriverFactoryTest, RedirectPatternTest) {
        ActivationDecision::ACTIVATION_LIST_NOT_MATCHED},
       {{false, false, true},
        {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC)},
-       F0M0L1,
+       MATCH,
        ActivationDecision::ACTIVATED},
       {{false, true, false},
        {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC)},
-       F0M1L0,
+       EMPTY,
        ActivationDecision::ACTIVATION_LIST_NOT_MATCHED},
       {{false, true, true},
        {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC)},
-       F0M1L1,
+       MATCH,
        ActivationDecision::ACTIVATED},
       {{true, false, false},
        {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC)},
-       F1M0L0,
+       EMPTY,
        ActivationDecision::ACTIVATION_LIST_NOT_MATCHED},
       {{true, false, true},
        {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC)},
-       F1M0L1,
+       MATCH,
        ActivationDecision::ACTIVATED},
       {{true, true, false},
        {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC)},
-       F1M1L0,
+       EMPTY,
        ActivationDecision::ACTIVATION_LIST_NOT_MATCHED},
       {{true, true, true},
        {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC)},
-       F1M1L1,
+       MATCH,
        ActivationDecision::ACTIVATED},
       {{false, true, false, false},
        {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC), GURL(kUrlD)},
-       F0M1L0,
+       EMPTY,
        ActivationDecision::ACTIVATION_LIST_NOT_MATCHED},
   };
 
-  for (size_t i = 0U; i < arraysize(kRedirectRedirectChainMatchPatternTestData);
-       ++i) {
-    auto test_data = kRedirectRedirectChainMatchPatternTestData[i];
+  for (size_t i = 0U; i < arraysize(kRedirectRecordHistogramsTestData); ++i) {

[engedy, 2017/04/20 11:16:11]

nit: If you are already changing this line, could you make this a range-based
for loop?

+    auto test_data = kRedirectRecordHistogramsTestData[i];
     NavigateAndExpectActivation(
         test_data.blacklisted_urls, test_data.navigation_chain,
         safe_browsing::SB_THREAT_TYPE_URL_PHISHING,
@@ -777,11 +766,11 @@ TEST_P(ContentSubresourceFilterDriverFactoryActivationLevelTest,
   factory()->set_configuration_for_testing(GetActiveConfiguration());
 
   const GURL url(kExampleUrlWithParams);
-  NavigateAndExpectActivation({true}, {url}, NO_REDIRECTS_HIT,
+  NavigateAndExpectActivation({true}, {url}, MATCH,
                               test_data.expected_activation_decision);
   factory()->AddHostOfURLToWhitelistSet(url);
   NavigateAndExpectActivation(
-      {true}, {GURL(kExampleUrlWithParams)}, NO_REDIRECTS_HIT,
+      {true}, {GURL(kExampleUrlWithParams)}, MATCH,
       GetActiveConfiguration().activation_level == ActivationLevel::DISABLED
           ? ActivationDecision::ACTIVATION_DISABLED
           : ActivationDecision::URL_WHITELISTED);
@@ -808,7 +797,7 @@ TEST_P(ContentSubresourceFilterDriverFactoryThreatTypeTest,
       {GURL(kUrlA), GURL(kUrlB), GURL(kUrlC), test_url}, test_data.threat_type,
       test_data.threat_type_metadata, content::Referrer(),
       ui::PAGE_TRANSITION_LINK,
-      effective_list != ActivationList::NONE ? F0M0L1 : EMPTY,
+      effective_list != ActivationList::NONE ? MATCH : EMPTY,
       test_data.expected_activation_decision);
 };
 
@@ -824,8 +813,8 @@ TEST_P(ContentSubresourceFilterDriverFactoryActivationScopeTest,
 
   const GURL test_url(kExampleUrlWithParams);
 
-  RedirectChainMatchPattern expected_pattern =
-      test_data.url_matches_activation_list ? NO_REDIRECTS_HIT : EMPTY;
+  RecordHistograms expected_pattern =
+      test_data.url_matches_activation_list ? MATCH : EMPTY;
   NavigateAndExpectActivation({test_data.url_matches_activation_list},
                               {test_url}, expected_pattern,
                               test_data.expected_activation_decision);
@@ -861,7 +850,7 @@ TEST_P(ContentSubresourceFilterDriverFactoryActivationScopeTest,
                                   "https://example.test"};
   for (auto* url : unsupported_urls) {
     SCOPED_TRACE(url);
-    RedirectChainMatchPattern expected_pattern = EMPTY;
+    RecordHistograms expected_pattern = EMPTY;
     NavigateAndExpectActivation(
         {test_data.url_matches_activation_list}, {GURL(url)}, expected_pattern,
         GetActiveConfiguration().activation_scope == ActivationScope::NO_SITES
@@ -870,8 +859,8 @@ TEST_P(ContentSubresourceFilterDriverFactoryActivationScopeTest,
   }
   for (auto* url : supported_urls) {
     SCOPED_TRACE(url);
-    RedirectChainMatchPattern expected_pattern =
-        test_data.url_matches_activation_list ? NO_REDIRECTS_HIT : EMPTY;
+    RecordHistograms expected_pattern =
+        test_data.url_matches_activation_list ? MATCH : EMPTY;
     NavigateAndExpectActivation({test_data.url_matches_activation_list},
                                 {GURL(url)}, expected_pattern,
                                 test_data.expected_activation_decision);