Move work done in CreateAndConnectStreamWithCreateHelper into StreamRequestImpl's ctor

net/websockets/websocket_stream.cc

Index: net/websockets/websocket_stream.cc
diff --git a/net/websockets/websocket_stream.cc b/net/websockets/websocket_stream.cc
index 399b8f0b0911e23c1c1a2acbfa8a5ac90cf22349..5297e09e238f7a65bcfc9114d6a64bf4917897b9 100644
--- a/net/websockets/websocket_stream.cc
+++ b/net/websockets/websocket_stream.cc
@@ -66,19 +66,34 @@ class StreamRequestImpl : public WebSocketStreamRequest {
   StreamRequestImpl(
       const GURL& url,
       const URLRequestContext* context,
+      const url::Origin& origin,
       scoped_ptr<WebSocketStream::ConnectDelegate> connect_delegate,
-      WebSocketHandshakeStreamCreateHelper* create_helper)
+      scoped_ptr<WebSocketHandshakeStreamCreateHelper> create_helper)
       : delegate_(new Delegate(this)),
         url_request_(url, DEFAULT_PRIORITY, delegate_.get(), context),
         connect_delegate_(connect_delegate.Pass()),
-        create_helper_(create_helper) {}
+        create_helper_(create_helper.get()) {

[Adam Rice, 2014/05/22 08:36:14]

Suggestion: use .release() here and then use create_helper_ instead of
create_helper.release() below. It will still be confusing but I think it will
make the ownership semantics a little bit clearer.

[tyoshino (SeeGerritForStatus), 2014/05/22 09:11:00]

Done.

+    HttpRequestHeaders headers;
+    headers.SetHeader(websockets::kUpgrade, websockets::kWebSocketLowercase);
+    headers.SetHeader(HttpRequestHeaders::kConnection, websockets::kUpgrade);
+    headers.SetHeader(HttpRequestHeaders::kOrigin, origin.string());
+    headers.SetHeader(websockets::kSecWebSocketVersion,
+                      websockets::kSupportedVersion);
+    url_request_.SetExtraRequestHeaders(headers);
+
+    url_request_.SetUserData(
+        WebSocketHandshakeStreamBase::CreateHelper::DataKey(),
+        create_helper.release());
+    url_request_.SetLoadFlags(LOAD_DISABLE_CACHE |
+                              LOAD_BYPASS_CACHE |
+                              LOAD_DO_NOT_PROMPT_FOR_LOGIN);
+    url_request_.Start();

[Adam Rice, 2014/05/22 08:36:14]

I am not sure about this. On the one hand, it violates the "no work in
constructors" rule from the style guide. The rest of the code can be considered
initialisation, but URLRequest::Start() indisputably does real work.

On the other hand, it makes the code neater to put it here.

[tyoshino (SeeGerritForStatus), 2014/05/22 09:11:00]

I was also wondering if Start() should be run outside or not. You're right.
Separated it.

Regarding the rest:
I'm fine with putting them in the new Start() method. I just wanted to move
these stuffs inside StreamRequestImpl. But then, I create_helper_ will be
orphaned between ctor and Start(). I can hold it by a scoped_ptr member, but
we'll immediately release it in Start() and after that we need to hold a raw
pointer instead for later stream() call. Can I keep them here by considering
them as initialisation?

[Adam Rice, 2014/05/22 09:22:02]

Agreed.

+  }
 
   // Destroying this object destroys the URLRequest, which cancels the request
   // and so terminates the handshake if it is incomplete.
   virtual ~StreamRequestImpl() {}
 
-  URLRequest* url_request() { return &url_request_; }
-
   void PerformUpgrade() {
     connect_delegate_->OnSuccess(create_helper_->stream()->Upgrade());
   }
@@ -157,37 +172,6 @@ void Delegate::OnReadCompleted(URLRequest* request, int bytes_read) {
   NOTREACHED();
 }
 
-// Internal implementation of CreateAndConnectStream and
-// CreateAndConnectStreamForTesting.
-scoped_ptr<WebSocketStreamRequest> CreateAndConnectStreamWithCreateHelper(
-    const GURL& socket_url,
-    scoped_ptr<WebSocketHandshakeStreamCreateHelper> create_helper,
-    const url::Origin& origin,
-    URLRequestContext* url_request_context,
-    const BoundNetLog& net_log,
-    scoped_ptr<WebSocketStream::ConnectDelegate> connect_delegate) {
-  scoped_ptr<StreamRequestImpl> request(
-      new StreamRequestImpl(socket_url,
-                            url_request_context,
-                            connect_delegate.Pass(),
-                            create_helper.get()));
-  HttpRequestHeaders headers;
-  headers.SetHeader(websockets::kUpgrade, websockets::kWebSocketLowercase);
-  headers.SetHeader(HttpRequestHeaders::kConnection, websockets::kUpgrade);
-  headers.SetHeader(HttpRequestHeaders::kOrigin, origin.string());
-  headers.SetHeader(websockets::kSecWebSocketVersion,
-                    websockets::kSupportedVersion);
-  request->url_request()->SetExtraRequestHeaders(headers);
-  request->url_request()->SetUserData(
-      WebSocketHandshakeStreamBase::CreateHelper::DataKey(),
-      create_helper.release());
-  request->url_request()->SetLoadFlags(LOAD_DISABLE_CACHE |
-                                       LOAD_BYPASS_CACHE |
-                                       LOAD_DO_NOT_PROMPT_FOR_LOGIN);
-  request->url_request()->Start();
-  return request.PassAs<WebSocketStreamRequest>();
-}
-
 }  // namespace
 
 WebSocketStreamRequest::~WebSocketStreamRequest() {}
@@ -207,12 +191,13 @@ scoped_ptr<WebSocketStreamRequest> WebSocketStream::CreateAndConnectStream(
   scoped_ptr<WebSocketHandshakeStreamCreateHelper> create_helper(
       new WebSocketHandshakeStreamCreateHelper(connect_delegate.get(),
                                                requested_subprotocols));
-  return CreateAndConnectStreamWithCreateHelper(socket_url,
-                                                create_helper.Pass(),
-                                                origin,
-                                                url_request_context,
-                                                net_log,
-                                                connect_delegate.Pass());
+  scoped_ptr<WebSocketStreamRequest> request(
+      new StreamRequestImpl(socket_url,
+                            url_request_context,
+                            origin,
+                            connect_delegate.Pass(),
+                            create_helper.Pass()));
+  return request.PassAs<WebSocketStreamRequest>();

[Adam Rice, 2014/05/22 08:36:14]

You don't need PassAs<>() here. Plain Pass() should work.

Alternatively, how about not creating the "request" variable and using
make_scoped_ptr() directly in the return statement?

[tyoshino (SeeGerritForStatus), 2014/05/22 09:11:00]

Oh, right! As I separated Start() call on URLRequest as the Start() method, I
need to create a local scoped_ptr instance. And, as we wanna keep it a member of
StreamRequestImpl, not WebSocketStreamRequest, I need PassAs now :)

[Adam Rice, 2014/05/22 09:22:02]

I did have the evil idea of having Start() return a scoped_ptr<> to |this|, but
that would just confuse people. This is fine.

 }
 
 // This is declared in websocket_test_util.h.
@@ -223,12 +208,13 @@ scoped_ptr<WebSocketStreamRequest> CreateAndConnectStreamForTesting(
     URLRequestContext* url_request_context,
     const BoundNetLog& net_log,
     scoped_ptr<WebSocketStream::ConnectDelegate> connect_delegate) {
-  return CreateAndConnectStreamWithCreateHelper(socket_url,
-                                                create_helper.Pass(),
-                                                origin,
-                                                url_request_context,
-                                                net_log,
-                                                connect_delegate.Pass());
+  scoped_ptr<WebSocketStreamRequest> request(
+      new StreamRequestImpl(socket_url,
+                            url_request_context,
+                            origin,
+                            connect_delegate.Pass(),
+                            create_helper.Pass()));
+  return request.PassAs<WebSocketStreamRequest>();
 }
 
 }  // namespace net