Added a net::BidirectionalStream to expose a bidirectional streaming interface

net/http/bidirectional_stream.h

+// Copyright 2015 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_HTTP_BIDIRECTIONAL_STREAM_H_
+#define NET_HTTP_BIDIRECTIONAL_STREAM_H_
+
+#include "base/basictypes.h"

[mmenke, 2015/12/08 22:58:34]

Not needed.

[xunjieli, 2015/12/10 23:25:50]

Done.

+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "net/base/request_priority.h"
+#include "net/http/bidirectional_stream_job.h"
+#include "net/http/http_stream_factory.h"
+#include "net/ssl/ssl_config.h"
+
+namespace net {
+
+class BoundNetLog;
+class HttpAuthController;
+class HttpNetworkSession;
+class HttpStream;
+class HttpStreamRequest;
+class IOBuffer;
+class ProxyInfo;
+class SpdyHeaderBlock;
+
+// A class to do HTTP/2 bidirectional streaming. Note that only one
+// ReadData or SendData should be in flight until the operation completes

[mmenke, 2015/12/08 22:58:34]

"one ReadData or SendData" is a bit ambiguous.  Could mean one of each, or one
total.  Maybe "Note that at most one each of ReadData or SendData should...". 
Alternatively, just move the comment above ReadData / SendData, and make each
comment exclusive to what it appears above.

[xunjieli, 2015/12/10 23:25:50]

Done. I edited it here. The ReadData/SendData methods also have comments which
say about the same thing. Thanks!

+// synchronously or asynchronously. The BidirectionalStream must be torn down

[mmenke, 2015/12/08 22:58:34]

Suggest removing the "synchronously or asynchronously" comment - don't think it
adds anything here.

[xunjieli, 2015/12/10 23:25:50]

Done.

+// before the HttpNetworkSession.
+class NET_EXPORT BidirectionalStream : public BidirectionalStreamJob::Delegate,
+                                       public HttpStreamRequest::Delegate {
+ public:
+  // Delegate interface to get notified of success of failure. Callbacks will be
+  // invoked asynchronously.
+  // TODO(xunjieli): Surface the protocol negotiated.
+  class NET_EXPORT Delegate {
+   public:
+    Delegate() {}
+
+    // Called when the request headers have been sent.
+    virtual void OnRequestHeadersSent() = 0;

[mmenke, 2015/12/08 22:58:34]

Should either remove request from this, or add "Response" to the two headers
received methods.

[xunjieli, 2015/12/10 23:25:50]

Done. Good idea!

+
+    // Called when response headers are received.
+    // The delegate should call BidirectionalStream::ReadData to start reading
+    // or call BidirectionalStream::Cancel to cancel the stream.
+    virtual void OnHeaders(const SpdyHeaderBlock& response_headers) = 0;

[mmenke, 2015/12/08 22:58:34]

Should add a Received to the end, to match the Sent method.

[xunjieli, 2015/12/10 23:25:50]

Done. Good idea!

+
+    // Called when read is completed asynchronously. |bytes_read| specifies how
+    // much data is available.
+    // The delegate should call BidirectionalStream::ReadData to continue

[mmenke, 2015/12/08 22:58:34]

should -> may?

[xunjieli, 2015/12/10 23:25:50]

Done.

+    // reading or call BidirectionalStream::Cancel to cancel the stream.
+    virtual void OnReadCompleted(int bytes_read) = 0;
+
+    // Called when the entire buffer passed through SendData is sent.
+    virtual void OnDataSent() = 0;

[mmenke, 2015/12/08 22:58:34]

OnSendCompleted, to match with OnReadCompleted?  Or could rename OnReadCompleted
to OnDataRead().  Should just be consistent.

[xunjieli, 2015/12/10 23:25:50]

Done. Great suggestion.

+
+    // Called when trailers are received.
+    virtual void OnTrailers(const SpdyHeaderBlock& trailers) = 0;

[mmenke, 2015/12/08 22:58:34]

Most other methods here end with a verb (OnHeaders is the other exception).  Add
a Received, to make it consistent.

[xunjieli, 2015/12/10 23:25:50]

Done.

+
+    // Called when the stream is closed or an error occurred.

[mmenke, 2015/12/08 22:58:34]

Is it called when Cancel() is called?

[xunjieli, 2015/12/10 23:25:50]

No, it's not called when Cancel() is called. I edited comment on Cancel()
method.

+    // No other delegate functions will be called after this.
+    virtual void OnFailed(int error) = 0;
+
+   protected:
+    virtual ~Delegate() {}
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Delegate);
+  };
+
+  struct NET_EXPORT RequestInfo {
+    RequestInfo();
+    ~RequestInfo();
+
+    // The requested URL.
+    GURL url;
+
+    // The method to use (GET, POST, etc.).
+    std::string method;
+
+    // Any extra request headers (including User-Agent).
+    HttpRequestHeaders extra_headers;
+  };
+
+  BidirectionalStream(const RequestInfo& request_info,
+                      RequestPriority priority,
+                      HttpNetworkSession* session,
+                      Delegate* delegate);
+
+  // Destroys the helper and cancels any pending request.

[mmenke, 2015/12/08 22:58:34]

--helper

[xunjieli, 2015/12/10 23:25:50]

Done.

+  ~BidirectionalStream() override;
+
+  // Reads at most |buf_len| bytes into |buf|. Returns the number of bytes read,
+  // or ERR_IO_PENDING if the read is to be completed asynchronously, or an
+  // error code if any error occurred.
+
+  // There should not be called again unless it IO completes synchronously or
+  // until OnReadCompleted is invoked.
+  int ReadData(IOBuffer* buf, int buf_len);
+
+  // Sends data. This should not be called again until OnDataSent is invoked.
+  void SendData(IOBuffer* data, int length, bool end_stream);

[mmenke, 2015/12/08 22:58:34]

Can either of these be called before OnHeaders?  What about before
OnRequestHeadersSent?

[xunjieli, 2015/12/10 23:25:50]

No. SpdyStream::SendData will check that the stream is opened (which is only set
right before calling delegate->OnRequestHeadersSent). ReadData also has a CHECK
to make sure the stream isn't idle.

Good point. I have added comment.

+
+  // If there is |stream_request_|, cancel it. If |stream_job_| is established,
+  // cancel it.
+  void Cancel();

[mmenke, 2015/12/08 22:58:34]

I guess there's no such thing as successful completion, since we don't
send/receive content-length headers.  Maybe we should call this Close() instead?

Also document about any in progress operations (Reads, writes, etc).  Can't make
promises about pending writes, but can make promises about invoking callbacks.

[xunjieli, 2015/12/10 23:25:50]

There's actually a notion of successful completion if both sides half closes
without an error. The server half closes by sending us END_STREAM, client can
half closes by sending the server END_STREAM.
We probably shouldn't call this Close() to avoid confuse this with half close.
Done.

[mmenke, 2015/12/11 16:42:53]

There's no way to tell this class to send the server END_STREAM.  Can only the
server initiate graceful shutdown?

[xunjieli, 2015/12/11 23:48:39]

No. The client initiate the half close by invoking SendData with end_stream =
true.

+

[mmenke, 2015/12/08 22:58:34]

Should we have a SetPriority method?  Suppose if the underlying API we're
implementing doesn't have it, we may not need one.  If we don't need one, can
make priority_ const.

[xunjieli, 2015/12/10 23:25:50]

Done. I think we only need to pass it during request time. Don't think we need
to set priority afterwards right now. I have added const.

+  // Returns the protocol used by this stream. Always between
+  // kProtoSPDYMinimumVersion and kProtoSPDYMaximumVersion.
+  NextProto GetProtocol() const;

[mmenke, 2015/12/08 22:58:34]

Should mention that none of these getters may be called until after headers have
been received.  Or just make them all return something reasonable in that case.

[xunjieli, 2015/12/10 23:25:50]

Done. Added comment.

+
+  // Total number of bytes received over the network of SPDY data, headers, and
+  // push_promise frames associated with this stream, including the size of
+  // frame headers, after SSL decryption and not including proxy overhead.
+  int64_t GetTotalReceivedBytes() const;

[mmenke, 2015/12/08 22:58:34]

include stdint.

[xunjieli, 2015/12/10 23:25:50]

Done.

+
+  // Total number of bytes sent over the network of SPDY frames associated with
+  // this stream, including the size of frame headers, before SSL encryption and
+  // not including proxy overhead. Note that some SPDY frames such as pings are
+  // not associated with any stream, and are not included in this value.
+  int64_t GetTotalSentBytes() const;
+
+  // TODO(xunjieli): implement a method to do flow control and a method to ping

[mmenke, 2015/12/08 22:58:34]

nit:  implement -> Implement

[xunjieli, 2015/12/10 23:25:50]

Done.

+  // remote end point.
+
+ private:
+  // BidirectionalStreamJob::Delegate implementation:
+  void OnRequestHeadersSent() override;
+  void OnHeaders(const SpdyHeaderBlock& response_headers) override;
+  void OnReadCompleted(int bytes_read) override;
+  void OnDataSent() override;
+  void OnTrailers(const SpdyHeaderBlock& trailers) override;
+  void OnFailed(int error) override;
+
+  // HttpStreamRequest::Delegate implementation:
+  void OnStreamReady(const SSLConfig& used_ssl_config,
+                     const ProxyInfo& used_proxy_info,
+                     HttpStream* stream) override;
+  void OnBidirectionalStreamJobReady(const SSLConfig& used_ssl_config,
+                                     const ProxyInfo& used_proxy_info,
+                                     BidirectionalStreamJob* stream) override;
+  void OnWebSocketHandshakeStreamReady(
+      const SSLConfig& used_ssl_config,
+      const ProxyInfo& used_proxy_info,
+      WebSocketHandshakeStreamBase* stream) override;
+  void OnStreamFailed(int status,
+                      const SSLConfig& used_ssl_config,
+                      SSLFailureState ssl_failure_state) override;
+  void OnCertificateError(int status,
+                          const SSLConfig& used_ssl_config,
+                          const SSLInfo& ssl_info) override;
+  void OnNeedsProxyAuth(const HttpResponseInfo& response_info,
+                        const SSLConfig& used_ssl_config,
+                        const ProxyInfo& used_proxy_info,
+                        HttpAuthController* auth_controller) override;
+  void OnNeedsClientAuth(const SSLConfig& used_ssl_config,
+                         SSLCertRequestInfo* cert_info) override;
+  void OnHttpsProxyTunnelResponse(const HttpResponseInfo& response_info,
+                                  const SSLConfig& used_ssl_config,
+                                  const ProxyInfo& used_proxy_info,
+                                  HttpStream* stream) override;
+
+  const RequestInfo request_info_;
+  RequestPriority priority_;
+  BoundNetLog net_log_;

[mmenke, 2015/12/08 22:58:34]

const

[xunjieli, 2015/12/10 23:25:50]

Done.

+
+  Delegate* delegate_;

[mmenke, 2015/12/08 22:58:34]

Delegate* const delegate_;

[xunjieli, 2015/12/10 23:25:50]

Done.

+
+  scoped_ptr<HttpStreamRequest> stream_request_;
+  scoped_ptr<BidirectionalStreamJob> stream_job_;
+
+  DISALLOW_COPY_AND_ASSIGN(BidirectionalStream);
+};
+
+}  // namespace net
+
+#endif  // NET_HTTP_BIDIRECTIONAL_STREAM_H_