From 35c0b8cd879b1ac806d8869a81878c9fb47a7c17 Mon Sep 17 00:00:00 2001 From: sky Date: Thu, 28 May 2015 16:56:18 -0700 Subject: [PATCH] Changes ownership of ViewManager From the docs: Each call to OnEmbed() results in a new ViewManager and new root View. ViewManager is deleted by any of the following: . If the root of the connection is destroyed. This happens if the owner of the root Embed()s another app in root, or the owner explicitly deletes root. . The connection to the viewmanager is lost. . Explicitly by way of calling delete. When the ViewManager is deleted all views are deleted (and observers notified). This is followed by notifying the delegate by way of OnViewManagerDisconnected(). BUG=none TEST=none R=ben@chromium.org Review URL: https://codereview.chromium.org/1149133006 Cr-Commit-Position: refs/heads/master@{#331888} --- .../public/cpp/lib/view_manager_client_impl.cc | 28 ++++++-- .../public/cpp/lib/view_manager_client_impl.h | 4 +- components/view_manager/public/cpp/view.h | 9 ++- components/view_manager/public/cpp/view_manager.h | 5 +- .../public/cpp/view_manager_delegate.h | 16 ++++- .../view_manager/view_manager_client_apptest.cc | 76 +++++++++++++++++++++- 6 files changed, 120 insertions(+), 18 deletions(-) diff --git a/components/view_manager/public/cpp/lib/view_manager_client_impl.cc b/components/view_manager/public/cpp/lib/view_manager_client_impl.cc index 78a3c2838f00..1e0cd488adfa 100644 --- a/components/view_manager/public/cpp/lib/view_manager_client_impl.cc +++ b/components/view_manager/public/cpp/lib/view_manager_client_impl.cc @@ -70,19 +70,26 @@ View* BuildViewTree(ViewManagerClientImpl* client, // Responsible for removing a root from the ViewManager when that view is // destroyed. -class RootObserver : public ViewObserver { +class ViewManagerClientImpl::RootObserver : public ViewObserver { public: - explicit RootObserver(View* root) : root_(root) {} - ~RootObserver() override {} + explicit RootObserver(View* root) : root_(root) { + root_->AddObserver(this); + } + ~RootObserver() override { + if (root_) + root_->RemoveObserver(this); + } private: // Overridden from ViewObserver: void OnViewDestroyed(View* view) override { DCHECK_EQ(view, root_); - static_cast(root_->view_manager()) - ->RootDestroyed(root_); view->RemoveObserver(this); - delete this; + View* root = root_; + root_ = nullptr; + static_cast(root->view_manager()) + ->RootDestroyed(root); + // WARNING: we've been deleted. } View* root_; @@ -107,6 +114,10 @@ ViewManagerClientImpl::ViewManagerClientImpl( } ViewManagerClientImpl::~ViewManagerClientImpl() { + // Destroy RootObserver early on so that when we delete |root_| below we don't + // attempt to delete this again. + root_observer_.reset(); + std::vector non_owned; while (!views_.empty()) { IdToViewMap::iterator it = views_.begin(); @@ -117,6 +128,7 @@ ViewManagerClientImpl::~ViewManagerClientImpl() { views_.erase(it); } } + // Delete the non-owned views last. In the typical case these are roots. The // exception is the window manager, which may know aboutother random views // that it doesn't own. @@ -285,7 +297,7 @@ void ViewManagerClientImpl::OnEmbed(ConnectionSpecificId connection_id, DCHECK(!root_); root_ = AddViewToViewManager(this, nullptr, root_data); - root_->AddObserver(new RootObserver(root_)); + root_observer_.reset(new RootObserver(root_)); focused_view_ = GetViewById(focused_view_id); @@ -433,6 +445,8 @@ void ViewManagerClientImpl::OnConnectionError() { void ViewManagerClientImpl::RootDestroyed(View* root) { DCHECK_EQ(root, root_); root_ = nullptr; + // When the root is gone we can't do anything useful. + delete this; } void ViewManagerClientImpl::OnActionCompleted(bool success) { diff --git a/components/view_manager/public/cpp/lib/view_manager_client_impl.h b/components/view_manager/public/cpp/lib/view_manager_client_impl.h index 7eb2f97a8509..cb20100dcc14 100644 --- a/components/view_manager/public/cpp/lib/view_manager_client_impl.h +++ b/components/view_manager/public/cpp/lib/view_manager_client_impl.h @@ -74,7 +74,7 @@ class ViewManagerClientImpl : public ViewManager, void SetViewManagerService(ViewManagerServicePtr service); private: - friend class RootObserver; + class RootObserver; typedef std::map IdToViewMap; @@ -149,6 +149,8 @@ class ViewManagerClientImpl : public ViewManager, ViewManagerServicePtr service_; const bool delete_on_error_; + scoped_ptr root_observer_; + MOJO_DISALLOW_COPY_AND_ASSIGN(ViewManagerClientImpl); }; diff --git a/components/view_manager/public/cpp/view.h b/components/view_manager/public/cpp/view.h index 82c988b6c9de..d1c371fbcc95 100644 --- a/components/view_manager/public/cpp/view.h +++ b/components/view_manager/public/cpp/view.h @@ -29,7 +29,9 @@ class ViewObserver; template struct ViewProperty; -// Views are owned by the ViewManager. +// Views are owned by the ViewManager. See ViewManagerDelegate for details on +// ownership. +// // TODO(beng): Right now, you'll have to implement a ViewObserver to track // destruction and NULL any pointers you have. // Investigate some kind of smart pointer or weak pointer for these. @@ -38,7 +40,10 @@ class View { using Children = std::vector; using SharedProperties = std::map>; - // Destroys this view and all its children. + // Destroys this view and all its children. Destruction is allowed for views + // that were created by this connection. For views from other connections + // (such as the root) Destroy() does nothing. If the destruction is allowed + // observers are notified and the View is immediately deleted. void Destroy(); ViewManager* view_manager() { return manager_; } diff --git a/components/view_manager/public/cpp/view_manager.h b/components/view_manager/public/cpp/view_manager.h index ee0b563b0b13..f9132305411b 100644 --- a/components/view_manager/public/cpp/view_manager.h +++ b/components/view_manager/public/cpp/view_manager.h @@ -16,6 +16,8 @@ class View; // is made every time an app is embedded. class ViewManager { public: + virtual ~ViewManager() {} + // Returns the URL of the application that embedded this application. virtual const std::string& GetEmbedderURL() const = 0; @@ -32,9 +34,6 @@ class ViewManager { // Creates and returns a new View (which is owned by the ViewManager). Views // are initially hidden, use SetVisible(true) to show. virtual View* CreateView() = 0; - - protected: - virtual ~ViewManager() {} }; } // namespace mojo diff --git a/components/view_manager/public/cpp/view_manager_delegate.h b/components/view_manager/public/cpp/view_manager_delegate.h index be252ce60410..592122327acb 100644 --- a/components/view_manager/public/cpp/view_manager_delegate.h +++ b/components/view_manager/public/cpp/view_manager_delegate.h @@ -15,12 +15,22 @@ class View; class ViewManager; // Interface implemented by an application using the view manager. +// +// Each call to OnEmbed() results in a new ViewManager and new root View. +// ViewManager is deleted by any of the following: +// . If the root of the connection is destroyed. This happens if the owner +// of the root Embed()s another app in root, or the owner explicitly deletes +// root. +// . The connection to the viewmanager is lost. +// . Explicitly by way of calling delete. +// +// When the ViewManager is deleted all views are deleted (and observers +// notified). This is followed by notifying the delegate by way of +// OnViewManagerDisconnected(). class ViewManagerDelegate { public: // Called when the application implementing this interface is embedded at - // |root|. Every embed results in a new ViewManager and root View being - // created. |root| and it's corresponding ViewManager are valid until - // OnViewManagerDisconnected() is called with the same object. + // |root|. // // |services| exposes the services offered by the embedder to the delegate. // diff --git a/components/view_manager/view_manager_client_apptest.cc b/components/view_manager/view_manager_client_apptest.cc index db34f0ed0b10..18b11b23fb5c 100644 --- a/components/view_manager/view_manager_client_apptest.cc +++ b/components/view_manager/view_manager_client_apptest.cc @@ -183,7 +183,9 @@ class ViewManagerTest : public test::ApplicationTestBase, public ViewManagerDelegate { public: ViewManagerTest() - : most_recent_view_manager_(nullptr), window_manager_(nullptr) {} + : most_recent_view_manager_(nullptr), + window_manager_(nullptr), + got_disconnect_(false) {} // Overridden from ApplicationDelegate: void Initialize(ApplicationImpl* app) override { @@ -211,6 +213,8 @@ class ViewManagerTest : public test::ApplicationTestBase, return vm; } + bool got_disconnect() const { return got_disconnect_; } + ApplicationDelegate* GetApplicationDelegate() override { return this; } // Overridden from ViewManagerDelegate: @@ -220,7 +224,9 @@ class ViewManagerTest : public test::ApplicationTestBase, most_recent_view_manager_ = root->view_manager(); QuitRunLoop(); } - void OnViewManagerDisconnected(ViewManager* view_manager) override {} + void OnViewManagerDisconnected(ViewManager* view_manager) override { + got_disconnect_ = true; + } private: // Overridden from testing::Test: @@ -246,6 +252,8 @@ class ViewManagerTest : public test::ApplicationTestBase, // root view). ViewManager* window_manager_; + bool got_disconnect_; + MOJO_DISALLOW_COPY_AND_ASSIGN(ViewManagerTest); }; @@ -611,4 +619,68 @@ TEST_F(ViewManagerTest, Focus) { } } +namespace { + +class DestroyedChangedObserver : public ViewObserver { + public: + DestroyedChangedObserver(View* view, bool* got_destroy) + : view_(view), + got_destroy_(got_destroy) { + view_->AddObserver(this); + } + ~DestroyedChangedObserver() override { + if (view_) + view_->RemoveObserver(this); + } + + private: + // Overridden from ViewObserver: + void OnViewDestroyed(View* view) override { + EXPECT_EQ(view, view_); + view_->RemoveObserver(this); + *got_destroy_ = true; + view_ = nullptr; + } + + View* view_; + bool* got_destroy_; + + MOJO_DISALLOW_COPY_AND_ASSIGN(DestroyedChangedObserver); +}; + +} // namespace + +// Verifies deleting a ViewManager sends the right notifications. +TEST_F(ViewManagerTest, DeleteViewManager) { + View* view = window_manager()->CreateView(); + ASSERT_NE(nullptr, view); + view->SetVisible(true); + window_manager()->GetRoot()->AddChild(view); + ViewManager* view_manager = Embed(window_manager(), view); + ASSERT_TRUE(view_manager); + bool got_destroy = false; + DestroyedChangedObserver observer(view_manager->GetRoot(), &got_destroy); + delete view_manager; + EXPECT_TRUE(got_disconnect()); + EXPECT_TRUE(got_destroy); +} + +// Verifies two Embed()s in the same view trigger deletion of the first +// ViewManager. +TEST_F(ViewManagerTest, DisconnectTriggersDelete) { + View* view = window_manager()->CreateView(); + ASSERT_NE(nullptr, view); + view->SetVisible(true); + window_manager()->GetRoot()->AddChild(view); + ViewManager* view_manager = Embed(window_manager(), view); + View* embedded_view = view_manager->CreateView(); + // Embed again, this should trigger disconnect and deletion of view_manager. + bool got_destroy; + DestroyedChangedObserver observer(embedded_view, &got_destroy); + EXPECT_FALSE(got_disconnect()); + ViewManager* view_manager2 = Embed(window_manager(), view); + EXPECT_NE(view_manager, view_manager2); + EXPECT_TRUE(got_disconnect()); +} + } // namespace mojo -- 2.11.4.GIT