Revert of [subresource_filter] Decide UNSUPPORTED_SCHEME only if we'd otherwise activate (patchset #3 id:40001 of https://codereview.chromium.org/2888163002/ )

Reason for revert:
Depends on https://codereview.chromium.org/2889913002 which is being reverted.

Original issue's description:
> [subresource_filter] Decide UNSUPPORTED_SCHEME only if we'd otherwise activate
>
> Right now the subresource filter reports UNSUPPORTED_SCHEME even if the
> feature is disabled and the navigation would never have activated. This
> patch aligns things closer to how they were before [1] landed, so that
> UNSUPPORTED_SCHEME is not logged if the matched configuration's
> activation level is DISABLED or there is no matched configuration.
>
> BUG=todo
>
> Review-Url: https://codereview.chromium.org/2888163002
> Cr-Commit-Position: refs/heads/master@{#473093}
> Committed: https://chromium.googlesource.com/chromium/src/+/a30d68388644568f7930aa57cdd2fa015816aeb9

TBR=bmcquade@chromium.org,csharrison@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=todo

Review-Url: https://codereview.chromium.org/2895593002
Cr-Commit-Position: refs/heads/master@{#473154}

Author: treib

chrome/browser/subresource_filter/subresource_filter_unittest.cc

@@ -33,7 +33,6 @@
 #include "components/subresource_filter/core/browser/ruleset_service.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_decision.h"
 #include "components/subresource_filter/core/common/test_ruleset_creator.h"
 #include "content/public/browser/navigation_throttle.h"
 #include "content/public/test/navigation_simulator.h"
@@ -257,42 +256,3 @@ TEST_F(SubresourceFilterTest, ExplicitWhitelisting_ShouldNotClearMetadata) {
   // blacklist.
   EXPECT_NE(nullptr, settings_manager()->GetSiteMetadata(url));
 }
-
-TEST_F(SubresourceFilterTest,
-       NavigationToBadSchemeUrlWithNoActivation_DoesNotReportBadScheme) {
-  // Don't report UNSUPPORTED_SCHEME if the navigation has no matching
-  // configuration.
-  scoped_configuration().ResetConfiguration(subresource_filter::Configuration(
-      subresource_filter::ActivationLevel::DISABLED,
-      subresource_filter::ActivationScope::NO_SITES));
-
-  GURL url("data:text/html,hello world");
-  SimulateNavigateAndCommit(url, main_rfh());
-  EXPECT_TRUE(CreateAndNavigateDisallowedSubframe(main_rfh()));
-  auto* driver_factory = subresource_filter::
-      ContentSubresourceFilterDriverFactory::FromWebContents(web_contents());
-  EXPECT_EQ(
-      subresource_filter::ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET,
-      driver_factory->GetActivationDecisionForLastCommittedPageLoad());
-
-  // Also don't report UNSUPPORTED_SCHEME if the navigation matches a
-  // configuration with DISABLED activation level.
-  scoped_configuration().ResetConfiguration(subresource_filter::Configuration(
-      subresource_filter::ActivationLevel::DISABLED,
-      subresource_filter::ActivationScope::ALL_SITES));
-
-  SimulateNavigateAndCommit(url, main_rfh());
-  EXPECT_TRUE(CreateAndNavigateDisallowedSubframe(main_rfh()));
-  EXPECT_EQ(subresource_filter::ActivationDecision::ACTIVATION_DISABLED,
-            driver_factory->GetActivationDecisionForLastCommittedPageLoad());
-
-  // Sanity check that UNSUPPORTED_SCHEME is reported if the navigation does
-  // activate.
-  scoped_configuration().ResetConfiguration(subresource_filter::Configuration(
-      subresource_filter::ActivationLevel::ENABLED,
-      subresource_filter::ActivationScope::ALL_SITES));
-  SimulateNavigateAndCommit(url, main_rfh());
-  EXPECT_TRUE(CreateAndNavigateDisallowedSubframe(main_rfh()));
-  EXPECT_EQ(subresource_filter::ActivationDecision::UNSUPPORTED_SCHEME,
-            driver_factory->GetActivationDecisionForLastCommittedPageLoad());
-}

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc

@@ -127,28 +127,25 @@ void ContentSubresourceFilterDriverFactory::OnSafeBrowsingMatchComputed(
   throttle_manager_->NotifyPageActivationComputed(navigation_handle, state);
 }
 
-// Be careful when modifying this method, as the order in which the
-// activation_decision_ is decided is very important and corresponds to UMA
-// metrics. In general we want to follow the pattern that
-// ACTIVATION_CONDITIONS_NOT_MET will always be logged if no configuration
-// matches this navigation. We log other decisions only if a configuration
-// matches and also would have activated.
 void ContentSubresourceFilterDriverFactory::
     ComputeActivationForMainFrameNavigation(
         content::NavigationHandle* navigation_handle,
         ActivationList matched_list) {
   const GURL& url(navigation_handle->GetURL());
 
-  bool scheme_is_http_or_https = url.SchemeIsHTTPOrHTTPS();
+  if (!url.SchemeIsHTTPOrHTTPS()) {
+    activation_decision_ = ActivationDecision::UNSUPPORTED_SCHEME;
+    activation_options_ = Configuration::ActivationOptions();
+    return;
+  }
+
   const auto config_list = GetEnabledConfigurations();
   const auto highest_priority_activated_config =
       std::find_if(config_list->configs_by_decreasing_priority().begin(),
                    config_list->configs_by_decreasing_priority().end(),
-                   [&url, scheme_is_http_or_https, matched_list,
-                    this](const Configuration& config) {
+                   [&url, matched_list, this](const Configuration& config) {
                      return DoesMainFrameURLSatisfyActivationConditions(
-                         url, scheme_is_http_or_https,
-                         config.activation_conditions, matched_list);
+                         url, config.activation_conditions, matched_list);
                    });
 
   bool has_activated_config =
@@ -167,18 +164,7 @@ void ContentSubresourceFilterDriverFactory::
     return;
   }
 
-  const Configuration::ActivationOptions& activation_options =
-      highest_priority_activated_config->activation_options;
-
-  // Log UNSUPPORTED_SCHEME if we would have otherwise activated.
-  if (!scheme_is_http_or_https &&
-      activation_options.activation_level != ActivationLevel::DISABLED) {
-    activation_decision_ = ActivationDecision::UNSUPPORTED_SCHEME;
-    activation_options_ = Configuration::ActivationOptions();
-    return;
-  }
-
-  activation_options_ = activation_options;
+  activation_options_ = highest_priority_activated_config->activation_options;
   activation_decision_ =
       activation_options_.activation_level == ActivationLevel::DISABLED
           ? ActivationDecision::ACTIVATION_DISABLED
@@ -188,18 +174,12 @@ void ContentSubresourceFilterDriverFactory::
 bool ContentSubresourceFilterDriverFactory::
     DoesMainFrameURLSatisfyActivationConditions(
         const GURL& url,
-        bool scheme_is_http_or_https,
         const Configuration::ActivationConditions& conditions,
         ActivationList matched_list) const {
-  // scheme_is_http_or_https implies url.SchemeIsHTTPOrHTTPS().
-  DCHECK(!scheme_is_http_or_https || url.SchemeIsHTTPOrHTTPS());
   switch (conditions.activation_scope) {
     case ActivationScope::ALL_SITES:
       return true;
     case ActivationScope::ACTIVATION_LIST:
-      // ACTIVATION_LIST does not support non http/s URLs.
-      if (!scheme_is_http_or_https)
-        return false;
       if (conditions.activation_list == matched_list)
         return true;
       if (conditions.activation_list == ActivationList::PHISHING_INTERSTITIAL &&

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h

@@ -101,10 +101,8 @@ class ContentSubresourceFilterDriverFactory
 
   // Returns whether a main-frame navigation to the given |url| satisfies the
   // activation |conditions| of a given configuration, except for |priority|.
-  // Pass |scheme_is_http_or_https| to avoid multiple string comparisons.
   bool DoesMainFrameURLSatisfyActivationConditions(
       const GURL& url,
-      bool scheme_is_http_or_https,
       const Configuration::ActivationConditions& conditions,
       ActivationList matched_list) const;
 

components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc

@@ -734,17 +734,9 @@ TEST_P(ContentSubresourceFilterDriverFactoryActivationScopeTest,
                                   "https://example.test"};
   for (auto* url : unsupported_urls) {
     SCOPED_TRACE(url);
-    ActivationDecision expected_decision =
-        ActivationDecision::UNSUPPORTED_SCHEME;
-    // We only log UNSUPPORTED_SCHEME if the navigation would have otherwise
-    // activated. Note that non http/s URLs will never match an activation list.
-    if (test_data.expected_activation_decision ==
-            ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET ||
-        test_data.activation_scope == ActivationScope::ACTIVATION_LIST) {
-      expected_decision = ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET;
-    }
     NavigateAndExpectActivation({test_data.url_matches_activation_list},
-                                {GURL(url)}, expected_decision);
+                                {GURL(url)},
+                                ActivationDecision::UNSUPPORTED_SCHEME);
   }
   for (auto* url : supported_urls) {
     SCOPED_TRACE(url);