Move work done in CreateAndConnectStreamWithCreateHelper into StreamRequestImpl's ctor

Move work done in CreateAndConnectStreamWithCreateHelper into StreamRequestImpl's ctor

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272882

[Adam Rice, 2014-05-22 08:36:13]

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
net/websockets/websocket_stream.cc:75: create_helper_(create_helper.get()) {
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.

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
net/websockets/websocket_stream.cc:90: url_request_.Start();
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.

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
net/websockets/websocket_stream.cc:200: return
request.PassAs<WebSocketStreamRequest>();
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]

Thanks

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
net/websockets/websocket_stream.cc:75: create_helper_(create_helper.get()) {
On 2014/05/22 08:36:14, Adam Rice wrote:
> 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.

Done.

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
net/websockets/websocket_stream.cc:90: url_request_.Start();
On 2014/05/22 08:36:14, Adam Rice wrote:
> 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.

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?

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
net/websockets/websocket_stream.cc:200: return
request.PassAs<WebSocketStreamRequest>();
On 2014/05/22 08:36:14, Adam Rice wrote:
> 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?

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]

lgtm

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
net/websockets/websocket_stream.cc:90: url_request_.Start();
On 2014/05/22 09:11:00, tyoshino wrote:
> On 2014/05/22 08:36:14, Adam Rice wrote:
> > 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.
> 
> 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?

Agreed.

https://codereview.chromium.org/291343004/diff/1/net/websockets/websocket_str...
net/websockets/websocket_stream.cc:200: return
request.PassAs<WebSocketStreamRequest>();
On 2014/05/22 09:11:00, tyoshino wrote:
> On 2014/05/22 08:36:14, Adam Rice wrote:
> > 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?
> 
> 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 :)

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

[tyoshino (SeeGerritForStatus), 2014-05-26 09:32:01]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-05-26 09:32:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tyoshino@chromium.org/291343004/40001

[commit-bot: I haz the power, 2014-05-26 23:44:19]

Change committed as 272882