Client interface for ServiceIPCServer. ServiceProcess implements it.

* Defines ServiceIPCServer::Client
* ServiceProcess is a Client
* Inject io_task_runner in ServiceIPCServer constructor
* g_service_process usage reduction: 4

BUG=402456

Review URL: https://codereview.chromium.org/1244503004

Cr-Commit-Position: refs/heads/master@{#339429}

4 files changed, 83 insertions, 51 deletions

diff --git a/chrome/service/service_ipc_server.cc b/chrome/service/service_ipc_server.cc
index dbc0065..59a5908 100644
--- a/chrome/service/service_ipc_server.cc
+++ b/chrome/service/service_ipc_server.cc
@@ -10,11 +10,18 @@
 #include "chrome/service/service_process.h"
 #include "ipc/ipc_logging.h"
 
-ServiceIPCServer::ServiceIPCServer(const IPC::ChannelHandle& channel_handle,
-                                   base::WaitableEvent* shutdown_event)
-    : channel_handle_(channel_handle),
+ServiceIPCServer::ServiceIPCServer(
+    Client* client,
+    const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
+    const IPC::ChannelHandle& channel_handle,
+    base::WaitableEvent* shutdown_event)
+    : client_(client),
+      io_task_runner_(io_task_runner),
+      channel_handle_(channel_handle),
       shutdown_event_(shutdown_event),
-      client_connected_(false) {
+      ipc_client_connected_(false) {
+  DCHECK(client);
+  DCHECK(shutdown_event);
 }
 
 bool ServiceIPCServer::Init() {
@@ -26,10 +33,13 @@ bool ServiceIPCServer::Init() {
 }
 
 void ServiceIPCServer::CreateChannel() {
-  channel_.reset(NULL); // Tear down the existing channel, if any.
+  channel_.reset();  // Tear down the existing channel, if any.
   channel_ = IPC::SyncChannel::Create(
-      channel_handle_, IPC::Channel::MODE_NAMED_SERVER, this,
-      g_service_process->io_task_runner().get(), true,
+      channel_handle_,
+      IPC::Channel::MODE_NAMED_SERVER,
+      this /* listener */,
+      io_task_runner_,
+      true /* create_pipe_now */,
       shutdown_event_);
 }
 
@@ -40,20 +50,18 @@ ServiceIPCServer::~ServiceIPCServer() {
 }
 
 void ServiceIPCServer::OnChannelConnected(int32 peer_pid) {
-  DCHECK(!client_connected_);
-  client_connected_ = true;
+  DCHECK(!ipc_client_connected_);
+  ipc_client_connected_ = true;
 }
 
 void ServiceIPCServer::OnChannelError() {
-  // When a client (typically a browser process) disconnects, the pipe is
-  // closed and we get an OnChannelError. Since we want to keep servicing
-  // client requests, we will recreate the channel.
-  bool client_was_connected = client_connected_;
-  client_connected_ = false;
-  // TODO(sanjeevr): Instead of invoking the service process for such handlers,
-  // define a Client interface that the ServiceProcess can implement.
+  // When an IPC client (typically a browser process) disconnects, the pipe is
+  // closed and we get an OnChannelError. If we want to keep servicing requests,
+  // we will recreate the channel if necessary.
+  bool client_was_connected = ipc_client_connected_;
+  ipc_client_connected_ = false;
   if (client_was_connected) {
-    if (g_service_process->HandleClientDisconnect()) {
+    if (client_->OnIPCClientDisconnect()) {
 #if defined(OS_WIN)
       // On Windows, once an error on a named pipe occurs, the named pipe is no
       // longer valid and must be re-created. This is not the case on Mac or
@@ -63,7 +71,7 @@ void ServiceIPCServer::OnChannelError() {
     }
   } else {
     // If the client was never even connected we had an error connecting.
-    if (!client_connected_) {
+    if (!ipc_client_connected_) {
       LOG(ERROR) << "Unable to open service ipc channel "
                  << "named: " << channel_handle_.name;
     }
@@ -81,11 +89,11 @@ bool ServiceIPCServer::Send(IPC::Message* msg) {
 
 bool ServiceIPCServer::OnMessageReceived(const IPC::Message& msg) {
   bool handled = true;
-  // When we get a message, always mark the client as connected. The
+  // When we get a message, always mark the IPC client as connected. The
   // ChannelProxy::Context is only letting OnChannelConnected get called once,
-  // so on the Mac and Linux, we never would set client_connected_ to true
+  // so on Mac and Linux, we never would set ipc_client_connected_ to true
   // again on subsequent connections.
-  client_connected_ = true;
+  ipc_client_connected_ = true;
   IPC_BEGIN_MESSAGE_MAP(ServiceIPCServer, msg)
     IPC_MESSAGE_HANDLER(ServiceMsg_EnableCloudPrintProxyWithRobot,
                         OnEnableCloudPrintProxyWithRobot)
@@ -141,10 +149,9 @@ void ServiceIPCServer::OnDisableCloudPrintProxy() {
 }
 
 void ServiceIPCServer::OnShutdown() {
-  g_service_process->Shutdown();
+  client_->OnShutdown();
 }
 
 void ServiceIPCServer::OnUpdateAvailable() {
-  g_service_process->SetUpdateAvailable();
+  client_->OnUpdateAvailable();
 }
-
diff --git a/chrome/service/service_ipc_server.h b/chrome/service/service_ipc_server.h
index 5e0885f..878020c 100644
--- a/chrome/service/service_ipc_server.h
+++ b/chrome/service/service_ipc_server.h
@@ -25,8 +25,26 @@ class WaitableEvent;
 // This class handles IPC commands for the service process.
 class ServiceIPCServer : public IPC::Listener, public IPC::Sender {
  public:
-  ServiceIPCServer(const IPC::ChannelHandle& handle,
-                   base::WaitableEvent* shutdown_event);
+  class Client {
+   public:
+    virtual ~Client() {}
+
+    // Called when the service process must shut down.
+    virtual void OnShutdown() = 0;
+
+    // Called when a product update is available.
+    virtual void OnUpdateAvailable() = 0;
+
+    // Called when the IPC channel is closed. A return value of true indicates
+    // that the IPC server should continue listening for new connections.
+    virtual bool OnIPCClientDisconnect() = 0;
+  };
+
+  ServiceIPCServer(
+      Client* client,
+      const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
+      const IPC::ChannelHandle& handle,
+      base::WaitableEvent* shutdown_event);
   ~ServiceIPCServer() override;
 
   bool Init();
@@ -34,7 +52,7 @@ class ServiceIPCServer : public IPC::Listener, public IPC::Sender {
   // IPC::Sender implementation.
   bool Send(IPC::Message* msg) override;
 
-  bool is_client_connected() const { return client_connected_; }
+  bool is_ipc_client_connected() const { return ipc_client_connected_; }
 
  private:
   friend class MockServiceIPCServer;
@@ -61,12 +79,14 @@ class ServiceIPCServer : public IPC::Listener, public IPC::Sender {
   // Helper method to create the sync channel.
   void CreateChannel();
 
+  Client* client_;
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
   IPC::ChannelHandle channel_handle_;
   scoped_ptr<IPC::SyncChannel> channel_;
   base::WaitableEvent* shutdown_event_;
 
-  // Indicates whether a client is currently connected to the channel.
-  bool client_connected_;
+  // Indicates whether an IPC client is currently connected to the channel.
+  bool ipc_client_connected_;
 
   // Calculates histograms deltas.
   scoped_ptr<base::HistogramDeltaSerialization> histogram_delta_serializer_;
diff --git a/chrome/service/service_process.cc b/chrome/service/service_process.cc
index 342d3d1..3ba883f 100644
--- a/chrome/service/service_process.cc
+++ b/chrome/service/service_process.cc
@@ -30,7 +30,6 @@
 #include "chrome/grit/generated_resources.h"
 #include "chrome/service/cloud_print/cloud_print_proxy.h"
 #include "chrome/service/net/service_url_request_context_getter.h"
-#include "chrome/service/service_ipc_server.h"
 #include "chrome/service/service_process_prefs.h"
 #include "net/base/network_change_notifier.h"
 #include "net/url_request/url_fetcher.h"
@@ -113,10 +112,10 @@ void PrepareRestartOnCrashEnviroment(
 }  // namespace
 
 ServiceProcess::ServiceProcess()
-  : shutdown_event_(true /* manual_reset */, false /* initially_signaled */),
-    main_message_loop_(NULL),
-    enabled_services_(0),
-    update_available_(false) {
+    : shutdown_event_(true /* manual_reset */, false /* initially_signaled */),
+      main_message_loop_(NULL),
+      enabled_services_(0),
+      update_available_(false) {
   DCHECK(!g_service_process);
   g_service_process = this;
 }
@@ -190,6 +189,8 @@ bool ServiceProcess::Initialize(base::MessageLoopForUI* message_loop,
 
   VLOG(1) << "Starting Service Process IPC Server";
   ipc_server_.reset(new ServiceIPCServer(
+      this /* client */,
+      io_task_runner(),
       service_process_state_->GetServiceProcessChannel(),
       &shutdown_event_));
   ipc_server_->Init();
@@ -261,11 +262,19 @@ void ServiceProcess::Terminate() {
                                               base::MessageLoop::QuitClosure());
 }
 
-bool ServiceProcess::HandleClientDisconnect() {
+void ServiceProcess::OnShutdown() {
+  Shutdown();
+}
+
+void ServiceProcess::OnUpdateAvailable() {
+  update_available_ = true;
+}
+
+bool ServiceProcess::OnIPCClientDisconnect() {
   // If there are no enabled services or if there is an update available
   // we want to shutdown right away. Else we want to keep listening for
   // new connections.
-  if (!enabled_services_ || update_available()) {
+  if (!enabled_services_ || update_available_) {
     Shutdown();
     return false;
   }
@@ -338,8 +347,8 @@ void ServiceProcess::ScheduleShutdownCheck() {
 
 void ServiceProcess::ShutdownIfNeeded() {
   if (0 == enabled_services_) {
-    if (ipc_server_->is_client_connected()) {
-      // If there is a client connected, we need to try again later.
+    if (ipc_server_->is_ipc_client_connected()) {
+      // If there is an IPC client connected, we need to try again later.
       // Note that there is still a timing window here because a client may
       // decide to connect at this point.
       // TODO(sanjeevr): Fix this timing window.
diff --git a/chrome/service/service_process.h b/chrome/service/service_process.h
index 1aac3c9c..13f1518 100644
--- a/chrome/service/service_process.h
+++ b/chrome/service/service_process.h
@@ -14,9 +14,9 @@
 #include "base/threading/thread.h"
 #include "base/synchronization/waitable_event.h"
 #include "chrome/service/cloud_print/cloud_print_proxy.h"
+#include "chrome/service/service_ipc_server.h"
 
 class ServiceProcessPrefs;
-class ServiceIPCServer;
 class ServiceURLRequestContextGetter;
 class ServiceProcessState;
 
@@ -32,7 +32,8 @@ class NetworkChangeNotifier;
 // process can live independently of the browser process.
 // ServiceProcess Design Notes
 // https://sites.google.com/a/chromium.org/dev/developers/design-documents/service-processes
-class ServiceProcess : public cloud_print::CloudPrintProxy::Client {
+class ServiceProcess : public ServiceIPCServer::Client,
+                       public cloud_print::CloudPrintProxy::Client {
  public:
   ServiceProcess();
   ~ServiceProcess() override;
@@ -67,18 +68,13 @@ class ServiceProcess : public cloud_print::CloudPrintProxy::Client {
     return &shutdown_event_;
   }
 
-  // Shutdown the service process. This is likely triggered by a IPC message.
+  // Shuts down the service process.
   void Shutdown();
 
-  void SetUpdateAvailable() {
-    update_available_ = true;
-  }
-  bool update_available() const { return update_available_; }
-
-  // Called by the IPC server when a client disconnects. A return value of
-  // true indicates that the IPC server should continue listening for new
-  // connections.
-  bool HandleClientDisconnect();
+  // ServiceIPCServer::Client implementation.
+  void OnShutdown() override;
+  void OnUpdateAvailable() override;
+  bool OnIPCClientDisconnect() override;
 
   cloud_print::CloudPrintProxy* GetCloudPrintProxy();
 
@@ -94,7 +90,7 @@ class ServiceProcess : public cloud_print::CloudPrintProxy::Client {
   // Schedule a call to ShutdownIfNeeded.
   void ScheduleShutdownCheck();
 
-  // Shuts down the process if no services are enabled and no clients are
+  // Shuts down the process if no services are enabled and no IPC client is
   // connected.
   void ShutdownIfNeeded();