0

Remove the PreloadingHoldback feature

It has been superseded by PreloadingConfig. The new setup no longer
counts held back preloads as non-eligible, instead given them a special
holdback status field. Eliminating the old setup will make data analysis
easier.

Bug: 1464173, 1449095
Change-Id: I9a3a33294df1b3944b23c6635fdd50966615d22e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4741041
Reviewed-by: Danil Somsikov <dsv@chromium.org>
Reviewed-by: Simon Pelchat <spelchat@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1180101}
This commit is contained in:
Domenic Denicola
2023-08-07 01:57:29 +00:00
committed by Chromium LUCI CQ
parent d36ce08bb6
commit c4fb853dd8
16 changed files with 23 additions and 223 deletions

@ -442,7 +442,7 @@ TabWebContentsDelegateAndroid::IsPrerender2Supported(
content::WebContents& web_contents) {
Profile* profile =
Profile::FromBrowserContext(web_contents.GetBrowserContext());
return prefetch::IsSomePreloadingEnabled(*profile->GetPrefs(), &web_contents);
return prefetch::IsSomePreloadingEnabled(*profile->GetPrefs());
}
std::unique_ptr<content::WebContents>

@ -7665,11 +7665,7 @@ bool ChromeContentBrowserClient::ShouldPreconnectNavigation(
if (!web_request_api || web_request_api->MayHaveProxies())
return false;
#endif
// Preloading is sometimes disabled globally in Chrome via Finch to monitor
// its impact. However, we do not want to evaluate the impact of preconnecting
// at the start of navigation as part of the preloading holdback, so ignore
// the Finch setting here.
return prefetch::IsSomePreloadingEnabledIgnoringFinch(
return prefetch::IsSomePreloadingEnabled(
*Profile::FromBrowserContext(browser_context)->GetPrefs()) ==
content::PreloadingEligibility::kEligible;
}

@ -8,9 +8,7 @@
#include "chrome/browser/prefetch/pref_names.h"
#include "components/pref_registry/pref_registry_syncable.h"
#include "components/prefs/pref_service.h"
#include "content/public/browser/devtools_agent_host.h"
#include "content/public/browser/preloading.h"
#include "content/public/common/content_features.h"
namespace prefetch {
void RegisterPredictionOptionsProfilePrefs(
@ -60,18 +58,6 @@ void SetPreloadPagesState(PrefService* prefs, PreloadPagesState state) {
}
content::PreloadingEligibility IsSomePreloadingEnabled(
const PrefService& prefs,
content::WebContents* web_contents) {
// Override kPreloadingHoldback when DevTools is opened to mitigate the cases
// in which developers are affected by kPreloadingHoldback.
if (!(web_contents && content::DevToolsAgentHost::HasFor(web_contents)) &&
base::FeatureList::IsEnabled(features::kPreloadingHoldback)) {
return content::PreloadingEligibility::kPreloadingDisabled;
}
return IsSomePreloadingEnabledIgnoringFinch(prefs);
}
content::PreloadingEligibility IsSomePreloadingEnabledIgnoringFinch(
const PrefService& prefs) {
// Arrange the results roughly in order of decreasing transience.
if (GetPreloadPagesState(prefs) == PreloadPagesState::kNoPreloading) {

@ -5,13 +5,8 @@
#ifndef CHROME_BROWSER_PREFETCH_PREFETCH_PREFS_H_
#define CHROME_BROWSER_PREFETCH_PREFETCH_PREFS_H_
#include "base/feature_list.h"
#include "content/public/browser/preloading.h"
namespace content {
class WebContents;
} // namespace content
namespace user_prefs {
class PrefRegistrySyncable;
}
@ -62,16 +57,7 @@ void SetPreloadPagesState(PrefService* prefs, PreloadPagesState state);
// Returns PreloadingEligibility:kEligible if preloading is not entirely
// disabled. Returns the first blocking reason encountered otherwise.
// TODO(crbug/1391411): Audit the all callsites whether the default_value =
// nullptr is suitable.
content::PreloadingEligibility IsSomePreloadingEnabled(
const PrefService& prefs,
content::WebContents* web_contents = nullptr);
// Returns PreloadingEligibility:kEligible if preloading is not entirely
// disabled. Returns the first blocking reason encountered otherwise.
// Ignores the PreloadingHoldback Finch feature.
content::PreloadingEligibility IsSomePreloadingEnabledIgnoringFinch(
const PrefService& prefs);
void RegisterPredictionOptionsProfilePrefs(

@ -79,7 +79,7 @@ class PrefetchPrefsPreloadingTest : public ::testing::Test {
PrefetchPrefsPreloadingTest() = default;
~PrefetchPrefsPreloadingTest() override = default;
// IsSomePreloadingEnabled[IgnoringFinch]() requires a threaded environment.
// IsSomePreloadingEnabled() requires a threaded environment.
base::test::TaskEnvironment task_environment_;
};
@ -115,102 +115,14 @@ TEST_F(PrefetchPrefsPreloadingTest, IsSomePreloadingEnabled) {
content::PreloadingEligibility::kEligible);
}
TEST_F(PrefetchPrefsPreloadingTest,
IsSomePreloadingEnabled_PreloadingHoldback) {
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(features::kPreloadingHoldback);
TestingPrefServiceSimple prefs;
prefs.registry()->RegisterIntegerPref(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kDefault));
prefs.SetInteger(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kDisabled));
EXPECT_EQ(prefetch::IsSomePreloadingEnabled(prefs),
content::PreloadingEligibility::kPreloadingDisabled);
prefs.SetInteger(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kStandard));
EXPECT_EQ(prefetch::IsSomePreloadingEnabled(prefs),
content::PreloadingEligibility::kPreloadingDisabled);
prefs.SetInteger(
prefs::kNetworkPredictionOptions,
static_cast<int>(
prefetch::NetworkPredictionOptions::kWifiOnlyDeprecated));
EXPECT_EQ(prefetch::IsSomePreloadingEnabled(prefs),
content::PreloadingEligibility::kPreloadingDisabled);
prefs.SetInteger(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kExtended));
EXPECT_EQ(prefetch::IsSomePreloadingEnabled(prefs),
content::PreloadingEligibility::kPreloadingDisabled);
}
TEST_F(PrefetchPrefsPreloadingTest, IsSomePreloadingEnabledIgnoringFinch) {
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(features::kPreloadingHoldback);
TestingPrefServiceSimple prefs;
prefs.registry()->RegisterIntegerPref(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kDefault));
prefs.SetInteger(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kDisabled));
EXPECT_EQ(prefetch::IsSomePreloadingEnabledIgnoringFinch(prefs),
content::PreloadingEligibility::kPreloadingDisabled);
prefs.SetInteger(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kStandard));
EXPECT_EQ(prefetch::IsSomePreloadingEnabledIgnoringFinch(prefs),
content::PreloadingEligibility::kEligible);
prefs.SetInteger(
prefs::kNetworkPredictionOptions,
static_cast<int>(
prefetch::NetworkPredictionOptions::kWifiOnlyDeprecated));
EXPECT_EQ(prefetch::IsSomePreloadingEnabledIgnoringFinch(prefs),
content::PreloadingEligibility::kEligible);
prefs.SetInteger(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kExtended));
EXPECT_EQ(prefetch::IsSomePreloadingEnabledIgnoringFinch(prefs),
content::PreloadingEligibility::kEligible);
}
class PrefetchPrefsWithBatterySaverTest : public ::testing::Test {
class PrefetchPrefsWithBatterySaverTest : public PrefetchPrefsPreloadingTest {
public:
PrefetchPrefsWithBatterySaverTest() = default;
~PrefetchPrefsWithBatterySaverTest() override = default;
void TearDown() override { battery::ResetIsBatterySaverEnabledForTesting(); }
// IsSomePreloadingEnabled[IgnoringFinch]() requires a threaded environment.
base::test::TaskEnvironment task_environment_;
};
TEST_F(PrefetchPrefsWithBatterySaverTest,
IsSomePreloadingEnabledIgnoringFinch) {
TestingPrefServiceSimple prefs;
prefs.registry()->RegisterIntegerPref(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kDefault));
battery::OverrideIsBatterySaverEnabledForTesting(false);
EXPECT_EQ(prefetch::IsSomePreloadingEnabledIgnoringFinch(prefs),
content::PreloadingEligibility::kEligible);
battery::OverrideIsBatterySaverEnabledForTesting(true);
EXPECT_EQ(prefetch::IsSomePreloadingEnabledIgnoringFinch(prefs),
content::PreloadingEligibility::kBatterySaverEnabled);
}
TEST_F(PrefetchPrefsWithBatterySaverTest, IsSomePreloadingEnabled) {
TestingPrefServiceSimple prefs;
prefs.registry()->RegisterIntegerPref(
@ -230,27 +142,24 @@ TEST_F(PrefetchPrefsWithBatterySaverTest, IsSomePreloadingEnabled) {
content::PreloadingEligibility::kBatterySaverEnabled);
}
class PrefetchPrefsWithDataSaverTest : public ::testing::Test {
class PrefetchPrefsWithDataSaverTest : public PrefetchPrefsPreloadingTest {
public:
PrefetchPrefsWithDataSaverTest() = default;
~PrefetchPrefsWithDataSaverTest() override = default;
void TearDown() override { data_saver::ResetIsDataSaverEnabledForTesting(); }
// IsSomePreloadingEnabledIgnoringFinch() requires a threaded environment.
base::test::TaskEnvironment task_environment_;
};
TEST_F(PrefetchPrefsWithDataSaverTest, IsSomePreloadingEnabledIgnoringFinch) {
TEST_F(PrefetchPrefsWithDataSaverTest, IsSomePreloadingEnabled) {
TestingPrefServiceSimple prefs;
prefs.registry()->RegisterIntegerPref(
prefs::kNetworkPredictionOptions,
static_cast<int>(prefetch::NetworkPredictionOptions::kDefault));
data_saver::OverrideIsDataSaverEnabledForTesting(false);
EXPECT_EQ(prefetch::IsSomePreloadingEnabledIgnoringFinch(prefs),
EXPECT_EQ(prefetch::IsSomePreloadingEnabled(prefs),
content::PreloadingEligibility::kEligible);
data_saver::OverrideIsDataSaverEnabledForTesting(true);
EXPECT_EQ(prefetch::IsSomePreloadingEnabledIgnoringFinch(prefs),
EXPECT_EQ(prefetch::IsSomePreloadingEnabled(prefs),
content::PreloadingEligibility::kDataSaverEnabled);
}

@ -285,8 +285,7 @@ bool SearchPrefetchService::MaybePrefetchURL(
return false;
}
auto eligibility =
prefetch::IsSomePreloadingEnabled(*profile_->GetPrefs(), web_contents);
auto eligibility = prefetch::IsSomePreloadingEnabled(*profile_->GetPrefs());
if (eligibility != content::PreloadingEligibility::kEligible) {
recorder.reason_ = SearchPrefetchEligibilityReason::kPrefetchDisabled;
SetEligibility(attempt, eligibility);

@ -104,11 +104,12 @@ class PrerenderBrowserTest : public PlatformBrowserTest {
class PrerenderHoldbackBrowserTest : public PrerenderBrowserTest {
public:
PrerenderHoldbackBrowserTest() {
feature_list_.InitAndEnableFeature(features::kPreloadingHoldback);
preloading_config_override_.SetHoldback("Prerender", "SpeculationRules",
true);
}
private:
base::test::ScopedFeatureList feature_list_;
content::test::PreloadingConfigOverride preloading_config_override_;
};
// An end-to-end test of prerendering and activating.
@ -276,7 +277,7 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, DisableNetworkPrediction) {
EXPECT_NE(host_id, content::RenderFrameHost::kNoFrameTreeNodeId);
}
// Tests that Devtools open overrides PreloadingHoldback.
// Tests that DevTools open overrides PreloadingConfig's holdback.
IN_PROC_BROWSER_TEST_F(PrerenderHoldbackBrowserTest,
PreloadingHoldbackOverridden) {
base::HistogramTester histogram_tester;
@ -285,8 +286,10 @@ IN_PROC_BROWSER_TEST_F(PrerenderHoldbackBrowserTest,
GURL url = embedded_test_server()->GetURL("/empty.html");
ASSERT_TRUE(content::NavigateToURL(GetActiveWebContents(), url));
PrefService* prefs = chrome_test_utils::GetProfile(this)->GetPrefs();
// IsSomePreloadingEnabled is *not* affected by PreloadingConfig.
ASSERT_EQ(prefetch::IsSomePreloadingEnabled(*prefs),
content::PreloadingEligibility::kPreloadingDisabled);
content::PreloadingEligibility::kEligible);
// Emulating Devtools attached to make PreloadingHoldback overridden. Retain
// the returned host until the test finishes to avoid DevTools termination.
@ -312,8 +315,8 @@ IN_PROC_BROWSER_TEST_F(PrerenderHoldbackBrowserTest,
kFinalStatusActivated, 1);
}
// Tests that Prerender2 cannot be triggered when PreloadingHoldback is not
// overridden by Devtools.
// Tests that Prerender2 cannot be triggered when PreloadingConfig's
// holdback is not overridden by DevTools.
IN_PROC_BROWSER_TEST_F(PrerenderHoldbackBrowserTest,
PreloadingHoldbackNotOverridden) {
// Navigate to an initial page.
@ -321,8 +324,11 @@ IN_PROC_BROWSER_TEST_F(PrerenderHoldbackBrowserTest,
ASSERT_TRUE(content::NavigateToURL(GetActiveWebContents(), url));
PrefService* prefs = chrome_test_utils::GetProfile(this)->GetPrefs();
// IsSomePreloadingEnabled is *not* affected by PreloadingConfig.
ASSERT_EQ(prefetch::IsSomePreloadingEnabled(*prefs),
content::PreloadingEligibility::kPreloadingDisabled);
content::PreloadingEligibility::kEligible);
content::test::PrerenderHostRegistryObserver registry_observer(
*GetActiveWebContents());

@ -1487,7 +1487,7 @@ content::PreloadingEligibility Browser::IsPrerender2Supported(
content::WebContents& web_contents) {
Profile* profile =
Profile::FromBrowserContext(web_contents.GetBrowserContext());
return prefetch::IsSomePreloadingEnabled(*profile->GetPrefs(), &web_contents);
return prefetch::IsSomePreloadingEnabled(*profile->GetPrefs());
}
std::unique_ptr<content::WebContents> Browser::ActivatePortalWebContents(

@ -286,17 +286,6 @@ class PrerenderDevToolsProtocolTest : public DevToolsProtocolTest {
std::unique_ptr<test::PrerenderTestHelper> prerender_helper_;
};
class PreloadingHoldbackDevToolsProtocolTest
: public PrerenderDevToolsProtocolTest {
public:
PreloadingHoldbackDevToolsProtocolTest() {
feature_list_.InitAndEnableFeature(features::kPreloadingHoldback);
}
private:
base::test::ScopedFeatureList feature_list_;
};
class MultiplePrerendersDevToolsProtocolTest
: public PrerenderDevToolsProtocolTest {
public:
@ -3928,26 +3917,6 @@ IN_PROC_BROWSER_TEST_F(
Eq("device.mojom.GamepadMonitor"));
}
IN_PROC_BROWSER_TEST_F(PrerenderDevToolsProtocolTest,
CheckReportedPreloadingFeatures) {
AttachToBrowserTarget();
base::Value::Dict paramsPreloadingHolback;
paramsPreloadingHolback.Set("featureState", "PreloadingHoldback");
const base::Value::Dict* result = SendCommand(
"SystemInfo.getFeatureState", std::move(paramsPreloadingHolback));
EXPECT_THAT(result->FindBool("featureEnabled"), false);
}
IN_PROC_BROWSER_TEST_F(PreloadingHoldbackDevToolsProtocolTest,
CheckReportedPreloadingFeatures) {
AttachToBrowserTarget();
base::Value::Dict params;
params.Set("featureState", "PreloadingHoldback");
const base::Value::Dict* result =
SendCommand("SystemInfo.getFeatureState", std::move(params));
EXPECT_THAT(result->FindBool("featureEnabled"), true);
}
IN_PROC_BROWSER_TEST_F(PrerenderDevToolsProtocolTest,
RenderFrameDevToolsAgentHostCacheEvictionCrash) {
ASSERT_TRUE(embedded_test_server()->Start());

@ -21,7 +21,6 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/child_process_data.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/common/content_features.h"
#include "gpu/config/gpu_feature_type.h"
#include "gpu/config/gpu_info.h"
#include "gpu/config/gpu_switches.h"
@ -442,12 +441,6 @@ void SystemInfoHandler::GetProcessInfo(
Response SystemInfoHandler::GetFeatureState(const String& in_featureState,
bool* featureEnabled) {
if (in_featureState == "PreloadingHoldback") {
*featureEnabled =
base::FeatureList::IsEnabled(features::kPreloadingHoldback);
return Response::Success();
}
return Response::InvalidParams("Unknown feature");
}

@ -789,12 +789,6 @@ BASE_FEATURE(kPreloadingConfig,
"PreloadingConfig",
base::FEATURE_ENABLED_BY_DEFAULT);
// Please note this feature is only used for experimental purposes, please don't
// enable this feature by default.
BASE_FEATURE(kPreloadingHoldback,
"PreloadingHoldback",
base::FEATURE_DISABLED_BY_DEFAULT);
// Enables exposure of the core milestone 1 (M1) APIs in the renderer without an
// origin trial token: Attribution Reporting, FLEDGE, Topics.
BASE_FEATURE(kPrivacySandboxAdsAPIsM1Override,

@ -171,7 +171,6 @@ CONTENT_EXPORT BASE_DECLARE_FEATURE(kPersistentOriginTrials);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kHighPriorityBeforeUnload);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPrefetchNewLimits);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPreloadCookies);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPreloadingHoldback);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPreloadingConfig);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPrivacySandboxAdsAPIsM1Override);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPrivacySandboxAdsAPIsOverride);

@ -1,8 +0,0 @@
Tests SystemInfo.getFeatureState() from browser target
{
id : <number>
result : {
featureEnabled : false
}
}

@ -1,10 +0,0 @@
(async function(testRunner) {
await testRunner.startBlank(
'Tests SystemInfo.getFeatureState() from browser target');
const response = await testRunner.browserP().SystemInfo.getFeatureState(
{featureState: 'PreloadingHoldback'});
testRunner.log(response);
testRunner.completeTest();
})

@ -1,9 +0,0 @@
Tests SystemInfo.getFeatureState() from frame target
{
id : <number>
result : {
featureEnabled : false
}
sessionId : <string>
}

@ -1,10 +0,0 @@
(async function(testRunner) {
const {dp} = await testRunner.startBlank(
'Tests SystemInfo.getFeatureState() from frame target');
const response =
await dp.SystemInfo.getFeatureState({featureState: 'PreloadingHoldback'});
testRunner.log(response);
testRunner.completeTest();
})