[SPDY] Remove some setters in SpdyStream

Instead pass parameters into SpdyStream's constructor. This simplifies SpdyStream, as we can now be sure that its priority is fixed.

Add function to convert from SpdyPriority to RequestPriority, and use them to set the correct priority for push streams.

Replace uses of Unretained() with weak pointers.

BUG=176582

Review URL: https://codereview.chromium.org/12989038

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

11 files changed, 107 insertions, 43 deletions

diff --git a/net/spdy/spdy_http_utils.cc b/net/spdy/spdy_http_utils.cc
index edb8f1b..41647c1 100644
--- a/net/spdy/spdy_http_utils.cc
+++ b/net/spdy/spdy_http_utils.cc
@@ -147,6 +147,16 @@ SpdyPriority ConvertRequestPriorityToSpdyPriority(
   }
 }
 
+NET_EXPORT_PRIVATE RequestPriority ConvertSpdyPriorityToRequestPriority(
+    SpdyPriority priority,
+    int protocol_version) {
+  // Handle invalid values gracefully, and pick LOW to map 2 back
+  // to for SPDY/2.
+  SpdyPriority idle_cutoff = (protocol_version == 2) ? 3 : 5;
+  return (priority >= idle_cutoff) ?
+      IDLE : static_cast<RequestPriority>(HIGHEST - priority);
+}
+
 GURL GetUrlFromHeaderBlock(const SpdyHeaderBlock& headers,
                            int protocol_version,
                            bool pushed) {
diff --git a/net/spdy/spdy_http_utils.h b/net/spdy/spdy_http_utils.h
index 9f1e9e3..9ca09de 100644
--- a/net/spdy/spdy_http_utils.h
+++ b/net/spdy/spdy_http_utils.h
@@ -10,6 +10,7 @@
 #include "net/base/request_priority.h"
 #include "net/spdy/spdy_framer.h"
 #include "net/spdy/spdy_header_block.h"
+#include "net/spdy/spdy_protocol.h"
 
 namespace net {
 
@@ -49,6 +50,10 @@ NET_EXPORT_PRIVATE SpdyPriority ConvertRequestPriorityToSpdyPriority(
     RequestPriority priority,
     int protocol_version);
 
+NET_EXPORT_PRIVATE RequestPriority ConvertSpdyPriorityToRequestPriority(
+    SpdyPriority priority,
+    int protocol_version);
+
 }  // namespace net
 
 #endif  // NET_SPDY_SPDY_HTTP_UTILS_H_
diff --git a/net/spdy/spdy_http_utils_unittest.cc b/net/spdy/spdy_http_utils_unittest.cc
index 5e299a9..b198808 100644
--- a/net/spdy/spdy_http_utils_unittest.cc
+++ b/net/spdy/spdy_http_utils_unittest.cc
@@ -4,11 +4,12 @@
 
 #include "net/spdy/spdy_http_utils.h"
 
-#include "testing/platform_test.h"
+#include "base/basictypes.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 
-namespace test {
+namespace {
 
 TEST(SpdyHttpUtilsTest, ConvertRequestPriorityToSpdy2Priority) {
   EXPECT_EQ(0, ConvertRequestPriorityToSpdyPriority(HIGHEST, 2));
@@ -17,6 +18,7 @@ TEST(SpdyHttpUtilsTest, ConvertRequestPriorityToSpdy2Priority) {
   EXPECT_EQ(2, ConvertRequestPriorityToSpdyPriority(LOWEST, 2));
   EXPECT_EQ(3, ConvertRequestPriorityToSpdyPriority(IDLE, 2));
 }
+
 TEST(SpdyHttpUtilsTest, ConvertRequestPriorityToSpdy3Priority) {
   EXPECT_EQ(0, ConvertRequestPriorityToSpdyPriority(HIGHEST, 3));
   EXPECT_EQ(1, ConvertRequestPriorityToSpdyPriority(MEDIUM, 3));
@@ -25,6 +27,31 @@ TEST(SpdyHttpUtilsTest, ConvertRequestPriorityToSpdy3Priority) {
   EXPECT_EQ(4, ConvertRequestPriorityToSpdyPriority(IDLE, 3));
 }
 
+TEST(SpdyHttpUtilsTest, ConvertSpdy2PriorityToRequestPriority) {
+  EXPECT_EQ(HIGHEST, ConvertSpdyPriorityToRequestPriority(0, 2));
+  EXPECT_EQ(MEDIUM, ConvertSpdyPriorityToRequestPriority(1, 2));
+  EXPECT_EQ(LOW, ConvertSpdyPriorityToRequestPriority(2, 2));
+  EXPECT_EQ(IDLE, ConvertSpdyPriorityToRequestPriority(3, 2));
+  // These are invalid values, but we should still handle them
+  // gracefully.
+  for (int i = 4; i < kuint8max; ++i) {
+    EXPECT_EQ(IDLE, ConvertSpdyPriorityToRequestPriority(i, 2));
+  }
+}
+
+TEST(SpdyHttpUtilsTest, ConvertSpdy3PriorityToRequestPriority) {
+  EXPECT_EQ(HIGHEST, ConvertSpdyPriorityToRequestPriority(0, 3));
+  EXPECT_EQ(MEDIUM, ConvertSpdyPriorityToRequestPriority(1, 3));
+  EXPECT_EQ(LOW, ConvertSpdyPriorityToRequestPriority(2, 3));
+  EXPECT_EQ(LOWEST, ConvertSpdyPriorityToRequestPriority(3, 3));
+  EXPECT_EQ(IDLE, ConvertSpdyPriorityToRequestPriority(4, 3));
+  // These are invalid values, but we should still handle them
+  // gracefully.
+  for (int i = 5; i < kuint8max; ++i) {
+    EXPECT_EQ(IDLE, ConvertSpdyPriorityToRequestPriority(i, 3));
+  }
+}
+
 TEST(SpdyHttpUtilsTest, ShowHttpHeaderValue){
 #if defined(SPDY_PROXY_AUTH_ORIGIN)
   EXPECT_FALSE(ShouldShowHttpHeaderValue("proxy-authorization"));
@@ -35,6 +62,6 @@ TEST(SpdyHttpUtilsTest, ShowHttpHeaderValue){
 #endif
 }
 
-}  // namespace test
+}  // namespace
 
 }  // namespace net
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index c6c3819..21fcd13 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -556,13 +556,10 @@ int SpdySession::CreateStream(const SpdyStreamRequest& request,
   }
 
   const std::string& path = request.url().PathForRequest();
-
-  *stream = new SpdyStream(this, false, request.net_log());
-
-  (*stream)->set_priority(request.priority());
-  (*stream)->set_path(path);
-  (*stream)->set_send_window_size(stream_initial_send_window_size_);
-  (*stream)->set_recv_window_size(stream_initial_recv_window_size_);
+  *stream = new SpdyStream(this, path, request.priority(),
+                           stream_initial_send_window_size_,
+                           stream_initial_recv_window_size_,
+                           false, request.net_log());
   created_streams_.insert(*stream);
 
   UMA_HISTOGRAM_CUSTOM_COUNTS(
@@ -933,7 +930,7 @@ int SpdySession::DoRead() {
   return connection_->socket()->Read(
       read_buffer_.get(),
       kReadBufferSize,
-      base::Bind(&SpdySession::OnReadComplete, base::Unretained(this)));
+      base::Bind(&SpdySession::OnReadComplete, weak_factory_.GetWeakPtr()));
 }
 
 int SpdySession::DoReadComplete(int result) {
@@ -1079,7 +1076,7 @@ void SpdySession::WriteSocket() {
     int rv = connection_->socket()->Write(
         in_flight_write_.buffer(),
         in_flight_write_.buffer()->BytesRemaining(),
-        base::Bind(&SpdySession::OnWriteComplete, base::Unretained(this)));
+        base::Bind(&SpdySession::OnWriteComplete, weak_factory_.GetWeakPtr()));
     if (rv == net::ERR_IO_PENDING)
       break;
 
@@ -1554,13 +1551,15 @@ void SpdySession::OnSynStream(SpdyStreamId stream_id,
     return;
   }
 
-  scoped_refptr<SpdyStream> stream(new SpdyStream(this, true, net_log_));
+  RequestPriority request_priority =
+      ConvertSpdyPriorityToRequestPriority(priority, GetProtocolVersion());
+  scoped_refptr<SpdyStream> stream(
+      new SpdyStream(this, gurl.PathForRequest(), request_priority,
+                     stream_initial_send_window_size_,
+                     stream_initial_recv_window_size_,
+                     true, net_log_));
   stream->set_stream_id(stream_id);
 
-  stream->set_path(gurl.PathForRequest());
-  stream->set_send_window_size(stream_initial_send_window_size_);
-  stream->set_recv_window_size(stream_initial_recv_window_size_);
-
   DeleteExpiredPushedStreams();
   unclaimed_pushed_streams_[url] =
       std::pair<scoped_refptr<SpdyStream>, base::TimeTicks> (
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index 8451bb3..0d52b90 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -198,9 +198,6 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>,
     static SpdyIOBuffer* CreateIOBuffer(SpdyFrame* frame,
                                         RequestPriority priority,
                                         SpdyStream* spdy_stream);
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(SpdyIOBufferProducer);
   };
 
   // Create a new SpdySession.
diff --git a/net/spdy/spdy_session_spdy2_unittest.cc b/net/spdy/spdy_session_spdy2_unittest.cc
index aea35f3..2a2ff72 100644
--- a/net/spdy/spdy_session_spdy2_unittest.cc
+++ b/net/spdy/spdy_session_spdy2_unittest.cc
@@ -11,6 +11,7 @@
 #include "net/base/io_buffer.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_log_unittest.h"
+#include "net/base/request_priority.h"
 #include "net/base/test_data_directory.h"
 #include "net/base/test_data_stream.h"
 #include "net/spdy/spdy_io_buffer.h"
@@ -296,7 +297,10 @@ TEST_F(SpdySessionSpdy2Test, DeleteExpiredPushStreams) {
   (*request_headers)["url"] = "/";
 
   scoped_refptr<SpdyStream> stream(
-      new SpdyStream(session, false, session->net_log_));
+      new SpdyStream(session, std::string(), DEFAULT_PRIORITY,
+                     kSpdyStreamInitialWindowSize,
+                     kSpdyStreamInitialWindowSize,
+                     false, session->net_log_));
   stream->set_spdy_headers(request_headers.Pass());
   session->ActivateStream(stream);
 
diff --git a/net/spdy/spdy_session_spdy3_unittest.cc b/net/spdy/spdy_session_spdy3_unittest.cc
index 7bf48f7..ff62bf6 100644
--- a/net/spdy/spdy_session_spdy3_unittest.cc
+++ b/net/spdy/spdy_session_spdy3_unittest.cc
@@ -15,6 +15,7 @@
 #include "net/base/io_buffer.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_log_unittest.h"
+#include "net/base/request_priority.h"
 #include "net/base/test_data_directory.h"
 #include "net/base/test_data_stream.h"
 #include "net/spdy/spdy_http_utils.h"
@@ -317,7 +318,10 @@ TEST_F(SpdySessionSpdy3Test, DeleteExpiredPushStreams) {
   (*request_headers)[":path"] = "/";
 
   scoped_refptr<SpdyStream> stream(
-      new SpdyStream(session, false, session->net_log_));
+      new SpdyStream(session, std::string(), DEFAULT_PRIORITY,
+                     kSpdyStreamInitialWindowSize,
+                     kSpdyStreamInitialWindowSize,
+                     false, session->net_log_));
   stream->set_spdy_headers(request_headers.Pass());
   session->ActivateStream(stream);
 
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index 6767821..e20fb8d 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -5,6 +5,7 @@
 #include "net/spdy/spdy_stream.h"
 
 #include "base/bind.h"
+#include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/stringprintf.h"
@@ -50,15 +51,21 @@ bool ContainsUpperAscii(const std::string& str) {
 }  // namespace
 
 SpdyStream::SpdyStream(SpdySession* session,
+                       const std::string& path,
+                       RequestPriority priority,
+                       int32 initial_send_window_size,
+                       int32 initial_recv_window_size,
                        bool pushed,
                        const BoundNetLog& net_log)
-    : continue_buffering_data_(true),
+    : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      continue_buffering_data_(true),
       stream_id_(0),
-      priority_(HIGHEST),
+      path_(path),
+      priority_(priority),
       slot_(0),
       send_stalled_by_flow_control_(false),
-      send_window_size_(kSpdyStreamInitialWindowSize),
-      recv_window_size_(kSpdyStreamInitialWindowSize),
+      send_window_size_(initial_send_window_size),
+      recv_window_size_(initial_recv_window_size),
       unacked_recv_window_bytes_(0),
       pushed_(pushed),
       response_received_(false),
@@ -739,7 +746,7 @@ int SpdyStream::DoGetDomainBoundCert() {
       GetUrl().GetOrigin().spec(), requested_cert_types,
       &domain_bound_cert_type_, &domain_bound_private_key_, &domain_bound_cert_,
       base::Bind(&SpdyStream::OnGetDomainBoundCertComplete,
-                 base::Unretained(this)),
+                 weak_ptr_factory_.GetWeakPtr()),
       &domain_bound_cert_request_handle_);
   return rv;
 }
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index db04f68..3ad34f9 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -12,6 +12,7 @@
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/bandwidth_metrics.h"
 #include "net/base/io_buffer.h"
@@ -112,6 +113,10 @@ class NET_EXPORT_PRIVATE SpdyStream
 
   // SpdyStream constructor
   SpdyStream(SpdySession* session,
+             const std::string& path,
+             RequestPriority priority,
+             int32 initial_send_window_size,
+             int32 initial_recv_window_size,
              bool pushed,
              const BoundNetLog& net_log);
 
@@ -136,20 +141,12 @@ class NET_EXPORT_PRIVATE SpdyStream
 
   // For pushed streams, we track a path to identify them.
   const std::string& path() const { return path_; }
-  void set_path(const std::string& path) { path_ = path; }
 
   RequestPriority priority() const { return priority_; }
-  void set_priority(RequestPriority priority) { priority_ = priority; }
 
   int32 send_window_size() const { return send_window_size_; }
-  void set_send_window_size(int32 window_size) {
-    send_window_size_ = window_size;
-  }
 
   int32 recv_window_size() const { return recv_window_size_; }
-  void set_recv_window_size(int32 window_size) {
-    recv_window_size_ = window_size;
-  }
 
   bool send_stalled_by_flow_control() { return send_stalled_by_flow_control_; }
 
@@ -364,14 +361,16 @@ class NET_EXPORT_PRIVATE SpdyStream
   // this stream's receive window size to go negative.
   void DecreaseRecvWindowSize(int32 delta_window_size);
 
+  base::WeakPtrFactory<SpdyStream> weak_ptr_factory_;
+
   // There is a small period of time between when a server pushed stream is
   // first created, and the pushed data is replayed. Any data received during
   // this time should continue to be buffered.
   bool continue_buffering_data_;
 
   SpdyStreamId stream_id_;
-  std::string path_;
-  RequestPriority priority_;
+  const std::string path_;
+  const RequestPriority priority_;
   size_t slot_;
 
   // Flow control variables.
diff --git a/net/spdy/spdy_stream_spdy2_unittest.cc b/net/spdy/spdy_stream_spdy2_unittest.cc
index 755e55c..564215e 100644
--- a/net/spdy/spdy_stream_spdy2_unittest.cc
+++ b/net/spdy/spdy_stream_spdy2_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/string_piece.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_log_unittest.h"
+#include "net/base/request_priority.h"
 #include "net/spdy/buffered_spdy_framer.h"
 #include "net/spdy/spdy_stream.h"
 #include "net/spdy/spdy_http_utils.h"
@@ -233,9 +234,14 @@ TEST_F(SpdyStreamSpdy2Test, PushedStream) {
   BoundNetLog net_log;
 
   // Conjure up a stream.
-  scoped_refptr<SpdyStream> stream = new SpdyStream(spdy_session,
-                                                    true,
-                                                    net_log);
+  scoped_refptr<SpdyStream> stream =
+      new SpdyStream(spdy_session,
+                     std::string(),
+                     DEFAULT_PRIORITY,
+                     kSpdyStreamInitialWindowSize,
+                     kSpdyStreamInitialWindowSize,
+                     true,
+                     net_log);
   stream->set_stream_id(2);
   EXPECT_FALSE(stream->response_received());
   EXPECT_FALSE(stream->HasUrl());
diff --git a/net/spdy/spdy_stream_spdy3_unittest.cc b/net/spdy/spdy_stream_spdy3_unittest.cc
index a50f8dc..a465c07 100644
--- a/net/spdy/spdy_stream_spdy3_unittest.cc
+++ b/net/spdy/spdy_stream_spdy3_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/string_piece.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_log_unittest.h"
+#include "net/base/request_priority.h"
 #include "net/spdy/buffered_spdy_framer.h"
 #include "net/spdy/spdy_stream.h"
 #include "net/spdy/spdy_http_utils.h"
@@ -230,9 +231,14 @@ TEST_F(SpdyStreamSpdy3Test, PushedStream) {
   BoundNetLog net_log;
 
   // Conjure up a stream.
-  scoped_refptr<SpdyStream> stream = new SpdyStream(spdy_session,
-                                                    true,
-                                                    net_log);
+  scoped_refptr<SpdyStream> stream =
+      new SpdyStream(spdy_session,
+                     std::string(),
+                     DEFAULT_PRIORITY,
+                     kSpdyStreamInitialWindowSize,
+                     kSpdyStreamInitialWindowSize,
+                     true,
+                     net_log);
   stream->set_stream_id(2);
   EXPECT_FALSE(stream->response_received());
   EXPECT_FALSE(stream->HasUrl());