Cleanup Fake* classes in remoting/protocol

remoting/protocol/fake_datagram_socket.cc

Index: remoting/protocol/fake_datagram_socket.cc
diff --git a/remoting/protocol/fake_datagram_socket.cc b/remoting/protocol/fake_datagram_socket.cc
new file mode 100644
index 0000000000000000000000000000000000000000..afd6d7de8cf0edcfadd1c61a6a4fb30d96c9fd08
--- /dev/null
+++ b/remoting/protocol/fake_datagram_socket.cc
@@ -0,0 +1,158 @@
+// 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.
+
+#include "remoting/protocol/fake_datagram_socket.h"
+
+#include "base/bind.h"
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "net/base/address_list.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/base/net_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+namespace protocol {
+
+FakeDatagramSocket::FakeDatagramSocket()
+    : read_pending_(false),
+      input_pos_(0),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_factory_(this) {
+}
+
+FakeDatagramSocket::~FakeDatagramSocket() {
+  EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
+}
+
+void FakeDatagramSocket::AppendInputPacket(const std::string& data) {
+  EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
+  input_packets_.push_back(data);
+
+  // Complete pending read if any.
+  if (read_pending_) {
+    read_pending_ = false;

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

nit: Suggest blank line after this, since next line is part of the "copy data to
buffer" logical block.

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

Done.

+    int result = std::min(static_cast<int>(data.size()), read_buffer_size_);
+    memcpy(read_buffer_->data(), data.data(), result);
+    input_pos_ = input_packets_.size();
+    read_buffer_ = NULL;

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

nit: Suggest blank line after this, since the next line is part of the "run
callback" logical block.

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

Done.

+    net::CompletionCallback callback = read_callback_;
+    read_callback_.Reset();
+    callback.Run(result);
+  }
+}
+
+void FakeDatagramSocket::PairWith(FakeDatagramSocket* peer_socket) {
+  EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
+  peer_socket_ = peer_socket->weak_factory_.GetWeakPtr();
+  peer_socket->peer_socket_ = weak_factory_.GetWeakPtr();
+}
+
+int FakeDatagramSocket::Read(net::IOBuffer* buf, int buf_len,
+                        const net::CompletionCallback& callback) {
+  EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
+  if (input_pos_ < static_cast<int>(input_packets_.size())) {
+    int result = std::min(
+        buf_len, static_cast<int>(input_packets_[input_pos_].size()));
+    memcpy(buf->data(), &(*input_packets_[input_pos_].begin()), result);

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

nit: You have to do this min+memcpy dance above as well; consider factoring it
out into a static helper function taking a destination data+length, and a source
buffer.

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

Done.

+    ++input_pos_;
+    return result;
+  } else {
+    read_pending_ = true;
+    read_buffer_ = buf;
+    read_buffer_size_ = buf_len;
+    read_callback_ = callback;
+    return net::ERR_IO_PENDING;
+  }
+}
+
+int FakeDatagramSocket::Write(net::IOBuffer* buf, int buf_len,
+                         const net::CompletionCallback& callback) {
+  EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
+  written_packets_.push_back(std::string());
+  written_packets_.back().assign(buf->data(), buf->data() + buf_len);
+
+  if (peer_socket_.get()) {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&FakeDatagramSocket::AppendInputPacket,
+                   peer_socket_,
+                   std::string(buf->data(), buf->data() + buf_len)));
+  }
+
+  return buf_len;
+}
+
+int FakeDatagramSocket::SetReceiveBufferSize(int32 size) {
+  NOTIMPLEMENTED();
+  return net::ERR_NOT_IMPLEMENTED;
+}
+
+int FakeDatagramSocket::SetSendBufferSize(int32 size) {
+  NOTIMPLEMENTED();
+  return net::ERR_NOT_IMPLEMENTED;
+}
+
+FakeDatagramChannelFactory::FakeDatagramChannelFactory()
+    : task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      async_creation_(false),
+      fail_(false),
+      weak_factory_(this) {
+}
+
+FakeDatagramChannelFactory::~FakeDatagramChannelFactory() {}

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

EXPECT_TRUE(channels_.empty()) here?

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

Done.

+
+void FakeDatagramChannelFactory::PairWith(
+    FakeDatagramChannelFactory* peer_factory) {
+  peer_factory_ = peer_factory->weak_factory_.GetWeakPtr();
+  peer_factory_->peer_factory_ = weak_factory_.GetWeakPtr();
+}
+
+FakeDatagramSocket* FakeDatagramChannelFactory::GetChannel(
+    const std::string& name) {
+  return channels_[name];
+}
+
+void FakeDatagramChannelFactory::CreateChannel(
+    const std::string& name,
+    const ChannelCreatedCallback& callback) {
+  scoped_ptr<FakeDatagramSocket> channel;
+  // If we are in the error state then we put NULL in the channels list, so that
+  // NotifyChannelCreated() still calls the callback.
+  if (!fail_)
+    channel.reset(new FakeDatagramSocket());

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

nit: I thought you were generally opting for { } even around single-line ifs?

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

nit: This would be more readable if you initialized |channel| with the new fake,
and then reset() it iff |fail_| were true.

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

Done.

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

Nah, I don't care about it as long as the code is consistent. And this is not
new code, I'm just moving it.

+
+  if (peer_factory_) {
+    ChannelsMap::iterator peer = peer_factory_->channels_.find(name);
+    if (peer != peer_factory_->channels_.end())
+      channel->PairWith(peer->second);

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

Won't this crash if |fail_| is true, since |channel| will be NULL?

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

Done.

+  }
+
+  channels_[name] = channel.release();
+
+  if (async_creation_) {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&FakeDatagramChannelFactory::NotifyChannelCreated,
+                   weak_factory_.GetWeakPtr(), name, callback));
+  } else {
+    NotifyChannelCreated(name, callback);
+  }
+}
+
+void FakeDatagramChannelFactory::NotifyChannelCreated(
+    const std::string& name,
+    const ChannelCreatedCallback& callback) {
+  if (channels_.find(name) != channels_.end())
+    callback.Run(scoped_ptr<net::Socket>(channels_[name]));

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

Don't you need to clear the channel from |channels_| as well? Otherwise it'll
still be in the map when the factory is deleted?

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

We need it in |channels_| so that the channel can be retrieved by the tests
later. Changed ChannelsMap to store weak pointers.

+}
+
+void FakeDatagramChannelFactory::CancelChannelCreation(
+    const std::string& name) {
+  channels_.erase(name);
+}
+
+}  // namespace protocol
+}  // namespace remoting