arc-bridge: Add the ARC Bridge Service

components/arc/arc_bridge_service.h

+// Copyright 2015 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 COMPONENTS_ARC_ARC_BRIDGE_SERVICE_H_
+#define COMPONENTS_ARC_ARC_BRIDGE_SERVICE_H_
+
+#include "base/macros.h"
+#include "base/observer_list.h"
+#include "base/sequenced_task_runner.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "ipc/ipc_listener.h"
+#include "ipc/ipc_message.h"
+
+class PrefRegistrySimple;
+class PrefService;
+
+namespace arc {
+
+class ArcBridgeTest;
+
+// The Chrome-side service that handles ARC instances and ARC bridge creation.
+// This service handles the lifetime of ARC instances and sets up the
+// communication channel (the ARC bridge) used to send and receive messages.
+class ArcBridgeService : public KeyedService,
+                         public IPC::Listener {

[hidehiko, 2015/10/28 17:48:45]

nit: can be fit in a line?

[Luis Héctor Chávez, 2015/10/28 21:06:32]

It can, but having it separate seems to be the predominant convention.

+ public:
+  enum State {

[hidehiko, 2015/10/28 17:48:45]

As this is statemachine, it'd be good to note the state transition briefly.

[Luis Héctor Chávez, 2015/10/29 23:43:52]

Done.

+    // An attempt to start the ARC instance was done, but it resulted in an
+    // error.
+    ERROR,
+
+    // ARC was not enabled in this machine.
+    DISABLED,
+
+    // ARC is enabled in this machine, but has not started.
+    STOPPED,
+
+    // The bridge has connected to the socket, but has not started the ARC
+    // instance.
+    CONNECTED,
+
+    // The ARC bridge has been set up and ARC is starting up.
+    STARTING,
+
+    // The ARC instance has been fully initialized and is now ready for user
+    // interaction.
+    READY
+  };
+
+  class Observer {
+   public:
+    // Called whenever the state of the bridge has changed.
+    virtual void OnStateChanged(State state) {}
+
+   protected:
+    virtual ~Observer() {}
+  };
+
+  ArcBridgeService(

[satorux1, 2015/10/28 05:45:23]

Explicit is missing?

[Luis Héctor Chávez, 2015/10/28 17:19:14]

Done.

+      const scoped_refptr<base::SequencedTaskRunner>& file_task_runner);
+  virtual ~ArcBridgeService();
+
+  // Registers ARC preferences.
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
+  // Reads from the preference store if ARC has been enabled.
+  static bool GetEnabledPref(PrefService* pref_service);
+
+  // HandleStartup() should be called upon profile startup.  This will only
+  // launch an instance if the instance service is available and it is enabled.
+  void HandleStartup();
+
+  // Shuts down the running instance, if any.  This is safe to call even if
+  // there is no instance running.
+  void Shutdown();
+
+  // Add or remove observers.

[satorux1, 2015/10/28 05:45:23]

nit: Adds or removes

[Luis Héctor Chávez, 2015/10/28 17:19:14]

Done.

+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  // Gets the current state of the bridge service.
+  State GetState() const { return state_; }
+
+  // Enables or disables the bridge for this session.
+  void SetEnabled(bool enabled);
+
+  // Request for registering an input device on the ARC instance.

[satorux1, 2015/10/28 05:45:23]

nit: Requests

[Luis Héctor Chávez, 2015/10/28 17:19:14]

Done.

+  // TODO(denniskempin): Make this interface more typesafe.
+  // |name| should be the displayable name of the emulated device (e.g. "Chrome
+  // OS Keyboard"), |device_type| the name of the device type (e.g. "keyboard")
+  // and |fd| a file descriptor that emulates the kernel events of the device.
+  bool RegisterInputDevice(const std::string& name,
+                           const std::string& device_type,
+                           base::ScopedFD fd);
+
+ private:
+  friend class arc::ArcBridgeTest;
+
+  // Binds to the socket specified by |socket_path|.
+  void SocketConnect(const std::string& socket_path,
+                     bool directory_creation_success);
+
+  // Internal connection method. Exposed for testing.

[hidehiko, 2015/10/28 17:48:45]

For testing, I'd recommend you to pass that socket path or Handle to this
service via ctor and to test via public methods, rather than accessing to the
private methods. It should be more reliable tests.

[hidehiko, 2015/10/29 16:29:46]

ping?

[Luis Héctor Chávez, 2015/10/29 23:43:52]

Sorry I missed this. We can't do that since this is supposed to be built through
a factory. There's also the fact that if we do create an alternate constructor,
we would need to add additional logic to this class to figure out if it should
try to do the D-Bus connection, and that doesn't sound great. An alternative
discussed offline was to extract all the connection logic from this class, but
that seems like too much amount of work at this point in time.

I don't plan to modify this particular code in the future, but if I end up doing
that, I'll revisit this.

+  bool Connect(const IPC::ChannelHandle& handle, IPC::Channel::Mode mode);

[satorux1, 2015/10/28 05:45:23]

If this function is named like ConnectForTesting(), the presubmit check will
warn if the function is called outside of tests.

[Luis Héctor Chávez, 2015/10/28 17:19:14]

The function is used outside of tests in arc_bridge_service.cc, so I can't do
that. Instead I changed the comment.

+
+  // Finish connecting after setting socket permissions.

[satorux1, 2015/10/28 05:45:23]

nit: Finishes

[Luis Héctor Chávez, 2015/10/28 17:19:14]

Done.

+  void FinishConnectAfterSocketPermissions(const std::string& socket_path,
+                                           bool socket_permissions_success);
+
+  // Called when the ARC instance has finished setting up and is ready for user
+  // interaction.
+  void OnInstanceReady();
+
+  // Change the current state and notify all observers.

[satorux1, 2015/10/28 05:45:23]

nit: Changes

[Luis Héctor Chávez, 2015/10/28 17:19:14]

Done.

+  void NotifyStateChanged(State state);

[hidehiko, 2015/10/28 17:48:45]

nit: How about ChangeState(), if this actually changes its status.
NotifyStateChanged sounds like just sending a notification *without* changing
its status?

[Luis Héctor Chávez, 2015/10/29 23:43:52]

Missed this as well. Was changed to SetState.

+
+  // Finishes shutdown after deleting the socket file.
+  void FinishShutdownAfterSocketDeleted(bool socket_deleted);
+
+  // IPC::Listener:
+  bool OnMessageReceived(const IPC::Message& message) override;
+
+  // DBus callbacks.
+  void OnInstanceStarted(chromeos::DBusMethodCallStatus status);
+  void OnInstanceStopped(chromeos::DBusMethodCallStatus status);
+
+  // Thread in which IPC messaging is performed.
+  base::Thread ipc_thread_;
+
+  // Task runner on which incoming messages are handled.
+  scoped_refptr<base::SequencedTaskRunner> origin_task_runner_;
+
+  // Task runner on which file operations are performed.
+  scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
+
+  // The channel through which messages are sent.
+  scoped_ptr<IPC::ChannelProxy> ipc_channel_;
+
+  // List of observers to notify of state changes.
+  // Makes sure list is empty on destruction.
+  base::ObserverList<Observer, true> observer_list_;
+
+  // The current state of the bridge.
+  ArcBridgeService::State state_;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcBridgeService);
+};
+
+}  // namespace arc
+
+#endif  // COMPONENTS_ARC_ARC_BRIDGE_SERVICE_H_