diff --git a/libcef/browser/views/overlay_view_host.cc b/libcef/browser/views/overlay_view_host.cc index 080d0ac5f..647b3bc80 100644 --- a/libcef/browser/views/overlay_view_host.cc +++ b/libcef/browser/views/overlay_view_host.cc @@ -31,7 +31,8 @@ class CefOverlayControllerImpl : public CefOverlayController { } bool IsSame(CefRefPtr that) override { - return that && that->GetContentsView()->IsSame(view_); + return IsValid() && that && that->IsValid() && + that->GetContentsView()->IsSame(view_); } CefRefPtr GetContentsView() override { return view_; } @@ -52,11 +53,17 @@ class CefOverlayControllerImpl : public CefOverlayController { void Destroy() override { if (IsValid()) { - host_->Destroy(); - view_ = nullptr; + // Results in a call to Destroyed(). + host_->Close(); + host_ = nullptr; } } + void Destroyed() { + DCHECK(view_); + view_ = nullptr; + } + void SetBounds(const CefRect& bounds) override { if (IsValid() && host_->docking_mode() == CEF_DOCKING_MODE_CUSTOM) { host_->SetOverlayBounds( @@ -155,7 +162,7 @@ class CefOverlayControllerImpl : public CefOverlayController { bool IsDrawn() override { return IsVisible(); } private: - CefOverlayViewHost* const host_; + CefOverlayViewHost* host_; CefRefPtr view_; IMPLEMENT_REFCOUNTING(CefOverlayControllerImpl); @@ -180,7 +187,8 @@ void CefOverlayViewHost::Init(views::View* host_view, cef_controller_ = new CefOverlayControllerImpl(this, view); - // Initialize the Widget. + // Initialize the Widget. |widget_| will be deleted by the NativeWidget or + // when WidgetDelegate::DeleteDelegate() deletes |this|. widget_ = std::make_unique(window_view_->GetWidget()); views::Widget::InitParams params(views::Widget::InitParams::TYPE_CONTROL); params.delegate = this; @@ -193,13 +201,23 @@ void CefOverlayViewHost::Init(views::View* host_view, : views::Widget::InitParams::Activatable::kNo; widget_->Init(std::move(params)); + // |widget_| should now be associated with |this|. + DCHECK_EQ(widget_.get(), GetWidget()); + // Make the Widget background transparent. The View might still be opaque. if (widget_->GetCompositor()) { widget_->GetCompositor()->SetBackgroundColor(SK_ColorTRANSPARENT); } + host_view_ = host_view; view_util::SetHostView(widget_.get(), host_view); + // Cause WidgetDelegate::DeleteDelegate() to delete |this| after executing the + // registered DeleteDelegate callback. + SetOwnedByWidget(true); + RegisterDeleteDelegateCallback( + base::BindOnce(&CefOverlayViewHost::Cleanup, base::Unretained(this))); + if (cef::IsChromeRuntimeEnabled()) { // Some attributes associated with a Chrome toolbar are located via the // Widget. See matching logic in BrowserView::AddedToWidget. @@ -239,15 +257,12 @@ void CefOverlayViewHost::Init(views::View* host_view, widget_->Hide(); } -void CefOverlayViewHost::Destroy() { +void CefOverlayViewHost::Close() { if (widget_ && !widget_->IsClosed()) { - // Remove the child View immediately. It may be reused by the client. - auto view = view_util::GetFor(view_, /*find_known_parent=*/false); - widget_->GetContentsView()->RemoveChildView(view_); - if (view) { - view_util::ResumeOwnership(view); - } + // Remove all references ASAP, before the Widget is destroyed. + Cleanup(); + // Eventually calls DeleteDelegate(). widget_->Close(); } } @@ -333,3 +348,37 @@ gfx::Rect CefOverlayViewHost::ComputeBounds() const { return gfx::Rect(x, y, prefsize.width(), prefsize.height()); } + +void CefOverlayViewHost::Cleanup() { + // This method may be called multiple times. For example, explicitly after the + // client calls CefOverlayController::Destroy or implicitly when the host + // Widget is being closed or destroyed. In most implicit cases + // CefWindowView::WindowClosing will call this before the host Widget is + // destroyed, allowing the client to optionally reuse the child View. However, + // if CefWindowView::WindowClosing is not called, DeleteDelegate will call + // this after the host Widget and all associated Widgets/Views have been + // destroyed. In the DeleteDelegate case |widget_| will return nullptr. + if (view_ && widget_) { + // Remove the child View immediately. It may be reused by the client. + auto view = view_util::GetFor(view_, /*find_known_parent=*/false); + widget_->GetContentsView()->RemoveChildView(view_); + if (view) { + view_util::ResumeOwnership(view); + } + view_->RemoveObserver(this); + view_ = nullptr; + } + + if (cef_controller_) { + CefOverlayControllerImpl* controller_impl = + static_cast(cef_controller_.get()); + controller_impl->Destroyed(); + cef_controller_ = nullptr; + } + + if (window_view_) { + window_view_->RemoveOverlayView(this, host_view_); + window_view_ = nullptr; + host_view_ = nullptr; + } +} diff --git a/libcef/browser/views/overlay_view_host.h b/libcef/browser/views/overlay_view_host.h index 2b80aae48..a55686b19 100644 --- a/libcef/browser/views/overlay_view_host.h +++ b/libcef/browser/views/overlay_view_host.h @@ -12,6 +12,7 @@ #include "include/views/cef_view.h" #include "ui/views/view_observer.h" +#include "ui/views/widget/unique_widget_ptr.h" #include "ui/views/widget/widget_delegate.h" class CefWindowView; @@ -34,7 +35,7 @@ class CefOverlayViewHost : public views::WidgetDelegate, // relative to views with layers and views with associated NativeViews. void Init(views::View* host_view, CefRefPtr view, bool can_activate); - void Destroy(); + void Close(); void MoveIfNecessary(); @@ -55,17 +56,22 @@ class CefOverlayViewHost : public views::WidgetDelegate, private: gfx::Rect ComputeBounds() const; + void Cleanup(); + // The CefWindowView that created us. - CefWindowView* const window_view_; + CefWindowView* window_view_; const cef_docking_mode_t docking_mode_; + // The host view that the overlay is positioned relative to. + views::View* host_view_ = nullptr; + // Our view, which is responsible for drawing the UI. views::View* view_ = nullptr; // The Widget implementation that is created and maintained by the overlay. // It contains |view_|. - std::unique_ptr widget_; + views::UniqueWidgetPtr widget_; CefRefPtr cef_controller_; diff --git a/libcef/browser/views/window_view.cc b/libcef/browser/views/window_view.cc index 83ff6514e..525db95ab 100644 --- a/libcef/browser/views/window_view.cc +++ b/libcef/browser/views/window_view.cc @@ -19,6 +19,7 @@ #include "libcef/browser/views/widget.h" #include "libcef/browser/views/window_impl.h" +#include "base/ranges/algorithm.h" #include "ui/base/hit_test.h" #include "ui/display/screen.h" #include "ui/views/widget/widget.h" @@ -397,8 +398,8 @@ void CefWindowView::CreateWidget(gfx::AcceleratedWidget parent_widget) { params.type = views::Widget::InitParams::TYPE_WINDOW; } - // WidgetDelegate::DeleteDelegate() will delete |this| after executing the - // registered callback. + // Cause WidgetDelegate::DeleteDelegate() to delete |this| after executing the + // registered DeleteDelegate callback. SetOwnedByWidget(true); RegisterDeleteDelegateCallback( base::BindOnce(&CefWindowView::DeleteDelegate, base::Unretained(this))); @@ -586,6 +587,9 @@ CefRefPtr CefWindowView::GetCefWindow() const { } void CefWindowView::DeleteDelegate() { + // Any overlays should already be removed. + DCHECK(overlay_hosts_.empty()); + // Remove all child Views before deleting the Window so that notifications // resolve correctly. RemoveAllChildViews(); @@ -632,6 +636,14 @@ ui::ImageModel CefWindowView::GetWindowAppIcon() { } void CefWindowView::WindowClosing() { + // Close any overlays now, before the Widget is destroyed. + // Use a copy of the array because the original may be modified while + // iterating. + std::vector overlay_hosts = overlay_hosts_; + for (auto* overlay_host : overlay_hosts) { + overlay_host->Close(); + } + #if BUILDFLAG(IS_LINUX) #if BUILDFLAG(IS_OZONE_X11) if (host_widget()) { @@ -647,6 +659,8 @@ void CefWindowView::WindowClosing() { #endif window_delegate_->OnWindowClosing(); + + views::WidgetDelegateView::WindowClosing(); } views::View* CefWindowView::GetContentsView() { @@ -754,7 +768,11 @@ void CefWindowView::OnWidgetActivationChanged(views::Widget* widget, void CefWindowView::OnWidgetBoundsChanged(views::Widget* widget, const gfx::Rect& new_bounds) { - MoveOverlaysIfNecessary(); + // Size is set to zero when the host Widget is hidden. We don't need to move + // overlays in that case. + if (!new_bounds.IsEmpty()) { + MoveOverlaysIfNecessary(); + } if (cef_delegate()) { cef_delegate()->OnWindowBoundsChanged( @@ -816,10 +834,10 @@ CefRefPtr CefWindowView::AddOverlayView( // Owned by the View hierarchy. Acts as a z-order reference for the overlay. auto overlay_host_view = AddChildView(std::make_unique()); - overlay_hosts_.push_back( - std::make_unique(this, docking_mode)); + // Owned by the resulting Widget, after calling Init(). + auto* overlay_host = new CefOverlayViewHost(this, docking_mode); + overlay_hosts_.push_back(overlay_host); - auto& overlay_host = overlay_hosts_.back(); overlay_host->Init(overlay_host_view, view, can_activate); return overlay_host->controller(); @@ -828,6 +846,18 @@ CefRefPtr CefWindowView::AddOverlayView( return nullptr; } +void CefWindowView::RemoveOverlayView(CefOverlayViewHost* host, + views::View* host_view) { + DCHECK_EQ(host_view->parent(), this); + RemoveChildView(host_view); + + const auto it = base::ranges::find_if( + overlay_hosts_, + [host](CefOverlayViewHost* current) { return current == host; }); + DCHECK(it != overlay_hosts_.end()); + overlay_hosts_.erase(it); +} + void CefWindowView::MoveOverlaysIfNecessary() { if (overlay_hosts_.empty()) { return; diff --git a/libcef/browser/views/window_view.h b/libcef/browser/views/window_view.h index bd22cf35c..5b373b0f5 100644 --- a/libcef/browser/views/window_view.h +++ b/libcef/browser/views/window_view.h @@ -110,6 +110,9 @@ class CefWindowView cef_docking_mode_t docking_mode, bool can_activate); + // Called from CefOverlayViewHost::Cleanup(). + void RemoveOverlayView(CefOverlayViewHost* host, views::View* host_view); + // Set/get the draggable regions. void SetDraggableRegions(const std::vector& regions); SkRegion* draggable_region() const { return draggable_region_.get(); } @@ -137,7 +140,7 @@ class CefWindowView views::Widget* host_widget() const; private: - // Called when removed from the Widget and before |this| is deleted. + // Called after Widget teardown starts, before |this| is deleted. void DeleteDelegate(); void MoveOverlaysIfNecessary(); @@ -165,7 +168,7 @@ class CefWindowView std::unique_ptr host_widget_destruction_observer_; // Hosts for overlay widgets. - std::vector> overlay_hosts_; + std::vector overlay_hosts_; }; #endif // CEF_LIBCEF_BROWSER_VIEWS_WINDOW_VIEW_H_ diff --git a/tests/ceftests/views/test_window_delegate.cc b/tests/ceftests/views/test_window_delegate.cc index e0d101606..862c1c938 100644 --- a/tests/ceftests/views/test_window_delegate.cc +++ b/tests/ceftests/views/test_window_delegate.cc @@ -35,7 +35,8 @@ const int TestWindowDelegate::kWSize = 400; // static void TestWindowDelegate::RunTest(CefRefPtr event, - std::unique_ptr config) { + std::unique_ptr config, + TestWindowDelegateFactory factory) { CefSize window_size{config->window_size, config->window_size}; if (!config->frameless) { @@ -64,8 +65,15 @@ void TestWindowDelegate::RunTest(CefRefPtr event, #endif } - CefWindow::CreateTopLevelWindow( - new TestWindowDelegate(event, std::move(config), window_size)); + TestWindowDelegate* delegate; + if (!factory.is_null()) { + delegate = std::move(factory).Run(event, std::move(config), window_size); + CHECK(delegate); + } else { + delegate = new TestWindowDelegate(event, std::move(config), window_size); + } + + CefWindow::CreateTopLevelWindow(delegate); } void TestWindowDelegate::OnWindowCreated(CefRefPtr window) { diff --git a/tests/ceftests/views/test_window_delegate.h b/tests/ceftests/views/test_window_delegate.h index 1fba5673d..3d497a3a7 100644 --- a/tests/ceftests/views/test_window_delegate.h +++ b/tests/ceftests/views/test_window_delegate.h @@ -5,6 +5,7 @@ #include #include "include/base/cef_callback.h" +#include "include/base/cef_callback_helpers.h" #include "include/base/cef_weak_ptr.h" #include "include/cef_waitable_event.h" #include "include/views/cef_window.h" @@ -41,6 +42,11 @@ class TestWindowDelegate : public CefWindowDelegate { cef_show_state_t initial_show_state = CEF_SHOW_STATE_NORMAL; }; + using TestWindowDelegateFactory = + base::OnceCallback event, + std::unique_ptr config, + const CefSize& window_size)>; + // Creates a Window with a new TestWindowDelegate instance and executes // |window_test| after the Window is created. |event| will be signaled once // the Window is closed. If |frameless| is true the Window will be created @@ -48,7 +54,8 @@ class TestWindowDelegate : public CefWindowDelegate { // immediately after |window_test| returns. Otherwise, the caller is // responsible for closing the Window passed to |window_test|. static void RunTest(CefRefPtr event, - std::unique_ptr config); + std::unique_ptr config, + TestWindowDelegateFactory factory = base::NullCallback()); // CefWindowDelegate methods: void OnWindowCreated(CefRefPtr window) override; @@ -63,12 +70,16 @@ class TestWindowDelegate : public CefWindowDelegate { bool OnKeyEvent(CefRefPtr window, const CefKeyEvent& event) override; - private: + protected: TestWindowDelegate(CefRefPtr event, std::unique_ptr config, const CefSize& window_size); ~TestWindowDelegate() override; + Config* config() const { return config_.get(); } + CefRefPtr window() const { return window_; } + + private: void OnCloseWindow(); void OnTimeoutWindow(); diff --git a/tests/ceftests/views/window_unittest.cc b/tests/ceftests/views/window_unittest.cc index fb4696ff2..d2dbac857 100644 --- a/tests/ceftests/views/window_unittest.cc +++ b/tests/ceftests/views/window_unittest.cc @@ -620,3 +620,173 @@ WINDOW_TEST_ASYNC(WindowFullscreenFrameless) WINDOW_TEST_ASYNC(WindowIcon) WINDOW_TEST_ASYNC(WindowIconFrameless) WINDOW_TEST_ASYNC(WindowAccelerator) + +namespace { + +enum class OverlayTestMode { + // Destroy the overlay after the Window is destroyed. + kDestroyAfterWindowDestroyImplicit, + kDestroyAfterWindowDestroyExplicit, + + // Destroy the overlay explicitly before the Window is shown. + kDestroyBeforeWindowShow, + kDestroyBeforeWindowShowAndAddAgain, + + // Destroy the overlay explicitly after the Window is shown. + kDestroyAfterWindowShow, + kDestroyAfterWindowShowAndAddAgain, +}; + +class OverlayTestWindowDelegate : public TestWindowDelegate { + public: + static TestWindowDelegate* Factory(OverlayTestMode test_mode, + CefRefPtr event, + std::unique_ptr config, + const CefSize& window_size) { + return new OverlayTestWindowDelegate(test_mode, event, std::move(config), + window_size); + } + + private: + OverlayTestWindowDelegate(OverlayTestMode test_mode, + CefRefPtr event, + std::unique_ptr config, + const CefSize& window_size) + : TestWindowDelegate(event, std::move(config), window_size), + test_mode_(test_mode) { + this->config()->on_window_created = base::BindOnce( + &OverlayTestWindowDelegate::RunWindowCreated, base::Unretained(this)); + this->config()->on_window_destroyed = base::BindOnce( + &OverlayTestWindowDelegate::RunWindowDestroyed, base::Unretained(this)); + } + + bool DestroyBeforeShow() const { + return test_mode_ == OverlayTestMode::kDestroyBeforeWindowShow || + test_mode_ == OverlayTestMode::kDestroyBeforeWindowShowAndAddAgain; + } + + bool DestroyAfterShow() const { + return test_mode_ == OverlayTestMode::kDestroyAfterWindowShow || + test_mode_ == OverlayTestMode::kDestroyAfterWindowShowAndAddAgain; + } + + bool AddAgain() const { + return test_mode_ == OverlayTestMode::kDestroyBeforeWindowShowAndAddAgain || + test_mode_ == OverlayTestMode::kDestroyAfterWindowShowAndAddAgain; + } + + void RunWindowCreated(CefRefPtr window) { + CreateOverlay(); + + if (DestroyBeforeShow()) { + DestroyOverlay(); + } + + window->Show(); + + if (DestroyAfterShow()) { + DestroyOverlay(); + } + } + + void RunWindowDestroyed(CefRefPtr window) { + if (test_mode_ == OverlayTestMode::kDestroyAfterWindowDestroyExplicit) { + DestroyOverlay(); + } + } + + void CreateOverlay() { + // |view_| may be reused. + if (!view_) { + view_ = CefPanel::CreatePanel(nullptr); + } + + // View is visible but not drawn. + EXPECT_EQ(nullptr, view_->GetWindow()); + EXPECT_TRUE(view_->IsVisible()); + EXPECT_FALSE(view_->IsDrawn()); + + EXPECT_FALSE(controller_); + controller_ = window()->AddOverlayView(view_, CEF_DOCKING_MODE_TOP_LEFT, + /*can_activate=*/false); + + // View is visible/drawn (because it belongs to the controller), but the + // controller itself is not. + EXPECT_FALSE(controller_->IsVisible()); + EXPECT_FALSE(controller_->IsDrawn()); + EXPECT_TRUE(window()->IsSame(view_->GetWindow())); + EXPECT_TRUE(view_->IsVisible()); + EXPECT_TRUE(view_->IsDrawn()); + + controller_->SetVisible(true); + + EXPECT_TRUE(controller_->IsValid()); + EXPECT_TRUE(controller_->GetContentsView()->IsSame(view_)); + EXPECT_TRUE(controller_->GetWindow()->IsSame(window())); + EXPECT_EQ(CEF_DOCKING_MODE_TOP_LEFT, controller_->GetDockingMode()); + + // Controller is visible/drawn if the host window is drawn. + if (window()->IsDrawn()) { + EXPECT_TRUE(controller_->IsVisible()); + EXPECT_TRUE(controller_->IsDrawn()); + } else { + EXPECT_FALSE(controller_->IsVisible()); + EXPECT_FALSE(controller_->IsDrawn()); + } + + EXPECT_TRUE(view_->IsVisible()); + EXPECT_TRUE(view_->IsDrawn()); + } + + void DestroyOverlay() { + // Disassociates the controller from the view and host window. + controller_->Destroy(); + + EXPECT_FALSE(controller_->IsValid()); + EXPECT_EQ(nullptr, controller_->GetContentsView()); + EXPECT_EQ(nullptr, controller_->GetWindow()); + EXPECT_FALSE(controller_->IsVisible()); + EXPECT_FALSE(controller_->IsDrawn()); + + // View is still visible but no longer drawn (because it no longer belongs + // to the controller). + EXPECT_EQ(nullptr, view_->GetWindow()); + EXPECT_TRUE(view_->IsVisible()); + EXPECT_FALSE(view_->IsDrawn()); + + controller_ = nullptr; + + if (AddAgain()) { + CreateOverlay(); + } + } + + OverlayTestMode const test_mode_; + CefRefPtr view_; + CefRefPtr controller_; +}; + +void WindowOverlay(OverlayTestMode test_mode, + CefRefPtr event) { + auto config = std::make_unique(); + TestWindowDelegate::RunTest( + event, std::move(config), + base::BindOnce(&OverlayTestWindowDelegate::Factory, test_mode)); +} + +} // namespace + +#define WINDOW_OVERLAY_TEST(name) \ + namespace { \ + void WindowOverlay##name##Impl(CefRefPtr event) { \ + WindowOverlay(OverlayTestMode::k##name, event); \ + } \ + } \ + WINDOW_TEST_ASYNC(WindowOverlay##name) + +WINDOW_OVERLAY_TEST(DestroyAfterWindowDestroyImplicit) +WINDOW_OVERLAY_TEST(DestroyAfterWindowDestroyExplicit) +WINDOW_OVERLAY_TEST(DestroyBeforeWindowShow) +WINDOW_OVERLAY_TEST(DestroyBeforeWindowShowAndAddAgain) +WINDOW_OVERLAY_TEST(DestroyAfterWindowShow) +WINDOW_OVERLAY_TEST(DestroyAfterWindowShowAndAddAgain)