Define Stable API for WebRTC/Quartc

net/quic/quartc/quartc_stream_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_STREAM_INTERFACE_H_
+#define NET_QUIC_QUARTC_QUARTC_STREAM_INTERFACE_H_
+
+namespace net {
+
+// QuartcSession will create implementations of this interface to send and
+// receive data.

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

It would be more clear as "Sends and receives data with a particular QUIC stream
ID, reliably and in-order.  To send/receive data out of order, use separate
streams.  To send/receive unreliably, Close a stream after reliability is no
longer needed."

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

Done.

+class QuartcStreamInterface {
+ public:
+  virtual ~QuartcStreamInterface() {}
+
+  // Return the ID of the underlying QUIC stream.

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

Just "The QUIC stream ID" would be enough.

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

Done.

+  virtual uint32_t stream_id() = 0;
+
+  // The size of unsent data which is buffered by the QuicConnection so far.
+  virtual uint64_t buffered_amount() = 0;

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

Just "The amount of data buffered by the QuicConnection" would be enough.

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

Done.

+
+  // For forward-compatibility

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

This comment can probably be removed.

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

Done.

+  struct WriteParameters {
+    bool fin = false;

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

This could use a comment.  When should one set fin = true?  What effect does it
have?

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

Done.

+  };
+
+  // Write data to the stream.

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

Perhaps "Sends data reliably and in-order, buffering if necessary until Close()
is called."

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

Done.

+  virtual void Write(const char* data,
+                     size_t size,
+                     const WriteParameters& param) = 0;
+
+  virtual void Close() = 0;

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

This could use a comment like "Once Close is called, no more data can be sent,
all buffered data will be dropped and no data will be retransmitted."

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

What happens to reading data when Close is called?  Does it no longer receive
any?

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

Done.

+
+  // Implemented by the WebRTC code.

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

This would be more clear as "Implemented by the user of the
QuartcStreamInterface to receive incoming data and be notified of state
changes."

+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Called when the data are ready(processed by QUIC).

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

Would be more clear as "Called when data has been received"

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

Done.

+    virtual void OnReceived(QuartcStreamInterface* stream,
+                            const char* data,
+                            size_t size) = 0;
+
+    // TODO(zhihuang) Creates a map from the integer error_code to WebRTC native
+    // error code.

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

Would be more clear as "Called when the stream is closed, either locally or by
the remote endpoint".

Also, we should document what the error codes are, or use an enum for them.

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

Done.

There are quite a few QuicErrorCode. Should we get some of them which we may
interested in?

+    virtual void OnClose(QuartcStreamInterface* stream, int error_code) = 0;
+
+    // Called when the buffered data has been sent.

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

When it changes or only when it decreases?  If the former, the comment should
say "Called when buffered_amount() changes".  If the latter, the it should be
called OnBufferedAmountDecreased and say "Called when buffered_amount()
decreases".

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

In our case, the buffered amount will change only when buffered date is sent. It
should be decrease only.

+    virtual void OnBufferedAmountChanged(QuartcStreamInterface* stream) = 0;
+  };
+
+  // The |delegate| is not owned by QuartcStream. This method should only be
+  // called once.

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

Why should it only be called once?  If the API user wants to change it, why not?
  

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