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

net/http/bidirectional_stream_create_helper.cc

+// 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.
+
+#include "net/http/bidirectional_stream_create_helper.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "net/base/net_errors.h"
+#include "net/http/bidirectional_stream.h"
+#include "net/http/http_request_info.h"
+#include "net/http/http_stream.h"
+#include "net/http/http_transaction_factory.h"
+#include "net/ssl/ssl_cert_request_info.h"
+#include "net/url_request/url_request_context.h"
+#include "url/gurl.h"
+
+namespace net {
+
+BidirectionalStreamCreateHelper::BidirectionalStreamCreateHelper(
+    const HttpRequestInfo* request_info,
+    RequestPriority priority,
+    const URLRequestContext* context,
+    Delegate* delegate)
+    : request_info_(request_info),
+      priority_(priority),
+      delegate_(delegate),
+      net_log_(BoundNetLog::Make(context->net_log(),
+                                 NetLog::SOURCE_BIDIRECTIONAL_STREAM)) {

[mmenke, 2015/10/21 22:40:16]

Hrm...this log is going to look a little weird.  Normally you have a start and
end event wrapping everything else, but since we don't have one object around
for the life of the stream to log them to, that's not going to happen here.

Weirdness around the BoundNetLog is what made me realize we needed a wrapper
class in the first place.  Looks like we're still going to have that weirdness,
though.

[xunjieli, 2015/10/22 20:01:41]

Should we make the embedder (who presumably will own the stream for its
lifetime) pass us a BoundNetLog (which it owns too) through the CreateHelper?
Not sure if that will prevent the weirdness.

[mmenke, 2015/10/22 20:13:36]

That wouldn't - the way things elsewhere is that there's a class that represents
the same object as the BoundNetLog, and it logs a start and end event.  I don't
think we want to make our embedder do that logging.

Here, there's no class that is around for the duration of the log.  Only way I
see to get that would be to make this stream take ownership of the created
BidirectionalStream, and then all calls to the stream to go through this.  Then
this could just log an entry on creation and destruction, and we'd done.

[xunjieli, 2015/10/23 17:51:23]

Done. Thanks for the detailed explanation! I think I understand the use of
BoundNetLog a little better. I have changed this wrapper class to own the
stream, and added proxy methods to manipulate the stream through this wrapper
class. Let me know what you think.

+  HttpTransactionFactory* factory = context->http_transaction_factory();
+  session_ = factory->GetSession();
+
+  session_->ssl_config_service()->GetSSLConfig(&server_ssl_config_);
+  proxy_ssl_config_ = server_ssl_config_;
+}
+
+BidirectionalStreamCreateHelper::~BidirectionalStreamCreateHelper() {
+  if (stream_)
+    stream_->Cancel();
+}
+
+void BidirectionalStreamCreateHelper::CreateBidirectionalStream() {
+  if (!request_info_->url.SchemeIs("https")) {
+    delegate_->OnStreamFailed(ERR_DISALLOWED_URL_SCHEME);

[mmenke, 2015/10/21 22:40:16]

Do we want to invoke callbacks synchronously, or guarantee that we call them all
asynchronously?

[xunjieli, 2015/10/22 20:01:41]

Done. Probably the latter to be more consistent?

[mmenke, 2015/10/22 20:13:36]

I think async is indeed the way to go - avoids re-entrancy issues (And is
basically required if BidirectionalStreamCreateHelper creation automatically
starts creating a stream, too).

+    return;
+  }
+  stream_request_.reset(
+      session_->http_stream_factory()->RequestBidirectionalStream(
+          *request_info_, priority_, server_ssl_config_, proxy_ssl_config_,
+          this, net_log_));
+}
+
+void BidirectionalStreamCreateHelper::OnStreamReady(
+    const SSLConfig& used_ssl_config,
+    const ProxyInfo& used_proxy_info,
+    HttpStream* stream) {
+  NOTREACHED();
+}
+
+void BidirectionalStreamCreateHelper::OnBidirectionalStreamReady(
+    const SSLConfig& used_ssl_config,
+    const ProxyInfo& used_proxy_info,
+    BidirectionalStream* stream) {
+  stream_.reset(stream);
+  delegate_->OnStreamCreated();

[mmenke, 2015/10/21 22:40:16]

Should we pass in the stream here (wrapped in a scoped_ptr)?

[xunjieli, 2015/10/22 20:01:41]

Done.

+}
+
+void BidirectionalStreamCreateHelper::OnWebSocketHandshakeStreamReady(
+    const SSLConfig& used_ssl_config,
+    const ProxyInfo& used_proxy_info,
+    WebSocketHandshakeStreamBase* stream) {
+  NOTREACHED();
+}
+
+void BidirectionalStreamCreateHelper::OnStreamFailed(
+    int result,
+    const SSLConfig& used_ssl_config,
+    SSLFailureState ssl_failure_state) {
+  DCHECK_NE(OK, result);

[mmenke, 2015/10/21 22:40:16]

If we want to DCHECK the result, think we really want:

DCHECK_LT(result, 0);
DCHECK_NE(result, ERR_IO_PENDING);

Same goes for OnCertificateError.

[xunjieli, 2015/10/22 20:01:41]

Done.

+  DCHECK(stream_request_.get());

[mmenke, 2015/10/21 22:40:16]

nit:  .get() isn't needed in any of these.

[xunjieli, 2015/10/22 20:01:41]

Done.

+  DCHECK(!stream_);
+
+  delegate_->OnStreamFailed(result);
+}
+
+void BidirectionalStreamCreateHelper::OnCertificateError(
+    int result,
+    const SSLConfig& used_ssl_config,
+    const SSLInfo& ssl_info) {
+  DCHECK_NE(OK, result);
+  DCHECK(stream_request_.get());
+  DCHECK(!stream_);
+
+  delegate_->OnStreamFailed(result);
+}
+
+void BidirectionalStreamCreateHelper::OnNeedsProxyAuth(
+    const HttpResponseInfo& proxy_response,
+    const SSLConfig& used_ssl_config,
+    const ProxyInfo& used_proxy_info,
+    HttpAuthController* auth_controller) {
+  DCHECK(stream_request_.get());
+  DCHECK(!stream_);
+
+  delegate_->OnStreamFailed(ERR_PROXY_AUTH_REQUESTED);
+}
+
+void BidirectionalStreamCreateHelper::OnNeedsClientAuth(
+    const SSLConfig& used_ssl_config,
+    SSLCertRequestInfo* cert_info) {
+  DCHECK(stream_request_.get());
+  DCHECK(!stream_);
+
+  delegate_->OnStreamFailed(ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
+}
+
+void BidirectionalStreamCreateHelper::OnHttpsProxyTunnelResponse(
+    const HttpResponseInfo& response_info,
+    const SSLConfig& used_ssl_config,
+    const ProxyInfo& used_proxy_info,
+    HttpStream* stream) {
+  NOTREACHED();

[mmenke, 2015/10/21 22:40:16]

nit:  include base/logging.h

[xunjieli, 2015/10/22 20:01:41]

Done.

+}
+
+}  // namespace net