Add WebSocketError to indicate decoding failure reason

Add WebSocketError to indicate decoding failure reason

BUG=none
TEST=net_unittests

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

[Takashi Toyoshima, 2012-08-01 14:16:21]

Hi Matt,

This is my first idea on error code handling.
As I replied in the previous review, it's OK to reuse net error code here.
What do you think about this?

[mmenke, 2012-08-01 14:44:58]

I'm happy with either approach.  I'm not sure how much we'd get out of extending
net error codes to include WebSocket errors.

It may make a couple things a little easier (net-internals already supports
network error codes, could include both websocket and connection errors in
histograms, can make WebSockets look a little more like other net objects), but
I'm not sure any of those really matter.  On the downside, we'd have to convert
net errors back to WebSocket errors before sending them, and possibly go the
other way as well.

Let's just go with this for now (Modulo comments).

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
File net/websockets/websocket_frame.h (right):

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame.h:24: WS_ERR_MESSAGE_TOO_BIG = 1009
It's a bit long, but I think it's worth writing WEB_SOCKET out, to make the code
clearer.  Otherwise, looks a bit too much like Windows socket errors for my
tastes (They actually use WSA, but look kinda similar).  Also, Google style
guide generally discourages abbreviations.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame.h:25: };
Wonder if it makes sense to move these to their own file.  Think they'll be
useful in files that don't need access to WebSocketFrameHeader?

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
File net/websockets/websocket_frame_parser.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame_parser.cc:140: payload_length >
static_cast<uint64>(kint64max))
nit:  Google style (Or maybe Chrome style, can't remember which) is to use
braces when the conditional takes more than one line.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
File net/websockets/websocket_frame_parser.h (right):

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame_parser.h:44: bool failed() const { return
failed_; }
Do we still need this?  If so, think we should be returning "error_code_ !=
WS_OK", and just get rid of the bool.  That way, shouldn't get any bugs where we
forget to set one or the other.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame_parser.h:45: WebSocketError error_code() const {
return error_code_; }
Suggest calling this websocket_error instead, to make it clearer that this isn't
a generic net error code.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
File net/websockets/websocket_frame_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame_parser_unittest.cc:48:
GG_UINT64_C(0x7FFFFFFFFFFFFFFF), true, net::WS_ERR_MESSAGE_TOO_BIG }
How about just moving this unnamed namespace into the net namespace below?

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame_parser_unittest.cc:394: EXPECT_EQ(net::WS_OK,
parser.error_code());
nit:  Use braces here (And please add them just below as well)

[Takashi Toyoshima, 2012-08-02 08:11:34]

Thanks, Matt.

I also add a function net::WebSocketErrorToNetError(). As you said, we will need
it sometime in the near future.

If we used NetError directly, it is not required. But we should provide
NetErrorToWebSocketError() for graceful closing handshakce. Now I feel providing
WebSocketErrorToNetError() is easier than doing NetErrorToWebSocketError(). I
mean that making a map from large set to small set is difficult. So, I prefer to
keep on using WebSocketError now.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
File net/websockets/websocket_frame.h (right):

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame.h:24: WS_ERR_MESSAGE_TOO_BIG = 1009
On 2012/08/01 14:44:59, Matt Menke wrote:
> It's a bit long, but I think it's worth writing WEB_SOCKET out, to make the
code
> clearer.  Otherwise, looks a bit too much like Windows socket errors for my
> tastes (They actually use WSA, but look kinda similar).  Also, Google style
> guide generally discourages abbreviations.

Done.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame.h:25: };
On 2012/08/01 14:44:59, Matt Menke wrote:
> Wonder if it makes sense to move these to their own file.  Think they'll be
> useful in files that don't need access to WebSocketFrameHeader?

Done.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
File net/websockets/websocket_frame_parser.h (right):

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame_parser.h:44: bool failed() const { return
failed_; }
Ah, that is right.
I'll remove this function and member.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame_parser.h:45: WebSocketError error_code() const {
return error_code_; }
On 2012/08/01 14:44:59, Matt Menke wrote:
> Suggest calling this websocket_error instead, to make it clearer that this
isn't
> a generic net error code.

Done.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
File net/websockets/websocket_frame_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame_parser_unittest.cc:48:
GG_UINT64_C(0x7FFFFFFFFFFFFFFF), true, net::WS_ERR_MESSAGE_TOO_BIG }
On 2012/08/01 14:44:59, Matt Menke wrote:
> How about just moving this unnamed namespace into the net namespace below?

Done.

https://chromiumcodereview.appspot.com/10824081/diff/2001/net/websockets/webs...
net/websockets/websocket_frame_parser_unittest.cc:394: EXPECT_EQ(net::WS_OK,
parser.error_code());
On 2012/08/01 14:44:59, Matt Menke wrote:
> nit:  Use braces here (And please add them just below as well)

Done.

[mmenke, 2012-08-02 14:08:21]

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
File net/websockets/websocket_frame_parser.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
net/websockets/websocket_frame_parser.cc:140: websocket_error_ =
WEB_SOCKET_ERR_PROTOCOL_ERROR;
nit:  Need brackets here, since the condition takes up two lines.

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
File net/websockets/websocket_frame_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
net/websockets/websocket_frame_parser_unittest.cc:54: const int
kNumFrameHeaderTests = arraysize(kFrameHeaderTests);
These should still be in the anonymous namespace.  Was thinking you could just
bring the entire anonymous namespace down, and put it inside of the net
namespace.

[Takashi Toyoshima, 2012-08-03 05:53:14]

Thank you, and sorry for my misunderstanding on your namespace comment.

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
File net/websockets/websocket_frame_parser.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
net/websockets/websocket_frame_parser.cc:140: websocket_error_ =
WEB_SOCKET_ERR_PROTOCOL_ERROR;
On 2012/08/02 14:08:22, Matt Menke wrote:
> nit:  Need brackets here, since the condition takes up two lines.

Done.

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
File net/websockets/websocket_frame_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
net/websockets/websocket_frame_parser_unittest.cc:54: const int
kNumFrameHeaderTests = arraysize(kFrameHeaderTests);
Ah, sorry. I misunderstand your first comment.
Do you mean move unnamed namespace into net namespace like,

namespace net {
namespace {
  ...
}
TEST...
}

[mmenke, 2012-08-03 14:22:01]

Nothing to apologize for.

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
File net/websockets/websocket_frame_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/7007/net/websockets/webs...
net/websockets/websocket_frame_parser_unittest.cc:54: const int
kNumFrameHeaderTests = arraysize(kFrameHeaderTests);
On 2012/08/03 05:53:14, toyoshim wrote:
> Ah, sorry. I misunderstand your first comment.
> Do you mean move unnamed namespace into net namespace like,
> 
> namespace net {
> namespace {
>   ...
> }
> TEST...
> }

Yes, that's exactly what I meant.

Come to think of it, you could probably put the tests in the anonymous
namespace, too - This is generally a good idea, anyways, except for tests that
need to friends of the classes they test.

[Takashi Toyoshima, 2012-08-06 09:15:08]

Matt, thank you for good advice.
I moved all tests into net::(anonymous) namespace.
Could you review this CL again?

[mmenke, 2012-08-06 14:08:31]

LGTM.

https://chromiumcodereview.appspot.com/10824081/diff/10008/net/websockets/web...
File net/websockets/websocket_frame_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/10008/net/websockets/web...
net/websockets/websocket_frame_parser_unittest.cc:34: bool failed;
Don't think we need this any more.  Can just replace it with checking
|error_code|.

[Takashi Toyoshima, 2012-08-06 14:19:31]

Ah, right.
Thank you for catching this.
I'll submit this change after removing this.

https://chromiumcodereview.appspot.com/10824081/diff/10008/net/websockets/web...
File net/websockets/websocket_frame_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/10824081/diff/10008/net/websockets/web...
net/websockets/websocket_frame_parser_unittest.cc:34: bool failed;
On 2012/08/06 14:08:31, Matt Menke wrote:
> Don't think we need this any more.  Can just replace it with checking
> |error_code|.

Done.

[commit-bot: I haz the power, 2012-08-06 14:19:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10824081/13002

[commit-bot: I haz the power, 2012-08-06 14:36:50]

Try job failure for 10824081-13002 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...