Revert "Prerender: Fix prerender new content timeout start timing"
This reverts commit ff97f6bc463f127de6b8f0f266165176a02d66c2.
Reason for revert: Breaks BFCache
Original change's description:
> Prerender: Fix prerender new content timeout start timing
>
> Prior to this change, we'd start the new content timeout timer in
> DidNavigate. For pre-render this meant starting the timer before
> activation. When activated, WasShown is called which reset the
> fallback surface if the was a timer running. This caused us to clear
> the graphics output immediately on activation and show blank. Without
> prerender, we would still show the fallback content for the timeout
> duration.
>
> With this patch we separate DidNavigate from creating the timer. We
> conditionally create the timer RenderFrameHostManager::CommitPending
> after the show call
>
> R=​rakina@chromium.org, jonross@chromium.org
>
> Bug: 1423006
> Change-Id: I1cd3f48837085346935ff5d307ebe8dc79dbe5f8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4418087
> Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
> Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
> Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
> Reviewed-by: Jonathan Ross <jonross@chromium.org>
> Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
> Reviewed-by: Bo Liu <boliu@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1132606}
Bug: 1423006
Change-Id: Idef95225b0d85a68ae11c0943d5ccfe19a905db9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4450847
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Vladimir Levin <vmpstr@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Owners-Override: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1132678}
diff --git a/content/browser/renderer_host/render_frame_host_impl_browsertest.cc b/content/browser/renderer_host/render_frame_host_impl_browsertest.cc
index 5a2eb2e..23c7f0c9 100644
--- a/content/browser/renderer_host/render_frame_host_impl_browsertest.cc
+++ b/content/browser/renderer_host/render_frame_host_impl_browsertest.cc
@@ -116,16 +116,9 @@
#if BUILDFLAG(IS_ANDROID)
#include "base/android/build_info.h"
-#include "content/browser/renderer_host/render_widget_host_view_android.h"
#include "third_party/blink/public/mojom/remote_objects/remote_objects.mojom.h"
-#include "ui/android/delegated_frame_host_android.h"
#endif // BUILDFLAG(IS_ANDROID)
-#if defined(USE_AURA)
-#include "content/browser/renderer_host/delegated_frame_host.h"
-#include "content/browser/renderer_host/render_widget_host_view_aura.h"
-#endif // defined(USE_AURA)
-
namespace content {
namespace {
@@ -7907,88 +7900,6 @@
EXPECT_EQ(expected_ftn, activated_rfh->owner_);
}
-IN_PROC_BROWSER_TEST_F(RenderFrameHostImplPrerenderBrowserTest,
- NewContentTimeoutIsNotSetInPrerender) {
- GURL url_a = embedded_test_server()->GetURL("/title1.html");
- GURL prerender_url = embedded_test_server()->GetURL("/title2.html");
- EXPECT_TRUE(NavigateToURL(shell(), url_a));
-
- // Load a page in the prerender.
- int host_id = prerender_helper().AddPrerender(prerender_url);
- RenderFrameHostImpl* prerender_frame_host = static_cast<RenderFrameHostImpl*>(
- prerender_helper().GetPrerenderedMainFrameHost(host_id));
- EXPECT_TRUE(prerender_frame_host);
- RenderWidgetHostImpl* prerender_rwhi = RenderWidgetHostImpl::From(
- prerender_frame_host->GetView()->GetRenderWidgetHost());
- EXPECT_FALSE(prerender_rwhi->IsContentRenderingTimeoutRunning());
-
- // Activate the prerendered page.
- prerender_helper().NavigatePrimaryPage(prerender_url);
-
- RenderFrameHostImpl* activated_rfh = web_contents()->GetPrimaryMainFrame();
- RenderWidgetHostImpl* activated_rwhi = RenderWidgetHostImpl::From(
- activated_rfh->GetView()->GetRenderWidgetHost());
- EXPECT_EQ(activated_rwhi, prerender_rwhi);
- EXPECT_TRUE(activated_rwhi->IsContentRenderingTimeoutRunning());
-}
-
-// TODO(vmpstr): Android takes the new timeout timer from the fallback render
-// widget host impl. This causes us to clear the surface on showing it. See
-// crbug.com/1423006 for the investigation.
-#if BUILDFLAG(IS_ANDROID)
-IN_PROC_BROWSER_TEST_F(RenderFrameHostImplPrerenderBrowserTest,
- DISABLED_ActivationSurfaceRangeIncludesFallback) {
-#else
-IN_PROC_BROWSER_TEST_F(RenderFrameHostImplPrerenderBrowserTest,
- ActivationSurfaceRangeIncludesFallback) {
-#endif
- GURL url_a = embedded_test_server()->GetURL("/title1.html");
- GURL prerender_url = embedded_test_server()->GetURL("/title2.html");
- EXPECT_TRUE(NavigateToURL(shell(), url_a));
-
- // Load a page in the prerender.
- prerender_helper().AddPrerender(prerender_url);
-
- RenderWidgetHostViewBase* initial_view =
- RenderWidgetHostImpl::From(web_contents()
- ->GetPrimaryMainFrame()
- ->GetView()
- ->GetRenderWidgetHost())
- ->GetView();
-
- viz::SurfaceId initial_surface_id = initial_view->GetCurrentSurfaceId();
- EXPECT_TRUE(initial_surface_id.is_valid());
-
- // Activate the prerendered page.
- prerender_helper().NavigatePrimaryPage(prerender_url);
-
- RenderWidgetHostViewBase* activated_view =
- RenderWidgetHostImpl::From(web_contents()
- ->GetPrimaryMainFrame()
- ->GetView()
- ->GetRenderWidgetHost())
- ->GetView();
-
- EXPECT_NE(activated_view, initial_view);
-
-#if defined(USE_AURA)
- DelegatedFrameHost* activated_dfh =
- static_cast<RenderWidgetHostViewAura*>(activated_view)
- ->GetDelegatedFrameHostForTesting();
- viz::SurfaceId fallback_surface_id =
- activated_dfh->GetFallbackSurfaceIdForTesting();
- EXPECT_TRUE(initial_surface_id.IsSameOrNewerThan(fallback_surface_id));
-#elif BUILDFLAG(IS_ANDROID)
- ui::DelegatedFrameHostAndroid* activated_dfh =
- static_cast<RenderWidgetHostViewAndroid*>(activated_view)
- ->delegated_frame_host_for_testing();
- viz::SurfaceId fallback_surface_id =
- activated_dfh->GetFallbackSurfaceIdForTesting();
- EXPECT_TRUE(initial_surface_id.IsSameOrNewerThan(fallback_surface_id))
- << initial_surface_id.ToString() << " " << fallback_surface_id.ToString();
-#endif
-}
-
using RenderFrameHostImplDeathTest = RenderFrameHostImplBrowserTest;
IN_PROC_BROWSER_TEST_F(RenderFrameHostImplDeathTest,
diff --git a/content/browser/renderer_host/render_frame_host_manager.cc b/content/browser/renderer_host/render_frame_host_manager.cc
index a583bb2..b51154d 100644
--- a/content/browser/renderer_host/render_frame_host_manager.cc
+++ b/content/browser/renderer_host/render_frame_host_manager.cc
@@ -678,20 +678,6 @@
// where a RenderFrameHost is swapped in.
if (!frame_tree_node_->frame_tree().IsHidden())
render_frame_host_->GetView()->Show();
-
- // TODO(crbug.com/1434403): For same RenderFrameHost, it isn't clear
- // whether we should start the new content timer, but to be safe, we start
- // it here. The TODO here is to remove this call when we can.
- //
- // Note that this is only OK to do for non-prerender. For prerendering path,
- // setting this timeout is incorrect because it causes a clear of graphical
- // output on prerender activation.
- if (render_frame_host_->lifecycle_state() !=
- LifecycleStateImpl::kPrerendering) {
- static_cast<RenderWidgetHostImpl*>(
- render_frame_host_->GetView()->GetRenderWidgetHost())
- ->StartNewContentRenderingTimeout();
- }
}
// If we are navigating away from a Page that has a form data associated with
@@ -4200,11 +4186,8 @@
// Make the new view show the contents of old view until it has something
// useful to show.
- bool needs_new_content_timeout = false;
- if (is_main_frame && old_view && old_view != new_view) {
+ if (is_main_frame && old_view && old_view != new_view)
new_view->TakeFallbackContentFrom(old_view);
- needs_new_content_timeout = true;
- }
// The RenderViewHost keeps track of the main RenderFrameHost routing id.
// If this is committing a main frame navigation, update it and set the
@@ -4338,11 +4321,6 @@
}
}
- if (needs_new_content_timeout) {
- static_cast<RenderWidgetHostImpl*>(new_view->GetRenderWidgetHost())
- ->StartNewContentRenderingTimeout();
- }
-
// The process will no longer try to exit, so we can decrement the count.
render_frame_host_->GetProcess()->RemovePendingView();
diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc
index 17d81f31..f6c7102 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -1452,14 +1452,12 @@
if (view_)
view_->DidNavigate();
- ClearPendingUserActivation();
-}
-
-void RenderWidgetHostImpl::StartNewContentRenderingTimeout() {
- if (!new_content_rendering_timeout_) {
+ if (!new_content_rendering_timeout_)
return;
- }
+
new_content_rendering_timeout_->Start(new_content_rendering_delay_);
+
+ ClearPendingUserActivation();
}
void RenderWidgetHostImpl::ForwardMouseEvent(const WebMouseEvent& mouse_event) {
@@ -2335,9 +2333,6 @@
void RenderWidgetHostImpl::GetContentRenderingTimeoutFrom(
RenderWidgetHostImpl* other) {
- // TODO(vmpstr): Android is the only caller of this function, and it isn't
- // clear why we should be taking a timer from the fallback surface's timer.
- // See crbug.com/1423006 for the investigation.
if (other->IsContentRenderingTimeoutRunning()) {
new_content_rendering_timeout_->Start(
other->new_content_rendering_timeout_->GetCurrentDelay());
diff --git a/content/browser/renderer_host/render_widget_host_impl.h b/content/browser/renderer_host/render_widget_host_impl.h
index 157f6de..dd25d63 100644
--- a/content/browser/renderer_host/render_widget_host_impl.h
+++ b/content/browser/renderer_host/render_widget_host_impl.h
@@ -506,15 +506,10 @@
// in flight change).
bool RequestRepaintForTesting();
- // Called after every cross-document navigation. Note that for prerender
- // navigations, this is called before the renderer is shown.
- void DidNavigate();
-
// Called after every cross-document navigation. The displayed graphics of
// the renderer is cleared after a certain timeout if it does not produce a
- // new CompositorFrame after navigation. This is called after either
- // navigation (for non-prerender pages) or activation (for prerender pages).
- void StartNewContentRenderingTimeout();
+ // new CompositorFrame after navigation.
+ void DidNavigate();
// Forwards the keyboard event with optional commands to the renderer. If
// |key_event| is not forwarded for any reason, then |commands| are ignored.
diff --git a/content/browser/renderer_host/render_widget_host_unittest.cc b/content/browser/renderer_host/render_widget_host_unittest.cc
index 5590ad0..68d74a9 100644
--- a/content/browser/renderer_host/render_widget_host_unittest.cc
+++ b/content/browser/renderer_host/render_widget_host_unittest.cc
@@ -2245,7 +2245,6 @@
// ClearDisplayedGraphics.
host_->WasShown({} /* record_tab_switch_time_request */);
host_->DidNavigate();
- host_->StartNewContentRenderingTimeout();
EXPECT_FALSE(host_->new_content_rendering_timeout_fired());
// Hide then show. ClearDisplayedGraphics must be called.
@@ -2257,7 +2256,6 @@
// Hide, navigate, then show. ClearDisplayedGraphics must be called.
host_->WasHidden();
host_->DidNavigate();
- host_->StartNewContentRenderingTimeout();
host_->WasShown({} /* record_tab_switch_time_request */);
EXPECT_TRUE(host_->new_content_rendering_timeout_fired());
}
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.h b/content/browser/renderer_host/render_widget_host_view_aura.h
index 6f0904d4..1c1d0257 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.h
+++ b/content/browser/renderer_host/render_widget_host_view_aura.h
@@ -400,10 +400,6 @@
ui::Compositor* GetCompositor() override;
- DelegatedFrameHost* GetDelegatedFrameHostForTesting() const {
- return delegated_frame_host_.get();
- }
-
protected:
~RenderWidgetHostViewAura() override;
diff --git a/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc b/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
index 1779f0b..83b7482 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
@@ -5654,7 +5654,6 @@
// No new LocalSurfaceId should be allocated for the first navigation and the
// timer should not fire.
widget_host_->DidNavigate();
- widget_host_->StartNewContentRenderingTimeout();
viz::LocalSurfaceId id1 = view_->GetLocalSurfaceId();
EXPECT_EQ(id0, id1);
{
@@ -5669,7 +5668,6 @@
// Start the timer. Verify that a new LocalSurfaceId is allocated.
widget_host_->DidNavigate();
- widget_host_->StartNewContentRenderingTimeout();
viz::LocalSurfaceId id2 = view_->GetLocalSurfaceId();
EXPECT_TRUE(id2.is_valid());
EXPECT_LT(id1.parent_sequence_number(), id2.parent_sequence_number());
diff --git a/content/browser/renderer_host/render_widget_host_view_base.h b/content/browser/renderer_host/render_widget_host_view_base.h
index 7b8afe1..d83e754 100644
--- a/content/browser/renderer_host/render_widget_host_view_base.h
+++ b/content/browser/renderer_host/render_widget_host_view_base.h
@@ -565,8 +565,6 @@
virtual void UnlockOrientation() {}
virtual void SetHasPersistentVideo(bool has_persistent_video) {}
- bool HasFallbackSurfaceForTesting() const { return HasFallbackSurface(); }
-
protected:
explicit RenderWidgetHostViewBase(RenderWidgetHost* host);
~RenderWidgetHostViewBase() override;
diff --git a/ui/android/delegated_frame_host_android.cc b/ui/android/delegated_frame_host_android.cc
index 775d4f4c..268170a 100644
--- a/ui/android/delegated_frame_host_android.cc
+++ b/ui/android/delegated_frame_host_android.cc
@@ -209,12 +209,6 @@
return viz::SurfaceId(frame_sink_id_, pre_navigation_local_surface_id_);
}
-viz::SurfaceId DelegatedFrameHostAndroid::GetFallbackSurfaceIdForTesting()
- const {
- return content_layer_->oldest_acceptable_fallback().value_or(
- viz::SurfaceId());
-}
-
void DelegatedFrameHostAndroid::ClearFallbackSurfaceForCommitPending() {
const absl::optional<viz::SurfaceId> fallback_surface_id =
content_layer_->oldest_acceptable_fallback();
diff --git a/ui/android/delegated_frame_host_android.h b/ui/android/delegated_frame_host_android.h
index 57a4bb60..8460cac 100644
--- a/ui/android/delegated_frame_host_android.h
+++ b/ui/android/delegated_frame_host_android.h
@@ -161,8 +161,6 @@
void SetTopControlsVisibleHeight(float height);
- viz::SurfaceId GetFallbackSurfaceIdForTesting() const;
-
private:
// FrameEvictorClient implementation.
void EvictDelegatedFrame(
