Replace WebSocketFrameChunk with WebSocketFrame

Most of the WebSocket code does not need to care about the original
frame boundaries. Move the chunk to frame conversion logic to
WebSocketBasicStream from WebSocketChannel and change all interfaces
except for WebSocketFrameParser to use WebSocketFrame instead of
WebSocketFrameChunk.

BUG=288603
TEST=net_unittests --gtest_filter=WebSocket*

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

[yhirano, 2013-09-13 10:03:17]

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.cc (right):

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.cc:349: const bool final = is_final_chunk
&& current_frame_header_->final;
[opt] I think is_final_chunk_in_message is a better name.

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.h (right):

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.h:97: // Converts a |chunk| to a |frame|.
|frame| should be NULL on entry to this
|*frame| should be Null?

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream_test.cc (right):

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:57: ScopedVector<WebSocketFrame>
GenerateWriteFrame() {
Why do you use ScopedVector instead of scoptr_ptr?
GenerateWriteFrame generates a frame, not multiple frames.
Or, should we rename it to GenerateWriteFrames?

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:204: data + data_size - start >
static_cast<size_t>(len)) {
Is the last condition necessary?

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:644: }  // namespace net
It's good to have a test that checks if the opcode of each non-first split text
frames is converted to OpCodeContinuation

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
File net/websockets/websocket_frame.cc (right):

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_frame.cc:207: char realigned_mask[kMaskingKeyLength] =
{0};
{} suffices for initializing the array.

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
File net/websockets/websocket_frame.h (right):

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_frame.h:97: WebSocketFrame(WebSocketFrameHeader::OpCode
opcode);
explicit

[Adam Rice, 2013-09-13 14:09:53]

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.cc (right):

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.cc:349: const bool final = is_final_chunk
&& current_frame_header_->final;
On 2013/09/13 10:03:17, yhirano wrote:
> [opt] I think is_final_chunk_in_message is a better name.

Done.

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream_test.cc (right):

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:57: ScopedVector<WebSocketFrame>
GenerateWriteFrame() {
On 2013/09/13 10:03:17, yhirano wrote:
> Why do you use ScopedVector instead of scoptr_ptr?
> GenerateWriteFrame generates a frame, not multiple frames.
> Or, should we rename it to GenerateWriteFrames?

I realised I was always calling it in the same way, and so I made it part of the
test fixture and renamed it to PrepareWriteFrame().

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:204: data + data_size - start >
static_cast<size_t>(len)) {
On 2013/09/13 10:03:17, yhirano wrote:
> Is the last condition necessary?

No. I have cleaned up this condition a bit.

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:644: }  // namespace net
On 2013/09/13 10:03:17, yhirano wrote:
> It's good to have a test that checks if the opcode of each non-first split
text
> frames is converted to OpCodeContinuation

Added test "ContinuationOpCodeUsed".

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
File net/websockets/websocket_frame.cc (right):

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_frame.cc:207: char realigned_mask[kMaskingKeyLength] =
{0};
On 2013/09/13 10:03:17, yhirano wrote:
> {} suffices for initializing the array.

I did not know that. Thank you.

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
File net/websockets/websocket_frame.h (right):

https://codereview.chromium.org/23604044/diff/18001/net/websockets/websocket_...
net/websockets/websocket_frame.h:97: WebSocketFrame(WebSocketFrameHeader::OpCode
opcode);
On 2013/09/13 10:03:17, yhirano wrote:
> explicit

Sorry. It looks like I forgot to run cpplint.

[yhirano, 2013-09-17 07:43:55]

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.cc (right):

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.cc:93: return result;
Can you add DCHECK(frame_chunks.empty()) here for clarification?

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.cc:346: 
Can you decouple L347 - L359 to a separate function like
scoped_ptr<WebSocketFrame> CreateFrame(is_final_chunk, data_buffer) which
returns NULL for an empty non-final chunk?

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.cc:363: opcode !=
WebSocketFrameHeader::kOpCodeContinuation) {
I think |if (WebSocketFrameHeader::IsKnownDataOpCode(opcode))| is sufficient.

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.h (right):

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.h:87: // returns OK and places the
frame(s) in frame_chunks.
This comment need to be updated.

[Adam Rice, 2013-09-17 10:42:34]

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.cc (right):

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.cc:93: return result;
On 2013/09/17 07:43:55, yhirano wrote:
> Can you add DCHECK(frame_chunks.empty()) here for clarification?

I added DCHECK(frames.empty()) since I think that's what you meant.

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.cc:363: opcode !=
WebSocketFrameHeader::kOpCodeContinuation) {
On 2013/09/17 07:43:55, yhirano wrote:
> I think |if (WebSocketFrameHeader::IsKnownDataOpCode(opcode))| is sufficient.

Done.

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.h (right):

https://codereview.chromium.org/23604044/diff/36001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.h:87: // returns OK and places the
frame(s) in frame_chunks.
On 2013/09/17 07:43:55, yhirano wrote:
> This comment need to be updated.

Done.

[yhirano, 2013-09-17 12:05:08]

lgtm

[Adam Rice, 2013-09-17 13:03:15]

+tyoshino for websockets OWNERS.

[tyoshino (SeeGerritForStatus), 2013-09-19 06:45:11]

https://codereview.chromium.org/23604044/diff/42001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.cc (right):

https://codereview.chromium.org/23604044/diff/42001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.cc:73: return
ConvertChunksToFrames(&frame_chunks, frames);
empty |frames| may be returned without attempting to read more data from socket
if frame_chunks is not enough?

[Adam Rice, 2013-09-20 08:09:27]

https://codereview.chromium.org/23604044/diff/42001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.cc (right):

https://codereview.chromium.org/23604044/diff/42001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.cc:73: return
ConvertChunksToFrames(&frame_chunks, frames);
On 2013/09/19 06:45:12, tyoshino wrote:
> empty |frames| may be returned without attempting to read more data from
socket
> if frame_chunks is not enough?

Good catch, thank you.

I was missing a test for this case, so I have added 2.

[tyoshino (SeeGerritForStatus), 2013-09-24 13:52:55]

ps12 _basic_stream.* and _stream.* look good.

i'm gonna review rest tomorrow.

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.h (right):

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.h:105: // Create a frame based on the
value of |is_final_chunk|, |data| and
Creates

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
File net/websockets/websocket_stream.h (right):

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
net/websockets/websocket_stream.h:108: // bytes of a new frame do not form a
complete frame header, then the
this sentence should also be removed or updated.

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
net/websockets/websocket_stream.h:116: // returned immediately.
will be returned?

[Adam Rice, 2013-09-25 02:10:51]

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream.h (right):

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
net/websockets/websocket_basic_stream.h:105: // Create a frame based on the
value of |is_final_chunk|, |data| and
On 2013/09/24 13:52:55, tyoshino wrote:
> Creates

Done.

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
File net/websockets/websocket_stream.h (right):

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
net/websockets/websocket_stream.h:108: // bytes of a new frame do not form a
complete frame header, then the
On 2013/09/24 13:52:55, tyoshino wrote:
> this sentence should also be removed or updated.

Done.

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
net/websockets/websocket_stream.h:116: // returned immediately.
On 2013/09/24 13:52:55, tyoshino wrote:
> will be returned?

"will be" isn't necessary here (it is implied by the previous clause), but I
have added it for clarity.

[tyoshino (SeeGerritForStatus), 2013-09-25 08:39:23]

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
File net/websockets/websocket_stream.h (right):

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
net/websockets/websocket_stream.h:116: // returned immediately.
On 2013/09/25 02:10:51, Adam Rice wrote:
> On 2013/09/24 13:52:55, tyoshino wrote:
> > will be returned?
> 
> "will be" isn't necessary here (it is implied by the previous clause), but I
> have added it for clarity.

OK. Thanks

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream_test.cc (right):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:41: const char
kPingFrameWithoutFin[] = "\x0A\x00";
ping is 0x09

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:47:
"ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ";
pong is 0x0a (| 0x80 = 0x8a)

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:153: // LAST_FRAME_BIG is
specified, the the rest of the data will be
the the -> the

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:199: // Create a WebSocketFrame
with a wire format matching kWriteFrame and add it
Creates

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (left):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:407: }
Looks like these two FailChannel() calls has been replaced with protocol_error
setting code when moved to websocket_basic_stream.cc. It returns
ERR_WS_PROTOCOL_ERROR now but does it still result in calling FailChannel() in
websocket_channel.cc?

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:366: data_buffer.swap(frame->data);
you can move these 2 lines down to L377

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_frame.h (right):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_frame.h:104: // |is given by |header.payload_length|.
|is -> is

[Adam Rice, 2013-09-26 03:07:58]

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_basic_stream_test.cc (right):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:41: const char
kPingFrameWithoutFin[] = "\x0A\x00";
On 2013/09/25 08:39:23, tyoshino wrote:
> ping is 0x09

Sorry. Fixed.

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:47:
"ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ";
On 2013/09/25 08:39:23, tyoshino wrote:
> pong is 0x0a (| 0x80 = 0x8a)

Fixed.

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:153: // LAST_FRAME_BIG is
specified, the the rest of the data will be
On 2013/09/25 08:39:23, tyoshino wrote:
> the the -> the

I intended to type "then the".

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_basic_stream_test.cc:199: // Create a WebSocketFrame
with a wire format matching kWriteFrame and add it
On 2013/09/25 08:39:23, tyoshino wrote:
> Creates

Done.

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (left):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:407: }
On 2013/09/25 08:39:23, tyoshino wrote:
> Looks like these two FailChannel() calls has been replaced with protocol_error
> setting code when moved to websocket_basic_stream.cc. It returns
> ERR_WS_PROTOCOL_ERROR now but does it still result in calling FailChannel() in
> websocket_channel.cc?

No. It closes the connection and sends code 1006, "Abnormal Closure", to the
renderer. This is a bug.

The bug actually already existed because the parser can return errors which get
translated to ERR_WS_PROTOCOL_ERROR.

At the moment there is no way to pass a detailed error message back. We will
have to use a generic message like "Protocol Error". I wonder if it worth adding
another method to WebSocketStream to get detailed error information? What do you
think?

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_channel.cc (right):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel.cc:366: data_buffer.swap(frame->data);
On 2013/09/25 08:39:23, tyoshino wrote:
> you can move these 2 lines down to L377

Thank you. Actually, it turned out I could remove them completely.

[tyoshino (SeeGerritForStatus), 2013-09-26 05:59:04]

lgtm

sorry for long delay.

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
File net/websockets/websocket_stream.h (right):

https://codereview.chromium.org/23604044/diff/55001/net/websockets/websocket_...
net/websockets/websocket_stream.h:108: // bytes of a new frame do not form a
complete frame header, then the
On 2013/09/25 02:10:51, Adam Rice wrote:
> On 2013/09/24 13:52:55, tyoshino wrote:
> > this sentence should also be removed or updated.
> 
> Done.

Thanks. Sorry, I realized I made this comment by mistake. But the new one still
looks fine.

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:263: if (source_frames[i].data) {
source_frames[i] -> source_frame

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:265: memcpy(result_frame->data->data(),
source_frames[i].data, frame_length);
source_frames[i] -> source_frame

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:952:
TEST_F(WebSocketChannelEventInterfaceTest, FragmentedFrames) {
FragmentedMessage would be better now.

[tyoshino (SeeGerritForStatus), 2013-09-26 06:00:25]

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_frame.h (right):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_frame.h:104: // |is given by |header.payload_length|.
On 2013/09/25 08:39:23, tyoshino wrote:
> |is -> is

And, please fix this.

[Adam Rice, 2013-09-26 06:25:54]

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_channel_test.cc (right):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:263: if (source_frames[i].data) {
On 2013/09/26 05:59:04, tyoshino wrote:
> source_frames[i] -> source_frame

Sorry I missed this. Done.

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:265: memcpy(result_frame->data->data(),
source_frames[i].data, frame_length);
On 2013/09/26 05:59:04, tyoshino wrote:
> source_frames[i] -> source_frame

Done.

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_channel_test.cc:952:
TEST_F(WebSocketChannelEventInterfaceTest, FragmentedFrames) {
On 2013/09/26 05:59:04, tyoshino wrote:
> FragmentedMessage would be better now.

Done.

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
File net/websockets/websocket_frame.h (right):

https://codereview.chromium.org/23604044/diff/63001/net/websockets/websocket_...
net/websockets/websocket_frame.h:104: // |is given by |header.payload_length|.
On 2013/09/26 06:00:25, tyoshino wrote:
> On 2013/09/25 08:39:23, tyoshino wrote:
> > |is -> is
> 
> And, please fix this.

Sorry! Fixed it now.

[commit-bot: I haz the power, 2013-09-26 06:34:09]

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

[commit-bot: I haz the power, 2013-09-26 12:14:30]

Change committed as 225452