[subresource_filter] s/ShouldSuppressActivation/OnPageActivationComputed

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/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
@@ skipping 108 matching lines @@
                          url, config.activation_conditions);
                    });
 
   if (highest_priority_activated_config ==
       config_list->configs_by_decreasing_priority().end()) {
     activation_decision_ = ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET;
     activation_options_ = Configuration::ActivationOptions();
     return;
   }
 
-  if (client_->ShouldSuppressActivation(navigation_handle)) {
-    activation_decision_ = ActivationDecision::URL_WHITELISTED;
-    activation_options_ = Configuration::ActivationOptions();
-    return;
-  }
-
   activation_options_ = highest_priority_activated_config->activation_options;
   activation_decision_ =
       activation_options_.activation_level == ActivationLevel::DISABLED
           ? ActivationDecision::ACTIVATION_DISABLED
           : ActivationDecision::ACTIVATED;
 }
 
 bool ContentSubresourceFilterDriverFactory::
     DoesMainFrameURLSatisfyActivationConditions(
         const GURL& url,
@@ skipping 50 matching lines @@
   RecordRedirectChainMatchPattern();
 
   if (activation_options_.should_whitelist_site_on_reload &&
       NavigationIsPageReload(url, referrer, transition)) {
     // Whitelist this host for the current as well as subsequent navigations.
     client_->WhitelistInCurrentWebContents(url);
   }
 
   ComputeActivationForMainFrameNavigation(navigation_handle);
   DCHECK_NE(activation_decision_, ActivationDecision::UNKNOWN);
+
+  // Check for whitelisted status last, so that the client gets an accurate

[engedy, 2017/05/11 09:13:14]

follow up nit: Any reason to not do this block of logic at the very end of
ComputeActivationForMainFrameNavigation?

[Charlie Harrison, 2017/05/11 12:07:15]

I chose to do this here so that we can continue to use early return semantics in
ComputeActivation*. Otherwise the method gets messier and harder to reason
about.

I don't feel very strongly about it though.

+  // indication of whether there would be activation otherwise.
+  bool whitelisted = client_->OnPageActivationComputed(
+      navigation_handle,
+      activation_options_.activation_level == ActivationLevel::ENABLED);
+
+  // Only reset the activation decision reason if we would have activated.
+  if (whitelisted && activation_decision_ == ActivationDecision::ACTIVATED) {
+    activation_decision_ = ActivationDecision::URL_WHITELISTED;
+    activation_options_ = Configuration::ActivationOptions();
+  }
+
   if (activation_decision_ != ActivationDecision::ACTIVATED) {
     DCHECK_EQ(activation_options_.activation_level, ActivationLevel::DISABLED);
     return;
   }
 
   DCHECK_NE(activation_options_.activation_level, ActivationLevel::DISABLED);
   ActivationState state = ActivationState(activation_options_.activation_level);
   state.measure_performance = ShouldMeasurePerformanceForPageLoad(
       activation_options_.performance_measurement_rate);
   // TODO(csharrison): Set state.enable_logging based on metadata returns from
@@ skipping 113 matching lines @@
       REPORT_REDIRECT_PATTERN_FOR_SUFFIX("SubresourceFilterOnly", hits_pattern,
                                          chain_size);
       break;
     default:
       NOTREACHED();
       break;
   }
 }
 
 }  // namespace subresource_filter