From 0ebf9a924d744ef98948ecb4f5d9e9a6fd3b42bf Mon Sep 17 00:00:00 2001 From: esprehn Date: Mon, 21 Mar 2016 17:41:05 -0700 Subject: Revert of Quit the message loop by default in ShellConnectionLost when ApplicationRunner is used (patchset #2 id:20001 of https://codereview.chromium.org/1819063002/ ) Reason for revert: Looks like to made the mojo_apptests go red. I think this is also making MUS tests fail? https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/39128 Original issue's description: > Quit the message loop by default in ShellConnectionLost when ApplicationRunner is used > > This was originally at https://codereview.chromium.org/1814223002/ , but Ben is out and > I need this for tests, so I'm taking over. > > BUG=none > TEST=covered by tests > TBR=ben@chromium.org > R=ben@chromium.org > > Committed: https://crrev.com/a50f9840749052fbdec087a304548217cc6fd00b > Cr-Commit-Position: refs/heads/master@{#382389} TBR=ben@chromium.org,sky@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=none Review URL: https://codereview.chromium.org/1821913002 Cr-Commit-Position: refs/heads/master@{#382460} --- mojo/services/network/network_service_delegate.cc | 4 ++++ mojo/services/network/network_service_delegate.h | 1 + mojo/services/tracing/tracing_app.cc | 6 +----- mojo/services/tracing/tracing_app.h | 2 +- mojo/shell/background/tests/test_service.cc | 4 ++-- mojo/shell/public/cpp/lib/application_runner.cc | 6 +----- mojo/shell/public/cpp/lib/shell_client.cc | 2 +- mojo/shell/public/cpp/lib/shell_connection.cc | 3 +-- mojo/shell/public/cpp/shell_client.h | 9 ++------- mojo/shell/public/cpp/shell_connection.h | 9 +-------- mojo/shell/tests/connect/connect_test_app.cc | 6 ++++++ mojo/shell/tests/connect/connect_test_driver.cc | 5 ++--- mojo/shell/tests/connect/connect_test_package.cc | 6 ++++++ mojo/shell/tests/lifecycle/app_client.cc | 4 ++++ mojo/shell/tests/lifecycle/app_client.h | 1 + 15 files changed, 34 insertions(+), 34 deletions(-) (limited to 'mojo') diff --git a/mojo/services/network/network_service_delegate.cc b/mojo/services/network/network_service_delegate.cc index b257331..1970928 100644 --- a/mojo/services/network/network_service_delegate.cc +++ b/mojo/services/network/network_service_delegate.cc @@ -78,6 +78,10 @@ bool NetworkServiceDelegate::AcceptConnection(Connection* connection) { return true; } +void NetworkServiceDelegate::ShellConnectionLost() { + base::MessageLoop::current()->QuitWhenIdle(); +} + void NetworkServiceDelegate::Create(Connection* connection, InterfaceRequest request) { new NetworkServiceImpl(ref_factory_.CreateRef(), std::move(request)); diff --git a/mojo/services/network/network_service_delegate.h b/mojo/services/network/network_service_delegate.h index 7098939..8997097 100644 --- a/mojo/services/network/network_service_delegate.h +++ b/mojo/services/network/network_service_delegate.h @@ -38,6 +38,7 @@ class NetworkServiceDelegate : public ShellClient, void Initialize(Connector* connector, const Identity& identity, uint32_t id) override; bool AcceptConnection(Connection* connection) override; + void ShellConnectionLost() override; // InterfaceFactory implementation. void Create(Connection* connection, diff --git a/mojo/services/tracing/tracing_app.cc b/mojo/services/tracing/tracing_app.cc index d921a6d..65b4421 100644 --- a/mojo/services/tracing/tracing_app.cc +++ b/mojo/services/tracing/tracing_app.cc @@ -42,12 +42,8 @@ bool TracingApp::AcceptConnection(mojo::Connection* connection) { return true; } -bool TracingApp::ShellConnectionLost() { - // TODO(beng): This is only required because TracingApp isn't run by - // ApplicationRunner - instead it's launched automatically by the standalone - // shell. It shouldn't be. +void TracingApp::ShellConnectionLost() { base::MessageLoop::current()->QuitWhenIdle(); - return false; } void TracingApp::Create(mojo::Connection* connection, diff --git a/mojo/services/tracing/tracing_app.h b/mojo/services/tracing/tracing_app.h index eb5934a..17f4132 100644 --- a/mojo/services/tracing/tracing_app.h +++ b/mojo/services/tracing/tracing_app.h @@ -34,7 +34,7 @@ class TracingApp private: // mojo::ShellClient implementation. bool AcceptConnection(mojo::Connection* connection) override; - bool ShellConnectionLost() override; + void ShellConnectionLost() override; // mojo::InterfaceFactory implementation. void Create(mojo::Connection* connection, diff --git a/mojo/shell/background/tests/test_service.cc b/mojo/shell/background/tests/test_service.cc index e69640d..d030a81 100644 --- a/mojo/shell/background/tests/test_service.cc +++ b/mojo/shell/background/tests/test_service.cc @@ -25,8 +25,8 @@ class TestClient : public ShellClient, connection->AddInterface(this); return true; } - bool ShellConnectionLost() override { - return true; + void ShellConnectionLost() override { + base::MessageLoop::current()->QuitWhenIdle(); } // InterfaceFactory: diff --git a/mojo/shell/public/cpp/lib/application_runner.cc b/mojo/shell/public/cpp/lib/application_runner.cc index 201aa62..5253053 100644 --- a/mojo/shell/public/cpp/lib/application_runner.cc +++ b/mojo/shell/public/cpp/lib/application_runner.cc @@ -5,12 +5,10 @@ #include "mojo/shell/public/cpp/application_runner.h" #include "base/at_exit.h" -#include "base/bind.h" #include "base/command_line.h" #include "base/memory/scoped_ptr.h" #include "base/message_loop/message_loop.h" #include "base/process/launch.h" -#include "base/run_loop.h" #include "mojo/shell/public/cpp/shell_client.h" #include "mojo/shell/public/cpp/shell_connection.h" @@ -56,9 +54,7 @@ MojoResult ApplicationRunner::Run(MojoHandle shell_client_request_handle, client_.get(), MakeRequest(MakeScopedHandle( MessagePipeHandle(shell_client_request_handle))))); - base::RunLoop run_loop; - connection_->set_connection_lost_closure(run_loop.QuitClosure()); - run_loop.Run(); + loop->Run(); // It's very common for the client to cache the app and terminate on errors. // If we don't delete the client before the app we run the risk of the // client having a stale reference to the app and trying to use it. diff --git a/mojo/shell/public/cpp/lib/shell_client.cc b/mojo/shell/public/cpp/lib/shell_client.cc index 3ca8cd7..c41999b 100644 --- a/mojo/shell/public/cpp/lib/shell_client.cc +++ b/mojo/shell/public/cpp/lib/shell_client.cc @@ -17,6 +17,6 @@ bool ShellClient::AcceptConnection(Connection* connection) { return false; } -bool ShellClient::ShellConnectionLost() { return true; } +void ShellClient::ShellConnectionLost() {} } // namespace mojo diff --git a/mojo/shell/public/cpp/lib/shell_connection.cc b/mojo/shell/public/cpp/lib/shell_connection.cc index 62ae598..d1cba7e 100644 --- a/mojo/shell/public/cpp/lib/shell_connection.cc +++ b/mojo/shell/public/cpp/lib/shell_connection.cc @@ -86,8 +86,7 @@ void ShellConnection::OnConnectionError() { // Note that the ShellClient doesn't technically have to quit now, it may live // on to service existing connections. All existing Connectors however are // invalid. - if (client_->ShellConnectionLost() && !connection_lost_closure_.is_null()) - connection_lost_closure_.Run(); + client_->ShellConnectionLost(); // We don't reset the connector as clients may have taken a raw pointer to it. // Connect() will return nullptr if they try to connect to anything. } diff --git a/mojo/shell/public/cpp/shell_client.h b/mojo/shell/public/cpp/shell_client.h index aff1aac..d6fca12 100644 --- a/mojo/shell/public/cpp/shell_client.h +++ b/mojo/shell/public/cpp/shell_client.h @@ -43,13 +43,8 @@ class ShellClient { // Called when ShellConnection's ShellClient binding (i.e. the pipe the // Mojo Shell has to talk to us over) is closed. A shell client may use this - // as a signal to terminate. Return true from this method to tell the - // ShellConnection to run its connection lost closure if it has one, false to - // prevent it from being run. The default implementation returns true. - // When used in conjunction with ApplicationRunner, returning true here quits - // the message loop created by ApplicationRunner, which results in the app - // quitting. - virtual bool ShellConnectionLost(); + // as a signal to terminate. + virtual void ShellConnectionLost(); private: DISALLOW_COPY_AND_ASSIGN(ShellClient); diff --git a/mojo/shell/public/cpp/shell_connection.h b/mojo/shell/public/cpp/shell_connection.h index 3e00cd3..a05f6f1 100644 --- a/mojo/shell/public/cpp/shell_connection.h +++ b/mojo/shell/public/cpp/shell_connection.h @@ -61,12 +61,7 @@ class ShellConnection : public shell::mojom::ShellClient { // TODO(rockot): Remove this once we get rid of app tests. void SetAppTestConnectorForTesting(shell::mojom::ConnectorPtr connector); - // Specify a function to be called when the connection to the shell is lost. - void set_connection_lost_closure(const base::Closure& closure) { - connection_lost_closure_ = closure; - } - -private: + private: // shell::mojom::ShellClient: void Initialize(shell::mojom::IdentityPtr identity, uint32_t id, @@ -95,8 +90,6 @@ private: Binding binding_; scoped_ptr connector_; - base::Closure connection_lost_closure_; - DISALLOW_COPY_AND_ASSIGN(ShellConnection); }; diff --git a/mojo/shell/tests/connect/connect_test_app.cc b/mojo/shell/tests/connect/connect_test_app.cc index 4180763..342e14d 100644 --- a/mojo/shell/tests/connect/connect_test_app.cc +++ b/mojo/shell/tests/connect/connect_test_app.cc @@ -79,6 +79,12 @@ class ConnectTestApp : public ShellClient, return true; } + void ShellConnectionLost() override { + if (base::MessageLoop::current() && + base::MessageLoop::current()->is_running()) { + base::MessageLoop::current()->QuitWhenIdle(); + } + } // InterfaceFactory: void Create(Connection* connection, diff --git a/mojo/shell/tests/connect/connect_test_driver.cc b/mojo/shell/tests/connect/connect_test_driver.cc index b1bdce4..ace64df 100644 --- a/mojo/shell/tests/connect/connect_test_driver.cc +++ b/mojo/shell/tests/connect/connect_test_driver.cc @@ -37,9 +37,8 @@ class Driver : public mojo::ShellClient, connection->AddInterface(this); return true; } - bool ShellConnectionLost() override { - // TODO(rockot): http://crbug.com/596621. Should be able to remove this - // override entirely. + void ShellConnectionLost() override { + // TODO: This should exit cleanly. _exit(1); } diff --git a/mojo/shell/tests/connect/connect_test_package.cc b/mojo/shell/tests/connect/connect_test_package.cc index 1458e65..c5a1a07 100644 --- a/mojo/shell/tests/connect/connect_test_package.cc +++ b/mojo/shell/tests/connect/connect_test_package.cc @@ -184,6 +184,12 @@ class ConnectTestShellClient connection->AddInterface(this); return true; } + void ShellConnectionLost() override { + if (base::MessageLoop::current() && + base::MessageLoop::current()->is_running()) { + base::MessageLoop::current()->QuitWhenIdle(); + } + } // InterfaceFactory: void Create(Connection* connection, diff --git a/mojo/shell/tests/lifecycle/app_client.cc b/mojo/shell/tests/lifecycle/app_client.cc index 657d967..a2400ec 100644 --- a/mojo/shell/tests/lifecycle/app_client.cc +++ b/mojo/shell/tests/lifecycle/app_client.cc @@ -20,6 +20,10 @@ bool AppClient::AcceptConnection(mojo::Connection* connection) { return true; } +void AppClient::ShellConnectionLost() { + GracefulQuit(); +} + void AppClient::Create(mojo::Connection* connection, LifecycleControlRequest request) { bindings_.AddBinding(this, std::move(request)); diff --git a/mojo/shell/tests/lifecycle/app_client.h b/mojo/shell/tests/lifecycle/app_client.h index 5a7d5e6..d389f20 100644 --- a/mojo/shell/tests/lifecycle/app_client.h +++ b/mojo/shell/tests/lifecycle/app_client.h @@ -37,6 +37,7 @@ class AppClient : public ShellClient, // ShellClient: bool AcceptConnection(Connection* connection) override; + void ShellConnectionLost() override; // InterfaceFactory: void Create(Connection* connection, LifecycleControlRequest request) override; -- cgit v1.1