Commit 87f8a6dd authored by Owen Min's avatar Owen Min Committed by Commit Bot

Revert "Fix evicted app windows showing as empty in overview"

This reverts commit e9ddea61.

Reason for revert: AppWindowBrowserTest.ShouldShowStaleContentOnEviction is flaky.
https://analysis.chromium.org/p/chromium/flake-portal/flakes/occurrences?key=ag9zfmZpbmRpdC1mb3ItbWVyVwsSBUZsYWtlIkxjaHJvbWl1bUBicm93c2VyX3Rlc3RzQEFwcFdpbmRvd0Jyb3dzZXJUZXN0LlNob3VsZFNob3dTdGFsZUNvbnRlbnRPbkV2aWN0aW9uDA

Original change's description:
> Fix evicted app windows showing as empty in overview
> 
> The Files app, as well as other apps, if they're evicted due to
> being occluded by other windows, will show as empty for a second or
> more in overview until reloaded.
> 
> This CL makes them behave the same way as browser windows, that is
> they show stale contents until reloaded.
> 
> BUG=986085, 866622
> TEST=Manually, follow steps on bug, and note the issue no longer shows.
> 
> Change-Id: If9a2b26b914fe55672d15d4b560cbe6d7e996591
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1711843
> Reviewed-by: kylechar <kylechar@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Reviewed-by: Ken Rockot <rockot@google.com>
> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
> Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#682390}

TBR=sky@chromium.org,kenrb@chromium.org,lazyboy@chromium.org,rockot@google.com,afakhry@chromium.org,kylechar@chromium.org

Change-Id: I7772e1ab6134a02d2cd6ad1d312623b424339693
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 986085, 866622
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1729511Reviewed-by: default avatarOwen Min <zmin@chromium.org>
Commit-Queue: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682759}
parent d57f90cd
...@@ -81,21 +81,13 @@ DelegatedFrameHost::~DelegatedFrameHost() { ...@@ -81,21 +81,13 @@ DelegatedFrameHost::~DelegatedFrameHost() {
host_frame_sink_manager_->InvalidateFrameSinkId(frame_sink_id_); host_frame_sink_manager_->InvalidateFrameSinkId(frame_sink_id_);
} }
void DelegatedFrameHost::AddObserverForTesting(Observer* observer) {
observers_.AddObserver(observer);
}
void DelegatedFrameHost::RemoveObserverForTesting(Observer* observer) {
observers_.RemoveObserver(observer);
}
void DelegatedFrameHost::WasShown( void DelegatedFrameHost::WasShown(
const viz::LocalSurfaceId& new_local_surface_id, const viz::LocalSurfaceId& new_local_surface_id,
const gfx::Size& new_dip_size, const gfx::Size& new_dip_size,
const base::Optional<RecordTabSwitchTimeRequest>& const base::Optional<RecordTabSwitchTimeRequest>&
record_tab_switch_time_request) { record_tab_switch_time_request) {
// Cancel any pending frame eviction and unpause it if paused. // Cancel any pending frame eviction and unpause it if paused.
SetFrameEvictionStateAndNotifyObservers(FrameEvictionState::kNotStarted); frame_eviction_state_ = FrameEvictionState::kNotStarted;
frame_evictor_->SetVisible(true); frame_evictor_->SetVisible(true);
if (record_tab_switch_time_request && compositor_) { if (record_tab_switch_time_request && compositor_) {
...@@ -189,16 +181,6 @@ void DelegatedFrameHost::CopyFromCompositingSurfaceInternal( ...@@ -189,16 +181,6 @@ void DelegatedFrameHost::CopyFromCompositingSurfaceInternal(
viz::SurfaceId(frame_sink_id_, local_surface_id_), std::move(request)); viz::SurfaceId(frame_sink_id_, local_surface_id_), std::move(request));
} }
void DelegatedFrameHost::SetFrameEvictionStateAndNotifyObservers(
FrameEvictionState frame_eviction_state) {
if (frame_eviction_state_ == frame_eviction_state)
return;
frame_eviction_state_ = frame_eviction_state;
for (auto& obs : observers_)
obs.OnFrameEvictionStateChanged(frame_eviction_state_);
}
bool DelegatedFrameHost::CanCopyFromCompositingSurface() const { bool DelegatedFrameHost::CanCopyFromCompositingSurface() const {
return local_surface_id_.is_valid(); return local_surface_id_.is_valid();
} }
...@@ -391,8 +373,7 @@ void DelegatedFrameHost::EvictDelegatedFrame() { ...@@ -391,8 +373,7 @@ void DelegatedFrameHost::EvictDelegatedFrame() {
// CrOS overview mode. // CrOS overview mode.
if (client_->ShouldShowStaleContentOnEviction() && if (client_->ShouldShowStaleContentOnEviction() &&
!stale_content_layer_->has_external_content()) { !stale_content_layer_->has_external_content()) {
SetFrameEvictionStateAndNotifyObservers( frame_eviction_state_ = FrameEvictionState::kPendingEvictionRequests;
FrameEvictionState::kPendingEvictionRequests);
auto callback = auto callback =
base::BindOnce(&DelegatedFrameHost::DidCopyStaleContent, GetWeakPtr()); base::BindOnce(&DelegatedFrameHost::DidCopyStaleContent, GetWeakPtr());
...@@ -418,7 +399,7 @@ void DelegatedFrameHost::DidCopyStaleContent( ...@@ -418,7 +399,7 @@ void DelegatedFrameHost::DidCopyStaleContent(
DCHECK_EQ(result->format(), viz::CopyOutputResult::Format::RGBA_TEXTURE); DCHECK_EQ(result->format(), viz::CopyOutputResult::Format::RGBA_TEXTURE);
DCHECK_NE(frame_eviction_state_, FrameEvictionState::kNotStarted); DCHECK_NE(frame_eviction_state_, FrameEvictionState::kNotStarted);
SetFrameEvictionStateAndNotifyObservers(FrameEvictionState::kNotStarted); frame_eviction_state_ = FrameEvictionState::kNotStarted;
ContinueDelegatedFrameEviction(); ContinueDelegatedFrameEviction();
auto transfer_resource = viz::TransferableResource::MakeGL( auto transfer_resource = viz::TransferableResource::MakeGL(
......
...@@ -69,19 +69,6 @@ class CONTENT_EXPORT DelegatedFrameHost ...@@ -69,19 +69,6 @@ class CONTENT_EXPORT DelegatedFrameHost
public viz::mojom::CompositorFrameSinkClient, public viz::mojom::CompositorFrameSinkClient,
public viz::HostFrameSinkClient { public viz::HostFrameSinkClient {
public: public:
enum class FrameEvictionState {
kNotStarted = 0, // Frame eviction is ready.
kPendingEvictionRequests // Frame eviction is paused with pending requests.
};
class Observer {
public:
virtual void OnFrameEvictionStateChanged(FrameEvictionState new_state) = 0;
protected:
virtual ~Observer() = default;
};
// |should_register_frame_sink_id| flag indicates whether DelegatedFrameHost // |should_register_frame_sink_id| flag indicates whether DelegatedFrameHost
// is responsible for registering the associated FrameSinkId with the // is responsible for registering the associated FrameSinkId with the
// compositor or not. This is set only on non-aura platforms, since aura is // compositor or not. This is set only on non-aura platforms, since aura is
...@@ -91,9 +78,6 @@ class CONTENT_EXPORT DelegatedFrameHost ...@@ -91,9 +78,6 @@ class CONTENT_EXPORT DelegatedFrameHost
bool should_register_frame_sink_id); bool should_register_frame_sink_id);
~DelegatedFrameHost() override; ~DelegatedFrameHost() override;
void AddObserverForTesting(Observer* observer);
void RemoveObserverForTesting(Observer* observer);
// ui::CompositorObserver implementation. // ui::CompositorObserver implementation.
void OnCompositingShuttingDown(ui::Compositor* compositor) override; void OnCompositingShuttingDown(ui::Compositor* compositor) override;
...@@ -189,14 +173,6 @@ class CONTENT_EXPORT DelegatedFrameHost ...@@ -189,14 +173,6 @@ class CONTENT_EXPORT DelegatedFrameHost
return weak_factory_.GetWeakPtr(); return weak_factory_.GetWeakPtr();
} }
const ui::Layer* stale_content_layer() const {
return stale_content_layer_.get();
}
FrameEvictionState frame_eviction_state() const {
return frame_eviction_state_;
}
private: private:
friend class DelegatedFrameHostClient; friend class DelegatedFrameHostClient;
FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraBrowserTest, FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraBrowserTest,
...@@ -224,9 +200,6 @@ class CONTENT_EXPORT DelegatedFrameHost ...@@ -224,9 +200,6 @@ class CONTENT_EXPORT DelegatedFrameHost
viz::CopyOutputRequest::ResultFormat format, viz::CopyOutputRequest::ResultFormat format,
viz::CopyOutputRequest::CopyOutputRequestCallback callback); viz::CopyOutputRequest::CopyOutputRequestCallback callback);
void SetFrameEvictionStateAndNotifyObservers(
FrameEvictionState frame_eviction_state);
const viz::FrameSinkId frame_sink_id_; const viz::FrameSinkId frame_sink_id_;
DelegatedFrameHostClient* const client_; DelegatedFrameHostClient* const client_;
const bool enable_viz_; const bool enable_viz_;
...@@ -258,6 +231,11 @@ class CONTENT_EXPORT DelegatedFrameHost ...@@ -258,6 +231,11 @@ class CONTENT_EXPORT DelegatedFrameHost
viz::LocalSurfaceId first_local_surface_id_after_navigation_; viz::LocalSurfaceId first_local_surface_id_after_navigation_;
enum class FrameEvictionState {
kNotStarted = 0, // Frame eviction is ready.
kPendingEvictionRequests // Frame eviction is paused with pending requests.
};
FrameEvictionState frame_eviction_state_ = FrameEvictionState::kNotStarted; FrameEvictionState frame_eviction_state_ = FrameEvictionState::kNotStarted;
// Layer responsible for displaying the stale content for the DFHC when the // Layer responsible for displaying the stale content for the DFHC when the
...@@ -267,8 +245,6 @@ class CONTENT_EXPORT DelegatedFrameHost ...@@ -267,8 +245,6 @@ class CONTENT_EXPORT DelegatedFrameHost
TabSwitchTimeRecorder tab_switch_time_recorder_; TabSwitchTimeRecorder tab_switch_time_recorder_;
base::ObserverList<Observer>::Unchecked observers_;
base::WeakPtrFactory<DelegatedFrameHost> weak_factory_{this}; base::WeakPtrFactory<DelegatedFrameHost> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(DelegatedFrameHost); DISALLOW_COPY_AND_ASSIGN(DelegatedFrameHost);
......
...@@ -459,8 +459,6 @@ class CONTENT_EXPORT RenderWidgetHostViewAura ...@@ -459,8 +459,6 @@ class CONTENT_EXPORT RenderWidgetHostViewAura
class WindowAncestorObserver; class WindowAncestorObserver;
friend class WindowAncestorObserver; friend class WindowAncestorObserver;
friend void VerifyStaleContentOnFrameEviction(
RenderWidgetHostView* render_widget_host_view);
// Allocate a new FrameSinkId if this object is the platform view of a // Allocate a new FrameSinkId if this object is the platform view of a
// RenderWidgetHostViewGuest. This FrameSinkId will not be actually used in // RenderWidgetHostViewGuest. This FrameSinkId will not be actually used in
......
...@@ -137,7 +137,6 @@ ...@@ -137,7 +137,6 @@
#endif #endif
#if defined(USE_AURA) #if defined(USE_AURA)
#include "content/browser/renderer_host/delegated_frame_host.h"
#include "content/browser/renderer_host/overscroll_controller.h" #include "content/browser/renderer_host/overscroll_controller.h"
#include "content/browser/renderer_host/render_widget_host_view_aura.h" #include "content/browser/renderer_host/render_widget_host_view_aura.h"
#include "ui/aura/test/window_event_dispatcher_test_api.h" #include "ui/aura/test/window_event_dispatcher_test_api.h"
...@@ -3093,78 +3092,6 @@ MockOverscrollController* MockOverscrollController::Create( ...@@ -3093,78 +3092,6 @@ MockOverscrollController* MockOverscrollController::Create(
return raw_mock; return raw_mock;
} }
namespace {
// This class interacts with the internals of the DelegatedFrameHost without
// exposing them in the header.
class EvictionStateWaiter : public DelegatedFrameHost::Observer {
public:
EvictionStateWaiter(DelegatedFrameHost* delegated_frame_host)
: delegated_frame_host_(delegated_frame_host) {
delegated_frame_host_->AddObserverForTesting(this);
}
~EvictionStateWaiter() override {
delegated_frame_host_->RemoveObserverForTesting(this);
}
void WaitForEvictionState(DelegatedFrameHost::FrameEvictionState state) {
if (delegated_frame_host_->frame_eviction_state() == state)
return;
waited_eviction_state_ = state;
base::RunLoop run_loop;
quit_closure_ = run_loop.QuitClosure();
run_loop.Run();
}
// DelegatedFrameHost::Observer:
void OnFrameEvictionStateChanged(
DelegatedFrameHost::FrameEvictionState new_state) override {
if (!quit_closure_.is_null() && (new_state == waited_eviction_state_))
std::move(quit_closure_).Run();
}
private:
DelegatedFrameHost* delegated_frame_host_;
DelegatedFrameHost::FrameEvictionState waited_eviction_state_;
base::OnceClosure quit_closure_;
DISALLOW_COPY_AND_ASSIGN(EvictionStateWaiter);
};
} // namespace
void VerifyStaleContentOnFrameEviction(
RenderWidgetHostView* render_widget_host_view) {
auto* render_widget_host_view_aura =
static_cast<RenderWidgetHostViewAura*>(render_widget_host_view);
DelegatedFrameHost* delegated_frame_host =
render_widget_host_view_aura->GetDelegatedFrameHost();
// Initially there should be no stale content set.
EXPECT_FALSE(
delegated_frame_host->stale_content_layer()->has_external_content());
EXPECT_EQ(delegated_frame_host->frame_eviction_state(),
DelegatedFrameHost::FrameEvictionState::kNotStarted);
// Hide the view and evict the frame, and expect that stale content will be
// set.
EvictionStateWaiter waiter{delegated_frame_host};
render_widget_host_view_aura->WasOccluded();
static_cast<viz::FrameEvictorClient*>(delegated_frame_host)
->EvictDelegatedFrame();
EXPECT_EQ(delegated_frame_host->frame_eviction_state(),
DelegatedFrameHost::FrameEvictionState::kPendingEvictionRequests);
// Wait until the stale frame content is copied and set onto the layer, i.e.
// the eviction state changes from kPendingEvictionRequests back to
// kNotStarted.
waiter.WaitForEvictionState(
DelegatedFrameHost::FrameEvictionState::kNotStarted);
EXPECT_TRUE(
delegated_frame_host->stale_content_layer()->has_external_content());
}
#endif // defined(USE_AURA) #endif // defined(USE_AURA)
ContextMenuFilter::ContextMenuFilter() ContextMenuFilter::ContextMenuFilter()
......
...@@ -1590,12 +1590,6 @@ class MockOverscrollController { ...@@ -1590,12 +1590,6 @@ class MockOverscrollController {
// Waits until the mock receives a consumed GestureScrollUpdate. // Waits until the mock receives a consumed GestureScrollUpdate.
virtual void WaitForConsumedScroll() = 0; virtual void WaitForConsumedScroll() = 0;
}; };
// Tests that a |render_widget_host_view| stores a stale content when its frame
// gets evicted. |render_widget_host_view| has to be a RenderWidgetHostViewAura.
void VerifyStaleContentOnFrameEviction(
RenderWidgetHostView* render_widget_host_view);
#endif // defined(USE_AURA) #endif // defined(USE_AURA)
// This class filters for FrameHostMsg_ContextMenu messages coming in // This class filters for FrameHostMsg_ContextMenu messages coming in
......
...@@ -454,10 +454,6 @@ void AppWindow::ExitPictureInPicture() { ...@@ -454,10 +454,6 @@ void AppWindow::ExitPictureInPicture() {
app_delegate_->ExitPictureInPicture(); app_delegate_->ExitPictureInPicture();
} }
bool AppWindow::ShouldShowStaleContentOnEviction(content::WebContents* source) {
return true;
}
bool AppWindow::OnMessageReceived(const IPC::Message& message, bool AppWindow::OnMessageReceived(const IPC::Message& message,
content::RenderFrameHost* render_frame_host) { content::RenderFrameHost* render_frame_host) {
bool handled = true; bool handled = true;
......
...@@ -451,7 +451,6 @@ class AppWindow : public content::WebContentsDelegate, ...@@ -451,7 +451,6 @@ class AppWindow : public content::WebContentsDelegate,
const viz::SurfaceId& surface_id, const viz::SurfaceId& surface_id,
const gfx::Size& natural_size) override; const gfx::Size& natural_size) override;
void ExitPictureInPicture() override; void ExitPictureInPicture() override;
bool ShouldShowStaleContentOnEviction(content::WebContents* source) override;
// content::WebContentsObserver implementation. // content::WebContentsObserver implementation.
bool OnMessageReceived(const IPC::Message& message, bool OnMessageReceived(const IPC::Message& message,
......
...@@ -66,18 +66,6 @@ IN_PROC_BROWSER_TEST_F(AppWindowBrowserTest, FrameInsetsForNoFrame) { ...@@ -66,18 +66,6 @@ IN_PROC_BROWSER_TEST_F(AppWindowBrowserTest, FrameInsetsForNoFrame) {
CloseAppWindow(app_window); CloseAppWindow(app_window);
} }
#if defined(OS_CHROMEOS)
IN_PROC_BROWSER_TEST_F(AppWindowBrowserTest, ShouldShowStaleContentOnEviction) {
AppWindow* app_window = CreateTestAppWindow("{}");
// Helper function as this test requires inspecting a number of content::
// internal objects.
content::VerifyStaleContentOnFrameEviction(
app_window->web_contents()->GetRenderWidgetHostView());
}
#endif // defined(OS_CHROMEOS)
} // namespace } // namespace
} // namespace extensions } // namespace extensions
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment