Changes to HttpNetworkTransaction for WebSocket Handshake

Add a SetWebSocketHandshakeStreamFactory() method to the HttpTransaction base class so that
URLRequestHttpJob can pass through the WebSocketHandshakeStreamBase::Factory object.

Implement OnWebSocketHandshakeStreamReady().

BUG=315027
TEST=net_unittests

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

[yhirano, 2013-11-06 05:00:04]

Is the following description correct?
  Implement OnWebSocketStreamReady().

https://codereview.chromium.org/23856018/diff/156001/net/http/http_network_tr...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/23856018/diff/156001/net/http/http_network_tr...
net/http/http_network_transaction.cc:1013:
!websocket_handshake_stream_base_factory_) {
Code in DoCreateStream does:
  if websocket_handshake_stream_base_factory_ and the request scheme is
WebSocket then
    treat the request as a WebSocket request
  else
    treat the request as a Http request

Code here does:
  if websocket_handshake_stream_base_factory_
    treat the request as a WebSocket request
  else
    treat the request as a Http request

I don't know which is correct, but I think they should be consistent.

[Adam Rice, 2013-11-07 01:45:22]

I updated the description.

https://codereview.chromium.org/23856018/diff/156001/net/http/http_network_tr...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/23856018/diff/156001/net/http/http_network_tr...
net/http/http_network_transaction.cc:1013:
!websocket_handshake_stream_base_factory_) {
On 2013/11/06 05:00:05, yhirano wrote:
> Code in DoCreateStream does:
>   if websocket_handshake_stream_base_factory_ and the request scheme is
> WebSocket then
>     treat the request as a WebSocket request
>   else
>     treat the request as a Http request
> 
> Code here does:
>   if websocket_handshake_stream_base_factory_
>     treat the request as a WebSocket request
>   else
>     treat the request as a Http request
> 
> I don't know which is correct, but I think they should be consistent.

I tried factoring out the code into a ForWebSocketHandshake() method, and using
that in both places. PTAL.

[yhirano, 2013-11-07 04:12:32]

lgtm

[tyoshino (SeeGerritForStatus), 2013-11-07 07:41:44]

lgtm

[Adam Rice, 2013-11-07 07:45:03]

+szym for OWNERS review.

[szym, 2013-11-08 20:45:51]

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_se...
File net/http/http_network_session_peer.h (right):

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_se...
net/http/http_network_session_peer.h:29: void
SetHttpStreamFactory(HttpStreamFactory* http_stream_factory);
Both of these (and SetClientSocketPoolManager) should accept scoped_ptr<...>. At
least add a comment that network session takes ownership.

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_tr...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_tr...
net/http/http_network_transaction_unittest.cc:11963:
FakeStreamRequest(RequestPriority priority,
nit: Add blank line after multi-line implementation.

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_tr...
net/http/http_network_transaction_unittest.cc:12215:
TEST_P(HttpNetworkTransactionTest, CreateWebSocketSecureHandshakeStream) {
These two tests are identical except the url. Suggest wrapping up in a simple
"test case" loop.

std::string test_cases[] = {
  "ws://...", "wss://...",
};

for (int i = 0; i < arraysize(test_cases); ++i) {
  // test body
  request.url = GURL(test_cases[i]);
  // test body
}

Alternatively, use parameterized unit test:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...

[Adam Rice, 2013-11-11 02:06:22]

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_se...
File net/http/http_network_session_peer.h (right):

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_se...
net/http/http_network_session_peer.h:29: void
SetHttpStreamFactory(HttpStreamFactory* http_stream_factory);
On 2013/11/08 20:45:51, szym wrote:
> Both of these (and SetClientSocketPoolManager) should accept scoped_ptr<...>.
At
> least add a comment that network session takes ownership.

Done.

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_tr...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_tr...
net/http/http_network_transaction_unittest.cc:11963:
FakeStreamRequest(RequestPriority priority,
On 2013/11/08 20:45:51, szym wrote:
> nit: Add blank line after multi-line implementation.

Done.

https://codereview.chromium.org/23856018/diff/216001/net/http/http_network_tr...
net/http/http_network_transaction_unittest.cc:12215:
TEST_P(HttpNetworkTransactionTest, CreateWebSocketSecureHandshakeStream) {
On 2013/11/08 20:45:51, szym wrote:
> These two tests are identical except the url. Suggest wrapping up in a simple
> "test case" loop.
> 
> std::string test_cases[] = {
>   "ws://...", "wss://...",
> };
> 
> for (int i = 0; i < arraysize(test_cases); ++i) {
>   // test body
>   request.url = GURL(test_cases[i]);
>   // test body
> }
> 
> Alternatively, use parameterized unit test:
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...

Done.

[szym, 2013-11-13 22:33:14]

lgtm
apologies for the delay

[commit-bot: I haz the power, 2013-11-13 23:12:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ricea@chromium.org/23856018/506001

[commit-bot: I haz the power, 2013-11-14 02:14:45]

Change committed as 235016