Minor cleanup of view manager . ViewManagerServiceImpl would previously delete itself or ConnectionManager would delete it. Now ConnectionManager always deletes it. . ViewManagerApp now terminates if either the nativew viewport is destroyed or connection to windowmanager errors out. BUG=none TEST=none R=ben@chromium.org Review URL: https://codereview.chromium.org/679213002
diff --git a/mojo/services/view_manager/BUILD.gn b/mojo/services/view_manager/BUILD.gn index a2f33f5..bc56282 100644 --- a/mojo/services/view_manager/BUILD.gn +++ b/mojo/services/view_manager/BUILD.gn
@@ -11,6 +11,7 @@ "access_policy_delegate.h", "connection_manager.cc", "connection_manager.h", + "connection_manager_delegate.h", "default_access_policy.cc", "default_access_policy.h", "display_manager.cc",
diff --git a/mojo/services/view_manager/connection_manager.cc b/mojo/services/view_manager/connection_manager.cc index 02b3eb7..253cc76 100644 --- a/mojo/services/view_manager/connection_manager.cc +++ b/mojo/services/view_manager/connection_manager.cc
@@ -5,9 +5,11 @@ #include "mojo/services/view_manager/connection_manager.h" #include "base/logging.h" +#include "base/stl_util.h" #include "mojo/converters/input_events/input_events_type_converters.h" #include "mojo/public/cpp/application/application_connection.h" #include "mojo/public/interfaces/application/service_provider.mojom.h" +#include "mojo/services/view_manager/connection_manager_delegate.h" #include "mojo/services/view_manager/view_manager_service_impl.h" namespace mojo { @@ -27,14 +29,20 @@ connection_manager_->FinishChange(); } -ConnectionManager::ConnectionManager( - ApplicationConnection* app_connection, - const Callback<void()>& native_viewport_closed_callback) +ConnectionManager::ConnectionManager(ApplicationConnection* app_connection, + ConnectionManagerDelegate* delegate) : app_connection_(app_connection), + delegate_(delegate), + window_manager_vm_service_(nullptr), next_connection_id_(1), - display_manager_(app_connection, this, native_viewport_closed_callback), + display_manager_( + app_connection, + this, + base::Bind(&ConnectionManagerDelegate::OnNativeViewportDestroyed, + base::Unretained(delegate))), root_(new ServerView(this, RootViewId())), - current_change_(NULL) { + current_change_(NULL), + in_destructor_(false) { app_connection->ConnectToService(&window_manager_); window_manager_.set_client(this); window_manager_.set_error_handler(this); @@ -45,8 +53,9 @@ } ConnectionManager::~ConnectionManager() { - while (!connections_created_by_connect_.empty()) - delete *(connections_created_by_connect_.begin()); + in_destructor_ = true; + + STLDeleteValues(&connection_map_); // All the connections should have been destroyed. DCHECK(connection_map_.empty()); root_.reset(); @@ -58,14 +67,10 @@ return id; } -void ConnectionManager::AddConnection(ViewManagerServiceImpl* connection) { - DCHECK_EQ(0u, connection_map_.count(connection->id())); - connection_map_[connection->id()] = connection; -} +void ConnectionManager::OnConnectionError(ViewManagerServiceImpl* connection) { + scoped_ptr<ViewManagerServiceImpl> connection_owner(connection); -void ConnectionManager::RemoveConnection(ViewManagerServiceImpl* connection) { connection_map_.erase(connection->id()); - connections_created_by_connect_.erase(connection); // Notify remaining connections so that they can cleanup. for (ConnectionMap::const_iterator i = connection_map_.begin(); @@ -73,6 +78,11 @@ ++i) { i->second->OnViewManagerServiceImplDestroyed(connection->id()); } + + if (connection == window_manager_vm_service_) { + window_manager_vm_service_ = nullptr; + delegate_->OnLostConnectionToWindowManager(); + } } void ConnectionManager::EmbedAtView( @@ -80,10 +90,29 @@ const String& url, Id transport_view_id, InterfaceRequest<ServiceProvider> service_provider) { - EmbedImpl(creator_id, - url, - ViewIdFromTransportId(transport_view_id), - service_provider.Pass())->set_delete_on_connection_error(); + MessagePipe pipe; + + ServiceProvider* view_manager_service_provider = + app_connection_->ConnectToApplication(url)->GetServiceProvider(); + + view_manager_service_provider->ConnectToService( + ViewManagerServiceImpl::Client::Name_, pipe.handle1.Pass()); + + std::string creator_url; + ConnectionMap::const_iterator it = connection_map_.find(creator_id); + if (it != connection_map_.end()) + creator_url = it->second->url(); + + ViewManagerServiceImpl* connection = + new ViewManagerServiceImpl(this, + creator_id, + creator_url, + url.To<std::string>(), + ViewIdFromTransportId(transport_view_id), + service_provider.Pass()); + AddConnection(connection); + WeakBindToPipe(connection, pipe.handle0.Pass()); + OnConnectionMessagedClient(connection->id()); } ViewManagerServiceImpl* ConnectionManager::GetConnection( @@ -190,54 +219,33 @@ current_change_ = NULL; } -ViewManagerServiceImpl* ConnectionManager::EmbedImpl( - const ConnectionSpecificId creator_id, - const String& url, - const ViewId& root_id, - InterfaceRequest<ServiceProvider> service_provider) { - MessagePipe pipe; - - ServiceProvider* view_manager_service_provider = - app_connection_->ConnectToApplication(url)->GetServiceProvider(); - - view_manager_service_provider->ConnectToService( - ViewManagerServiceImpl::Client::Name_, pipe.handle1.Pass()); - - std::string creator_url; - ConnectionMap::const_iterator it = connection_map_.find(creator_id); - if (it != connection_map_.end()) - creator_url = it->second->url(); - - ViewManagerServiceImpl* connection = - new ViewManagerServiceImpl(this, - creator_id, - creator_url, - url.To<std::string>(), - root_id, - service_provider.Pass()); - WeakBindToPipe(connection, pipe.handle0.Pass()); - connections_created_by_connect_.insert(connection); - OnConnectionMessagedClient(connection->id()); - return connection; +void ConnectionManager::AddConnection(ViewManagerServiceImpl* connection) { + DCHECK_EQ(0u, connection_map_.count(connection->id())); + connection_map_[connection->id()] = connection; } void ConnectionManager::OnViewDestroyed(const ServerView* view) { - ProcessViewDeleted(view->id()); + if (!in_destructor_) + ProcessViewDeleted(view->id()); } void ConnectionManager::OnWillChangeViewHierarchy( const ServerView* view, const ServerView* new_parent, const ServerView* old_parent) { - if (!display_manager_.in_setup()) + if (!in_destructor_ && !display_manager_.in_setup()) ProcessWillChangeViewHierarchy(view, new_parent, old_parent); } void ConnectionManager::OnViewHierarchyChanged(const ServerView* view, const ServerView* new_parent, const ServerView* old_parent) { + if (in_destructor_) + return; + if (!display_manager_.in_setup()) ProcessViewHierarchyChanged(view, new_parent, old_parent); + // TODO(beng): optimize. if (old_parent) { display_manager_.SchedulePaint(old_parent, @@ -252,6 +260,9 @@ void ConnectionManager::OnViewBoundsChanged(const ServerView* view, const gfx::Rect& old_bounds, const gfx::Rect& new_bounds) { + if (in_destructor_) + return; + ProcessViewBoundsChanged(view, old_bounds, new_bounds); if (!view->parent()) return; @@ -262,16 +273,21 @@ } void ConnectionManager::OnViewSurfaceIdChanged(const ServerView* view) { - display_manager_.SchedulePaint(view, gfx::Rect(view->bounds().size())); + if (!in_destructor_) + display_manager_.SchedulePaint(view, gfx::Rect(view->bounds().size())); } void ConnectionManager::OnViewReordered(const ServerView* view, const ServerView* relative, OrderDirection direction) { - display_manager_.SchedulePaint(view, gfx::Rect(view->bounds().size())); + if (!in_destructor_) + display_manager_.SchedulePaint(view, gfx::Rect(view->bounds().size())); } void ConnectionManager::OnWillChangeViewVisibility(const ServerView* view) { + if (in_destructor_) + return; + for (ConnectionMap::iterator i = connection_map_.begin(); i != connection_map_.end(); ++i) { @@ -304,19 +320,24 @@ void ConnectionManager::Create(ApplicationConnection* connection, InterfaceRequest<ViewManagerService> request) { - // TODO(sky): If we lose this connection we should tear down. - ViewManagerServiceImpl* service = + if (window_manager_vm_service_) { + VLOG(1) << "ViewManager interface requested more than once."; + return; + } + + window_manager_vm_service_ = new ViewManagerServiceImpl(this, kInvalidConnectionId, std::string(), std::string("mojo:window_manager"), RootViewId(), InterfaceRequest<ServiceProvider>()); - BindToRequest(service, &request); + AddConnection(window_manager_vm_service_); + WeakBindToRequest(window_manager_vm_service_, &request); } void ConnectionManager::OnConnectionError() { - // We've lost the connection to the WindowManager. + delegate_->OnLostConnectionToWindowManager(); } } // namespace service
diff --git a/mojo/services/view_manager/connection_manager.h b/mojo/services/view_manager/connection_manager.h index 01d273d..6f5edd8 100644 --- a/mojo/services/view_manager/connection_manager.h +++ b/mojo/services/view_manager/connection_manager.h
@@ -26,6 +26,7 @@ namespace service { +class ConnectionManagerDelegate; class ViewManagerServiceImpl; // ConnectionManager manages the set of connections to the ViewManager (all the @@ -70,14 +71,14 @@ }; ConnectionManager(ApplicationConnection* app_connection, - const Callback<void()>& native_viewport_closed_callback); + ConnectionManagerDelegate* delegate); ~ConnectionManager() override; // Returns the id for the next ViewManagerServiceImpl. ConnectionSpecificId GetAndAdvanceNextConnectionId(); - void AddConnection(ViewManagerServiceImpl* connection); - void RemoveConnection(ViewManagerServiceImpl* connection); + // Invoked when a ViewManagerServiceImpl's connection encounters an error. + void OnConnectionError(ViewManagerServiceImpl* connection); // See description of ViewManagerService::Embed() for details. This assumes // |transport_view_id| is valid. @@ -152,12 +153,8 @@ return current_change_ && current_change_->connection_id() == connection_id; } - // Implementation of the two embed variants. - ViewManagerServiceImpl* EmbedImpl( - ConnectionSpecificId creator_id, - const String& url, - const ViewId& root_id, - InterfaceRequest<ServiceProvider> service_provider); + // Adds |connection| to internal maps. + void AddConnection(ViewManagerServiceImpl* connection); // Overridden from ServerViewDelegate: void OnViewDestroyed(const ServerView* view) override; @@ -192,6 +189,13 @@ ApplicationConnection* app_connection_; + ConnectionManagerDelegate* delegate_; + + // The ViewManager implementation provided to the initial connection (the + // WindowManager). + // NOTE: |window_manager_vm_service_| is also in |connection_map_|. + ViewManagerServiceImpl* window_manager_vm_service_; + WindowManagerInternalServicePtr window_manager_; // ID to use for next ViewManagerServiceImpl. @@ -204,14 +208,12 @@ scoped_ptr<ServerView> root_; - // Set of ViewManagerServiceImpls created by way of Connect(). These have to - // be explicitly destroyed. - std::set<ViewManagerServiceImpl*> connections_created_by_connect_; - // If non-null we're processing a change. The ScopedChange is not owned by us // (it's created on the stack by ViewManagerServiceImpl). ScopedChange* current_change_; + bool in_destructor_; + DISALLOW_COPY_AND_ASSIGN(ConnectionManager); };
diff --git a/mojo/services/view_manager/connection_manager_delegate.h b/mojo/services/view_manager/connection_manager_delegate.h new file mode 100644 index 0000000..748f52f --- /dev/null +++ b/mojo/services/view_manager/connection_manager_delegate.h
@@ -0,0 +1,23 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef MOJO_SERVICES_VIEW_MANAGER_CONNECTION_MANAGER_DELEGATE_H_ +#define MOJO_SERVICES_VIEW_MANAGER_CONNECTION_MANAGER_DELEGATE_H_ + +namespace mojo { +namespace service { + +class ConnectionManagerDelegate { + public: + virtual void OnNativeViewportDestroyed() = 0; + virtual void OnLostConnectionToWindowManager() = 0; + + protected: + virtual ~ConnectionManagerDelegate() {} +}; + +} // namespace service +} // namespace mojo + +#endif // MOJO_SERVICES_VIEW_MANAGER_CONNECTION_MANAGER_DELEGATE_H_
diff --git a/mojo/services/view_manager/main.cc b/mojo/services/view_manager/main.cc index ad84d72..66d8195 100644 --- a/mojo/services/view_manager/main.cc +++ b/mojo/services/view_manager/main.cc
@@ -6,29 +6,33 @@ #include "mojo/public/c/system/main.h" #include "mojo/public/cpp/application/application_connection.h" #include "mojo/public/cpp/application/application_delegate.h" +#include "mojo/public/cpp/application/application_impl.h" #include "mojo/services/view_manager/connection_manager.h" +#include "mojo/services/view_manager/connection_manager_delegate.h" namespace mojo { namespace service { -class ViewManagerApp : public ApplicationDelegate { +class ViewManagerApp : public ApplicationDelegate, + public ConnectionManagerDelegate { public: ViewManagerApp() {} ~ViewManagerApp() override {} bool ConfigureIncomingConnection(ApplicationConnection* connection) override { if (!connection_manager_.get()) { - connection_manager_.reset(new ConnectionManager( - connection, - base::Bind(&ViewManagerApp::OnNativeViewportDeleted, - base::Unretained(this)))); + connection_manager_.reset(new ConnectionManager(connection, this)); + return true; } - return true; + VLOG(1) << "ViewManager allows only one connection."; + return false; } private: - void OnNativeViewportDeleted() { - // TODO(sky): Need to tear down here. + // ConnectionManagerDelegate: + void OnNativeViewportDestroyed() override { ApplicationImpl::Terminate(); } + void OnLostConnectionToWindowManager() override { + ApplicationImpl::Terminate(); } scoped_ptr<ConnectionManager> connection_manager_;
diff --git a/mojo/services/view_manager/view_manager_service_impl.cc b/mojo/services/view_manager/view_manager_service_impl.cc index 8bd9c91..588018b 100644 --- a/mojo/services/view_manager/view_manager_service_impl.cc +++ b/mojo/services/view_manager/view_manager_service_impl.cc
@@ -5,6 +5,7 @@ #include "mojo/services/view_manager/view_manager_service_impl.h" #include "base/bind.h" +#include "base/stl_util.h" #include "mojo/converters/geometry/geometry_type_converters.h" #include "mojo/converters/input_events/input_events_type_converters.h" #include "mojo/converters/surfaces/surfaces_type_converters.h" @@ -28,7 +29,6 @@ url_(url), creator_id_(creator_id), creator_url_(creator_url), - delete_on_connection_error_(false), service_provider_(service_provider.Pass()) { CHECK(GetView(root_id)); roots_.insert(ViewIdToTransportId(root_id)); @@ -39,14 +39,7 @@ } ViewManagerServiceImpl::~ViewManagerServiceImpl() { - // Delete any views we created. - if (!view_map_.empty()) { - ConnectionManager::ScopedChange change(this, connection_manager_, true); - while (!view_map_.empty()) - delete view_map_.begin()->second; - } - - connection_manager_->RemoveConnection(this); + DestroyViews(); } const ServerView* ViewManagerServiceImpl::GetView(const ViewId& id) const { @@ -201,8 +194,7 @@ } void ViewManagerServiceImpl::OnConnectionError() { - if (delete_on_connection_error_) - delete this; + connection_manager_->OnConnectionError(this); } bool ViewManagerServiceImpl::IsViewKnown(const ServerView* view) const { @@ -361,6 +353,18 @@ } } +void ViewManagerServiceImpl::DestroyViews() { + if (!view_map_.empty()) { + ConnectionManager::ScopedChange change(this, connection_manager_, true); + // If we get here from the destructor we're not going to get + // ProcessViewDeleted(). Copy the map and delete from the copy so that we + // don't have to worry about whether |view_map_| changes or not. + ViewMap view_map_copy; + view_map_.swap(view_map_copy); + STLDeleteValues(&view_map_copy); + } +} + void ViewManagerServiceImpl::CreateView( Id transport_view_id, const Callback<void(ErrorCode)>& callback) { @@ -542,8 +546,6 @@ } void ViewManagerServiceImpl::OnConnectionEstablished() { - connection_manager_->AddConnection(this); - std::vector<const ServerView*> to_send; for (ViewIdSet::const_iterator i = roots_.begin(); i != roots_.end(); ++i) GetUnknownViewsFrom(GetView(ViewIdFromTransportId(*i)), &to_send);
diff --git a/mojo/services/view_manager/view_manager_service_impl.h b/mojo/services/view_manager/view_manager_service_impl.h index ef9fc57..455bd0b 100644 --- a/mojo/services/view_manager/view_manager_service_impl.h +++ b/mojo/services/view_manager/view_manager_service_impl.h
@@ -49,10 +49,6 @@ InterfaceRequest<ServiceProvider> service_provider); ~ViewManagerServiceImpl() override; - // Used to mark this connection as originating from a call to - // ViewManagerService::Connect(). When set OnConnectionError() deletes |this|. - void set_delete_on_connection_error() { delete_on_connection_error_ = true; } - ConnectionSpecificId id() const { return id_; } ConnectionSpecificId creator_id() const { return creator_id_; } const std::string& url() const { return url_; } @@ -152,6 +148,9 @@ // |view| is the view that is changing to the drawn state |new_drawn_value|. void NotifyDrawnStateChanged(const ServerView* view, bool new_drawn_value); + // Deletes all Views we own. + void DestroyViews(); + // ViewManagerService: void CreateView(Id transport_view_id, const Callback<void(ErrorCode)>& callback) override; @@ -227,9 +226,6 @@ // connections. ViewIdSet roots_; - // See description above setter. - bool delete_on_connection_error_; - InterfaceRequest<ServiceProvider> service_provider_; DISALLOW_COPY_AND_ASSIGN(ViewManagerServiceImpl);