Expose the p2p client in content/public

content/public/renderer/socket_client.h

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.

[Alpha Left Google, 2013/10/25 21:28:48]

This is still called socket_client.h though, should it be renamed to
p2p_socket_client.h to be precise? The other P2PXXX classes are placed under a
p2p/ directory that's why they don't have a p2p_ prefix. I think this file needs
to be more clear.

[jam, 2013/11/20 23:26:24]

+1

[hubbe, 2013/11/21 22:07:30]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_PUBLIC_RENDERER_SOCKET_CLIENT_H_
+#define CONTENT_PUBLIC_RENDERER_SOCKET_CLIENT_H_
+
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "content/public/common/p2p_sockets.h"
+#include "net/base/ip_endpoint.h"
+
+namespace content {
+
+// P2P socket that routes all calls over IPC.
+//
+// The object runs on two threads: IPC thread and delegate thread. The
+// IPC thread is used to interact with P2PSocketDispatcher. All
+// callbacks to the user of this class are called on the delegate
+// thread which is specified in Init().

[jam, 2013/11/20 23:26:24]

you're describing the implementation details. do we need to expose that to the
embedder?

Same for the ref counted base class and the threading. can we hide that and
expose only a single threaded interface?

[hubbe, 2013/11/21 22:07:30]

Fixed.
I'm not sure what the proper way to handle this is actually. The implementation
uses lots of callbacks with ref-counts to prevent the object from going away
prematurely. I could potentially change the callbacks to use weak pointers, but
that seems difficult since the callbacks cross threads.

[hubbe, 2013/11/21 22:07:30]

Fixed.
Not sure how to handle this actually. The implementation uses callbacks across
threads, and the ref-counting is used to prevent the object from going away
prematurely. There are other ways to handle this, but ref-counting seems like
the cleanest. Let me know if you have a better idea.

[jam, 2013/11/23 02:03:14]

What I mean is that this is an interface. The implementation could be ref
counted, but does that have to be exposed on the interface?

[hubbe, 2013/11/25 19:15:07]

I think so, because we expect the callers to use scoped_refptr<P2PSocketClient>
to manage the pointer(s) they get back from the factory method.

[jam, 2013/11/27 21:30:18]

what is the ownership here? i.e. can the embedder layer just hold on to a raw
pointer?

[hubbe, 2013/11/27 23:23:01]

That's not the intent, the intent is for the user to always use a scoped_refptr.
I changed the factory to return one.

[jam, 2013/11/28 00:08:57]

ok, so you're saying that the ownership of this class belongs to the creator? if
so, lgtm

+class P2PSocketClient : public base::RefCountedThreadSafe<P2PSocketClient> {
+ public:
+  P2PSocketClient() {}
+
+  // Delegate is called on the the same thread on which P2PSocketCLient is
+  // created.
+  class Delegate {

[jam, 2013/11/20 23:26:24]

see http://www.chromium.org/developers/content-module/content-api for the
guidelines for the public API.

in particular, each interface should be in a separate file (i.e.
p2p_socket_client_delegate.h)

[hubbe, 2013/11/21 22:07:30]

Done.

+   public:
+    virtual ~Delegate() { }
+
+    virtual void OnOpen(const net::IPEndPoint& address) = 0;

[jam, 2013/11/20 23:26:24]

nit: document every method

[hubbe, 2013/11/21 22:07:30]

Done.

+    virtual void OnIncomingTcpConnection(const net::IPEndPoint& address,
+                                         P2PSocketClient* client) = 0;
+    virtual void OnSendComplete() = 0;
+    virtual void OnError() = 0;
+    virtual void OnDataReceived(const net::IPEndPoint& address,
+                                const std::vector<char>& data) = 0;
+  };
+
+  // Initialize socket of the specified |type| and connected to the
+  // specified |address|. |address| matters only when |type| is set to
+  // P2P_SOCKET_TCP_CLIENT.
+  virtual void Init(P2PSocketType type,
+                    const net::IPEndPoint& local_address,
+                    const net::IPEndPoint& remote_address,
+                    Delegate* delegate) = 0;
+
+  // Send the |data| to the |address|.
+  virtual void Send(const net::IPEndPoint& address,
+                    const std::vector<char>& data) = 0;
+
+  // Send the |data| to the |address| using Differentiated Services Code Point
+  // |dscp|.
+  virtual void SendWithDscp(const net::IPEndPoint& address,
+                            const std::vector<char>& data,
+                            net::DiffServCodePoint dscp) = 0;
+
+  // Must be called before the socket is destroyed.
+  virtual void Close() = 0;
+
+  virtual int socket_id() const = 0;
+  virtual void set_delegate(Delegate* delegate) = 0;

[jam, 2013/11/20 23:26:24]

nit: these two methods don't conform to google style naming conventions. since
they're virtual they should be GetSocketID and SetDelegate

[hubbe, 2013/11/21 22:07:30]

Done.

+
+ protected:
+  virtual ~P2PSocketClient() {}
+
+ private:
+  // Calls destructor.
+  friend class base::RefCountedThreadSafe<P2PSocketClient>;
+
+  DISALLOW_COPY_AND_ASSIGN(P2PSocketClient);

[jam, 2013/11/20 23:26:24]

nit: don't need this since pure virtual interfaces can't be copied

[hubbe, 2013/11/21 22:07:30]

Done.

+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_RENDERER_SOCKET_CLIENT_H_