Rename PepperP2PChannel to PepperTransportSocketAdaptor

remoting/protocol/pepper_transport_socket_adaptor.cc

Index: remoting/protocol/pepper_transport_socket_adaptor.cc
diff --git a/remoting/protocol/pepper_transport_socket_adaptor.cc b/remoting/protocol/pepper_transport_socket_adaptor.cc
new file mode 100644
index 0000000000000000000000000000000000000000..93c6c818a342996e4bfafc54d3cba55cf9c12aad
--- /dev/null
+++ b/remoting/protocol/pepper_transport_socket_adaptor.cc
@@ -0,0 +1,255 @@
+// Copyright (c) 2011 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/pepper_transport_socket_adaptor.h"
+
+#include "base/logging.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "ppapi/c/pp_errors.h"
+#include "ppapi/cpp/dev/transport_dev.h"
+#include "ppapi/cpp/var.h"
+
+namespace remoting {
+namespace protocol {
+
+namespace {
+
+const char kUdpProtocol[] = "udp";
+const char kTcpProtocol[] = "tcp";
+
+// Maps value returned by Recv() and Send() Pepper methods to net::Error.
+int PPErrorToNetError(int result) {
+  if (result > 0)
+    return result;
+
+  switch (result) {
+    case PP_OK:
+      return net::OK;
+    case PP_OK_COMPLETIONPENDING:
+      return net::ERR_IO_PENDING;
+    default:
+      return net::ERR_FAILED;
+  }
+}
+
+}  // namespace
+
+PepperTransportSocketAdaptor::PepperTransportSocketAdaptor(
+    pp::Instance* pp_instance,
+    const std::string& name,
+    ChannelType type,
+    Observer* observer)
+    : name_(name),
+      observer_(observer),
+      connected_(false),
+      get_address_pending_(false),
+      read_callback_(NULL),
+      write_callback_(NULL) {
+  callback_factory_.Initialize(this);
+  transport_.reset(new pp::Transport_Dev(
+      pp_instance, name_.c_str(),
+      type == STREAM ? kTcpProtocol : kUdpProtocol));

[Wez, 2011/08/02 23:13:24]

See previous comments regarding stream vs datagram sockets.

[Sergey Ulanov, 2011/08/03 00:02:16]

Done.

+}
+
+PepperTransportSocketAdaptor::~PepperTransportSocketAdaptor() {
+  observer_->OnChannelDeleted();
+}
+
+void PepperTransportSocketAdaptor::AddRemoteCandidate(
+    const std::string& candidate) {
+  DCHECK(CalledOnValidThread());
+  transport_->ReceiveRemoteAddress(candidate);
+}
+
+int PepperTransportSocketAdaptor::Read(net::IOBuffer* buf, int buf_len,
+                                       net::CompletionCallback* callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!read_callback_);
+  DCHECK(!read_buffer_);
+
+  int result = PPErrorToNetError(transport_->Recv(
+      buf->data(), buf_len,
+      callback_factory_.NewOptionalCallback(
+          &PepperTransportSocketAdaptor::OnRead)));

[Wez, 2011/08/02 23:13:24]

Is there any need for this to be an optional callback?

[Sergey Ulanov, 2011/08/03 00:02:16]

Callbacks for the upper layer (Socket interface) are optional, so required
callbacks wouldn't simplify this code much. It's better to have them optional
here because otherwise there is higher change that we will have bugs in the
calling code, i.e. it not handling optional callbacks properly. optional
callbacks may also be a bit faster.

[Wez, 2011/08/03 00:34:48]

We could verify that calling-code copes with optional callbacks by swapping out
this layer in favour of something that does optional callbacks, in tests.

[Sergey Ulanov, 2011/08/03 00:43:37]

Yes, but in ether case required callbacks will not simplify this code - they
will reduce code size only by one line, yet make the code a little bit less
efficient, so there is no point in using required callbacks here.

+
+  if (result == net::ERR_IO_PENDING) {
+    read_callback_ = callback;
+    read_buffer_ = buf;
+  }
+
+  return result;
+}
+
+int PepperTransportSocketAdaptor::Write(net::IOBuffer* buf, int buf_len,
+                            net::CompletionCallback* callback) {

[Wez, 2011/08/02 23:13:24]

nit: Indentation.

[Sergey Ulanov, 2011/08/03 00:02:16]

Done.

+  DCHECK(CalledOnValidThread());
+  DCHECK(!write_callback_);
+  DCHECK(!write_buffer_);
+
+  int result = PPErrorToNetError(transport_->Send(
+      buf->data(), buf_len,
+      callback_factory_.NewOptionalCallback(
+          &PepperTransportSocketAdaptor::OnWrite)));

[Wez, 2011/08/02 23:13:24]

No need to be optional?

[Sergey Ulanov, 2011/08/03 00:02:16]

Same as Read()

+
+  if (result == net::ERR_IO_PENDING) {
+    write_callback_ = callback;
+    write_buffer_ = buf;
+  }
+
+  return result;
+}
+
+bool PepperTransportSocketAdaptor::SetReceiveBufferSize(int32 size) {
+  DCHECK(CalledOnValidThread());
+  NOTIMPLEMENTED();

[Wez, 2011/08/02 23:13:24]

Do we need to have a bug for adding buffer-size controls to P2P?

[Sergey Ulanov, 2011/08/03 00:02:16]

crbug.com/91439

+  return false;
+}
+
+bool PepperTransportSocketAdaptor::SetSendBufferSize(int32 size) {
+  DCHECK(CalledOnValidThread());
+  NOTIMPLEMENTED();
+  return false;
+}
+
+int PepperTransportSocketAdaptor::Connect(net::CompletionCallback* callback) {
+  connect_callback_ = callback;
+
+  // This will return false when the GetNextAddress() returns an

[Wez, 2011/08/02 23:13:24]

nit: Don't need "the".

[Sergey Ulanov, 2011/08/03 00:02:16]

Done.

+  // error. Particularly it is useful to detect when the P2P Transport
+  // API is not supported.

[Wez, 2011/08/02 23:13:24]

nit: "Particularly it is useful ... " -> "This helps us ..."

[Sergey Ulanov, 2011/08/03 00:02:16]

Done.

+  if (!ProcessCandidates())
+    return net::ERR_FAILED;

[Wez, 2011/08/02 23:13:24]

Can we return a more helpful error in this case?  Perhaps ProcessCandidates()
could return net::OK on success, or an error to return on failure?

[Sergey Ulanov, 2011/08/03 00:02:16]

Done.

+
+  int result = transport_->Connect(
+      callback_factory_.NewRequiredCallback(
+          &PepperTransportSocketAdaptor::OnConnect));
+  DCHECK_EQ(result, PP_OK_COMPLETIONPENDING);
+
+  return net::ERR_IO_PENDING;
+}
+
+void PepperTransportSocketAdaptor::Disconnect() {
+  NOTIMPLEMENTED();

[Wez, 2011/08/02 23:13:24]

How does one disconnect a Pepper stream transport?

[Sergey Ulanov, 2011/08/03 00:02:16]

Just delete it.

[Wez, 2011/08/03 00:34:48]

Would it make sense to simply delete |transport_| here, in that case?

[Sergey Ulanov, 2011/08/03 00:43:37]

Yes, we could do that, but then we would need to handle the case transport_ =
NULL everywhere. It isn't really necessary because this function is not used for
chromoting connections anyway.

+}
+
+bool PepperTransportSocketAdaptor::IsConnected() const {
+  return connected_;
+}
+
+bool PepperTransportSocketAdaptor::IsConnectedAndIdle() const {
+  NOTIMPLEMENTED();
+  return false;
+}
+
+int PepperTransportSocketAdaptor::GetPeerAddress(
+    net::AddressList* address) const {
+  NOTIMPLEMENTED();
+  return net::ERR_FAILED;
+}
+
+int PepperTransportSocketAdaptor::GetLocalAddress(
+    net::IPEndPoint* address) const {
+  NOTIMPLEMENTED();
+  return net::ERR_FAILED;
+}
+
+const net::BoundNetLog& PepperTransportSocketAdaptor::NetLog() const {
+  return net_log_;
+}
+
+void PepperTransportSocketAdaptor::SetSubresourceSpeculation() {
+  NOTIMPLEMENTED();
+}
+
+void PepperTransportSocketAdaptor::SetOmniboxSpeculation() {
+  NOTIMPLEMENTED();
+}
+
+bool PepperTransportSocketAdaptor::WasEverUsed() const {
+  return true;

[Wez, 2011/08/02 23:13:24]

nit: NOTIMPLEMENTED(), too?

[Sergey Ulanov, 2011/08/03 00:02:16]

Done.

+}
+
+bool PepperTransportSocketAdaptor::UsingTCPFastOpen() const {
+  NOTIMPLEMENTED();
+  return true;
+}
+
+int64 PepperTransportSocketAdaptor::NumBytesRead() const {
+  NOTIMPLEMENTED();
+  return 0;
+}
+
+base::TimeDelta PepperTransportSocketAdaptor::GetConnectTimeMicros() const {
+  NOTIMPLEMENTED();
+  return base::TimeDelta();
+}
+
+
+bool PepperTransportSocketAdaptor::ProcessCandidates() {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!get_address_pending_);
+
+  while (true) {
+    pp::Var address;
+    int result = transport_->GetNextAddress(
+        &address, callback_factory_.NewOptionalCallback(
+            &PepperTransportSocketAdaptor::OnNextAddress));

[Wez, 2011/08/02 23:13:24]

Simplify this by making it use a required callback, so that it's just a chain of
calls to OnNextAddress(), each dispatching GetNextAddress() until done?  Then
there's no need for |get_address_pending_|, and ProcessCandidates becomes part
of OnNextAddress.

[Sergey Ulanov, 2011/08/03 00:02:16]

That's not possible with GetNextAddress(). Semantic of GetNextAddress() is
different from Read() and Write(): when the callback is called it does NOT save
the candidate, instead GetNextAddress() should be called again. In other words
it can return candidates only synchronously. It's implemented this way because
it's difficult to manage lifetime of C++ wrappers for calls like this.

[Wez, 2011/08/03 00:34:48]

OK.  The GetNextAddress() interface strikes me as unfortunate for callers. :(

+    if (result == PP_OK_COMPLETIONPENDING) {
+      get_address_pending_ = true;
+      break;
+    }
+
+    if (result == PP_OK) {
+      observer_->OnChannelNewCandidate(address.AsString());
+    } else {
+      LOG(ERROR) << "GetNextAddress() returned an error " << result;
+      return false;
+    }
+  }
+  return true;
+}
+
+void PepperTransportSocketAdaptor::OnNextAddress(int32_t result) {
+  DCHECK(CalledOnValidThread());
+
+  get_address_pending_ = false;
+  ProcessCandidates();
+}
+
+void PepperTransportSocketAdaptor::OnConnect(int result) {
+  DCHECK(connect_callback_);
+
+  if (result == PP_OK)
+    connected_ = true;
+
+  net::CompletionCallback* callback = connect_callback_;
+  connect_callback_ = NULL;
+  callback->Run(PPErrorToNetError(result));
+}
+
+void PepperTransportSocketAdaptor::OnRead(int32_t result) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(read_callback_);
+  DCHECK(read_buffer_);
+
+  net::CompletionCallback* callback = read_callback_;
+  read_callback_ = NULL;
+  read_buffer_ = NULL;
+  callback->Run(PPErrorToNetError(result));
+}
+
+void PepperTransportSocketAdaptor::OnWrite(int32_t result) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(write_callback_);
+  DCHECK(write_buffer_);
+
+  net::CompletionCallback* callback = write_callback_;
+  write_callback_ = NULL;
+  write_buffer_ = NULL;
+  callback->Run(PPErrorToNetError(result));
+}
+
+}  // namespace protocol
+}  // namespace remoting