Cleanup Fake* classes in remoting/protocol

remoting/protocol/fake_datagram_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 REMOTING_PROTOCOL_FAKE_DATAGRAM_SOCKET_H_
+#define REMOTING_PROTOCOL_FAKE_DATAGRAM_SOCKET_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "net/base/completion_callback.h"
+#include "net/socket/socket.h"
+#include "remoting/protocol/datagram_channel_factory.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+}
+
+namespace remoting {
+namespace protocol {
+
+// FakeDatagramSocket implement net::StreamSocket interface. All data written to
+// FakeDatagramSocket is stored in a buffer returned by written_packets().
+// Read() reads data from another buffer that can be set with
+// AppendInputPacket(). Pending reads are supported, so if there is a pending
+// read AppendInputPacket() calls the read callback.
+//
+// Two fake sockets can be connected to each other using the
+// PairWith() method, e.g.: a->PairWith(b). After this all data
+// written to |a| can be read from |b| and vice versa. Two connected
+// sockets |a| and |b| must be created and used on the same thread.
+class FakeDatagramSocket : public net::Socket {
+ public:
+  FakeDatagramSocket();
+  virtual ~FakeDatagramSocket();
+
+  const std::vector<std::string>& written_packets() const {
+    return written_packets_;
+  }
+
+  void AppendInputPacket(const std::string& data);

[Wez, 2014/09/20 00:25:09]

Consider replacing the term "input" with "to read", so this becomes
AppendPacketToRead(), since the "input" packets are the ones we want to have the
caller read via Read()?

[Wez, 2014/09/20 00:25:10]

Would it be simpler to always create these things in pairs, like pipes, and
thereby avoid the need for AppendInputPacket or written_packets()?

[Sergey Ulanov, 2014/09/22 19:22:11]

We had AppendInputPacket() before this CL. I think AppendPacketToRead() has the
same change of being confusing as AppendInputPacket(), so I don't think it needs
to be renamed.

[Sergey Ulanov, 2014/09/22 19:22:11]

No. For some tests it's easier to have not connected seconds and just validate
what have been written in the second without creating a paired socket.

+  int input_pos() const { return input_pos_; }

[Wez, 2014/09/20 00:25:09]

What does |input_pos| mean? Is it a position in packets, bytes, etc?

[Sergey Ulanov, 2014/09/22 19:22:11]

Added comment

+  void PairWith(FakeDatagramSocket* peer_socket);

[Wez, 2014/09/20 00:25:09]

Clarify that caller retains ownership of |peer_socket|?

[Sergey Ulanov, 2014/09/22 19:22:11]

it's not scoped_ptr<> so it should be clear that ownership is not passed here. 

[Wez, 2014/09/23 17:07:38]

Except that there is still old code in the codebase that pre-dates scoped_ptr<>,
and code which interfaces with e.g. WebRTC, that doesn't have scoped_ptr<>.

[Sergey Ulanov, 2014/09/23 19:06:08]

I believe all code in src/remoting should be using scoped_ptr<> to pass
ownership, except the parts that depend on WebRTC. I tried to clean that up a
while ago. If there is still code that doesn't scoped_ptr<> and it could, we
need to fix it.

+
+  // net::Socket implementation.
+  virtual int Read(net::IOBuffer* buf, int buf_len,
+                   const net::CompletionCallback& callback) OVERRIDE;
+  virtual int Write(net::IOBuffer* buf, int buf_len,
+                    const net::CompletionCallback& callback) OVERRIDE;
+

[Wez, 2014/09/20 00:25:09]

nit: Remove blank line here

[Sergey Ulanov, 2014/09/22 19:22:11]

Done.

+  virtual int SetReceiveBufferSize(int32 size) OVERRIDE;
+  virtual int SetSendBufferSize(int32 size) OVERRIDE;
+
+ private:
+  base::WeakPtr<FakeDatagramSocket> peer_socket_;
+
+  bool read_pending_;

[Wez, 2014/09/20 00:25:10]

nit: has_pending_read_ or has_read_pending_ or is_read_pending_?

[Sergey Ulanov, 2014/09/22 19:22:11]

We use read_pending_ in other places as well, and it's also used in src/net.
Removed this variable - !read_callback_.is_null() can be used instead.

+  scoped_refptr<net::IOBuffer> read_buffer_;
+  int read_buffer_size_;
+  net::CompletionCallback read_callback_;
+
+  std::vector<std::string> written_packets_;
+  std::vector<std::string> input_packets_;
+  int input_pos_;
+
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  base::WeakPtrFactory<FakeDatagramSocket> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeDatagramSocket);
+};
+
+class FakeDatagramChannelFactory : public DatagramChannelFactory {
+ public:
+  FakeDatagramChannelFactory();
+  virtual ~FakeDatagramChannelFactory();
+
+  void set_async_creation(bool async_creation) {

[Wez, 2014/09/20 00:25:10]

nit: set_asynchronous, or set_is_asynchronous or set_asynchronous_create?

[Sergey Ulanov, 2014/09/22 19:22:11]

Done.

+    async_creation_ = async_creation;
+  }
+
+  void set_fail(bool fail) { fail_ = fail; }

[Wez, 2014/09/20 00:25:09]

nit: set_fail_create_channel or set_fail_create?

[Sergey Ulanov, 2014/09/22 19:22:11]

Done.

+
+  void PairWith(FakeDatagramChannelFactory* peer_factory);

[Wez, 2014/09/20 00:25:10]

What does it mean to pair two factories?

[Sergey Ulanov, 2014/09/22 19:22:11]

It means that the sockets they create are paired as soon as they are created.
Added comment.

[Wez, 2014/09/23 17:07:38]

Suggest tweaking comment to make clear that the created sockets are paired with
the socket of the same channel name in the peer factory.

[Sergey Ulanov, 2014/09/23 19:06:08]

Done.

+
+  FakeDatagramSocket* GetChannel(const std::string& name);

[Wez, 2014/09/20 00:25:09]

What are the ownership semantics of GetChannel - looks like it's intended to
return a reference to the socket?

Also clarify how this is intended to be used? I assume it's there for tests to
be able to retrieve the FakeDatagramSocket that was created for a particular
caller, to feed data into?

[Sergey Ulanov, 2014/09/22 19:22:11]

Added comment 
Renamed to GetFakeChannel() to make the intent clearer. and added comment.

+
+  // DatagramChannelFactory interface.
+  virtual void CreateChannel(const std::string& name,
+                             const ChannelCreatedCallback& callback) OVERRIDE;
+  virtual void CancelChannelCreation(const std::string& name) OVERRIDE;
+
+ private:
+  typedef std::map<std::string, FakeDatagramSocket*> ChannelsMap;
+
+  void NotifyChannelCreated(const std::string& name,
+                            const ChannelCreatedCallback& callback);
+
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  bool async_creation_;
+  ChannelsMap channels_;
+
+  base::WeakPtr<FakeDatagramChannelFactory> peer_factory_;

[Wez, 2014/09/20 00:25:10]

nit: In the FakeDatagramSocket the peer weakptrfactory comes first - be
consistent.

[Sergey Ulanov, 2014/09/22 19:22:11]

Done.

+
+  bool fail_;
+
+  base::WeakPtrFactory<FakeDatagramChannelFactory> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeDatagramChannelFactory);
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_FAKE_DATAGRAM_SOCKET_H_