Move work done in CreateAndConnectStreamWithCreateHelper into StreamRequestImpl's ctor

net/websockets/websocket_stream.cc

 // Copyright 2013 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/websockets/websocket_stream.h"
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "net/http/http_request_headers.h"
@@ skipping 48 matching lines @@
  private:
   StreamRequestImpl* owner_;
   HandshakeResult result_;
 };
 
 class StreamRequestImpl : public WebSocketStreamRequest {
  public:
   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());
   }
 
   void ReportFailure() {
     std::string failure_message;
     if (create_helper_->stream()) {
       failure_message = create_helper_->stream()->GetFailureMessage();
     } else {
       switch (url_request_.status().status()) {
@@ skipping 58 matching lines @@
 void Delegate::OnSSLCertificateError(URLRequest* request,
                                      const SSLInfo& ssl_info,
                                      bool fatal) {
   request->Cancel();
 }
 
 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() {}
 
 WebSocketStream::WebSocketStream() {}
 WebSocketStream::~WebSocketStream() {}
 
 WebSocketStream::ConnectDelegate::~ConnectDelegate() {}
 
 scoped_ptr<WebSocketStreamRequest> WebSocketStream::CreateAndConnectStream(
     const GURL& socket_url,
     const std::vector<std::string>& requested_subprotocols,
     const url::Origin& origin,
     URLRequestContext* url_request_context,
     const BoundNetLog& net_log,
     scoped_ptr<ConnectDelegate> connect_delegate) {
   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.
 scoped_ptr<WebSocketStreamRequest> CreateAndConnectStreamForTesting(
     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) {
-  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