Prototype for copresenceSockets.

components/copresence_sockets/public/copresence_socket.h

+// Copyright 2014 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_COPRESENCE_SOCKETS_COPRESENCE_SOCKET_H_
+#define COMPONENTS_COPRESENCE_SOCKETS_COPRESENCE_SOCKET_H_
+
+#include <string>
+
+#include "base/macros.h"
+
+namespace copresence_sockets {
+
+// A CopresenceSocket is an object that is used to send receive data. Currently
+// this object is only created by a CopresencePeer once it receives a
+// connection request. This class is currently just a pure interface.

[Ryan Sleevi, 2014/10/01 19:47:25]

LAYERING: Document how to use this class, not how it is used. THis is a public
API, comments about who does what don't belong (unless it's a delegate and
you're documenting how/under what conditions the delegate is invoked)

[rkc, 2014/10/01 22:38:48]

Changed.
Done.

+// TODO(rkc): Add the ability to connect to a remote CopresencePeer.
+class CopresenceSocket {
+ public:
+  typedef base::Callback<void(const std::string&)> ReceiveCallback;

[Ryan Sleevi, 2014/10/01 19:47:25]

DESIGN: std::string vs base::StringPiece

[rkc, 2014/10/01 22:38:48]

Changed to using net::IOBuffer instead.

+
+  CopresenceSocket() {}
+  virtual ~CopresenceSocket() {}

[Ryan Sleevi, 2014/10/01 19:47:24]

DESIGN: Is a public destructor correct for this class?

Only do so if ownership of a CopresenceSocket implementation is directly passed
into your interface. Otherwise, prefer a protected destructor, to ensure it's
the owning class that is responsible for managing the lifetime of this object.

[rkc, 2014/10/01 22:38:48]

So CopresenceSockets will be owned by classes other than the CopresencePeer also
(which will hold ownership only of the base pointer), so having a public
destructor is what we want.

+
+  // Attempt to send data on this socket. If we were unable to send the data,
+  // the method returns false.

[Ryan Sleevi, 2014/10/01 19:47:24]

STYLE: Pronouns in comments considered harmful -
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M

// Send |data| on this socket. If the socket was unable to send data, returns
false.

[rkc, 2014/10/01 22:38:48]

Done.

+  // TODO(rkc): Expand the bool into more a more detailed failures enum.
+  virtual bool Send(const std::string& data) = 0;

[Ryan Sleevi, 2014/10/01 19:47:24]

DESIGN: Synchronous methods for sockets are ALWAYS an anti-pattern and STRONGLY
discouraged as they have been a LARGE source of bugs and latency.

Your socket also assumes all data will always be sent, which seems like a poor
decision.

[Ryan Sleevi, 2014/10/01 19:47:25]

DESIGN: Don't use std::string for buffers of data.

Nominally, net::IOBuffer is the canonical form for this within //net and the
overall resource loading stack. Alternatively, base::StringPiece allows for
zero-copy when dealing with alternative forms (e.g. an std::vector<>)

[rkc, 2014/10/01 22:38:48]

Done.

[rkc, 2014/10/01 22:38:48]

We are designing this API so that Send only means that the send 'started', not
that it completed. This API can be implemented on BT, UDP, TCP, BLE, or a host
of other transports, not all of which will be able to give a send confirmation.
Added more documentation to this method to clarify.

+  virtual void Receive(ReceiveCallback callback) = 0;

[Ryan Sleevi, 2014/10/01 19:47:24]

STYLE: Pass as const-ref.

[Ryan Sleevi, 2014/10/01 19:47:25]

DESIGN: Let the caller specify how much data they're interested in?

[rkc, 2014/10/01 22:38:48]

Done.

[rkc, 2014/10/01 22:38:48]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CopresenceSocket);
+};
+
+}  // namespace copresence_sockets
+
+#endif  // COMPONENTS_COPRESENCE_SOCKETS_COPRESENCE_SOCKET_H_