From c0c8a41eb335f085ba0a08d61013fba98e3df51b Mon Sep 17 00:00:00 2001 From: James Robinson Date: Wed, 21 Jan 2015 18:36:01 -0800 Subject: [PATCH] Remove [Client=] annotation from ServiceProvider This removes the symmetrical nature of ServiceProvider and consistently passes and uses a ServiceProvider + ServiceProvider& pair in places that wish to bidirectionally expose services, such as the view manager. The view manager library now deals with InterfaceRequest and ServiceProviderPtr objects (i.e. c++ wrappers for handles) instead of a concrete implementation of ServiceProvider to make it easier for callers. A number of places that were assuming a particular ServiceProvider would always exist are updated to reflect the nullability of the parameters in mojom and places that do not wish to ever look up or provide services now pass nullptr instead of doomed pipe handles. The JS application startup classes are reworked a bit to accomodate exposing services on the third ConnectToApplication/AcceptConnection parameter. BUG=449432 R=abarth@chromium.org, sky@chromium.org Review URL: https://codereview.chromium.org/858103002 --- engine/core/html/HTMLIFrameElement.cpp | 7 +++--- engine/core/html/HTMLIFrameElement.h | 2 +- engine/v8_inspector/inspector_backend_mojo.cc | 14 +++++------ services/inspector/server.cc | 14 ++++++----- tools/debugger/debugger.cc | 23 ++++++++----------- tools/debugger/debugger.h | 13 ++++++----- tools/tester/test_runner.cc | 9 ++++---- tools/tester/test_runner.h | 2 ++ tools/tester/tester.cc | 14 +++++------ viewer/document_view.cc | 14 ++++++----- viewer/document_view.h | 10 ++++---- viewer/internals.cc | 11 +++++---- 12 files changed, 68 insertions(+), 65 deletions(-) diff --git a/engine/core/html/HTMLIFrameElement.cpp b/engine/core/html/HTMLIFrameElement.cpp index 07a1cc25933..9fb3e0ba918 100644 --- a/engine/core/html/HTMLIFrameElement.cpp +++ b/engine/core/html/HTMLIFrameElement.cpp @@ -58,7 +58,7 @@ void HTMLIFrameElement::OnViewDestroyed(mojo::View* view) ScriptValue HTMLIFrameElement::takeServiceProvider(ScriptState* scriptState) { - return ScriptValue(scriptState, gin::ConvertToV8(scriptState->isolate(), m_serviceProvider.release())); + return ScriptValue(scriptState, gin::ConvertToV8(scriptState->isolate(), m_services.PassMessagePipe().release())); } void HTMLIFrameElement::createView() @@ -76,10 +76,9 @@ void HTMLIFrameElement::createView() if (!m_contentView) return; - mojo::MessagePipe pipe; - m_serviceProvider = pipe.handle0.Pass(); m_contentView->Embed(mojo::String::From(url.string().utf8().data()), - mojo::MakeRequest(pipe.handle1.Pass())); + GetProxy(&m_services), + nullptr); // TODO(abarth) Expose the exposedService parameter. m_contentView->AddObserver(this); } diff --git a/engine/core/html/HTMLIFrameElement.h b/engine/core/html/HTMLIFrameElement.h index 56ac0ab6b6d..a95654c2c6f 100644 --- a/engine/core/html/HTMLIFrameElement.h +++ b/engine/core/html/HTMLIFrameElement.h @@ -44,7 +44,7 @@ private: void createView(); mojo::View* m_contentView; - mojo::ScopedMessagePipeHandle m_serviceProvider; + mojo::ServiceProviderPtr m_services; }; } // namespace blink diff --git a/engine/v8_inspector/inspector_backend_mojo.cc b/engine/v8_inspector/inspector_backend_mojo.cc index c4349efdead..decf67018bd 100644 --- a/engine/v8_inspector/inspector_backend_mojo.cc +++ b/engine/v8_inspector/inspector_backend_mojo.cc @@ -106,13 +106,13 @@ InspectorBackendMojoImpl::~InspectorBackendMojoImpl() { void InspectorBackendMojoImpl::Connect() { mojo::Shell* shell = host_->GetShell(); - mojo::InterfaceRequest service_provider_request; - mojo::MessagePipe pipe; - service_provider_request.Bind(pipe.handle0.Pass()); - inspector_service_provider_.BindToHandle(pipe.handle1.Pass()); - shell->ConnectToApplication("mojo:sky_inspector_server", - service_provider_request.Pass(), nullptr); - mojo::ConnectToService(&inspector_service_provider_, &frontend_); + + mojo::ServiceProviderPtr services; + mojo::ServiceProviderPtr exposed_services; + inspector_service_provider_.Bind(GetProxy(&exposed_services)); + shell->ConnectToApplication("mojo:sky_inspector_server", GetProxy(&services), + exposed_services.Pass()); + mojo::ConnectToService(services.get(), &frontend_); // Theoretically we should load our state from the inspector cookie. inspector_state_ = diff --git a/services/inspector/server.cc b/services/inspector/server.cc index c5cd23e58bb..ac051e90e00 100644 --- a/services/inspector/server.cc +++ b/services/inspector/server.cc @@ -39,12 +39,14 @@ class Server : public mojo::ApplicationDelegate, mojo::ApplicationConnection* connection) override { connection->AddService(this); connection->AddService(this); - // The application connecting to us may implement InspectorBackend, - // attempt to establish a connection to find out. If it doesn't then this - // pipe will close. - InspectorBackendPtr backend; - connection->ConnectToService(&backend); - backends_.AddInterfacePtr(backend.Pass()); + if (connection->GetServiceProvider()) { + // The application connecting to us may implement InspectorBackend, + // attempt to establish a connection to find out. If it doesn't then this + // pipe will close. + InspectorBackendPtr backend; + connection->ConnectToService(&backend); + backends_.AddInterfacePtr(backend.Pass()); + } return true; } diff --git a/tools/debugger/debugger.cc b/tools/debugger/debugger.cc index 6eaf7fe9196..c540b15f937 100644 --- a/tools/debugger/debugger.cc +++ b/tools/debugger/debugger.cc @@ -15,6 +15,7 @@ SkyDebugger::SkyDebugger() content_(nullptr), navigator_host_factory_(this), weak_factory_(this) { + exposed_services_impl_.AddService(&navigator_host_factory_); } SkyDebugger::~SkyDebugger() { @@ -45,8 +46,8 @@ bool SkyDebugger::ConfigureOutgoingConnection( void SkyDebugger::OnEmbed( mojo::View* root, - mojo::ServiceProviderImpl* exported_services, - scoped_ptr imported_services) { + mojo::InterfaceRequest services, + mojo::ServiceProviderPtr exposed_services) { root_ = root; root_->AddObserver(this); @@ -64,13 +65,10 @@ void SkyDebugger::OnEmbed( NavigateToURL(pending_url_); } -void SkyDebugger::Embed( - const mojo::String& url, - mojo::InterfaceRequest service_provider) { - scoped_ptr exported_services( - new mojo::ServiceProviderImpl()); - // exported_services->AddService(TBD) -- no exported services for now. - content_->Embed(url, exported_services.Pass()); +void SkyDebugger::Embed(const mojo::String& url, + mojo::InterfaceRequest services, + mojo::ServiceProviderPtr exposed_services) { + content_->Embed(url, nullptr, nullptr); } void SkyDebugger::OnViewManagerDisconnected(mojo::ViewManager* view_manager) { @@ -97,10 +95,9 @@ void SkyDebugger::NavigateToURL(const mojo::String& url) { // embedded into the view and content_ created. // Just save the last one. if (content_) { - scoped_ptr exported_services( - new mojo::ServiceProviderImpl()); - exported_services->AddService(&navigator_host_factory_); - viewer_services_ = content_->Embed(url, exported_services.Pass()); + mojo::ServiceProviderPtr exposed_services; + exposed_services_impl_.Bind(GetProxy(&exposed_services)); + content_->Embed(url, GetProxy(&viewer_services_), exposed_services.Pass()); } else { pending_url_ = url; } diff --git a/tools/debugger/debugger.h b/tools/debugger/debugger.h index 9aa5b2faf67..d7412a257e9 100644 --- a/tools/debugger/debugger.h +++ b/tools/debugger/debugger.h @@ -48,8 +48,8 @@ class SkyDebugger : public mojo::ApplicationDelegate, // Overridden from mojo::ViewManagerDelegate: void OnEmbed(mojo::View* root, - mojo::ServiceProviderImpl* exported_services, - scoped_ptr imported_services) override; + mojo::InterfaceRequest services, + mojo::ServiceProviderPtr exposed_services) override; void OnViewManagerDisconnected(mojo::ViewManager* view_manager) override; // Overriden from mojo::ViewObserver: @@ -63,9 +63,9 @@ class SkyDebugger : public mojo::ApplicationDelegate, mojo::InterfaceRequest request) override; // Overridden from WindowManagerDelegate - void Embed( - const mojo::String& url, - mojo::InterfaceRequest service_provider) override; + void Embed(const mojo::String& url, + mojo::InterfaceRequest services, + mojo::ServiceProviderPtr exposed_services) override; scoped_ptr window_manager_app_; @@ -73,9 +73,10 @@ class SkyDebugger : public mojo::ApplicationDelegate, mojo::View* content_; std::string pending_url_; - scoped_ptr viewer_services_; + mojo::ServiceProviderPtr viewer_services_; NavigatorHostFactory navigator_host_factory_; + mojo::ServiceProviderImpl exposed_services_impl_; base::WeakPtrFactory weak_factory_; diff --git a/tools/tester/test_runner.cc b/tools/tester/test_runner.cc index b321badc303..e187cfa8e44 100644 --- a/tools/tester/test_runner.cc +++ b/tools/tester/test_runner.cc @@ -7,7 +7,6 @@ #include #include "base/bind.h" #include "mojo/public/cpp/application/connect.h" -#include "mojo/public/cpp/application/service_provider_impl.h" #include "mojo/services/view_manager/public/cpp/view.h" namespace sky { @@ -24,11 +23,11 @@ TestRunner::TestRunner(TestRunnerClient* client, mojo::View* container, enable_pixel_dumping_(enable_pixel_dumping) { CHECK(client); - scoped_ptr exported_services( - new mojo::ServiceProviderImpl()); - exported_services->AddService(&test_harness_factory_); + mojo::ServiceProviderPtr test_harness_provider; + test_harness_provider_impl_.AddService(&test_harness_factory_); + test_harness_provider_impl_.Bind(GetProxy(&test_harness_provider)); - container->Embed(url, exported_services.Pass()); + container->Embed(url, nullptr, test_harness_provider.Pass()); } TestRunner::~TestRunner() { diff --git a/tools/tester/test_runner.h b/tools/tester/test_runner.h index 7e2d45b9234..5a775355f6a 100644 --- a/tools/tester/test_runner.h +++ b/tools/tester/test_runner.h @@ -6,6 +6,7 @@ #define SKY_TOOLS_TESTER_TEST_RUNNER_H_ #include "base/memory/weak_ptr.h" +#include "mojo/public/cpp/application/service_provider_impl.h" #include "sky/tools/tester/test_harness_impl.h" namespace mojo{ @@ -40,6 +41,7 @@ class TestRunner { private: TestHarnessFactory test_harness_factory_; + mojo::ServiceProviderImpl test_harness_provider_impl_; TestRunnerClient* client_; base::WeakPtrFactory weak_ptr_factory_; bool enable_pixel_dumping_; diff --git a/tools/tester/tester.cc b/tools/tester/tester.cc index 635bc6b1a53..d46c84f1bfa 100644 --- a/tools/tester/tester.cc +++ b/tools/tester/tester.cc @@ -83,10 +83,9 @@ class SkyTester : public mojo::ApplicationDelegate, } // Overridden from mojo::ViewManagerDelegate: - virtual void OnEmbed( - mojo::View* root, - mojo::ServiceProviderImpl* exported_services, - scoped_ptr remote_service_provider) override { + virtual void OnEmbed(mojo::View* root, + mojo::InterfaceRequest services, + mojo::ServiceProviderPtr exposed_services) override { root_ = root; root_->AddObserver(this); @@ -101,10 +100,9 @@ class SkyTester : public mojo::ApplicationDelegate, } // Overridden from window_manager::WindowManagerDelegate: - virtual void Embed( - const mojo::String& url, - mojo::InterfaceRequest service_provider) override { - } + virtual void Embed(const mojo::String& url, + mojo::InterfaceRequest services, + mojo::ServiceProviderPtr exposed_services) override {} virtual void OnViewManagerDisconnected( mojo::ViewManager* view_manager) override { diff --git a/viewer/document_view.cc b/viewer/document_view.cc index 71e672694b7..cf48567326b 100644 --- a/viewer/document_view.cc +++ b/viewer/document_view.cc @@ -81,6 +81,7 @@ DocumentView::DocumentView( mojo::URLResponsePtr response, mojo::Shell* shell) : response_(response.Pass()), + exported_services_(services.Pass()), shell_(shell), web_view_(nullptr), root_(nullptr), @@ -89,9 +90,8 @@ DocumentView::DocumentView( bitmap_rasterizer_(nullptr), debugger_id_(s_next_debugger_id++), weak_factory_(this) { - // TODO(jamesr): Is this right? exported_services_.AddService(&view_manager_client_factory_); - mojo::WeakBindToPipe(&exported_services_, services.PassMessagePipe()); + inspector_service_provider_impl_.AddService(&inspector_service_factory_); } DocumentView::~DocumentView() { @@ -107,12 +107,14 @@ base::WeakPtr DocumentView::GetWeakPtr() { void DocumentView::OnEmbed( mojo::View* root, - mojo::ServiceProviderImpl* exported_services, - scoped_ptr imported_services) { + mojo::InterfaceRequest services, + mojo::ServiceProviderPtr exposed_services) { root_ = root; - imported_services_ = imported_services.Pass(); + imported_services_ = exposed_services.Pass(); navigator_host_.set_service_provider(imported_services_.get()); - exported_services->AddService(&inspector_service_factory_); + + if (services.is_pending()) + inspector_service_provider_impl_.Bind(services.Pass()); Load(response_.Pass()); diff --git a/viewer/document_view.h b/viewer/document_view.h index 05509bcead6..57bf30ba5d2 100644 --- a/viewer/document_view.h +++ b/viewer/document_view.h @@ -103,10 +103,9 @@ class DocumentView : public blink::ServiceProvider, mojo::Shell* Shell() override; // ViewManagerDelegate methods: - void OnEmbed( - mojo::View* root, - mojo::ServiceProviderImpl* exported_services, - scoped_ptr imported_services) override; + void OnEmbed(mojo::View* root, + mojo::InterfaceRequest services, + mojo::ServiceProviderPtr exposed_services) override; void OnViewManagerDisconnected(mojo::ViewManager* view_manager) override; // ViewObserver methods: @@ -124,13 +123,14 @@ class DocumentView : public blink::ServiceProvider, mojo::URLResponsePtr response_; mojo::ServiceProviderImpl exported_services_; - scoped_ptr imported_services_; + mojo::ServiceProviderPtr imported_services_; mojo::Shell* shell_; mojo::LazyInterfacePtr navigator_host_; blink::WebView* web_view_; mojo::View* root_; mojo::ViewManagerClientFactory view_manager_client_factory_; InspectorServiceFactory inspector_service_factory_; + mojo::ServiceProviderImpl inspector_service_provider_impl_; scoped_ptr layer_host_; scoped_refptr root_layer_; RasterizerBitmap* bitmap_rasterizer_; // Used for pixel tests. diff --git a/viewer/internals.cc b/viewer/internals.cc index 01cf5db3bf5..09db9802b52 100644 --- a/viewer/internals.cc +++ b/viewer/internals.cc @@ -40,7 +40,8 @@ gin::Handle Internals::Create( Internals::Internals(DocumentView* document_view) : document_view_(document_view->GetWeakPtr()), shell_binding_(this) { - mojo::ConnectToService(document_view->imported_services(), &test_harness_); + if (document_view_->imported_services()) + mojo::ConnectToService(document_view_->imported_services(), &test_harness_); } Internals::~Internals() { @@ -77,13 +78,15 @@ void Internals::NotifyTestComplete(const std::string& test_result) { return; std::vector pixels; document_view_->GetPixelsForTesting(&pixels); - test_harness_->OnTestComplete(test_result, - mojo::Array::From(pixels)); + if (test_harness_) { + test_harness_->OnTestComplete(test_result, + mojo::Array::From(pixels)); + } } mojo::Handle Internals::ConnectToEmbedderService( const std::string& interface_name) { - if (!document_view_) + if (!document_view_ || !document_view_->imported_services()) return mojo::Handle(); mojo::MessagePipe pipe;