From 3764afed669afcba8bb9f05be1c22510090b4e6a Mon Sep 17 00:00:00 2001 From: Ryan Sturm Date: Wed, 3 Oct 2018 19:49:03 +0000 Subject: [PATCH] Moving ownership of DRP blacklist skipping to Previews This CL moves the state of whether DRP can skip the long term blacklist checks for Previews from DRPConfig to PreviewsDecider. This happens through some thread Posts, and will eventually move to one thread PostTask instead of 2 when Previews is on the UI thread. Bug: 842233 Change-Id: Id8129338eeb1b2401f8f70ab2d02aebcbbd5c0e1 Reviewed-on: https://chromium-review.googlesource.com/c/1252783 Commit-Queue: Ryan Sturm Reviewed-by: Tarun Bansal Reviewed-by: Steven Holte Reviewed-by: Istiaque Ahmed Reviewed-by: Matt Menke Cr-Commit-Position: refs/heads/master@{#596331} --- ...hrome_resource_dispatcher_host_delegate.cc | 11 ++- .../data_reduction_proxy_chrome_settings.cc | 21 ++++- .../data_reduction_proxy_chrome_settings.h | 8 ++ .../previews_infobar_delegate_unittest.cc | 3 +- .../previews_ui_tab_helper_unittest.cc | 3 +- .../browser/profiles/profile_impl_io_data.cc | 1 + .../render_view_context_menu_unittest.cc | 2 +- .../browser/data_reduction_proxy_config.cc | 63 --------------- .../browser/data_reduction_proxy_config.h | 28 ------- ...a_reduction_proxy_config_service_client.cc | 2 +- ...on_proxy_config_service_client_unittest.cc | 8 +- .../data_reduction_proxy_config_unittest.cc | 78 ------------------- .../browser/data_reduction_proxy_io_data.cc | 17 ++-- .../browser/data_reduction_proxy_io_data.h | 14 +--- ...duction_proxy_network_delegate_unittest.cc | 19 +---- .../browser/data_reduction_proxy_service.cc | 6 ++ .../browser/data_reduction_proxy_service.h | 3 + .../browser/data_reduction_proxy_settings.h | 4 + .../data_reduction_proxy_test_utils.cc | 5 ++ .../browser/data_reduction_proxy_test_utils.h | 9 +++ .../previews/content/previews_decider_impl.cc | 28 ++++++- .../previews/content/previews_decider_impl.h | 18 ++++- .../content/previews_decider_impl_unittest.cc | 45 +++++++++-- .../previews/content/previews_ui_service.cc | 11 +++ .../previews/content/previews_ui_service.h | 4 + components/previews/core/previews_decider.h | 5 +- .../previews/core/test_previews_decider.cc | 2 +- .../previews/core/test_previews_decider.h | 2 +- tools/metrics/histograms/enums.xml | 3 + tools/metrics/histograms/histograms.xml | 3 + 30 files changed, 198 insertions(+), 228 deletions(-) diff --git a/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc b/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc index dc25f92a6ac8b6..8c94cdf706c916 100644 --- a/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc +++ b/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc @@ -700,8 +700,15 @@ ChromeResourceDispatcherHostDelegate::DetermineEnabledPreviews( if (data_reduction_proxy_io_data && previews_decider_impl) { previews::PreviewsUserData::Create(url_request, previews_decider_impl->GeneratePageId()); - if (data_reduction_proxy_io_data->ShouldAcceptServerPreview( - *url_request, previews_decider_impl)) { + if (url_request->url().SchemeIsHTTPOrHTTPS() && + previews_decider_impl->ShouldAllowPreviewAtECT( + *url_request, previews::PreviewsType::LITE_PAGE, + net::EFFECTIVE_CONNECTION_TYPE_4G, std::vector(), + true) && + previews_decider_impl->ShouldAllowPreviewAtECT( + *url_request, previews::PreviewsType::LOFI, + net::EFFECTIVE_CONNECTION_TYPE_4G, std::vector(), + true)) { previews_state |= content::SERVER_LOFI_ON; previews_state |= content::SERVER_LITE_PAGE_ON; } diff --git a/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc b/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc index 82e1225640d1b5..ff73cb7ab80422 100644 --- a/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc +++ b/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc @@ -17,6 +17,8 @@ #include "build/build_config.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/metrics/chrome_metrics_service_accessor.h" +#include "chrome/browser/previews/previews_service.h" +#include "chrome/browser/previews/previews_service_factory.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_manager.h" #include "chrome/common/pref_names.h" @@ -30,6 +32,7 @@ #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h" #include "components/prefs/pref_service.h" #include "components/prefs/scoped_user_pref_update.h" +#include "components/previews/content/previews_ui_service.h" #include "components/proxy_config/proxy_config_pref_names.h" #include "components/proxy_config/proxy_prefs.h" #include "content/public/browser/browser_thread.h" @@ -175,8 +178,8 @@ DataReductionProxyChromeSettings::MigrateDataReductionProxyOffProxyPrefsHelper( DataReductionProxyChromeSettings::DataReductionProxyChromeSettings() : data_reduction_proxy::DataReductionProxySettings(), - data_reduction_proxy_enabled_pref_name_(prefs::kDataSaverEnabled) { -} + data_reduction_proxy_enabled_pref_name_(prefs::kDataSaverEnabled), + profile_(nullptr) {} DataReductionProxyChromeSettings::~DataReductionProxyChromeSettings() { } @@ -192,10 +195,12 @@ void DataReductionProxyChromeSettings::InitDataReductionProxySettings( data_reduction_proxy::DataReductionProxyIOData* io_data, PrefService* profile_prefs, net::URLRequestContextGetter* request_context_getter, + Profile* profile, scoped_refptr url_loader_factory, std::unique_ptr store, const scoped_refptr& ui_task_runner, const scoped_refptr& db_task_runner) { + profile_ = profile; DCHECK_CURRENTLY_ON(content::BrowserThread::UI); #if defined(OS_ANDROID) // On mobile we write Data Reduction Proxy prefs directly to the pref service. @@ -231,6 +236,18 @@ void DataReductionProxyChromeSettings::InitDataReductionProxySettings( MigrateDataReductionProxyOffProxyPrefs(profile_prefs); } +void DataReductionProxyChromeSettings::SetIgnoreLongTermBlackListRules( + bool ignore_long_term_black_list_rules) { + // |previews_service| is null if |profile_| is off the record. + PreviewsService* previews_service = + PreviewsServiceFactory::GetForProfile(profile_); + if (previews_service && previews_service->previews_ui_service()) { + previews_service->previews_ui_service() + ->SetIgnoreLongTermBlackListForServerPreviews( + ignore_long_term_black_list_rules); + } +} + // static data_reduction_proxy::Client DataReductionProxyChromeSettings::GetClient() { #if defined(OS_ANDROID) diff --git a/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h b/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h index 49189b5aca96cc..2fa3d693e47b9e 100644 --- a/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h +++ b/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h @@ -14,6 +14,7 @@ #include "components/keyed_service/core/keyed_service.h" class PrefService; +class Profile; namespace base { class SequencedTaskRunner; @@ -72,6 +73,7 @@ class DataReductionProxyChromeSettings data_reduction_proxy::DataReductionProxyIOData* io_data, PrefService* profile_prefs, net::URLRequestContextGetter* request_context_getter, + Profile* profile, scoped_refptr url_loader_factory, std::unique_ptr store, const scoped_refptr& ui_task_runner, @@ -90,6 +92,9 @@ class DataReductionProxyChromeSettings data_reduction_proxy_enabled_pref_name_ = pref_name; } + void SetIgnoreLongTermBlackListRules( + bool ignore_long_term_black_list_rules) override; + private: // Helper method for migrating the Data Reduction Proxy away from using the // proxy pref. Returns the ProxyPrefMigrationResult value indicating the @@ -99,6 +104,9 @@ class DataReductionProxyChromeSettings std::string data_reduction_proxy_enabled_pref_name_; + // Null before InitDataReductionProxySettings is called. + Profile* profile_; + DISALLOW_COPY_AND_ASSIGN(DataReductionProxyChromeSettings); }; diff --git a/chrome/browser/previews/previews_infobar_delegate_unittest.cc b/chrome/browser/previews/previews_infobar_delegate_unittest.cc index 36b760f3de0874..013b8b569cd81a 100644 --- a/chrome/browser/previews/previews_infobar_delegate_unittest.cc +++ b/chrome/browser/previews/previews_infobar_delegate_unittest.cc @@ -35,6 +35,7 @@ #include "chrome/grit/generated_resources.h" #include "chrome/test/base/chrome_render_view_host_test_harness.h" #include "chrome/test/base/testing_browser_process.h" +#include "chrome/test/base/testing_profile.h" #include "components/blacklist/opt_out_blacklist/opt_out_blacklist_data.h" #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h" #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h" @@ -185,7 +186,7 @@ class PreviewsInfoBarDelegateUnitTest drp_test_context_->GetDataReductionProxyEnabledPrefName()); data_reduction_proxy_settings->InitDataReductionProxySettings( drp_test_context_->io_data(), drp_test_context_->pref_service(), - drp_test_context_->request_context_getter(), + drp_test_context_->request_context_getter(), profile(), base::MakeRefCounted(), base::WrapUnique(new data_reduction_proxy::DataStore()), base::ThreadTaskRunnerHandle::Get(), diff --git a/chrome/browser/previews/previews_ui_tab_helper_unittest.cc b/chrome/browser/previews/previews_ui_tab_helper_unittest.cc index a82e3f54eb3739..cc2ef9dff798c5 100644 --- a/chrome/browser/previews/previews_ui_tab_helper_unittest.cc +++ b/chrome/browser/previews/previews_ui_tab_helper_unittest.cc @@ -19,6 +19,7 @@ #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.h" #include "chrome/browser/previews/previews_ui_tab_helper.h" #include "chrome/test/base/chrome_render_view_host_test_harness.h" +#include "chrome/test/base/testing_profile.h" #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h" #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h" #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h" @@ -80,7 +81,7 @@ class PreviewsUITabHelperUnitTest : public ChromeRenderViewHostTestHarness { drp_test_context_->GetDataReductionProxyEnabledPrefName()); data_reduction_proxy_settings->InitDataReductionProxySettings( drp_test_context_->io_data(), drp_test_context_->pref_service(), - drp_test_context_->request_context_getter(), + drp_test_context_->request_context_getter(), profile(), base::MakeRefCounted(), base::WrapUnique(new data_reduction_proxy::DataStore()), base::ThreadTaskRunnerHandle::Get(), diff --git a/chrome/browser/profiles/profile_impl_io_data.cc b/chrome/browser/profiles/profile_impl_io_data.cc index 790785eda4eeb4..dede943a6f474c 100644 --- a/chrome/browser/profiles/profile_impl_io_data.cc +++ b/chrome/browser/profiles/profile_impl_io_data.cc @@ -244,6 +244,7 @@ ProfileImplIOData::Handle::CreateMainRequestContextGetter( base::FeatureList::IsEnabled(network::features::kNetworkService) ? nullptr : main_request_context_getter_.get(), + profile_, content::BrowserContext::GetDefaultStoragePartition(profile_) ->GetURLLoaderFactoryForBrowserProcess(), std::move(store), diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc b/chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc index 449100e3ce289e..4f73e9d0be3d02 100644 --- a/chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc +++ b/chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc @@ -439,7 +439,7 @@ class RenderViewContextMenuPrefsTest : public ChromeRenderViewHostTestHarness { drp_test_context_->GetDataReductionProxyEnabledPrefName()); settings->InitDataReductionProxySettings( drp_test_context_->io_data(), drp_test_context_->pref_service(), - drp_test_context_->request_context_getter(), + drp_test_context_->request_context_getter(), profile(), base::MakeRefCounted(), std::make_unique(), base::ThreadTaskRunnerHandle::Get(), diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc index b898de0ef795aa..7e5dc6ceca85af 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc @@ -71,17 +71,6 @@ base::LazySequencedTaskRunner g_get_network_id_task_runner = base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)); #endif -// Values of the UMA DataReductionProxy.Protocol.NotAcceptingTransform histogram -// defined in metrics/histograms/histograms.xml. This enum must remain -// synchronized with DataReductionProxyProtocolNotAcceptingTransformReason in -// tools/metrics/histograms/enums.xml. -enum NotAcceptingTransformReason { - NOT_ACCEPTING_TRANSFORM_DISABLED = 0, - NOT_ACCEPTING_TRANSFORM_BLACKLISTED = 1, - NOT_ACCEPTING_TRANSFORM_CELLULAR_ONLY = 2, - NOT_ACCEPTING_TRANSFORM_REASON_BOUNDARY -}; - // Values of the UMA DataReductionProxy.NetworkChangeEvents histograms. // This enum must remain synchronized with the enum of the same // name in metrics/histograms/histograms.xml. @@ -209,7 +198,6 @@ DataReductionProxyConfig::DataReductionProxyConfig( configurator_(configurator), event_creator_(event_creator), connection_type_(network::mojom::ConnectionType::CONNECTION_UNKNOWN), - ignore_long_term_black_list_rules_(false), network_properties_manager_(nullptr), weak_factory_(this) { DCHECK(io_task_runner_); @@ -792,46 +780,6 @@ bool DataReductionProxyConfig::enabled_by_user_and_reachable() const { return enabled_by_user_ && !unreachable_; } -bool DataReductionProxyConfig::IsBlackListedOrDisabled( - const net::URLRequest& request, - const previews::PreviewsDecider& previews_decider, - previews::PreviewsType previews_type) const { - // Make sure request is not locally blacklisted. - // Pass in net::EFFECTIVE_CONNECTION_TYPE_4G as the threshold since we - // just want to check blacklisting here. - // TODO(crbug.com/720102): Consider new method to just check blacklist. - return !previews_decider.ShouldAllowPreviewAtECT( - request, previews_type, net::EFFECTIVE_CONNECTION_TYPE_4G, - std::vector(), ignore_long_term_black_list_rules_); -} - -bool DataReductionProxyConfig::ShouldAcceptServerPreview( - const net::URLRequest& request, - const previews::PreviewsDecider& previews_decider) const { - DCHECK(thread_checker_.CalledOnValidThread()); - DCHECK((request.load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED) != 0); - DCHECK(request.url().SchemeIsHTTPOrHTTPS()); - - if (!previews::params::ArePreviewsAllowed() || - !base::FeatureList::IsEnabled( - features::kDataReductionProxyDecidesTransform)) { - return false; - } - - if (IsBlackListedOrDisabled(request, previews_decider, - previews::PreviewsType::LITE_PAGE) || - IsBlackListedOrDisabled(request, previews_decider, - previews::PreviewsType::LOFI)) { - UMA_HISTOGRAM_ENUMERATION( - "DataReductionProxy.Protocol.NotAcceptingTransform", - NOT_ACCEPTING_TRANSFORM_BLACKLISTED, - NOT_ACCEPTING_TRANSFORM_REASON_BOUNDARY); - return false; - } - - return true; -} - base::TimeTicks DataReductionProxyConfig::GetTicksNow() const { DCHECK(thread_checker_.CalledOnValidThread()); return base::TimeTicks::Now(); @@ -888,15 +836,4 @@ void DataReductionProxyConfig::EnableGetNetworkIdAsynchronously() { } #endif // defined(OS_CHROMEOS) -void DataReductionProxyConfig::SetIgnoreLongTermBlackListRules( - bool ignore_long_term_black_list_rules) { - DCHECK(thread_checker_.CalledOnValidThread()); - ignore_long_term_black_list_rules_ = ignore_long_term_black_list_rules; -} - -bool DataReductionProxyConfig::IgnoreBlackListLongTermRulesForTesting() const { - DCHECK(thread_checker_.CalledOnValidThread()); - return ignore_long_term_black_list_rules_; -} - } // namespace data_reduction_proxy diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h index 4d681667df8756..33665e80b62415 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h @@ -43,10 +43,6 @@ class URLRequestContextGetter; class URLRequestStatus; } // namespace net -namespace previews { -class PreviewsDecider; -} - namespace data_reduction_proxy { class DataReductionProxyConfigValues; @@ -159,15 +155,6 @@ class DataReductionProxyConfig virtual bool ContainsDataReductionProxy( const net::ProxyConfig::ProxyRules& proxy_rules) const; - // Returns whether the client should report to the data reduction proxy that - // it is willing to accept server previews for |request|. - // |previews_decider| is used to check if |request| is locally blacklisted. - // Should only be used if the kDataReductionProxyDecidesTransform feature is - // enabled. - bool ShouldAcceptServerPreview( - const net::URLRequest& request, - const previews::PreviewsDecider& previews_decider) const; - // Returns true if the data saver has been enabled by the user, and the data // saver proxy is reachable. bool enabled_by_user_and_reachable() const; @@ -202,19 +189,12 @@ class DataReductionProxyConfig // TODO(https://crbug.com/821607): Remove after the bug is resolved. void EnableGetNetworkIdAsynchronously(); #endif // defined(OS_CHROMEOS) - - // When triggering previews, prevent long term black list rules. - void SetIgnoreLongTermBlackListRules(bool ignore_long_term_black_list_rules); - // Called when there is a change in the HTTP RTT estimate. void OnRTTOrThroughputEstimatesComputed(base::TimeDelta http_rtt); // Returns the current HTTP RTT estimate. base::Optional GetHttpRttEstimate() const; - // Returns the value set in SetIgnoreLongTermBlackListRules. - bool IgnoreBlackListLongTermRulesForTesting() const; - protected: virtual base::TimeTicks GetTicksNow() const; @@ -312,11 +292,6 @@ class DataReductionProxyConfig bool is_https, base::TimeDelta* min_retry_delay) const; - // Returns whether the request is blacklisted (or if Lo-Fi is disabled). - bool IsBlackListedOrDisabled( - const net::URLRequest& request, - const previews::PreviewsDecider& previews_decider, - previews::PreviewsType previews_type) const; // Checks if the current network has captive portal, and handles the result. // If the captive portal probe was blocked on the current network, disables @@ -378,9 +353,6 @@ class DataReductionProxyConfig bool warmup_url_fetch_in_flight_secure_proxy_; bool warmup_url_fetch_in_flight_core_proxy_; - // When triggerring previews, prevent long term black list rules. - bool ignore_long_term_black_list_rules_; - // Should be accessed only on the IO thread. Guaranteed to be non-null during // the lifetime of |this| if accessed on the IO thread. NetworkPropertiesManager* network_properties_manager_; diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc index 3c5c6458df925e..0c401ed26d3770 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc @@ -606,7 +606,7 @@ bool DataReductionProxyConfigServiceClient::ParseAndApplyProxyConfig( if (!config.has_proxy_config()) return false; - config_->SetIgnoreLongTermBlackListRules( + io_data_->SetIgnoreLongTermBlackListRules( config.ignore_long_term_black_list_rules()); // An empty proxy config is OK, and allows the server to effectively turn off diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc index 90ad91109b0fd1..e70debbecd9a2e 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc @@ -348,6 +348,10 @@ class DataReductionProxyConfigServiceClientTest : public testing::Test { return test_context_->io_data()->pingback_reporting_fraction(); } + bool ignore_blacklist() const { + return test_context_->io_data()->ignore_blacklist(); + } + void RunUntilIdle() { test_context_->RunUntilIdle(); } @@ -1358,7 +1362,7 @@ TEST_F(DataReductionProxyConfigServiceClientTest, config_client()->ApplySerializedConfig( half_reporting_fraction_encoded_config()); EXPECT_EQ(0.5f, pingback_reporting_fraction()); - EXPECT_FALSE(config()->IgnoreBlackListLongTermRulesForTesting()); + EXPECT_FALSE(ignore_blacklist()); } TEST_F(DataReductionProxyConfigServiceClientTest, @@ -1366,7 +1370,7 @@ TEST_F(DataReductionProxyConfigServiceClientTest, Init(true); config_client()->ApplySerializedConfig(ignore_black_list_encoded_config()); - EXPECT_TRUE(config()->IgnoreBlackListLongTermRulesForTesting()); + EXPECT_TRUE(ignore_blacklist()); } TEST_F(DataReductionProxyConfigServiceClientTest, EmptyConfigDisablesDRP) { diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc index e35d1b5c3bab91..0b08eb0882ebe2 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc @@ -874,84 +874,6 @@ TEST_F(DataReductionProxyConfigTest, } } -TEST_F(DataReductionProxyConfigTest, - ShouldAcceptServerPreviewAllPreviewsDisabled) { - base::test::ScopedFeatureList scoped_feature_list; - scoped_feature_list.InitFromCommandLine( - "DataReductionProxyDecidesTransform" /* enable_features */, - "Previews" /* disable_features */); - - net::TestURLRequestContext context; - net::TestDelegate delegate; - std::unique_ptr request = - context.CreateRequest(GURL("http://origin.net:80" - ""), - net::IDLE, &delegate, TRAFFIC_ANNOTATION_FOR_TESTS); - request->SetLoadFlags(request->load_flags() | - net::LOAD_MAIN_FRAME_DEPRECATED); - std::unique_ptr previews_decider = - std::make_unique(true); - EXPECT_FALSE( - test_config()->ShouldAcceptServerPreview(*request, *previews_decider)); -} - -TEST_F(DataReductionProxyConfigTest, - ShouldAcceptServerPreviewServerPreviewsDisabled) { - base::test::ScopedFeatureList scoped_feature_list; - scoped_feature_list.InitFromCommandLine( - "Previews" /* enable_features */, - "DataReductionProxyDecidesTransform" /* disable_features */); - - net::TestURLRequestContext context; - net::TestDelegate delegate; - std::unique_ptr request = - context.CreateRequest(GURL("http://origin.net:80" - ""), - net::IDLE, &delegate, TRAFFIC_ANNOTATION_FOR_TESTS); - request->SetLoadFlags(request->load_flags() | - net::LOAD_MAIN_FRAME_DEPRECATED); - std::unique_ptr previews_decider = - std::make_unique(true); - EXPECT_FALSE( - test_config()->ShouldAcceptServerPreview(*request, *previews_decider)); -} - -TEST_F(DataReductionProxyConfigTest, ShouldAcceptServerPreview) { - // Turn on proxy-decides-transform feature to satisfy DCHECK. - base::test::ScopedFeatureList scoped_feature_list; - scoped_feature_list.InitFromCommandLine( - "Previews,DataReductionProxyDecidesTransform" /* enable_features */, - std::string() /* disable_features */); - base::FieldTrialList field_trial_list(nullptr); - base::FieldTrialList::CreateFieldTrial( - "DataReductionProxyPreviewsBlackListTransition", "Enabled"); - - base::HistogramTester histogram_tester; - net::TestURLRequestContext context_; - net::TestDelegate delegate_; - std::unique_ptr request = - context_.CreateRequest(GURL("http://origin.net:80"), net::IDLE, - &delegate_, TRAFFIC_ANNOTATION_FOR_TESTS); - request->SetLoadFlags(request->load_flags() | - net::LOAD_MAIN_FRAME_DEPRECATED); - std::unique_ptr previews_decider = - std::make_unique(true); - - // Verify true for no flags. - EXPECT_TRUE( - test_config()->ShouldAcceptServerPreview(*request, *previews_decider)); - - // Verify PreviewsDecider check. - base::CommandLine::ForCurrentProcess()->InitFromArgv(0, nullptr); - previews_decider = std::make_unique(false); - EXPECT_FALSE( - test_config()->ShouldAcceptServerPreview(*request, *previews_decider)); - histogram_tester.ExpectBucketCount( - "DataReductionProxy.Protocol.NotAcceptingTransform", - 1 /* NOT_ACCEPTING_TRANSFORM_BLACKLISTED */, 1); - previews_decider = std::make_unique(true); -} - TEST_F(DataReductionProxyConfigTest, HandleWarmupFetcherResponse) { base::HistogramTester histogram_tester; const net::URLRequestStatus kSuccess(net::URLRequestStatus::SUCCESS, net::OK); diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc index 8ee53dc874df74..d0987c18649d7f 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc @@ -30,7 +30,6 @@ #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h" #include "components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h" #include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h" -#include "components/previews/core/previews_decider.h" #include "net/base/load_flags.h" #include "net/http/http_request_headers.h" #include "net/url_request/http_user_agent_settings.h" @@ -340,15 +339,13 @@ void DataReductionProxyIOData::SetDataReductionProxyConfiguration( config_client_->ApplySerializedConfig(serialized_config); } -bool DataReductionProxyIOData::ShouldAcceptServerPreview( - const net::URLRequest& request, - previews::PreviewsDecider* previews_decider) { - DCHECK(previews_decider); - DCHECK((request.load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED) != 0); - if (!config_ || !request.url().SchemeIsHTTPOrHTTPS()) { - return false; - } - return config_->ShouldAcceptServerPreview(request, *previews_decider); +void DataReductionProxyIOData::SetIgnoreLongTermBlackListRules( + bool ignore_long_term_black_list_rules) { + ui_task_runner_->PostTask( + FROM_HERE, + base::BindOnce( + &DataReductionProxyService::SetIgnoreLongTermBlackListRules, service_, + ignore_long_term_black_list_rules)); } void DataReductionProxyIOData::UpdateDataUseForHost(int64_t network_bytes, diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h index 171a49b4e76e50..79b7c7dc57a2bb 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h @@ -41,10 +41,6 @@ namespace network { class NetworkConnectionTracker; } -namespace previews { -class PreviewsDecider; -} - namespace data_reduction_proxy { class DataReductionProxyBypassStats; @@ -107,12 +103,10 @@ class DataReductionProxyIOData : public DataReductionProxyEventStorageDelegate { // Applies a serialized Data Reduction Proxy configuration. void SetDataReductionProxyConfiguration(const std::string& serialized_config); - // Returns true when server previews should be activated. When server previews - // are active, URL requests are modified to request low fidelity versions of - // the resources.|previews_decider| is a non-null object that determines - // eligibility of showing the preview based on past opt outs. - bool ShouldAcceptServerPreview(const net::URLRequest& request, - previews::PreviewsDecider* previews_decider); + // When triggering previews, prevent long term black list rules. Overridden in + // testing. + virtual void SetIgnoreLongTermBlackListRules( + bool ignore_long_term_black_list_rules); // Bridge methods to safely call to the UI thread objects. void UpdateDataUseForHost(int64_t network_bytes, diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc index 2fb9cb2328eaf7..2c1c7cdc855d9d 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc @@ -913,11 +913,6 @@ TEST_F(DataReductionProxyNetworkDelegateTest, AuthenticationTest) { TEST_F(DataReductionProxyNetworkDelegateTest, LoFiTransitions) { Init(USE_INSECURE_PROXY, false); - base::test::ScopedFeatureList scoped_feature_list; - scoped_feature_list.InitWithFeatures( - {previews::features::kPreviews, - features::kDataReductionProxyDecidesTransform}, - {}); // Enable Lo-Fi. bool is_data_reduction_proxy_enabled[] = {false, true}; @@ -945,16 +940,13 @@ TEST_F(DataReductionProxyNetworkDelegateTest, LoFiTransitions) { std::unique_ptr fake_request = context()->CreateRequest( GURL(kTestURL), net::IDLE, &delegate, TRAFFIC_ANNOTATION_FOR_TESTS); fake_request->SetLoadFlags(net::LOAD_MAIN_FRAME_DEPRECATED); - lofi_decider()->SetIsUsingLoFi(config()->ShouldAcceptServerPreview( - *fake_request, test_previews_decider)); + lofi_decider()->SetIsUsingLoFi(true); NotifyNetworkDelegate(fake_request.get(), data_reduction_proxy_info, proxy_retry_info, &headers); VerifyHeaders(is_data_reduction_proxy_enabled[i], true, headers); VerifyDataReductionProxyData(*fake_request, - is_data_reduction_proxy_enabled[i], - config()->ShouldAcceptServerPreview( - *fake_request, test_previews_decider)); + is_data_reduction_proxy_enabled[i], true); } { @@ -1028,14 +1020,11 @@ TEST_F(DataReductionProxyNetworkDelegateTest, LoFiTransitions) { std::unique_ptr fake_request = context()->CreateRequest( GURL(kTestURL), net::IDLE, &delegate, TRAFFIC_ANNOTATION_FOR_TESTS); fake_request->SetLoadFlags(net::LOAD_MAIN_FRAME_DEPRECATED); - lofi_decider()->SetIsUsingLoFi(config()->ShouldAcceptServerPreview( - *fake_request, test_previews_decider)); + lofi_decider()->SetIsUsingLoFi(true); NotifyNetworkDelegate(fake_request.get(), data_reduction_proxy_info, proxy_retry_info, &headers); VerifyDataReductionProxyData(*fake_request, - is_data_reduction_proxy_enabled[i], - config()->ShouldAcceptServerPreview( - *fake_request, test_previews_decider)); + is_data_reduction_proxy_enabled[i], true); } } } diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc index f4fa66ea5ee709..3f2e4d5c39ab86 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc @@ -275,6 +275,12 @@ void DataReductionProxyService::SetProxyRequestHeadersOnUI( settings_->SetProxyRequestHeaders(headers); } +void DataReductionProxyService::SetIgnoreLongTermBlackListRules( + bool ignore_long_term_black_list_rules) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + settings_->SetIgnoreLongTermBlackListRules(ignore_long_term_black_list_rules); +} + void DataReductionProxyService::LoadHistoricalDataUsage( const HistoricalDataUsageCallback& load_data_usage_callback) { std::unique_ptr> data_usage( diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h index 5f7e563c5646e7..320e0097ec982a 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h @@ -143,6 +143,9 @@ class DataReductionProxyService // cleared. void OnCacheCleared(const base::Time start, const base::Time end); + // When triggering previews, prevent long term black list rules. + void SetIgnoreLongTermBlackListRules(bool ignore_long_term_black_list_rules); + // Returns the current network quality estimates. net::EffectiveConnectionType GetEffectiveConnectionType() const; base::Optional GetHttpRttEstimate() const; diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h index feafcc4478a10f..a028961872d050 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h @@ -143,6 +143,10 @@ class DataReductionProxySettings : public DataReductionProxyServiceObserver { // some of them should have. bool IsDataReductionProxyUnreachable(); + // When triggering previews, prevent long term black list rules. + virtual void SetIgnoreLongTermBlackListRules( + bool ignore_long_term_black_list_rules) {} + ContentLengthList GetDailyContentLengths(const char* pref_name); // Configures data reduction proxy. |at_startup| is true when this method is diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc index 79f146d8ccad7a..f206866b047858 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc @@ -294,6 +294,11 @@ void TestDataReductionProxyIOData::SetPingbackReportingFraction( pingback_reporting_fraction_ = pingback_reporting_fraction; } +void TestDataReductionProxyIOData::SetIgnoreLongTermBlackListRules( + bool ignore_long_term_black_list_rules) { + ignore_blacklist_ = ignore_long_term_black_list_rules; +} + void TestDataReductionProxyIOData::SetDataReductionProxyService( base::WeakPtr data_reduction_proxy_service) { if (!service_set_) diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h index 45371878a81fcc..157997dcfee40a 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h @@ -263,10 +263,16 @@ class TestDataReductionProxyIOData : public DataReductionProxyIOData { // Records the reporting fraction that was set by parsing a config. void SetPingbackReportingFraction(float pingback_reporting_fraction) override; + // Records |ignore_long_term_black_list_rules| as |ignore_blacklist_|. + void SetIgnoreLongTermBlackListRules( + bool ignore_long_term_black_list_rules) override; + float pingback_reporting_fraction() const { return pingback_reporting_fraction_; } + bool ignore_blacklist() const { return ignore_blacklist_; } + private: // Allowed SetDataReductionProxyService to be re-entrant. bool service_set_; @@ -274,6 +280,9 @@ class TestDataReductionProxyIOData : public DataReductionProxyIOData { // Reporting fraction last set via SetPingbackReportingFraction. float pingback_reporting_fraction_; + // Whether the long term blacklist rules should be ignored. + bool ignore_blacklist_ = false; + TestDataReductionProxyRequestOptions* test_request_options_; }; diff --git a/components/previews/content/previews_decider_impl.cc b/components/previews/content/previews_decider_impl.cc index a62125b7b7a444..996ccac7838dec 100644 --- a/components/previews/content/previews_decider_impl.cc +++ b/components/previews/content/previews_decider_impl.cc @@ -112,6 +112,7 @@ void PreviewsDeciderImpl::Initialize( std::unique_ptr previews_opt_guide, const PreviewsIsEnabledCallback& is_enabled_callback, blacklist::BlacklistData::AllowedTypesAndVersions allowed_previews) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(ui_task_runner_->BelongsToCurrentThread()); is_enabled_callback_ = is_enabled_callback; previews_ui_service_ = previews_ui_service; @@ -127,6 +128,7 @@ void PreviewsDeciderImpl::Initialize( void PreviewsDeciderImpl::OnNewBlacklistedHost(const std::string& host, base::Time time) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(io_task_runner_->BelongsToCurrentThread()); ui_task_runner_->PostTask( FROM_HERE, base::BindOnce(&PreviewsUIService::OnNewBlacklistedHost, @@ -134,6 +136,7 @@ void PreviewsDeciderImpl::OnNewBlacklistedHost(const std::string& host, } void PreviewsDeciderImpl::OnUserBlacklistedStatusChange(bool blacklisted) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(io_task_runner_->BelongsToCurrentThread()); ui_task_runner_->PostTask( FROM_HERE, @@ -142,6 +145,7 @@ void PreviewsDeciderImpl::OnUserBlacklistedStatusChange(bool blacklisted) { } void PreviewsDeciderImpl::OnBlacklistCleared(base::Time time) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(io_task_runner_->BelongsToCurrentThread()); ui_task_runner_->PostTask( FROM_HERE, base::BindOnce(&PreviewsUIService::OnBlacklistCleared, @@ -151,6 +155,7 @@ void PreviewsDeciderImpl::OnBlacklistCleared(base::Time time) { void PreviewsDeciderImpl::InitializeOnIOThread( std::unique_ptr previews_opt_out_store, blacklist::BlacklistData::AllowedTypesAndVersions allowed_previews) { + DETACH_FROM_SEQUENCE(sequence_checker_); DCHECK(io_task_runner_->BelongsToCurrentThread()); previews_black_list_.reset( new PreviewsBlackList(std::move(previews_opt_out_store), clock_, this, @@ -164,6 +169,7 @@ void PreviewsDeciderImpl::InitializeOnIOThread( void PreviewsDeciderImpl::OnResourceLoadingHints( const GURL& document_gurl, const std::vector& patterns_to_block) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(io_task_runner_->BelongsToCurrentThread()); ui_task_runner_->PostTask( FROM_HERE, @@ -182,6 +188,7 @@ void PreviewsDeciderImpl::LogPreviewNavigation(const GURL& url, PreviewsType type, base::Time time, uint64_t page_id) const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); ui_task_runner_->PostTask( FROM_HERE, base::BindOnce(&PreviewsUIService::LogPreviewNavigation, @@ -195,6 +202,7 @@ void PreviewsDeciderImpl::LogPreviewDecisionMade( PreviewsType type, std::vector&& passed_reasons, uint64_t page_id) const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); LogPreviewsEligibilityReason(reason, type); ui_task_runner_->PostTask( FROM_HERE, base::BindOnce(&PreviewsUIService::LogPreviewDecisionMade, @@ -206,6 +214,7 @@ void PreviewsDeciderImpl::AddPreviewNavigation(const GURL& url, bool opt_out, PreviewsType type, uint64_t page_id) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(io_task_runner_->BelongsToCurrentThread()); base::Time time = previews_black_list_->AddPreviewNavigation(url, opt_out, type); @@ -217,11 +226,13 @@ void PreviewsDeciderImpl::AddPreviewNavigation(const GURL& url, void PreviewsDeciderImpl::ClearBlackList(base::Time begin_time, base::Time end_time) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(io_task_runner_->BelongsToCurrentThread()); previews_black_list_->ClearBlackList(begin_time, end_time); } void PreviewsDeciderImpl::SetIgnorePreviewsBlacklistDecision(bool ignored) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(io_task_runner_->BelongsToCurrentThread()); blacklist_ignored_ = ignored; ui_task_runner_->PostTask( @@ -232,6 +243,7 @@ void PreviewsDeciderImpl::SetIgnorePreviewsBlacklistDecision(bool ignored) { bool PreviewsDeciderImpl::ShouldAllowPreview(const net::URLRequest& request, PreviewsType type) const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(type == PreviewsType::OFFLINE || type == PreviewsType::LITE_PAGE_REDIRECT || type == PreviewsType::NOSCRIPT || @@ -248,7 +260,8 @@ bool PreviewsDeciderImpl::ShouldAllowPreviewAtECT( PreviewsType type, net::EffectiveConnectionType effective_connection_type_threshold, const std::vector& host_blacklist_from_finch, - bool ignore_long_term_black_list_rules) const { + bool is_server_preview) const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); if (!previews::params::ArePreviewsAllowed()) { return false; } @@ -289,7 +302,8 @@ bool PreviewsDeciderImpl::ShouldAllowPreviewAtECT( // The blacklist will disallow certain hosts for periods of time based on // user's opting out of the preview. PreviewsEligibilityReason status = previews_black_list_->IsLoadedAndAllowed( - request.url(), type, ignore_long_term_black_list_rules, + request.url(), type, + is_server_preview && ignore_long_term_blacklist_for_server_previews_, &passed_reasons); if (status != PreviewsEligibilityReason::ALLOWED) { @@ -408,6 +422,7 @@ void PreviewsDeciderImpl::LoadResourceHints(const net::URLRequest& request) { bool PreviewsDeciderImpl::IsURLAllowedForPreview(const net::URLRequest& request, PreviewsType type) const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(PreviewsType::NOSCRIPT == type || PreviewsType::RESOURCE_LOADING_HINTS == type); if (previews_black_list_ && !blacklist_ignored_) { @@ -451,6 +466,7 @@ PreviewsDeciderImpl::ShouldAllowPreviewPerOptimizationHints( const net::URLRequest& request, PreviewsType type, std::vector* passed_reasons) const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(type == PreviewsType::LITE_PAGE_REDIRECT || type == PreviewsType::NOSCRIPT || type == PreviewsType::RESOURCE_LOADING_HINTS); @@ -493,6 +509,7 @@ PreviewsDeciderImpl::IsURLAllowedForPreviewByOptmizationHints( const net::URLRequest& request, PreviewsType type, std::vector* passed_reasons) const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(type == PreviewsType::LITE_PAGE_REDIRECT || type == PreviewsType::NOSCRIPT || type == PreviewsType::RESOURCE_LOADING_HINTS); @@ -516,4 +533,11 @@ uint64_t PreviewsDeciderImpl::GeneratePageId() { return ++page_id_; } +void PreviewsDeciderImpl::SetIgnoreLongTermBlackListForServerPreviews( + bool ignore_long_term_blacklist_for_server_previews) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + ignore_long_term_blacklist_for_server_previews_ = + ignore_long_term_blacklist_for_server_previews; +} + } // namespace previews diff --git a/components/previews/content/previews_decider_impl.h b/components/previews/content/previews_decider_impl.h index 4691a42b2de810..df2e37b72c661c 100644 --- a/components/previews/content/previews_decider_impl.h +++ b/components/previews/content/previews_decider_impl.h @@ -15,6 +15,7 @@ #include "base/macros.h" #include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" +#include "base/sequence_checker.h" #include "base/single_thread_task_runner.h" #include "base/time/time.h" #include "components/blacklist/opt_out_blacklist/opt_out_blacklist_data.h" @@ -117,10 +118,16 @@ class PreviewsDeciderImpl : public PreviewsDecider, PreviewsType type, net::EffectiveConnectionType effective_connection_type_threshold, const std::vector& host_blacklist_from_finch, - bool ignore_long_term_black_list_rules) const override; + bool is_server_preview) const override; bool IsURLAllowedForPreview(const net::URLRequest& request, PreviewsType type) const override; + // Set whether ignoring the long term blacklist rules is allowed for calls to + // ShouldAllowPreviewAtECT that have |can_ignore_long_term_black_list_rules| + // set to true. + void SetIgnoreLongTermBlackListForServerPreviews( + bool ignore_long_term_blacklist_for_server_previews); + void LoadResourceHints(const net::URLRequest& request) override; // Generates a page ID that is guaranteed to be unique from any other page ID @@ -176,8 +183,15 @@ class PreviewsDeciderImpl : public PreviewsDecider, // Whether the decisions made by PreviewsBlackList should be ignored or not. // This can be changed by chrome://interventions-internals to test/debug the // behavior of Previews decisions. + // This is related to a test flag and should only be true when the user has + // set it in flags. See previews::IsPreviewsBlacklistIgnoredViaFlag. bool blacklist_ignored_; + // Whether ignoring the blacklist is allowed for calls to + // ShouldAllowPreviewAtECT that have + // |is_server_preview| true. + bool ignore_long_term_blacklist_for_server_previews_ = false; + base::Clock* clock_; // The UI and IO thread task runners. |ui_task_runner_| is used to post @@ -192,6 +206,8 @@ class PreviewsDeciderImpl : public PreviewsDecider, uint64_t page_id_; + SEQUENCE_CHECKER(sequence_checker_); + base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(PreviewsDeciderImpl); diff --git a/components/previews/content/previews_decider_impl_unittest.cc b/components/previews/content/previews_decider_impl_unittest.cc index fd5755e7a3cfcb..73a16c873aae93 100644 --- a/components/previews/content/previews_decider_impl_unittest.cc +++ b/components/previews/content/previews_decider_impl_unittest.cc @@ -9,6 +9,7 @@ #include #include #include +#include #include "base/bind.h" #include "base/bind_helpers.h" @@ -105,13 +106,14 @@ class TestPreviewsBlackList : public PreviewsBlackList { PreviewsType type, bool ignore_long_term_black_list_rules, std::vector* passed_reasons) const override { - PreviewsEligibilityReason ordered_reasons[] = { + std::vector ordered_reasons = { PreviewsEligibilityReason::BLACKLIST_DATA_NOT_LOADED, - PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT, - PreviewsEligibilityReason::USER_BLACKLISTED, - PreviewsEligibilityReason::HOST_BLACKLISTED, - PreviewsEligibilityReason::ALLOWED, - }; + PreviewsEligibilityReason::USER_RECENTLY_OPTED_OUT}; + + if (!ignore_long_term_black_list_rules) { + ordered_reasons.push_back(PreviewsEligibilityReason::USER_BLACKLISTED); + ordered_reasons.push_back(PreviewsEligibilityReason::HOST_BLACKLISTED); + } for (auto reason : ordered_reasons) { if (status_ == reason) { @@ -119,8 +121,8 @@ class TestPreviewsBlackList : public PreviewsBlackList { } passed_reasons->push_back(reason); } - NOTREACHED(); - return status_; + + return PreviewsEligibilityReason::ALLOWED; } private: @@ -1785,6 +1787,33 @@ TEST_F(PreviewsDeciderImplTest, GeneratePageIdMakesUniqueNonZero) { EXPECT_EQ(page_id_set.end(), page_id_set.find(0u)); } +TEST_F(PreviewsDeciderImplTest, TestIgnoreLongTermRule) { + // Verify that when long term rules can be ignored, and the caller is fine + // with ignoring long term rules, they are not checked. + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature(features::kPreviews); + InitializeUIService(); + + previews_decider_impl()->SetIgnoreLongTermBlackListForServerPreviews(true); + + std::unique_ptr blacklist = + std::make_unique( + PreviewsEligibilityReason::HOST_BLACKLISTED, previews_decider_impl()); + previews_decider_impl()->InjectTestBlacklist(std::move(blacklist)); + + // LoFi and LitePage check NQE on their own. + network_quality_estimator()->set_effective_connection_type( + net::EFFECTIVE_CONNECTION_TYPE_3G); + + base::HistogramTester histogram_tester; + EXPECT_FALSE(previews_decider_impl()->ShouldAllowPreviewAtECT( + *CreateRequest(), PreviewsType::LITE_PAGE, + net::EFFECTIVE_CONNECTION_TYPE_4G, std::vector(), false)); + EXPECT_TRUE(previews_decider_impl()->ShouldAllowPreviewAtECT( + *CreateRequest(), PreviewsType::LITE_PAGE, + net::EFFECTIVE_CONNECTION_TYPE_4G, std::vector(), true)); +} + } // namespace } // namespace previews diff --git a/components/previews/content/previews_ui_service.cc b/components/previews/content/previews_ui_service.cc index 0ff68ee23198f0..f661fe0f1c6bd7 100644 --- a/components/previews/content/previews_ui_service.cc +++ b/components/previews/content/previews_ui_service.cc @@ -124,6 +124,17 @@ PreviewsLogger* PreviewsUIService::previews_logger() const { return logger_.get(); } +// When triggering previews, prevent long term black list rules. +void PreviewsUIService::SetIgnoreLongTermBlackListForServerPreviews( + bool ignore_long_term_black_list_rules_allowed) { + DCHECK(thread_checker_.CalledOnValidThread()); + io_task_runner_->PostTask( + FROM_HERE, + base::BindOnce( + &PreviewsDeciderImpl::SetIgnoreLongTermBlackListForServerPreviews, + previews_decider_impl_, ignore_long_term_black_list_rules_allowed)); +} + void PreviewsUIService::ClearBlackList(base::Time begin_time, base::Time end_time) { DCHECK(thread_checker_.CalledOnValidThread()); diff --git a/components/previews/content/previews_ui_service.h b/components/previews/content/previews_ui_service.h index 0c4ffdb26be6e3..981fe0410482c3 100644 --- a/components/previews/content/previews_ui_service.h +++ b/components/previews/content/previews_ui_service.h @@ -128,6 +128,10 @@ class PreviewsUIService { // return null. PreviewsLogger* previews_logger() const; + // When triggering previews, prevent long term black list rules. + void SetIgnoreLongTermBlackListForServerPreviews( + bool ignore_long_term_black_list_rules_allowed); + private: // The IO thread portion of the inter-thread communication for previews/. base::WeakPtr previews_decider_impl_; diff --git a/components/previews/core/previews_decider.h b/components/previews/core/previews_decider.h index b2fa3705cd22b5..41dbb101c9b76d 100644 --- a/components/previews/core/previews_decider.h +++ b/components/previews/core/previews_decider.h @@ -26,12 +26,15 @@ class PreviewsDecider { // preview will be disallowed; preview types that check network quality before // calling ShouldAllowPreviewAtECT should pass in // EFFECTIVE_CONNECTION_TYPE_4G. + // |is_server_preview| means that the blacklist does + // not need to be checked for long term rules when Previews has been + // configured to allow skipping the blacklist. virtual bool ShouldAllowPreviewAtECT( const net::URLRequest& request, PreviewsType type, net::EffectiveConnectionType effective_connection_type_threshold, const std::vector& host_blacklist_from_finch, - bool ignore_long_term_black_list_rules) const = 0; + bool is_server_preview) const = 0; // Same as ShouldAllowPreviewAtECT, but uses the previews default // EffectiveConnectionType and no blacklisted hosts from the server. diff --git a/components/previews/core/test_previews_decider.cc b/components/previews/core/test_previews_decider.cc index a90d491ad8bbd0..247e8155bf524f 100644 --- a/components/previews/core/test_previews_decider.cc +++ b/components/previews/core/test_previews_decider.cc @@ -16,7 +16,7 @@ bool TestPreviewsDecider::ShouldAllowPreviewAtECT( previews::PreviewsType type, net::EffectiveConnectionType effective_connection_type_threshold, const std::vector& host_blacklist_from_server, - bool ignore_long_term_black_list_rules) const { + bool is_server_preview) const { return allow_previews_; } diff --git a/components/previews/core/test_previews_decider.h b/components/previews/core/test_previews_decider.h index 70d00cb0c52511..1b76db7a76d3e3 100644 --- a/components/previews/core/test_previews_decider.h +++ b/components/previews/core/test_previews_decider.h @@ -21,7 +21,7 @@ class TestPreviewsDecider : public previews::PreviewsDecider { previews::PreviewsType type, net::EffectiveConnectionType effective_connection_type_threshold, const std::vector& host_blacklist_from_server, - bool ignore_long_term_black_list_rules) const override; + bool is_server_preview) const override; bool ShouldAllowPreview(const net::URLRequest& request, previews::PreviewsType type) const override; bool IsURLAllowedForPreview(const net::URLRequest& request, diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml index 4d159e3cd26dc3..30e82e64a9d398 100644 --- a/tools/metrics/histograms/enums.xml +++ b/tools/metrics/histograms/enums.xml @@ -9845,6 +9845,9 @@ Called by update_net_error_codes.py.--> + + Deprecated 10/2018. + + + Obsolete as of 10/2018. + dougarnett@chromium.org Records the reason that a page request is not accepting proxy server