[Previews] Adds DeferAllScript support to Decider and OptGuide
Adds new opt guide proto optimization type for DeferAllScript with
whitelist checking for it from PreviewsDeciderImpl thru PreviewsHints.
Bug: 965277
Change-Id: I7be59e6eda9b64d310027ac2c3ddfb7f91ac76eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1673754
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Doug Arnett <dougarnett@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672122}
diff --git a/components/optimization_guide/proto/hints.proto b/components/optimization_guide/proto/hints.proto
index fd24217..2f4ea430 100644
--- a/components/optimization_guide/proto/hints.proto
+++ b/components/optimization_guide/proto/hints.proto
@@ -118,6 +118,9 @@
LITE_PAGE_REDIRECT = 3;
// This optimization does nothing (no-op).
OPTIMIZATION_NONE = 4;
+ // This optimization defers execution of parser-blocking script until after
+ // parsing completes.
+ DEFER_ALL_SCRIPT = 5;
}
// Presents semantics for how page load URLs should be matched.
diff --git a/components/previews/content/hints_fetcher.cc b/components/previews/content/hints_fetcher.cc
index 7c06cd2c..9ddd2206 100644
--- a/components/previews/content/hints_fetcher.cc
+++ b/components/previews/content/hints_fetcher.cc
@@ -55,7 +55,8 @@
optimization_guide::proto::NOSCRIPT);
get_hints_request_->add_supported_optimizations(
optimization_guide::proto::RESOURCE_LOADING);
- // TODO(dougarnett): Add DEFER_ALL_SCRIPT once supported.
+ get_hints_request_->add_supported_optimizations(
+ optimization_guide::proto::DEFER_ALL_SCRIPT);
static_assert(static_cast<int>(PreviewsType::DEFER_ALL_SCRIPT) + 1 ==
static_cast<int>(PreviewsType::LAST),
"PreviewsType has been updated, make sure Optimization Guide "
diff --git a/components/previews/content/previews_decider_impl.cc b/components/previews/content/previews_decider_impl.cc
index d21753c..a523ac0 100644
--- a/components/previews/content/previews_decider_impl.cc
+++ b/components/previews/content/previews_decider_impl.cc
@@ -51,11 +51,11 @@
case PreviewsType::NOSCRIPT:
case PreviewsType::RESOURCE_LOADING_HINTS:
case PreviewsType::LITE_PAGE_REDIRECT:
+ case PreviewsType::DEFER_ALL_SCRIPT:
return true;
// These types do not have server optimization hints.
case PreviewsType::OFFLINE:
case PreviewsType::LITE_PAGE:
- case PreviewsType::DEFER_ALL_SCRIPT:
return false;
case PreviewsType::NONE:
case PreviewsType::UNSPECIFIED:
@@ -333,7 +333,8 @@
return ShouldAllowPreviewPerOptimizationHints(previews_data, url, type,
passed_reasons);
} else if (type == PreviewsType::RESOURCE_LOADING_HINTS ||
- type == PreviewsType::NOSCRIPT) {
+ type == PreviewsType::NOSCRIPT ||
+ type == PreviewsType::DEFER_ALL_SCRIPT) {
return PreviewsEligibilityReason::HOST_NOT_WHITELISTED_BY_SERVER;
}
}
@@ -373,7 +374,8 @@
PreviewsType type) const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(PreviewsType::NOSCRIPT == type ||
- PreviewsType::RESOURCE_LOADING_HINTS == type);
+ PreviewsType::RESOURCE_LOADING_HINTS == type ||
+ PreviewsType::DEFER_ALL_SCRIPT == type);
if (previews_black_list_ && !blacklist_ignored_) {
std::vector<PreviewsEligibilityReason> passed_reasons;
// The blacklist will disallow certain hosts for periods of time based on
@@ -412,7 +414,8 @@
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(type == PreviewsType::LITE_PAGE_REDIRECT ||
type == PreviewsType::NOSCRIPT ||
- type == PreviewsType::RESOURCE_LOADING_HINTS);
+ type == PreviewsType::RESOURCE_LOADING_HINTS ||
+ type == PreviewsType::DEFER_ALL_SCRIPT);
// For LitePageRedirect, ensure it is not blacklisted for this request, and
// hints have been fully loaded.
//
@@ -446,7 +449,8 @@
std::vector<PreviewsEligibilityReason>* passed_reasons) const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(type == PreviewsType::NOSCRIPT ||
- type == PreviewsType::RESOURCE_LOADING_HINTS);
+ type == PreviewsType::RESOURCE_LOADING_HINTS ||
+ type == PreviewsType::DEFER_ALL_SCRIPT);
if (!previews_opt_guide_ || !previews_opt_guide_->has_hints())
return PreviewsEligibilityReason::OPTIMIZATION_HINTS_NOT_AVAILABLE;
diff --git a/components/previews/content/previews_decider_impl_unittest.cc b/components/previews/content/previews_decider_impl_unittest.cc
index 9efe547..5d28f9b 100644
--- a/components/previews/content/previews_decider_impl_unittest.cc
+++ b/components/previews/content/previews_decider_impl_unittest.cc
@@ -193,14 +193,16 @@
PreviewsType type,
net::EffectiveConnectionType* ect_threshold) const override {
EXPECT_TRUE(type == PreviewsType::NOSCRIPT ||
- type == PreviewsType::RESOURCE_LOADING_HINTS);
+ type == PreviewsType::RESOURCE_LOADING_HINTS ||
+ type == PreviewsType::DEFER_ALL_SCRIPT);
// Use default ect trigger threshold for the preview type.
*ect_threshold = previews::params::GetECTThresholdForPreview(type);
if (type == PreviewsType::NOSCRIPT) {
return url.host().compare("whitelisted.example.com") == 0 ||
url.host().compare("noscript_only_whitelisted.example.com") == 0;
}
- if (type == PreviewsType::RESOURCE_LOADING_HINTS) {
+ if (type == PreviewsType::RESOURCE_LOADING_HINTS ||
+ type == PreviewsType::DEFER_ALL_SCRIPT) {
return url.host().compare("whitelisted.example.com") == 0;
}
return false;
@@ -425,6 +427,7 @@
allowed_types[static_cast<int>(PreviewsType::LITE_PAGE_REDIRECT)] = 0;
allowed_types[static_cast<int>(PreviewsType::NOSCRIPT)] = 0;
allowed_types[static_cast<int>(PreviewsType::RESOURCE_LOADING_HINTS)] = 0;
+ allowed_types[static_cast<int>(PreviewsType::DEFER_ALL_SCRIPT)] = 0;
std::unique_ptr<TestPreviewsDeciderImpl> previews_decider_impl =
std::make_unique<TestPreviewsDeciderImpl>(&clock_);
@@ -1186,7 +1189,7 @@
scoped_feature_list.InitWithFeatures(
{features::kPreviews, features::kLitePageServerPreviews,
features::kOptimizationHints, features::kNoScriptPreviews,
- features::kResourceLoadingHints},
+ features::kResourceLoadingHints, features::kDeferAllScriptPreviews},
{});
InitializeUIService();
@@ -1203,6 +1206,11 @@
&user_data, GURL("https://whitelisted.example.com"), false,
PreviewsType::RESOURCE_LOADING_HINTS));
+ // DeferAllScript is allowed before commit without hints.
+ EXPECT_TRUE(previews_decider_impl()->ShouldAllowPreviewAtNavigationStart(
+ &user_data, GURL("https://whitelisted.example.com"), false,
+ PreviewsType::DEFER_ALL_SCRIPT));
+
// LitePageRedirect is not allowed before commit without hints.
EXPECT_FALSE(previews_decider_impl()->ShouldAllowPreviewAtNavigationStart(
&user_data, GURL("https://whitelisted.example.com"), false,
@@ -1223,6 +1231,9 @@
PreviewsType::RESOURCE_LOADING_HINTS));
EXPECT_TRUE(previews_decider_impl()->ShouldAllowPreviewAtNavigationStart(
&user_data, GURL("https://whitelisted.example.com"), false,
+ PreviewsType::DEFER_ALL_SCRIPT));
+ EXPECT_TRUE(previews_decider_impl()->ShouldAllowPreviewAtNavigationStart(
+ &user_data, GURL("https://whitelisted.example.com"), false,
PreviewsType::LITE_PAGE_REDIRECT));
}
@@ -1412,6 +1423,185 @@
}
}
+TEST_F(PreviewsDeciderImplTest, DeferAllScriptNotAllowedByDefault) {
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitWithFeatures(
+ {features::kPreviews, features::kOptimizationHints}, {});
+ InitializeUIService();
+ InitializeOptimizationGuideHints();
+
+ base::HistogramTester histogram_tester;
+ PreviewsUserData user_data(kDefaultPageId);
+
+ EXPECT_FALSE(previews_decider_impl()->ShouldAllowPreviewAtNavigationStart(
+ &user_data, GURL("https://www.google.com"), false,
+ PreviewsType::DEFER_ALL_SCRIPT));
+}
+
+TEST_F(PreviewsDeciderImplTest,
+ DeferAllScriptDisallowedWithoutOptimizationHints) {
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitWithFeatures(
+ {features::kPreviews, features::kDeferAllScriptPreviews},
+ {features::kOptimizationHints});
+ InitializeUIService();
+
+ base::HistogramTester histogram_tester;
+ PreviewsUserData user_data(kDefaultPageId);
+ EXPECT_FALSE(previews_decider_impl()->ShouldAllowPreviewAtNavigationStart(
+ &user_data, GURL("https://whitelisted.example.com"), false,
+ PreviewsType::DEFER_ALL_SCRIPT));
+ histogram_tester.ExpectUniqueSample(
+ "Previews.EligibilityReason",
+ static_cast<int>(
+ PreviewsEligibilityReason::HOST_NOT_WHITELISTED_BY_SERVER),
+ 1);
+ histogram_tester.ExpectUniqueSample(
+ "Previews.EligibilityReason.DeferAllScript",
+ static_cast<int>(
+ PreviewsEligibilityReason::HOST_NOT_WHITELISTED_BY_SERVER),
+ 1);
+}
+
+TEST_F(PreviewsDeciderImplTest, DeferAllScriptAllowedByFeatureAndWhitelist) {
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitWithFeatures(
+ {features::kPreviews, features::kDeferAllScriptPreviews,
+ features::kOptimizationHints},
+ {});
+ InitializeUIService();
+ InitializeOptimizationGuideHints();
+
+ for (const auto& test_ect : {net::EFFECTIVE_CONNECTION_TYPE_OFFLINE,
+ net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G,
+ net::EFFECTIVE_CONNECTION_TYPE_3G}) {
+ ReportEffectiveConnectionType(test_ect);
+
+ base::HistogramTester histogram_tester;
+ PreviewsUserData user_data(kDefaultPageId);
+
+ // Check whitelisted URL.
+ EXPECT_TRUE(previews_decider_impl()->ShouldAllowPreviewAtNavigationStart(
+ &user_data, GURL("https://whitelisted.example.com"), false,
+ PreviewsType::DEFER_ALL_SCRIPT));
+ EXPECT_EQ(test_ect, user_data.navigation_ect());
+ histogram_tester.ExpectUniqueSample(
+ "Previews.EligibilityReason.DeferAllScript",
+ static_cast<int>(PreviewsEligibilityReason::ALLOWED), 1);
+ }
+}
+
+TEST_F(PreviewsDeciderImplTest,
+ DeferAllScriptAllowedByFeatureWithoutKnownHints) {
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitWithFeatures(
+ {features::kPreviews, features::kDeferAllScriptPreviews,
+ features::kOptimizationHints},
+ {});
+ InitializeUIService();
+ InitializeOptimizationGuideHints();
+
+ base::HistogramTester histogram_tester;
+ PreviewsUserData user_data(kDefaultPageId);
+
+ // Verify preview allowed initially for url without known hints.
+ EXPECT_TRUE(previews_decider_impl()->ShouldAllowPreviewAtNavigationStart(
+ &user_data, GURL("https://www.google.com"), false,
+ PreviewsType::DEFER_ALL_SCRIPT));
+
+ histogram_tester.ExpectBucketCount(
+ "Previews.EligibilityReason.DeferAllScript",
+ static_cast<int>(PreviewsEligibilityReason::ALLOWED), 1);
+}
+
+TEST_F(PreviewsDeciderImplTest, DeferAllScriptCommitTimeWhitelistCheck) {
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitWithFeatures(
+ {features::kPreviews, features::kDeferAllScriptPreviews,
+ features::kOptimizationHints},
+ {});
+ InitializeUIService();
+ InitializeOptimizationGuideHints();
+
+ // First verify not allowed for non-whitelisted url.
+ {
+ base::HistogramTester histogram_tester;
+ PreviewsUserData user_data(kDefaultPageId);
+ user_data.set_navigation_ect(net::EFFECTIVE_CONNECTION_TYPE_2G);
+ EXPECT_FALSE(previews_decider_impl()->ShouldCommitPreview(
+ &user_data, GURL("https://www.google.com"),
+ PreviewsType::DEFER_ALL_SCRIPT));
+
+ histogram_tester.ExpectUniqueSample(
+ "Previews.EligibilityReason.DeferAllScript",
+ static_cast<int>(
+ PreviewsEligibilityReason::HOST_NOT_WHITELISTED_BY_SERVER),
+ 1);
+ }
+
+ // Now verify preview for whitelisted url.
+ {
+ base::HistogramTester histogram_tester;
+ PreviewsUserData user_data(kDefaultPageId);
+ user_data.set_navigation_ect(net::EFFECTIVE_CONNECTION_TYPE_2G);
+ EXPECT_TRUE(previews_decider_impl()->ShouldCommitPreview(
+ &user_data, GURL("https://whitelisted.example.com"),
+ PreviewsType::DEFER_ALL_SCRIPT));
+
+ // Expect no eligibility logging.
+ histogram_tester.ExpectTotalCount(
+ "Previews.EligibilityReason.DeferAllScript", 0);
+ }
+
+ // Verify preview not allowed for whitelisted url when network is not slow.
+ {
+ base::HistogramTester histogram_tester;
+ PreviewsUserData user_data(kDefaultPageId);
+ user_data.set_navigation_ect(net::EFFECTIVE_CONNECTION_TYPE_4G);
+ EXPECT_FALSE(previews_decider_impl()->ShouldCommitPreview(
+ &user_data, GURL("https://whitelisted.example.com"),
+ PreviewsType::DEFER_ALL_SCRIPT));
+
+ histogram_tester.ExpectUniqueSample(
+ "Previews.EligibilityReason.DeferAllScript",
+ static_cast<int>(PreviewsEligibilityReason::NETWORK_NOT_SLOW), 1);
+ }
+
+ // Verify preview not allowed for whitelisted url for unknown network quality.
+ {
+ base::HistogramTester histogram_tester;
+ PreviewsUserData user_data(kDefaultPageId);
+ user_data.set_navigation_ect(net::EFFECTIVE_CONNECTION_TYPE_OFFLINE);
+ EXPECT_FALSE(previews_decider_impl()->ShouldCommitPreview(
+ &user_data, GURL("https://whitelisted.example.com"),
+ PreviewsType::DEFER_ALL_SCRIPT));
+
+ histogram_tester.ExpectUniqueSample(
+ "Previews.EligibilityReason.DeferAllScript",
+ static_cast<int>(PreviewsEligibilityReason::DEVICE_OFFLINE), 1);
+ }
+
+ // Verify preview not allowed for session limited ECT threshold.
+ {
+ base::test::ScopedFeatureList nested_scoped_list;
+ nested_scoped_list.InitAndEnableFeatureWithParameters(
+ features::kSlowPageTriggering,
+ {{"session_max_ect_trigger", "Offline"}});
+ base::HistogramTester histogram_tester;
+ PreviewsUserData user_data(kDefaultPageId);
+ user_data.set_navigation_ect(net::EFFECTIVE_CONNECTION_TYPE_2G);
+ EXPECT_FALSE(previews_decider_impl()->ShouldCommitPreview(
+ &user_data, GURL("https://whitelisted.example.com"),
+ PreviewsType::DEFER_ALL_SCRIPT));
+
+ histogram_tester.ExpectUniqueSample(
+ "Previews.EligibilityReason.DeferAllScript",
+ static_cast<int>(
+ PreviewsEligibilityReason::NETWORK_NOT_SLOW_FOR_SESSION),
+ 1);
+ }
+}
+
TEST_F(PreviewsDeciderImplTest, LogPreviewNavigationPassInCorrectParams) {
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(features::kPreviews);
diff --git a/components/previews/content/previews_hints.cc b/components/previews/content/previews_hints.cc
index 12aa84c..e0e7204 100644
--- a/components/previews/content/previews_hints.cc
+++ b/components/previews/content/previews_hints.cc
@@ -125,6 +125,8 @@
return PreviewsType::LITE_PAGE_REDIRECT;
case optimization_guide::proto::OPTIMIZATION_NONE:
return PreviewsType::NONE;
+ case optimization_guide::proto::DEFER_ALL_SCRIPT:
+ return PreviewsType::DEFER_ALL_SCRIPT;
}
}
@@ -143,6 +145,8 @@
case optimization_guide::proto::OPTIMIZATION_NONE:
// Always consider enabled to allow as no-op optimization.
return true;
+ case optimization_guide::proto::DEFER_ALL_SCRIPT:
+ return previews::params::IsDeferAllScriptPreviewsEnabled();
}
}
diff --git a/components/previews/content/previews_hints_unittest.cc b/components/previews/content/previews_hints_unittest.cc
index 83b54ddd..66a1e11 100644
--- a/components/previews/content/previews_hints_unittest.cc
+++ b/components/previews/content/previews_hints_unittest.cc
@@ -517,9 +517,9 @@
}
TEST_F(PreviewsHintsTest,
- IsWhitelistedForSecondOptimizationNoScriptWithFirstDisabled) {
+ IsWhitelistedForSecondOptimizationDeferAllScriptWithFirstDisabled) {
base::test::ScopedFeatureList scoped_list;
- scoped_list.InitWithFeatures({features::kNoScriptPreviews},
+ scoped_list.InitWithFeatures({features::kDeferAllScriptPreviews},
{features::kResourceLoadingHints});
optimization_guide::proto::Configuration config;
@@ -528,7 +528,7 @@
hint1->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
hint1->set_version("someversion");
- // Page hint with NOSCRIPT optimization
+ // Page hint with RESOURCE_LOADING and DEFER_ALL_SCRIPT optimizations
optimization_guide::proto::PageHint* page_hint1 = hint1->add_page_hints();
page_hint1->set_page_pattern("/has_multiple_optimizations/");
optimization_guide::proto::Optimization* optimization1 =
@@ -537,7 +537,8 @@
optimization_guide::proto::RESOURCE_LOADING);
optimization_guide::proto::Optimization* optimization2 =
page_hint1->add_whitelisted_optimizations();
- optimization2->set_optimization_type(optimization_guide::proto::NOSCRIPT);
+ optimization2->set_optimization_type(
+ optimization_guide::proto::DEFER_ALL_SCRIPT);
ParseConfig(config);
@@ -547,7 +548,7 @@
std::string serialized_hint_version_string;
EXPECT_TRUE(MaybeLoadHintAndCheckIsWhitelisted(
GURL("https://www.somedomain.org/has_multiple_optimizations/"),
- PreviewsType::NOSCRIPT, &inflation_percent, &ect_threshold,
+ PreviewsType::DEFER_ALL_SCRIPT, &inflation_percent, &ect_threshold,
&serialized_hint_version_string));
EXPECT_FALSE(MaybeLoadHintAndCheckIsWhitelisted(
GURL("https://www.somedomain.org/has_multiple_optimizations/"),
@@ -559,7 +560,7 @@
IsWhitelistedForSecondOptimizationResourceLoadingWithFirstDisabled) {
base::test::ScopedFeatureList scoped_list;
scoped_list.InitWithFeatures({features::kResourceLoadingHints},
- {features::kNoScriptPreviews});
+ {features::kDeferAllScriptPreviews});
optimization_guide::proto::Configuration config;
optimization_guide::proto::Hint* hint1 = config.add_hints();
@@ -567,12 +568,13 @@
hint1->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
hint1->set_version("someversion");
- // Page hint with NOSCRIPT optimization
+ // Page hint with RESOURCE_LOADING and DEFER_ALL_SCRIPT optimizations
optimization_guide::proto::PageHint* page_hint1 = hint1->add_page_hints();
page_hint1->set_page_pattern("/has_multiple_optimizations/");
optimization_guide::proto::Optimization* optimization1 =
page_hint1->add_whitelisted_optimizations();
- optimization1->set_optimization_type(optimization_guide::proto::NOSCRIPT);
+ optimization1->set_optimization_type(
+ optimization_guide::proto::DEFER_ALL_SCRIPT);
optimization_guide::proto::Optimization* optimization2 =
page_hint1->add_whitelisted_optimizations();
optimization2->set_optimization_type(
diff --git a/components/previews/content/previews_optimization_guide_unittest.cc b/components/previews/content/previews_optimization_guide_unittest.cc
index ca17590f..91b78c0 100644
--- a/components/previews/content/previews_optimization_guide_unittest.cc
+++ b/components/previews/content/previews_optimization_guide_unittest.cc
@@ -673,6 +673,41 @@
}
TEST_F(PreviewsOptimizationGuideTest,
+ ProcessHintsWithValidCommandLineOverrideForDeferAllScript) {
+ base::test::ScopedFeatureList scoped_list;
+ scoped_list.InitWithFeatures({features::kDeferAllScriptPreviews}, {});
+
+ optimization_guide::proto::Configuration config;
+ optimization_guide::proto::Hint* hint = config.add_hints();
+ hint->set_key("somedomain.org");
+ hint->set_key_representation(optimization_guide::proto::HOST_SUFFIX);
+ optimization_guide::proto::PageHint* page_hint = hint->add_page_hints();
+ page_hint->set_page_pattern("defer_default_2g");
+ optimization_guide::proto::Optimization* optimization =
+ page_hint->add_whitelisted_optimizations();
+ optimization->set_optimization_type(
+ optimization_guide::proto::DEFER_ALL_SCRIPT);
+
+ std::string encoded_config;
+ config.SerializeToString(&encoded_config);
+ base::Base64Encode(encoded_config, &encoded_config);
+
+ base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+ switches::kHintsProtoOverride, encoded_config);
+ CreateServiceAndGuide();
+
+ // Verify page matches and ECT thresholds.
+ PreviewsUserData user_data(kDefaultPageId);
+ net::EffectiveConnectionType ect_threshold;
+
+ EXPECT_TRUE(guide()->GetHintsForTesting());
+ EXPECT_TRUE(MaybeLoadOptimizationHintsAndCheckIsWhitelisted(
+ &user_data, GURL("https://somedomain.org/defer_default_2g"),
+ PreviewsType::DEFER_ALL_SCRIPT, &ect_threshold));
+ EXPECT_EQ(net::EFFECTIVE_CONNECTION_TYPE_2G, ect_threshold);
+}
+
+TEST_F(PreviewsOptimizationGuideTest,
ProcessHintsWithValidCommandLineOverrideAndPreexistingData) {
base::test::ScopedFeatureList scoped_list;
scoped_list.InitWithFeatures(
