Add a new QuicChromiumClientSession::Handle class

net/quic/chromium/bidirectional_stream_quic_impl.cc

Index: net/quic/chromium/bidirectional_stream_quic_impl.cc
diff --git a/net/quic/chromium/bidirectional_stream_quic_impl.cc b/net/quic/chromium/bidirectional_stream_quic_impl.cc
index 338401042803050ed70b7a8e3021b25c2c3a77dc..a487d6e2728252c4cb9458b57c0b0bdd55fbf369 100644
--- a/net/quic/chromium/bidirectional_stream_quic_impl.cc
+++ b/net/quic/chromium/bidirectional_stream_quic_impl.cc
@@ -21,9 +21,8 @@
 namespace net {
 
 BidirectionalStreamQuicImpl::BidirectionalStreamQuicImpl(
-    const base::WeakPtr<QuicChromiumClientSession>& session)
+    QuicChromiumClientSession::Handle session)
     : session_(session),
-      was_handshake_confirmed_(session->IsCryptoHandshakeConfirmed()),
       stream_(nullptr),
       request_info_(nullptr),
       delegate_(nullptr),
@@ -38,19 +37,13 @@ BidirectionalStreamQuicImpl::BidirectionalStreamQuicImpl(
       has_sent_headers_(false),
       has_received_headers_(false),
       send_request_headers_automatically_(true),
-      weak_factory_(this) {
-  DCHECK(session_);
-  session_->AddObserver(this);
-}
+      weak_factory_(this) {}
 
 BidirectionalStreamQuicImpl::~BidirectionalStreamQuicImpl() {
   if (stream_) {
     delegate_ = nullptr;
     stream_->Reset(QUIC_STREAM_CANCELLED);
   }
-
-  if (session_)
-    session_->RemoveObserver(this);
 }
 
 void BidirectionalStreamQuicImpl::Start(
@@ -63,9 +56,10 @@ void BidirectionalStreamQuicImpl::Start(
   CHECK(delegate);
 
   send_request_headers_automatically_ = send_request_headers_automatically;
-  if (!session_) {
-    NotifyError(was_handshake_confirmed_ ? ERR_QUIC_PROTOCOL_ERROR
-                                         : ERR_QUIC_HANDSHAKE_FAILED);
+  if (!session_.IsConnected()) {
+    NotifyError(session_.IsCryptoHandshakeConfirmed()
+                    ? ERR_QUIC_PROTOCOL_ERROR
+                    : ERR_QUIC_HANDSHAKE_FAILED);
     return;
   }
 
@@ -73,7 +67,7 @@ void BidirectionalStreamQuicImpl::Start(
   request_info_ = request_info;
 
   stream_request_ =
-      session_->CreateStreamRequest(request_info_->method == "POST");
+      session_.CreateStreamRequest(request_info_->method == "POST");
   int rv = stream_request_->StartRequest(base::Bind(

[xunjieli, 2017/05/04 16:54:42]

Instead of requiring BidiStream/HttpStream to know about both StreamRequest and
Handle, have you considered eliminating one of these two from a consumer point
of view?

For example, we can make StreamRequest a nested class within Handle.
Handle::Start() calls StreamRequest::StartRequest(). Then BidiStream/HttpStream
doesn't need to know about StreamRequest.

If StreamRequest::StartRequest() finishes with an error that leads to the
Session being synchronously destroyed, we can handle that in the Handle class.

Right now Handle and StreamRequest have references to each other. This
cross-reference takes some effort to understand.

[Ryan Hamilton, 2017/05/04 18:45:53]

I hear ya! I spent a bunch of time thinking about this because, as you say, it
seems like hiding the Request in the Handle makes sense. However, I think it
complicates the Handle a bit, because I think it makes copy semantics
problematic. If I have a handle and start a request and then make a copy of the
handle do they both have the same request? That can't work. Do they get
different requests? That also sounds problematic.

So one solution would be to make the Handle non-copyable, but I'm not in love
with that approach either 'cause I think it makes life a bit harder on the
caller.  

Long story short, I'm inclined to leave it as-is, but happy to revisit if you
feel strongly.

[Ryan Hamilton, 2017/05/05 03:50:24]

Now that we decided to make the handle non-copyable this is a non-issue, so I've
made this change. It's a nice cleanup! thanks

       &BidirectionalStreamQuicImpl::OnStreamReady, weak_factory_.GetWeakPtr()));
   if (rv == ERR_IO_PENDING)
@@ -81,7 +75,7 @@ void BidirectionalStreamQuicImpl::Start(
 
   if (rv == OK) {
     OnStreamReady(rv);
-  } else if (!was_handshake_confirmed_) {
+  } else if (!session_.IsCryptoHandshakeConfirmed()) {
     NotifyError(ERR_QUIC_HANDSHAKE_FAILED);
   }
 }
@@ -152,8 +146,7 @@ void BidirectionalStreamQuicImpl::SendData(const scoped_refptr<IOBuffer>& data,
     DCHECK(!send_request_headers_automatically_);
     // Creates a bundler only if there are headers to be sent along with the
     // single data buffer.
-    bundler.reset(new QuicConnection::ScopedPacketBundler(
-        session_->connection(), QuicConnection::SEND_ACK_IF_PENDING));
+    bundler = session_.CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING);
     SendRequestHeaders();
   }
 
@@ -184,8 +177,8 @@ void BidirectionalStreamQuicImpl::SendvData(
     return;
   }
 
-  QuicConnection::ScopedPacketBundler bundler(
-      session_->connection(), QuicConnection::SEND_ACK_IF_PENDING);
+  std::unique_ptr<QuicConnection::ScopedPacketBundler> bundler(
+      session_.CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING));
   if (!has_sent_headers_) {
     DCHECK(!send_request_headers_automatically_);
     SendRequestHeaders();
@@ -241,7 +234,7 @@ void BidirectionalStreamQuicImpl::OnHeadersAvailable(
   negotiated_protocol_ = kProtoQUIC;
   if (!has_received_headers_) {
     has_received_headers_ = true;
-    connect_timing_ = session_->GetConnectTiming();
+    connect_timing_ = session_.GetConnectTiming();
     if (delegate_)
       delegate_->OnHeadersReceived(headers);
   } else {
@@ -268,13 +261,13 @@ void BidirectionalStreamQuicImpl::OnDataAvailable() {
 }
 
 void BidirectionalStreamQuicImpl::OnClose() {
-  DCHECK(session_);
   DCHECK(stream_);
 
   if (stream_->connection_error() != QUIC_NO_ERROR ||
       stream_->stream_error() != QUIC_STREAM_NO_ERROR) {
-    NotifyError(was_handshake_confirmed_ ? ERR_QUIC_PROTOCOL_ERROR
-                                         : ERR_QUIC_HANDSHAKE_FAILED);
+    NotifyError(session_.IsCryptoHandshakeConfirmed()
+                    ? ERR_QUIC_PROTOCOL_ERROR
+                    : ERR_QUIC_HANDSHAKE_FAILED);
     return;
   }
 
@@ -298,21 +291,6 @@ bool BidirectionalStreamQuicImpl::HasSendHeadersComplete() {
   return has_sent_headers_;
 }
 
-void BidirectionalStreamQuicImpl::OnCryptoHandshakeConfirmed() {
-  was_handshake_confirmed_ = true;
-}
-
-void BidirectionalStreamQuicImpl::OnSuccessfulVersionNegotiation(
-    const QuicVersion& version) {}
-
-void BidirectionalStreamQuicImpl::OnSessionClosed(
-    int error,
-    bool /*port_migration_detected*/) {
-  DCHECK_NE(OK, error);
-  session_.reset();
-  NotifyError(error);
-}
-
 void BidirectionalStreamQuicImpl::OnStreamReady(int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
   DCHECK(rv == OK || !stream_);
@@ -361,11 +339,6 @@ void BidirectionalStreamQuicImpl::NotifyStreamReady() {
 }
 
 void BidirectionalStreamQuicImpl::ResetStream() {
-  if (session_) {
-    session_->RemoveObserver(this);
-    session_ = nullptr;
-  }
-
   if (!stream_)
     return;
   closed_stream_received_bytes_ = stream_->stream_bytes_read();