Define Stable API for WebRTC/Quartc

net/quic/quartc/quartc_session_interface.h

+// Copyright (c) 2016 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 NET_QUIC_QUARTC_QUARTC_SESSION_INTERFACE_H_
+#define NET_QUIC_QUARTC_QUARTC_SESSION_INTERFACE_H_
+
+#include "net/quic/quartc/quartc_stream_interface.h"
+
+namespace net {
+
+// This interface defines a session over peer-to-peer transport that
+// negotiates the crypto handshake (using QuicCryptoHandshake) and provides
+// reading/writing of data using QUIC.

[pthatcher2, 2016/10/05 22:12:07]

The comment does need "This interface defines a ...".  Plus, we don't need to be
peer-to-peer specific.  We could just say "Given a PacketTransport, provides a
way to send and receive separate streams of reliable, in-order, encrypted data. 
For example, this can build on top of a WebRTC IceTransport for sending and
receiving data over QUIC."

[zhihuang1, 2016/10/13 06:22:40]

Done.

+class QuartcSessionInterface {
+ public:
+  virtual ~QuartcSessionInterface() {}
+
+  virtual void StartCryptoHandshake() = 0;
+
+  // Only needed when using SRTP with QuicTransport
+  // Key Exporter interface from RFC 5705
+  // Arguments are:
+  // label               -- the exporter label.
+  //                        part of the RFC defining each exporter usage (IN)
+  // context/context_len -- a context to bind to for this connection;
+  //                        optional, can be NULL, 0 (IN)
+  // use_context         -- whether to use the context value
+  //                        (needed to distinguish no context from
+  //                        zero-length ones).
+  // result              -- where to put the computed value
+  // result_len          -- the length of the computed value
+  virtual bool ExportKeyingMaterial(const std::string& label,
+                                    const uint8_t* context,
+                                    size_t context_len,
+                                    bool used_context,
+                                    uint8_t* result,
+                                    size_t result_len) = 0;
+
+  // For forward-compatibility. More parameters could be added through the
+  // struct without changing the API.
+  struct OutgoingStreamParameters {};
+
+  virtual QuartcStreamInterface* CreateOutgoingStream(
+      const OutgoingStreamParameters& params) = 0;
+
+  // Implemented by WebRTC (ICE) code. Used by QUIC for sending and receiving
+  // (hopefully UDP) packets.

[pthatcher2, 2016/10/05 22:12:07]

This would be more clear as "Send and receive packets, like a virtual UDP
socket.  For example, this could be implemented by WebRTC's IceTransport."

[zhihuang1, 2016/10/13 06:22:40]

Done.

+  class Transport {
+   public:
+    virtual ~Transport() {}
+
+    // Called by the QuartcPacketWriter to check if the underneath transport
+    // writable.

[pthatcher2, 2016/10/05 22:12:07]

"transport writable" => "transport is writable"

And it could be more clear with a first sentence which says "True if packets
written are expected to be sent.  False if packets will be dropped."

[zhihuang1, 2016/10/13 06:22:40]

Done.

+    virtual bool CanWrite() = 0;
+
+    // Called by the QuartcPacketWriter when writing packets to the network.
+    virtual int Write(const char* buffer, size_t buf_len) = 0;

[pthatcher2, 2016/10/05 22:12:06]

We should comment on what the return value is.

[zhihuang1, 2016/10/13 06:22:40]

Done.

+
+    // Implemented by QUIC code. Used to receive notifications from the
+    // WebRTC side through the Transport.

[pthatcher2, 2016/10/05 22:12:07]

Would be more clear as "Callbacks called by the PacketTransport to notify the
user of the PacketTransport of certain events".

[zhihuang1, 2016/10/13 06:22:40]

I found that moving the callbacks of PacketTransport::Delegate to
QuartcSessionInterface will simplify the code.

+    class Delegate {
+     public:
+      virtual ~Delegate() {}
+
+      // Called when the WebRTC network layer becomes writable.

[pthatcher2, 2016/10/05 22:12:07]

This would be more clear as "Called when CanWrite() changes from false to true."
  We don't need to reference WebRTC.

[zhihuang1, 2016/10/13 06:22:40]

Done.

+      virtual void OnCanWrite() = 0;
+
+      // Called when the WebRTC network received data. The QUIC side will be
+      // responsible for further processing such as decryption.

[pthatcher2, 2016/10/05 22:12:07]

I think this would be more clear as just "Called when a packet has been received
and should be handled by the QuicConnection."  We don't need to reference
WebRTC.

[zhihuang1, 2016/10/13 06:22:40]

Done.

+      virtual bool OnReceived(const char* data, size_t data_len) = 0;
+    };
+
+    // The |delegate| is not owned by Transport. This method should only be
+    // called once.
+    virtual void SetDelegate(Delegate* delegate) = 0;
+  };
+
+  // Implemented by WebRTC code. Used by the WebRTC to receive notifications
+  // from QUIC side.

[pthatcher2, 2016/10/05 22:12:07]

"Callbacks called by the QuartcSession to notify the user of the QuartcSession
of certain events".

[zhihuang1, 2016/10/13 06:22:40]

Done.

+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Called when the crypto handshake complete. The WebRTC side will change

[pthatcher2, 2016/10/05 22:12:07]

"handshake complete" => "handshake is complete"

+    // the QUIC states based on this.

[pthatcher2, 2016/10/05 22:12:07]

No need to reference WebRTC here.

[zhihuang1, 2016/10/13 06:22:40]

Done.

+    virtual void OnCryptoHandshakeComplete() = 0;
+
+    // Called when the received the data from a incoming stream.

[pthatcher2, 2016/10/05 22:12:07]

More clear would be "Called when a new stream is received from the remote
endpoint".

+    virtual void OnIncomingStream(QuartcStreamInterface* stream) = 0;
+
+    // TODO(zhihuang) Create mapping from integer error code to WebRTC error
+    // code.
+    virtual void OnConnectionClosed(int error_code, bool from_remote) = 0;

[pthatcher2, 2016/10/05 22:12:06]

Needs something like "Called when the connection is closed.  This means all of
the streams will be closed and no new streams can be created."

+
+    // TODO(zhihuang): Add proof verification.
+  };
+
+  // The |delegate| is not owned by QuartcSession. This method should only be
+  // called once.

[pthatcher2, 2016/10/05 22:12:07]

Why only once?

[zhihuang1, 2016/10/13 06:22:40]

I feel that the delegate implies a kind of 1:1 relationship. 

Beside, it seems that other delegates in QUIC are only be set once. For example.
https://cs.chromium.org/chromium/src/net/quic/chromium/quic_chromium_client_s...

But I'm not sure if there are other consideration (delegate context switch?)
about this.

In our case, I think it will also be fine to replace the delegate.

+  virtual void SetDelegate(Delegate* delegate) = 0;
+};
+
+}  // namespace net
+
+#endif  // NET_QUIC_QUARTC_QUARTC_SESSION_INTERFACE_H_