Support parsing a Sec-WebSocket-Extensions header with multiple elements

Support parsing a Sec-WebSocket-Extensions header with multiple elements

This change is mainly for the WebSocket server implementation in Chromium.

As there's only one known extension so far and it's illegal to return
multiple permessage-deflate elements to the client, we don't need this
parser improvement for the client.

R=yhirano,dgozman,eroman
BUG=471646
Committed: https://crrev.com/38ee68c20fe305db2eb4f3db50d3c9cbfad03d45
Cr-Commit-Position: refs/heads/master@{#323189}

[tyoshino (SeeGerritForStatus), 2015-03-30 05:31:31]

Patchset #1 (id:1) has been deleted

[tyoshino (SeeGerritForStatus), 2015-03-30 05:48:56]

Patchset #1 (id:20001) has been deleted

[tyoshino (SeeGerritForStatus), 2015-03-30 05:50:38]

tyoshino@chromium.org changed reviewers:
+ dgozman@chromium.org, yhirano@chromium.org

[yhirano, 2015-03-30 11:02:03]

https://codereview.chromium.org/1047623002/diff/40001/net/server/web_socket_e...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1047623002/diff/40001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:236: void
WebSocketEncoder::ParseExtensions(const std::string& header_value,
Please fix header comments.

https://codereview.chromium.org/1047623002/diff/40001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:269: const std::string& name = param.name();
TODO: This is too permissive. Fail the connection when an invalid value is
given.

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

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:306: std::string*
concatenated_header_value,
Why did you change the name to concatenated_header_value? This function  checks
if the headers consist of valid WebSocket extensions, so |extensions| or
|accepted_exceptions| is a good name, I think.

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.cc (right):

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:34: has_error_ |= current_ != end_;
'|=' is a bitwise operation, so I prefer the original statement.

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.h (right):

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.h:40: void ConsumeExtension();
Please keep the parameter. All Consume* methods store the output to given
objects.

[dgozman, 2015-03-30 11:04:40]

net/server looks good. Deferring to yhirano@.

https://codereview.chromium.org/1047623002/diff/40001/net/server/web_socket_e...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1047623002/diff/40001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:276: *client_window_bits = bits;
nit: wrap in {} please.

[tyoshino (SeeGerritForStatus), 2015-03-31 06:17:45]

https://codereview.chromium.org/1047623002/diff/40001/net/server/web_socket_e...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1047623002/diff/40001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:236: void
WebSocketEncoder::ParseExtensions(const std::string& header_value,
On 2015/03/30 11:02:03, yhirano wrote:
> Please fix header comments.

Done.

https://codereview.chromium.org/1047623002/diff/40001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:269: const std::string& name = param.name();
On 2015/03/30 11:02:03, yhirano wrote:
> TODO: This is too permissive. Fail the connection when an invalid value is
> given.

Done.

https://codereview.chromium.org/1047623002/diff/40001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:276: *client_window_bits = bits;
On 2015/03/30 11:04:40, dgozman wrote:
> nit: wrap in {} please.

Done.

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

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:306: std::string*
concatenated_header_value,
On 2015/03/30 11:02:03, yhirano wrote:
> Why did you change the name to concatenated_header_value? This function 
checks
> if the headers consist of valid WebSocket extensions, so |extensions| or
> |accepted_exceptions| is a good name, I think.

I want to
- emphasize that this variable holds a descriptor, not a WebSocketExtension
struct
- give a good pair of names to |accepted_extensions| and |extensions|. It's not
clear what |extensions| means as there's also |accepted_extensions|. It sounds
like that |extensions| is a list of not-yet-accepted extensions.

I couldn't come up with any good pair of names, so decided to just describe
what's done in this method. I gave it another try. PTAL.

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.cc (right):

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:34: has_error_ |= current_ != end_;
On 2015/03/30 11:02:03, yhirano wrote:
> '|=' is a bitwise operation, so I prefer the original statement.

Oops.. Fixed

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.h (right):

https://codereview.chromium.org/1047623002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.h:40: void ConsumeExtension();
On 2015/03/30 11:02:03, yhirano wrote:
> Please keep the parameter. All Consume* methods store the output to given
> objects.

Done.

[tyoshino (SeeGerritForStatus), 2015-03-31 06:20:31]

Refined the CL description as client side developers are confused.

[yhirano, 2015-03-31 08:18:53]

lgtm

[tyoshino (SeeGerritForStatus), 2015-03-31 08:39:21]

tyoshino@chromium.org changed reviewers:
+ eroman@chromium.org

[tyoshino (SeeGerritForStatus), 2015-03-31 08:39:31]

+eroman

[eroman, 2015-03-31 21:11:13]

lgtm

https://codereview.chromium.org/1047623002/diff/90001/net/server/web_socket_e...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1047623002/diff/90001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:259: // there're multiple permessage-deflate
extensions or there is any unknown
there're --> there are
there is any unknown --> there are any unknown

https://codereview.chromium.org/1047623002/diff/90001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:261: for (size_t i = 0; i < extensions.size();
++i) {
nit: You use a traditional for loop here, however in the inner loop it is a
C++11 style "for in" loop. For consistency I suggest using a for-in in both
locations.

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

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:329: for (size_t i = 0; i <
extensions.size(); ++i) {
optional: "for-in" loop

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:339: params))
Add curly braces since the if occupies more than 1 line

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.cc (right):

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:25: if (has_error_) {
Should this be done before the call to extensions_push()?

As an aside, while reading this I find the has_error_ member to be a bit
fragile. I wonder if it would be clearer to have each of the "Consume*()"
methods instead return their success as a boolean, and mark the return value as
something that cannot be ignored to the compiler.

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:27: return;
I suggest doing a break instead, and leaving the check at the bottom to clear
extensions_ (so it is just done in one place).

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
File net/websockets/websocket_extension_parser_test.cc (right):

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_extension_parser_test.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
As an aside: This test is named *_test.cc. Note that the usual naming in net/*
is to name these *_unittest.cc

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_extension_parser_test.cc:112: "foo,",                
// second extension is incomplete
I suggest adding a couple more invalid variations involving whitespace

[tyoshino (SeeGerritForStatus), 2015-04-01 04:57:57]

https://codereview.chromium.org/1047623002/diff/90001/net/server/web_socket_e...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1047623002/diff/90001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:259: // there're multiple permessage-deflate
extensions or there is any unknown
On 2015/03/31 21:11:12, eroman wrote:
> there're --> there are
> there is any unknown --> there are any unknown

Done.

https://codereview.chromium.org/1047623002/diff/90001/net/server/web_socket_e...
net/server/web_socket_encoder.cc:261: for (size_t i = 0; i < extensions.size();
++i) {
On 2015/03/31 21:11:12, eroman wrote:
> nit: You use a traditional for loop here, however in the inner loop it is a
> C++11 style "for in" loop. For consistency I suggest using a for-in in both
> locations.

Done.

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

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:329: for (size_t i = 0; i <
extensions.size(); ++i) {
On 2015/03/31 21:11:12, eroman wrote:
> optional: "for-in" loop

Done.

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_basic_handshake_stream.cc:339: params))
On 2015/03/31 21:11:12, eroman wrote:
> Add curly braces since the if occupies more than 1 line

Done.

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.cc (right):

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:25: if (has_error_) {
On 2015/03/31 21:11:12, eroman wrote:
> Should this be done before the call to extensions_push()?

Done.

> As an aside, while reading this I find the has_error_ member to be a bit
> fragile. I wonder if it would be clearer to have each of the "Consume*()"
> methods instead return their success as a boolean, and mark the return value
as
> something that cannot be ignored to the compiler.

Will do. Added TODO

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:27: return;
On 2015/03/31 21:11:12, eroman wrote:
> I suggest doing a break instead, and leaving the check at the bottom to clear
> extensions_ (so it is just done in one place).

Done.

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
File net/websockets/websocket_extension_parser_test.cc (right):

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_extension_parser_test.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2015/03/31 21:11:12, eroman wrote:
> As an aside: This test is named *_test.cc. Note that the usual naming in net/*
> is to name these *_unittest.cc

Will do renaming.

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_extension_parser_test.cc:112: "foo,",                
// second extension is incomplete
On 2015/03/31 21:11:12, eroman wrote:
> I suggest adding a couple more invalid variations involving whitespace

Done.

[tyoshino (SeeGerritForStatus), 2015-04-01 04:58:04]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-04-01 04:58:04]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1047623002/#ps110001
(title: "Addressed #12")

[tyoshino (SeeGerritForStatus), 2015-04-01 04:58:08]

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-04-01 04:58:27]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2015-04-01 04:58:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1047623002/110001

[commit-bot: I haz the power, 2015-04-01 05:53:03]

Committed patchset #5 (id:110001)

[commit-bot: I haz the power, 2015-04-01 05:54:05]

Patchset 5 (id:??) landed as
https://crrev.com/38ee68c20fe305db2eb4f3db50d3c9cbfad03d45
Cr-Commit-Position: refs/heads/master@{#323189}

[tyoshino (SeeGerritForStatus), 2015-04-01 06:52:17]

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
File net/websockets/websocket_extension_parser_test.cc (right):

https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
net/websockets/websocket_extension_parser_test.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2015/04/01 04:57:57, tyoshino wrote:
> On 2015/03/31 21:11:12, eroman wrote:
> > As an aside: This test is named *_test.cc. Note that the usual naming in
net/*
> > is to name these *_unittest.cc
> 
> Will do renaming.

Hmm, all of the unit test files are named _test.cc in net/websockets/.

net/websockets/: All _test.cc
net/spdy/: Both
net/quic/: Both
net/tools/: Both
The other directories: All _unittest.cc

[yhirano, 2015-04-01 07:00:37]

On 2015/04/01 06:52:17, tyoshino wrote:
>
https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
> File net/websockets/websocket_extension_parser_test.cc (right):
> 
>
https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocke...
> net/websockets/websocket_extension_parser_test.cc:1: // Copyright 2013 The
> Chromium Authors. All rights reserved.
> On 2015/04/01 04:57:57, tyoshino wrote:
> > On 2015/03/31 21:11:12, eroman wrote:
> > > As an aside: This test is named *_test.cc. Note that the usual naming in
> net/*
> > > is to name these *_unittest.cc
> > 
> > Will do renaming.
> 
> Hmm, all of the unit test files are named _test.cc in net/websockets/.
> 
> net/websockets/: All _test.cc
> net/spdy/: Both
> net/quic/: Both
> net/tools/: Both
> The other directories: All _unittest.cc

Yeah, we chose _test.cc as _unittest.cc was deperecated[1].
It was approved[2] by a net owner. See also: [3]

1: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#File_Names
2: https://chromiumcodereview.appspot.com/23614009
3: https://chromiumcodereview.appspot.com/12764006

[eroman, 2015-04-01 07:39:36]

I see thanks for the explanation!
No renaming necessary

On Wed, Apr 1, 2015 at 12:00 AM, <yhirano@chromium.org> wrote:

> On 2015/04/01 06:52:17, tyoshino wrote:
>
> https://codereview.chromium.org/1047623002/diff/90001/net/
> websockets/websocket_extension_parser_test.cc
>
>> File net/websockets/websocket_extension_parser_test.cc (right):
>>
>
>
> https://codereview.chromium.org/1047623002/diff/90001/net/
> websockets/websocket_extension_parser_test.cc#newcode1
>
>> net/websockets/websocket_extension_parser_test.cc:1: // Copyright 2013
>> The
>> Chromium Authors. All rights reserved.
>> On 2015/04/01 04:57:57, tyoshino wrote:
>> > On 2015/03/31 21:11:12, eroman wrote:
>> > > As an aside: This test is named *_test.cc. Note that the usual naming
>> in
>> net/*
>> > > is to name these *_unittest.cc
>> >
>> > Will do renaming.
>>
>
>  Hmm, all of the unit test files are named _test.cc in net/websockets/.
>>
>
>  net/websockets/: All _test.cc
>> net/spdy/: Both
>> net/quic/: Both
>> net/tools/: Both
>> The other directories: All _unittest.cc
>>
>
> Yeah, we chose _test.cc as _unittest.cc was deperecated[1].
> It was approved[2] by a net owner. See also: [3]
>
> 1: https://google-styleguide.googlecode.com/svn/trunk/
> cppguide.html#File_Names
> 2: https://chromiumcodereview.appspot.com/23614009
> 3: https://chromiumcodereview.appspot.com/12764006
>
>
> https://codereview.chromium.org/1047623002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tyoshino (SeeGerritForStatus), 2015-04-01 07:43:12]

Thanks Yutaka for investigating the log!