Several cleanups of QuicSession, et al. as per the Chrome code review.

Merge internal change: 38012917


Review URL: https://chromiumcodereview.appspot.com/11365153

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@167128 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 58 insertions, 69 deletions

diff --git a/net/quic/quic_crypto_stream.h b/net/quic/quic_crypto_stream.h
index dfcd66c..91cb95f 100644
--- a/net/quic/quic_crypto_stream.h
+++ b/net/quic/quic_crypto_stream.h
@@ -52,6 +52,8 @@ class NET_EXPORT_PRIVATE QuicCryptoStream
  private:
   CryptoFramer crypto_framer_;
   bool handshake_complete_;
+
+  DISALLOW_COPY_AND_ASSIGN(QuicCryptoStream);
 };
 
 }  // namespace net
diff --git a/net/quic/quic_crypto_stream_test.cc b/net/quic/quic_crypto_stream_test.cc
index 5aee0e4..c74130f 100644
--- a/net/quic/quic_crypto_stream_test.cc
+++ b/net/quic/quic_crypto_stream_test.cc
@@ -35,8 +35,19 @@ class MockQuicCryptoStream : public QuicCryptoStream {
     }
   }
 
+  std::vector<CryptoTag>* message_tags() {
+    return &message_tags_;
+  }
+
+  std::vector<std::map<CryptoTag, string> >* message_maps() {
+    return &message_maps_;
+  }
+
+ private:
   std::vector<CryptoTag> message_tags_;
-  std::vector<map<CryptoTag, string> > message_maps_;
+  std::vector<std::map<CryptoTag, string> > message_maps_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockQuicCryptoStream);
 };
 
 class QuicCryptoStreamTest : public ::testing::Test {
@@ -57,12 +68,16 @@ class QuicCryptoStreamTest : public ::testing::Test {
     message_data_.reset(framer.ConstructHandshakeMessage(message_));
   }
 
+ protected:
   IPEndPoint addr_;
   MockConnection* connection_;
   MockSession session_;
   MockQuicCryptoStream stream_;
   CryptoHandshakeMessage message_;
   scoped_ptr<QuicData> message_data_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(QuicCryptoStreamTest);
 };
 
 TEST_F(QuicCryptoStreamTest, NotInitiallyConected) {
@@ -79,11 +94,11 @@ TEST_F(QuicCryptoStreamTest, ProcessData) {
   EXPECT_EQ(message_data_->length(),
             stream_.ProcessData(message_data_->data(),
                                 message_data_->length()));
-  ASSERT_EQ(1u, stream_.message_tags_.size());
-  EXPECT_EQ(kSHLO, stream_.message_tags_[0]);
-  EXPECT_EQ(2u, stream_.message_maps_[0].size());
-  EXPECT_EQ("abc",stream_.message_maps_[0][1]);
-  EXPECT_EQ("def", stream_.message_maps_[0][2]);
+  ASSERT_EQ(1u, stream_.message_tags()->size());
+  EXPECT_EQ(kSHLO, (*stream_.message_tags())[0]);
+  EXPECT_EQ(2u, (*stream_.message_maps())[0].size());
+  EXPECT_EQ("abc", (*stream_.message_maps())[0][1]);
+  EXPECT_EQ("def", (*stream_.message_maps())[0][2]);
 }
 
 TEST_F(QuicCryptoStreamTest, ProcessBadData) {
diff --git a/net/quic/quic_session.cc b/net/quic/quic_session.cc
index 2021887..66ba4e0 100644
--- a/net/quic/quic_session.cc
+++ b/net/quic/quic_session.cc
@@ -176,17 +176,15 @@ bool QuicSession::IsClosedStream(QuicStreamId id) {
     return false;
   }
   if (id % 2 == next_stream_id_ % 2) {
-    // If the stream was locally initiated we strictly in-order creation.
-    // If the id is in the range of created streams and it's not active, it
-    // must have been closed.
+    // Locally created streams are strictly in-order.  If the id is in the
+    // range of created streams and it's not active, it must have been closed.
     return id < next_stream_id_;
-  } else {
-    // For peer created streams, we also need to consider
-    // implicitly created streams.
+  }
+  // For peer created streams, we also need to consider implicitly created
+  // streams.
     return id <= largest_peer_created_stream_id_ &&
         implicitly_created_streams_.count(id) == 0;
   }
-}
 
 size_t QuicSession::GetNumOpenStreams() {
   return stream_map_.size() + implicitly_created_streams_.size();
diff --git a/net/quic/quic_session.h b/net/quic/quic_session.h
index cc4edc6..07ff22f 100644
--- a/net/quic/quic_session.h
+++ b/net/quic/quic_session.h
@@ -81,6 +81,8 @@ class NET_EXPORT_PRIVATE QuicSession : public QuicConnectionVisitorInterface {
   QuicStreamId GetNextStreamId();
 
   // Returns true if the stream existed previously and has been closed.
+  // Returns false if the stream is still active or if the stream has
+  // not yet been created.
   bool IsClosedStream(QuicStreamId id);
 
   ReliableQuicStream* GetIncomingReliableStream(QuicStreamId stream_id);
diff --git a/net/quic/quic_stream_sequencer.cc b/net/quic/quic_stream_sequencer.cc
index f730379..4853ceb 100644
--- a/net/quic/quic_stream_sequencer.cc
+++ b/net/quic/quic_stream_sequencer.cc
@@ -41,17 +41,17 @@ QuicStreamSequencer::~QuicStreamSequencer() {
 
 bool QuicStreamSequencer::WillAcceptStreamFrame(
     const QuicStreamFrame& frame) const {
-  QuicStreamOffset byte_offset = frame.offset;
   size_t data_len = frame.data.size();
   DCHECK_LE(data_len, max_frame_memory_);
 
+  uint64 byte_offset = frame.offset;
   if (byte_offset < num_bytes_consumed_ ||
       frames_.find(byte_offset) != frames_.end()) {
     return false;
   }
   if (data_len > max_frame_memory_) {
-    // We're never going to buffer this frame and we can't pass it up the
-    // stream might only consume part of it and we'd need a partial ack.
+    // We're never going to buffer this frame and we can't pass it up.
+    // The stream might only consume part of it and we'd need a partial ack.
     //
     // Ideally this should never happen, as we check that
     // max_frame_memory_ > kMaxPacketSize and lower levels should reject
@@ -67,16 +67,15 @@ bool QuicStreamSequencer::WillAcceptStreamFrame(
 }
 
 bool QuicStreamSequencer::OnStreamFrame(const QuicStreamFrame& frame) {
-  QuicStreamOffset byte_offset = frame.offset;
-  const char* data = frame.data.data();
-  size_t data_len = frame.data.size();
-
   if (!WillAcceptStreamFrame(frame)) {
     // This should not happen, as WillAcceptFrame should be called before
     // OnStreamFrame.  Error handling should be done by the caller.
     return false;
   }
 
+  QuicStreamOffset byte_offset = frame.offset;
+  const char* data = frame.data.data();
+  size_t data_len = frame.data.size();
   if (byte_offset == num_bytes_consumed_) {
     DVLOG(1) << "Processing byte offset " << byte_offset;
     size_t bytes_consumed = stream_->ProcessData(data, data_len);
@@ -137,41 +136,17 @@ bool QuicStreamSequencer::MaybeCloseStream() {
   return false;
 }
 
-void QuicStreamSequencer::AdvanceReadablePtr(size_t data_read) {
-  FrameMap::iterator it = frames_.begin();
-
-  while (data_read) {
-    if (it->first != num_bytes_consumed_ || it == frames_.end()) {
-      stream_->Close(QUIC_SERVER_ERROR_PROCESSING_STREAM);  // Programming error
-      return;
-    }
-
-    if (data_read >= it->second.size()) {
-      data_read -= it->second.size();
-      num_bytes_consumed_ += it->second.size();
-      frames_.erase(it);
-      it = frames_.begin();
-    } else {
-      frames_.insert(make_pair(it->first + data_read,
-                                  it->second.substr(data_read)));
-      frames_.erase(frames_.begin());
-      num_bytes_consumed_ += data_read;
-      data_read = 0;
-    }
-  }
-}
-
-bool QuicStreamSequencer::HasBytesToRead() {
-  FrameMap::iterator it = frames_.begin();
+bool QuicStreamSequencer::HasBytesToRead() const {
+  FrameMap::const_iterator it = frames_.begin();
 
   return it != frames_.end() && it->first == num_bytes_consumed_;
 }
 
-bool QuicStreamSequencer::IsHalfClosed() {
+bool QuicStreamSequencer::IsHalfClosed() const {
   return num_bytes_consumed_ >= close_offset_;
 }
 
-bool QuicStreamSequencer::IsClosed() {
+bool QuicStreamSequencer::IsClosed() const {
   return num_bytes_consumed_ >= close_offset_ && half_close_ == false;
 }
 
diff --git a/net/quic/quic_stream_sequencer.h b/net/quic/quic_stream_sequencer.h
index 971aaec..4f5463e 100644
--- a/net/quic/quic_stream_sequencer.h
+++ b/net/quic/quic_stream_sequencer.h
@@ -50,28 +50,25 @@ class NET_EXPORT_PRIVATE QuicStreamSequencer {
   // Once data is buffered, it's up to the stream to read it when the stream
   // can handle more data.  The following three functions make that possible.
 
-  // Advances the readable pointer num_bytes bytes, releasing any buffered data
-  // which is no longer in uses
-  void AdvanceReadablePtr(size_t num_bytes);
-
   // Returns true if the sequncer has bytes available for reading.
-  bool HasBytesToRead();
+  bool HasBytesToRead() const;
 
   // Returns true if the sequencer has delivered a half close.
-  bool IsHalfClosed();
+  bool IsHalfClosed() const;
 
   // Returns true if the sequencer has delivered a full close.
-  bool IsClosed();
+  bool IsClosed() const;
 
  private:
-  bool MaybeCloseStream();
-
   friend class QuicStreamSequencerPeer;
+
   // TODO(alyssar) use something better than strings.
   typedef map<QuicStreamOffset, string> FrameMap;
 
   void FlushBufferedFrames();
 
+  bool MaybeCloseStream();
+
   ReliableQuicStream* stream_;  // The stream which owns this sequencer.
   QuicStreamOffset num_bytes_consumed_;  // The last data consumed by the stream
   FrameMap frames_;  // sequence number -> frame
diff --git a/net/quic/quic_stream_sequencer_test.cc b/net/quic/quic_stream_sequencer_test.cc
index 676b0d7..148ace0f 100644
--- a/net/quic/quic_stream_sequencer_test.cc
+++ b/net/quic/quic_stream_sequencer_test.cc
@@ -47,11 +47,11 @@ class QuicStreamSequencerPeer : public QuicStreamSequencer {
     max_frame_memory_ = limit;
   }
 
-  ReliableQuicStream* stream() { return stream_; }
-  uint64 num_bytes_consumed() { return num_bytes_consumed_; }
-  FrameMap* frames() { return &frames_; }
-  int32 max_frame_memory() { return max_frame_memory_; }
-  QuicStreamOffset close_offset() { return close_offset_; }
+  const ReliableQuicStream* stream() const { return stream_; }
+  uint64 num_bytes_consumed() const { return num_bytes_consumed_; }
+  const FrameMap* frames() const { return &frames_; }
+  int32 max_frame_memory() const { return max_frame_memory_; }
+  QuicStreamOffset close_offset() const { return close_offset_; }
 };
 
 class MockStream : public ReliableQuicStream {
diff --git a/net/quic/reliable_quic_stream.cc b/net/quic/reliable_quic_stream.cc
index 3cdde26..d9f9343 100644
--- a/net/quic/reliable_quic_stream.cc
+++ b/net/quic/reliable_quic_stream.cc
@@ -58,7 +58,7 @@ bool ReliableQuicStream::OnStreamFrame(const QuicStreamFrame& frame) {
 void ReliableQuicStream::OnStreamReset(QuicErrorCode error,
                                        QuicStreamOffset offset) {
   error_ = error;
-  sequencer_.CloseStreamAtOffset(offset, false);  // Full close
+  sequencer_.CloseStreamAtOffset(offset, false);  // Full close.
 }
 
 void ReliableQuicStream::ConnectionClose(QuicErrorCode error, bool from_peer) {
@@ -83,11 +83,11 @@ void ReliableQuicStream::Close(QuicErrorCode error) {
   session()->SendRstStream(id(), error, offset_);
 }
 
-bool ReliableQuicStream::IsHalfClosed() {
+bool ReliableQuicStream::IsHalfClosed() const {
   return sequencer_.IsHalfClosed();
 }
 
-bool ReliableQuicStream::HasBytesToRead() {
+bool ReliableQuicStream::HasBytesToRead() const {
   return sequencer_.HasBytesToRead();
 }
 
diff --git a/net/quic/reliable_quic_stream.h b/net/quic/reliable_quic_stream.h
index 7ff880a..2457bc0 100644
--- a/net/quic/reliable_quic_stream.h
+++ b/net/quic/reliable_quic_stream.h
@@ -45,12 +45,12 @@ class NET_EXPORT_PRIVATE ReliableQuicStream {
 
   // This block of functions wraps the sequencer's functions of the same
   // name.
-  virtual bool IsHalfClosed();
-  virtual bool HasBytesToRead();
+  virtual bool IsHalfClosed() const;
+  virtual bool HasBytesToRead() const;
 
-  QuicStreamId id() { return id_; }
+  QuicStreamId id() const { return id_; }
 
-  QuicErrorCode error() { return error_; }
+  QuicErrorCode error() const { return error_; }
 
  protected:
   virtual int WriteData(base::StringPiece data, bool fin);