Skip to content

Commit

Permalink
[bfcache] Launch BackForwardCacheEnabledForNonPluginEmbed
Browse files Browse the repository at this point in the history
It's currently Stable 50% with the following metrics:

- Cache Hit: 28.86% => 29.15% (p-value: ◆◆◆◆)
- Blocklisted feature: ContainsPlugins 12.27% -> 0.97%

This CL removes the flag guarding this feature, and update `back_forward_cache_browsertest.cc` to use `kDummy` as blocklisted feature instead (as browser test in content/ cannot load pdf plugin)

Bug: 1325192
Change-Id: Ifcd97bf9be8d657a70fb7de2e5d201889394a0ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3925075
Commit-Queue: Ming-Ying Chung <mych@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1052811}
  • Loading branch information
mingyc authored and Chromium LUCI CQ committed Sep 29, 2022
1 parent e0180d8 commit 5a60c32
Show file tree
Hide file tree
Showing 13 changed files with 14 additions and 81 deletions.
7 changes: 0 additions & 7 deletions chrome/browser/chrome_back_forward_cache_browsertest.cc
Expand Up @@ -678,13 +678,6 @@ class ChromeBackForwardCacheBrowserWithEmbedTest
}

protected:
void SetUpCommandLine(base::CommandLine* command_line) override {
// Allow BackForwardCache for non-plugin embedded elements.
EnableFeatureAndSetParams(
blink::features::kBackForwardCacheEnabledForNonPluginEmbed, "", "");
ChromeBackForwardCacheBrowserTest::SetUpCommandLine(command_line);
}

void ExpectBlocklistedFeature(
blink::scheduler::WebSchedulerTrackedFeature feature,
base::Location location) {
Expand Down
16 changes: 7 additions & 9 deletions content/browser/back_forward_cache_browsertest.cc
Expand Up @@ -2502,19 +2502,17 @@ bool BackForwardCacheBrowserTest::IsUnloadAllowedToEnterBackForwardCache() {
}

bool BackForwardCacheBrowserTest::AddBlocklistedFeature(RenderFrameHost* rfh) {
return ExecJs(rfh, R"(
let object = document.createElement("object");
object.type = "application/x-blink-test-plugin";
document.body.appendChild(object);
)");
// Add kDummy as blocking feature.
RenderFrameHostImplWrapper rfh_a(rfh);
rfh_a->UseDummyStickyBackForwardCacheDisablingFeatureForTesting();
return true;
}

void BackForwardCacheBrowserTest::ExpectNotRestoredDueToBlocklistedFeature(
base::Location location) {
ExpectNotRestored(
{NotRestoredReason::kBlocklistedFeatures},
{blink::scheduler::WebSchedulerTrackedFeature::kContainsPlugins}, {}, {},
{}, location);
ExpectNotRestored({NotRestoredReason::kBlocklistedFeatures},
{blink::scheduler::WebSchedulerTrackedFeature::kDummy}, {},
{}, {}, location);
}

const ukm::TestAutoSetUkmRecorder& BackForwardCacheBrowserTest::ukm_recorder() {
Expand Down
4 changes: 0 additions & 4 deletions third_party/blink/common/features.cc
Expand Up @@ -1301,10 +1301,6 @@ BASE_FEATURE(kBackForwardCacheAppBanner,
"BackForwardCacheAppBanner",
base::FEATURE_ENABLED_BY_DEFAULT);

BASE_FEATURE(kBackForwardCacheEnabledForNonPluginEmbed,
"BackForwardCacheEnabledForNonPluginEmbed",
base::FEATURE_DISABLED_BY_DEFAULT);

// Initialize CSSDefaultStyleSheets early in renderer startup.
BASE_FEATURE(kDefaultStyleSheetsEarlyInit,
"DefaultStyleSheetsEarlyInit",
Expand Down
5 changes: 0 additions & 5 deletions third_party/blink/public/common/features.h
Expand Up @@ -572,11 +572,6 @@ BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kPrefetchAndroidFonts);
// back/forward cache.
BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kBackForwardCacheAppBanner);

// Enables back/forward cache for non-plugin embeds.
// TODO(crbug.com/1325192): Remove once the bug is resolved.
BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(
kBackForwardCacheEnabledForNonPluginEmbed);

BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kDefaultStyleSheetsEarlyInit);

BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kSystemColorChooser);
Expand Down
25 changes: 7 additions & 18 deletions third_party/blink/renderer/core/html/html_plugin_element.cc
Expand Up @@ -126,14 +126,6 @@ HTMLPlugInElement::HTMLPlugInElement(const QualifiedName& tag_name,
// simpler to make both classes share the same codepath in this class.
needs_plugin_update_(!flags.IsCreatedByParser()) {
SetHasCustomStyleCallbacks();
if (!base::FeatureList::IsEnabled(
features::kBackForwardCacheEnabledForNonPluginEmbed)) {
if (auto* context = doc.GetExecutionContext()) {
context->GetScheduler()->RegisterStickyFeature(
SchedulingPolicy::Feature::kContainsPlugins,
{SchedulingPolicy::DisableBackForwardCache()});
}
}
}

HTMLPlugInElement::~HTMLPlugInElement() {
Expand Down Expand Up @@ -697,16 +689,13 @@ bool HTMLPlugInElement::LoadPlugin(const KURL& url,
layout_object->GetFrameView()->AddPlugin(plugin);
}

if (base::FeatureList::IsEnabled(
features::kBackForwardCacheEnabledForNonPluginEmbed)) {
// Disable back/forward cache when a document uses a plugin. This is not
// done in the constructor since |HTMLPlugInElement| is a base class for
// HTMLObjectElement and HTMLEmbedElement which can host child browsing
// contexts instead.
GetExecutionContext()->GetScheduler()->RegisterStickyFeature(
SchedulingPolicy::Feature::kContainsPlugins,
{SchedulingPolicy::DisableBackForwardCache()});
}
// Disable back/forward cache when a document uses a plugin. This is not
// done in the constructor since `HTMLPlugInElement` is a base class for
// HTMLObjectElement and HTMLEmbedElement which can host child browsing
// contexts instead.
GetExecutionContext()->GetScheduler()->RegisterStickyFeature(
SchedulingPolicy::Feature::kContainsPlugins,
{SchedulingPolicy::DisableBackForwardCache()});

GetDocument().SetContainsPlugins();
// TODO(esprehn): WebPluginContainerImpl::SetCcLayer() also schedules a
Expand Down
Expand Up @@ -150,8 +150,6 @@ TEST_F(SchedulingAffectingFeaturesTest, CacheControl_Navigation) {
}

TEST_F(SchedulingAffectingFeaturesTest, Plugins) {
scoped_feature_list_.InitAndEnableFeature(
features::kBackForwardCacheEnabledForNonPluginEmbed);
{
SimRequest main_resource("https://example.com/", "text/html");
LoadURL("https://example.com/");
Expand Down Expand Up @@ -181,8 +179,6 @@ TEST_F(SchedulingAffectingFeaturesTest, Plugins) {
}

TEST_F(SchedulingAffectingFeaturesTest, NonPlugins) {
scoped_feature_list_.InitAndEnableFeature(
features::kBackForwardCacheEnabledForNonPluginEmbed);
{
SimRequest main_resource("https://example.com/", "text/html");
LoadURL("https://example.com/");
Expand Down
4 changes: 0 additions & 4 deletions third_party/blink/web_tests/FlagSpecificConfig
Expand Up @@ -3,10 +3,6 @@
"name": "back-forward-cache",
"args": ["--enable-features=BackForwardCache"]
},
{
"name": "back-forward-cache-embed",
"args": ["--enable-features=BackForwardCacheEnabledForNonPluginEmbed"]
},
{
"name": "disable-layout-ng",
"args": ["--disable-blink-features=LayoutNG,LayoutNGBlockFragmentation"]
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

0 comments on commit 5a60c32

Please sign in to comment.