Fail WebSocket channel when handshake fails.

Fail WebSocket channel when handshake fails.

Call WebSocketMsg_NotifyFailure instead of
WebSocketMsg_AddChannelResponse(true, ...) when the opening handshake fails.

BUG=310405
R=ricea@chromium.org

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

[Adam Rice, 2013-12-06 07:02:30]

Thank you for doing this.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:100: *failure_message =
"'Sec-WebSocket-Accept' header is missing";
In ValidateUpgrade() and ValidateSecWebSocketAccept(), the first two error
messages are the same except for the name of the header. I wonder if it would be
cleaner to share the code for that part?

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:118: std::string*
failure_message) {
Indentation.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:147: &state,
websockets::kSecWebSocketProtocol, &token) &&
The indentation looks weird here. I would expect it to be indented four spaces
from the '(' on the previous line. I am not sure though.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:176: "but no response is
received";
Grammatically, this should be "no response was received", but since we are
aiming to match the messages for the layout tests we should keep this as it is,
at least while we have two implementations.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_channel.h:203: // operation. Needs renaming &
refactoring.
It is certainly intended to implement the "Fail the WebSocket Connection"
operation as defined in RFC6455. In practice it is only used for protocol errors
after the handshake has completed.

The interface does need revising to allow for logging console messages.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_stream.cc:74: if (create_helper_->stream())
Calling url_request_.status().error() can give you information about why the
connection failed in the case that no stream was ever created. Although I'm not
sure how this method gets called in that case.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:57: std::string sub_protocols_header;
This looks too much like an implementation to appear in a unit test, in my
opinion. If someone changes a test and this logic and the logic in
websocket_basic_handshake_stream.cc both fail in the same way, we will never
know about it.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:385: EXPECT_EQ("", failure_message());
It will be hard to produce a good failure message for this case, since it is
treated as a valid response. We may have to use the same "Invalid status code"
message as for the test above.

[yhirano, 2013-12-06 08:54:56]

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:100: *failure_message =
"'Sec-WebSocket-Accept' header is missing";
On 2013/12/06 07:02:31, Adam Rice wrote:
> In ValidateUpgrade() and ValidateSecWebSocketAccept(), the first two error
> messages are the same except for the name of the header. I wonder if it would
be
> cleaner to share the code for that part?

Done.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:118: std::string*
failure_message) {
On 2013/12/06 07:02:31, Adam Rice wrote:
> Indentation.

Done.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:147: &state,
websockets::kSecWebSocketProtocol, &token) &&
On 2013/12/06 07:02:31, Adam Rice wrote:
> The indentation looks weird here. I would expect it to be indented four spaces
> from the '(' on the previous line. I am not sure though.

Done.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:176: "but no response is
received";
On 2013/12/06 07:02:31, Adam Rice wrote:
> Grammatically, this should be "no response was received", but since we are
> aiming to match the messages for the layout tests we should keep this as it
is,
> at least while we have two implementations.

Done.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_channel.h:203: // operation. Needs renaming &
refactoring.
On 2013/12/06 07:02:31, Adam Rice wrote:
> It is certainly intended to implement the "Fail the WebSocket Connection"
> operation as defined in RFC6455. In practice it is only used for protocol
errors
> after the handshake has completed.
> 
> The interface does need revising to allow for logging console messages.
My understanding is that "_Fail the WebSocket Connection_" and just closing the
connection should be distinguished, is that right?
For example, WebSocketChannel::ProcessFrame calls FailChannel if the incoming
frame is masked, but the spec says that "a client MUST close a connection".
Currently FailChannels does NOT for "_Fail the WebSocket Connection_" but for
close. In other word, WebSocketChannel::ProcessFrame is correct -- just the
method name is wrong.
Thus I think renaming is needed.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_stream.cc:74: if (create_helper_->stream())
On 2013/12/06 07:02:31, Adam Rice wrote:
> Calling url_request_.status().error() can give you information about why the
> connection failed in the case that no stream was ever created. Although I'm
not
> sure how this method gets called in that case.

Done.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:57: std::string sub_protocols_header;
On 2013/12/06 07:02:31, Adam Rice wrote:
> This looks too much like an implementation to appear in a unit test, in my
> opinion. If someone changes a test and this logic and the logic in
> websocket_basic_handshake_stream.cc both fail in the same way, we will never
> know about it.

Done.

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_stream_test.cc:385: EXPECT_EQ("", failure_message());
On 2013/12/06 07:02:31, Adam Rice wrote:
> It will be hard to produce a good failure message for this case, since it is
> treated as a valid response. We may have to use the same "Invalid status code"
> message as for the test above.

Done.

[Adam Rice, 2013-12-09 01:21:41]

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_channel.h:203: // operation. Needs renaming &
refactoring.
On 2013/12/06 08:54:56, yhirano wrote:
> On 2013/12/06 07:02:31, Adam Rice wrote:
> > It is certainly intended to implement the "Fail the WebSocket Connection"
> > operation as defined in RFC6455. In practice it is only used for protocol
> errors
> > after the handshake has completed.
> > 
> > The interface does need revising to allow for logging console messages.
> My understanding is that "_Fail the WebSocket Connection_" and just closing
the
> connection should be distinguished, is that right?
> For example, WebSocketChannel::ProcessFrame calls FailChannel if the incoming
> frame is masked, but the spec says that "a client MUST close a connection".
> Currently FailChannels does NOT for "_Fail the WebSocket Connection_" but for
> close. In other word, WebSocketChannel::ProcessFrame is correct -- just the
> method name is wrong.
> Thus I think renaming is needed.
> 

The spec goes on to say "In this case, it MAY use the status code 1002",
implying that it sends a Close frame before closing the TCP/IP connection. In
which case the behaviour is identical to _Fail the WebSocket Connection_ and it
is just a mistake in the RFC that it doesn't use that wording.

The WHATWG WebSocket API appears to require us to _Close the WebSocket
Connection_ without performing _Fail the WebSocket connection_ if the output
buffer is full (
http://www.whatwg.org/specs/web-apps/current-work/multipage/network.html#dom-...
). This wording appears to be new (replacing the old circular definition of
"with prejudice") but creates an unhelpful special case.

[yhirano, 2013-12-09 06:11:13]

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
File net/websockets/websocket_channel.h (right):

https://codereview.chromium.org/105833003/diff/20001/net/websockets/websocket...
net/websockets/websocket_channel.h:203: // operation. Needs renaming &
refactoring.
On 2013/12/09 01:21:41, Adam Rice wrote:
> On 2013/12/06 08:54:56, yhirano wrote:
> > On 2013/12/06 07:02:31, Adam Rice wrote:
> > > It is certainly intended to implement the "Fail the WebSocket Connection"
> > > operation as defined in RFC6455. In practice it is only used for protocol
> > errors
> > > after the handshake has completed.
> > > 
> > > The interface does need revising to allow for logging console messages.
> > My understanding is that "_Fail the WebSocket Connection_" and just closing
> the
> > connection should be distinguished, is that right?
> > For example, WebSocketChannel::ProcessFrame calls FailChannel if the
incoming
> > frame is masked, but the spec says that "a client MUST close a connection".
> > Currently FailChannels does NOT for "_Fail the WebSocket Connection_" but
for
> > close. In other word, WebSocketChannel::ProcessFrame is correct -- just the
> > method name is wrong.
> > Thus I think renaming is needed.
> > 
> 
> The spec goes on to say "In this case, it MAY use the status code 1002",
> implying that it sends a Close frame before closing the TCP/IP connection. In
> which case the behaviour is identical to _Fail the WebSocket Connection_ and
it
> is just a mistake in the RFC that it doesn't use that wording.
> 
> The WHATWG WebSocket API appears to require us to _Close the WebSocket
> Connection_ without performing _Fail the WebSocket connection_ if the output
> buffer is full (
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/network.html#dom-...
> ). This wording appears to be new (replacing the old circular definition of
> "with prejudice") but creates an unhelpful special case.

Discussed offline.
I deleted the comment.
http://crbug.com/326899 will fix FailChannel.

[Adam Rice, 2013-12-09 07:41:12]

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
net/websockets/websocket_channel_test.cc:1160: // true means failure.
Please remove this "// true means failure" comment as it is meaningless now.

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
File net/websockets/websocket_handshake_stream_base.h (right):

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
net/websockets/websocket_handshake_stream_base.h:69: // Returns a string
describes the connection failure detailed information.
s/describes/describing/

"detailed information" sounds strange here; probably better to use just
"information" or "reason".

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
net/websockets/websocket_handshake_stream_base.h:70: // Returns an empty string
if there is no such failure.
The word "such" is not needed here.

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
File net/websockets/websocket_stream.h (right):

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
net/websockets/websocket_stream.h:61: // |message| is the detailed information
of the failure.
You could say "|message| contains detailed information about the failure" or
just "|message| contains details of the failure".

[yhirano, 2013-12-09 07:48:09]

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
net/websockets/websocket_channel_test.cc:1160: // true means failure.
On 2013/12/09 07:41:12, Adam Rice wrote:
> Please remove this "// true means failure" comment as it is meaningless now.

Done.

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
File net/websockets/websocket_handshake_stream_base.h (right):

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
net/websockets/websocket_handshake_stream_base.h:69: // Returns a string
describes the connection failure detailed information.
On 2013/12/09 07:41:12, Adam Rice wrote:
> s/describes/describing/
> 
> "detailed information" sounds strange here; probably better to use just
> "information" or "reason".

Done.

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
net/websockets/websocket_handshake_stream_base.h:70: // Returns an empty string
if there is no such failure.
On 2013/12/09 07:41:12, Adam Rice wrote:
> The word "such" is not needed here.

Done.

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
File net/websockets/websocket_stream.h (right):

https://codereview.chromium.org/105833003/diff/60001/net/websockets/websocket...
net/websockets/websocket_stream.h:61: // |message| is the detailed information
of the failure.
On 2013/12/09 07:41:12, Adam Rice wrote:
> You could say "|message| contains detailed information about the failure" or
> just "|message| contains details of the failure".

Done.

[Adam Rice, 2013-12-09 08:09:48]

lgtm

[yhirano, 2013-12-09 08:16:06]

+tyoshino for net/websockets/
+szym for net/

[szym, 2013-12-09 18:49:58]

lgtm

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:8: #include <iterator>
include <string>

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:40: std::string
MissingHeader(const std::string& header_name) {
MissingHeaderMessage or ...Msg

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
File net/websockets/websocket_handshake_stream_base.h (right):

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
net/websockets/websocket_handshake_stream_base.h:71: virtual std::string
FailureMessage() const = 0;
By convention this should be GetFailureMessage()

[yhirano, 2013-12-10 03:30:09]

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:8: #include <iterator>
On 2013/12/09 18:49:58, szym wrote:
> include <string>

Done.

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
net/websockets/websocket_basic_handshake_stream.cc:40: std::string
MissingHeader(const std::string& header_name) {
On 2013/12/09 18:49:58, szym wrote:
> MissingHeaderMessage or ...Msg

Done.

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
File net/websockets/websocket_handshake_stream_base.h (right):

https://codereview.chromium.org/105833003/diff/70016/net/websockets/websocket...
net/websockets/websocket_handshake_stream_base.h:71: virtual std::string
FailureMessage() const = 0;
On 2013/12/09 18:49:58, szym wrote:
> By convention this should be GetFailureMessage()

Done.

[tyoshino (SeeGerritForStatus), 2013-12-16 07:26:18]

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:208: &state,
websockets::kSecWebSocketExtensions, &token)) {
let's change "token" to some other name since "token" is used in the ABNF of
Sec-WebSocket-Extensions header.

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:214: "'WebSocket-Extensions'
header value is rejected by the parser: " +
Sec-

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_channel_test.cc:968: deleting_ = EVENT_ON_FAIL_CHANNEL
| EVENT_ON_DROP_CHANNEL;
why EVENT_ON_DROP_CHANNEL are kept

the same question for the rest.

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_stream_test.cc:57: std::string sub_protocols_header;
what's this?

[yhirano, 2013-12-16 08:05:37]

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:208: &state,
websockets::kSecWebSocketExtensions, &token)) {
On 2013/12/16 07:26:18, tyoshino wrote:
> let's change "token" to some other name since "token" is used in the ABNF of
> Sec-WebSocket-Extensions header.

Done.

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:214: "'WebSocket-Extensions'
header value is rejected by the parser: " +
On 2013/12/16 07:26:18, tyoshino wrote:
> Sec-

Done.

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_channel_test.cc:968: deleting_ = EVENT_ON_FAIL_CHANNEL
| EVENT_ON_DROP_CHANNEL;
On 2013/12/16 07:26:18, tyoshino wrote:
> why EVENT_ON_DROP_CHANNEL are kept
> 
> the same question for the rest.
Because OnDropChannel always drops the channel and returns CHANNEL_DELETED.

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_stream_test.cc:57: std::string sub_protocols_header;
On 2013/12/16 07:26:18, tyoshino wrote:
> what's this?
Deleted

[tyoshino (SeeGerritForStatus), 2014-01-09 04:08:47]

lg

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_channel_test.cc:968: deleting_ = EVENT_ON_FAIL_CHANNEL
| EVENT_ON_DROP_CHANNEL;
On 2013/12/16 08:05:38, yhirano wrote:
> On 2013/12/16 07:26:18, tyoshino wrote:
> > why EVENT_ON_DROP_CHANNEL are kept
> > 
> > the same question for the rest.
> Because OnDropChannel always drops the channel and returns CHANNEL_DELETED. 

Oh. The cases in which the channel is normally deleted are just listed?

https://codereview.chromium.org/105833003/diff/170001/net/websockets/websocke...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/170001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:166: !(has_multiple_protocols
&& has_invalid_protocol)) {
how about
- just breaking at each if-clause
- do EnumerateHeader() inside the while block and put last_value (possibly
renamed to parsed_value or something) in the while block and substitute it to
value only when the EnumerateHeader() call is success?

https://codereview.chromium.org/105833003/diff/170001/net/websockets/websocke...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/105833003/diff/170001/net/websockets/websocke...
net/websockets/websocket_stream.cc:78: if (status.status() ==
URLRequestStatus::CANCELED) {
use switch?

[yhirano, 2014-01-09 05:24:51]

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/105833003/diff/110001/net/websockets/websocke...
net/websockets/websocket_channel_test.cc:968: deleting_ = EVENT_ON_FAIL_CHANNEL
| EVENT_ON_DROP_CHANNEL;
On 2014/01/09 04:08:48, tyoshino wrote:
> On 2013/12/16 08:05:38, yhirano wrote:
> > On 2013/12/16 07:26:18, tyoshino wrote:
> > > why EVENT_ON_DROP_CHANNEL are kept
> > > 
> > > the same question for the rest.
> > Because OnDropChannel always drops the channel and returns CHANNEL_DELETED. 
> 
> Oh. The cases in which the channel is normally deleted are just listed?

Sorry, I was wrong.
My intention was, there was no need to distinguish DROP and FAIL in these tests.
But as discussed locally, I got your point and reverted many of them.
I leave the change on OnFlowControlAfterSend because it intends "Delete the
channel as usual" on the line.

https://codereview.chromium.org/105833003/diff/170001/net/websockets/websocke...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/170001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:166: !(has_multiple_protocols
&& has_invalid_protocol)) {
On 2014/01/09 04:08:48, tyoshino wrote:
> how about
> - just breaking at each if-clause
> - do EnumerateHeader() inside the while block and put last_value (possibly
> renamed to parsed_value or something) in the while block and substitute it to
> value only when the EnumerateHeader() call is success?

I moved the EnumerateHeader call to the while body, but I think placing simple
predictions on while head is a good thing.

https://codereview.chromium.org/105833003/diff/170001/net/websockets/websocke...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/105833003/diff/170001/net/websockets/websocke...
net/websockets/websocket_stream.cc:78: if (status.status() ==
URLRequestStatus::CANCELED) {
On 2014/01/09 04:08:48, tyoshino wrote:
> use switch?

Done.

[tyoshino (SeeGerritForStatus), 2014-01-09 05:52:04]

lgtm

https://codereview.chromium.org/105833003/diff/260002/net/websockets/websocke...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/105833003/diff/260002/net/websockets/websocke...
net/websockets/websocket_stream.cc:77: switch (url_request_.status().status()) {
how about listing all cases so that -Wswitch-enum can catch lack?

[tyoshino (SeeGerritForStatus), 2014-01-09 05:53:54]

https://codereview.chromium.org/105833003/diff/260002/net/websockets/websocke...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/260002/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:156: std::string last_parsed;
i suggested last_parsed for the temporary variable. as you're using "temp_value"
for that, i think it's ok to use "value" for this variable.

[commit-bot: I haz the power, 2014-01-09 06:04:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/105833003/400001

[yhirano, 2014-01-09 06:04:18]

https://codereview.chromium.org/105833003/diff/260002/net/websockets/websocke...
File net/websockets/websocket_basic_handshake_stream.cc (right):

https://codereview.chromium.org/105833003/diff/260002/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:156: std::string last_parsed;
On 2014/01/09 05:53:55, tyoshino wrote:
> i suggested last_parsed for the temporary variable. as you're using
"temp_value"
> for that, i think it's ok to use "value" for this variable.

Done.

https://codereview.chromium.org/105833003/diff/260002/net/websockets/websocke...
File net/websockets/websocket_stream.cc (right):

https://codereview.chromium.org/105833003/diff/260002/net/websockets/websocke...
net/websockets/websocket_stream.cc:77: switch (url_request_.status().status()) {
On 2014/01/09 05:52:05, tyoshino wrote:
> how about listing all cases so that -Wswitch-enum can catch lack?

Done.

[tyoshino (SeeGerritForStatus), 2014-01-09 07:00:45]

lgtm again

[commit-bot: I haz the power, 2014-01-09 10:38:08]

Change committed as 243835