arc-bridge: Add IPC message definitions

chromeos/arc/bridge/common/arc_bridge.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.
+
+// A communication channel between Chrome and an ARC instance.
+// This component will send and receive messages across a socket that is
+// available to the ARC instance when it is started.
+
+#ifndef CHROMEOS_ARC_LIBARCBRIDGE_ARC_ARC_BRIDGE_H_
+#define CHROMEOS_ARC_LIBARCBRIDGE_ARC_ARC_BRIDGE_H_
+
+#include "base/macros.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "ipc/ipc_listener.h"
+#include "ipc/ipc_message.h"
+
+namespace arc {
+
+// Abstract class that represents one of the ARC bridge endpoints.
+class BridgeEndpoint : public IPC::Listener {

[hidehiko, 2015/10/26 09:15:00]

What's the usage of BridgeEndpoint?

If this is just a utility to implement Bridge{Host,Instance}Endpoint classes,
it's better not to inherit, instead of composition.
Or, in this case, I think you can define some utility functions in .cc file, I
guess.

[Shuhei Takahashi, 2015/10/26 11:13:40]

+1

If you still want to use a base class, please rename it to BridgeEndpointBase,
and protect the constructor.

[Luis Héctor Chávez, 2015/10/26 21:03:27]

Having it as a base class avoids repeating the Connect method for testing.

+ public:
+  BridgeEndpoint();
+  virtual ~BridgeEndpoint();
+
+  // Connects the endpoint of the bridge through the specified channel handle.
+  // This function is mainly for testing, since callers can call the

[Shuhei Takahashi, 2015/10/26 11:13:40]

Do you mean "This function is mainly *exposed* for testing"?

[Luis Héctor Chávez, 2015/10/26 21:03:27]

Done

+  // subclasses' Socketconnect().
+  bool Connect(const IPC::ChannelHandle& handle, IPC::Channel::Mode mode);
+
+ protected:
+  scoped_ptr<IPC::ChannelProxy> channel_;
+
+  base::Thread ipc_thread_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BridgeEndpoint);
+};
+
+// Instance side of the ARC bridge.

[satorux1, 2015/10/26 01:58:52]

I felt that relationship with 'instance side' and 'host side' was not clear.
Could you document it here?

[Luis Héctor Chávez, 2015/10/26 21:03:27]

Done.

+class BridgeInstanceEndpoint : public BridgeEndpoint {
+ public:
+  BridgeInstanceEndpoint();
+  virtual ~BridgeInstanceEndpoint();
+
+  bool SocketConnect(const std::string& socket_path);
+
+  // Messages from the instance to the host.
+  bool InstanceReady();
+
+ private:
+  bool OnMessageReceived(const IPC::Message& message) override;

[satorux1, 2015/10/26 01:58:52]

please add function comments to these functions. it's a bit tedious but Chrome
follows this style rather thoroughly. :)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...

Every function declaration should have comments immediately preceding it that
describe what the function does and how to use it.

[Luis Héctor Chávez, 2015/10/26 21:03:27]

Done.

+  void OnPing();

[satorux1, 2015/10/26 01:58:52]

What's ping? Is this a sample function or does it have a real use case? Please
document.

[Luis Héctor Chávez, 2015/10/26 21:03:27]

It was added at some point to avoid a compilation error due to a variable
created in the message processing part was not used since there were no other
messages defined. Now we have one, so it's not needed.

+  virtual void OnRegisterInputDevice(const std::string& name,

[satorux1, 2015/10/26 01:58:52]

In particular, please document how sub classes are supposed to implement this.

[Luis Héctor Chávez, 2015/10/26 21:03:27]

Done.

+                                     const std::string& device_type,
+                                     base::FileDescriptor fd) = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(BridgeInstanceEndpoint);
+};
+
+// Host side of the ARC bridge.
+class BridgeHostEndpoint : public BridgeEndpoint {

[Shuhei Takahashi, 2015/10/26 11:13:40]

nit: To follow IPC naming convention, maybe want to rename this to
BridgeInstanceHostEndpoint.

[Luis Héctor Chávez, 2015/10/26 21:03:27]

Oh I was not aware of this convention. Renamed to ArcInstanceBridgeHostBase
since it still is not a concrete class but has some logic.

+ public:
+  BridgeHostEndpoint();
+  virtual ~BridgeHostEndpoint();
+
+  bool SocketConnect(const std::string& socket_path);

[satorux1, 2015/10/26 01:58:52]

ditto. function comment is missing.

[Luis Héctor Chávez, 2015/10/26 21:03:27]

Done.

+
+  // Messages from the host to the instance.
+  bool Ping();
+  bool RegisterInputDevice(const std::string& name,
+                           const std::string& device_type,
+                           base::ScopedFD fd);
+
+ private:
+  bool OnMessageReceived(const IPC::Message& message) override;
+  virtual void OnInstanceReady() = 0;
+  virtual void OnPong() = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(BridgeHostEndpoint);
+};
+
+}  // namespace arc
+
+#endif  // CHROMEOS_ARC_LIBARCBRIDGE_ARC_ARC_BRIDGE_H_