Add QuicStream and friends to QUIC code. (issue 11300020)

[r...@chromium.org]

Reviewers: jar,

Message:
friendly ping

Description:
Add QuicStream and friends to QUIC code.


Please review this at http://codereview.chromium.org/11300020/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M net/net.gyp
A net/quic/quic_crypto_stream.h
A net/quic/quic_crypto_stream.cc
A net/quic/quic_crypto_stream_test.cc
A net/quic/quic_session.h
A net/quic/quic_session.cc
A net/quic/quic_session_test.cc
A net/quic/quic_stream_sequencer.h
A net/quic/quic_stream_sequencer.cc
A net/quic/quic_stream_sequencer_test.cc
A net/quic/reliable_quic_stream.h
A net/quic/reliable_quic_stream.cc
M net/quic/test_tools/quic_test_utils.h
M net/quic/test_tools/quic_test_utils.cc

[j...@chromium.org]

I haven't finished reviewing... and am only up to
quic_stream_sequencer_test.cc,
but since I had some questions already, and you had pinged, I thought I'd
pass
this back to you to comment on (while I continue the review).

Thanks.


https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream.h
File net/quic/quic_crypto_stream.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream.h#newcode54
net/quic/quic_crypto_stream.h:54: };
nit: NO_COPY_AND_ASSIGN macro.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream_test.cc
File net/quic/quic_crypto_stream_test.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream_test.cc#newcode38
net/quic/quic_crypto_stream_test.cc:38: std::vector<CryptoTag>
message_tags_;
nit: I think even here in test land, members should be private, and we
should use accessors.

You can use protected in test fixtures, but I don't think this
qualifies.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream_test.cc#newcode40
net/quic/quic_crypto_stream_test.cc:40: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream_test.cc#newcode60
net/quic/quic_crypto_stream_test.cc:60: IPEndPoint addr_;
These should probably be protected:

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_session.cc
File net/quic/quic_session.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_session.cc#newcode179
net/quic/quic_session.cc:179: // If the stream was locally initiated we
strictly in-order creation.
nit: typo? "...we strictly in-order creation"

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_session.cc#newcode183
net/quic/quic_session.cc:183: } else {
nit: with return on line 182, else and curlies are not needed.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_session.cc#newcode187
net/quic/quic_session.cc:187: implicitly_created_streams_.count(id) ==
0;
I don't think there are every any entries in implicitly_created_streams_
that are larger than the largest_peer_created_stream_id.

Why are we checking in line 187?

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc
File net/quic/quic_stream_sequencer.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode45
net/quic/quic_stream_sequencer.cc:45: size_t data_len =
frame.data.size();
nit: declare/init as close to first use as possible.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode54
net/quic/quic_stream_sequencer.cc:54: // stream might only consume part
of it and we'd need a partial ack.
nit: typo? "...can't pass it up the stream might..."

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode72
net/quic/quic_stream_sequencer.cc:72: size_t data_len =
frame.data.size();
nit: define/init as close to first use as possible.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode75
net/quic/quic_stream_sequencer.cc:75: // This should not happen, as
WillAcceptFrame should be called before
Shouldn't this be a DCHECK(), even if we do put this defensive code
here?

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode142
net/quic/quic_stream_sequencer.cc:142: size_t idx = 0;
nit: idx --> index

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode148
net/quic/quic_stream_sequencer.cc:148:
const_cast<char*>(it->second.c_str()));
Should this be second.data()? I think the other call may instigate an
allocation and strcpy just to make room for the null.

The other uses of c_str() in this file might also need to be changed.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode159
net/quic/quic_stream_sequencer.cc:159: FrameMap::iterator it =
frames_.begin();
nit: putting this after line 161 will mean that you don't have to reset
it in line 171 (and make the loop clearer that it is always pointing to
frames.begin() ).

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode162
net/quic/quic_stream_sequencer.cc:162: if (it->first !=
num_bytes_consumed_ || it == frames_.end()) {
Shouldn't you first test for frames_.end()?

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode184
net/quic/quic_stream_sequencer.cc:184: size_t iov_idx = 0;
nit: iov_index? (I found other folks using the "iov" abbreviation, and
figure maybe it is common in such code... but the idx might as well be
spelled out per style guide).

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode195
net/quic/quic_stream_sequencer.cc:195: char* iov_ptr =
static_cast<char*>(iov[iov_idx].iov_base) + iov_offset;
This has me a bit scared. Some of the iov[].iov_base are actually
pointers into some strings, that came out as "const char*". I'm a bit
wary of using that as reusable memory, and overwriting as in lines 196.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode217
net/quic/quic_stream_sequencer.cc:217:
it->second.substr(frame_offset)));
nit: indent

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode221
net/quic/quic_stream_sequencer.cc:221: return num_bytes_consumed_ -
initial_bytes_consumed;
The header says this should return the number of iov entries used, which
makes more sense, since I don't think there is any other way to deduce
that.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode257
net/quic/quic_stream_sequencer.cc:257:
frames_.insert(make_pair(num_bytes_consumed_, new_data));
I was curious why you used a slightly different pattern here, than you
did with seemingly similar code on line 216-218.

I don't know that either is better, so I was curious (and I had to reach
each pattern carefully to make sense of it).

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.h
File net/quic/quic_stream_sequencer.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.h#newcode64
net/quic/quic_stream_sequencer.h:64: // iovs used. Any buffered data no
longer in use will be released.
Reading the code, it appears to return the number of bytes transferred,
rather than the number of iovs used.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.h#newcode77
net/quic/quic_stream_sequencer.h:77: bool MaybeCloseStream();
nit: methods after typedefs etc.

https://codereview.chromium.org/11300020/

[aly...@google.com]

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc
File net/quic/quic_stream_sequencer.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode148
net/quic/quic_stream_sequencer.cc:148:
const_cast<char*>(it->second.c_str()));

+1 to data() just for clarity

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode195
net/quic/quic_stream_sequencer.cc:195: char* iov_ptr =
static_cast<char*>(iov[iov_idx].iov_base) + iov_offset;

I think there's some confusion about the memory model.

Readv is called with user-supplied buffers, and N bytes are copied into
the buffers.

GetReadableRegions is called to get access to the underlying sequencer
buffers (in this case strings).

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode195
net/quic/quic_stream_sequencer.cc:195: char* iov_ptr =
static_cast<char*>(iov[iov_idx].iov_base) + iov_offset;

I think there's some confusion about the memory model, which we should
clarify in comments in the .h file.

In Readv we're memcpy-ing into caller supplied buffers.
In GetReadableRegions we're providing access into the sequencer's
'buffers'

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode221
net/quic/quic_stream_sequencer.cc:221: return num_bytes_consumed_ -
initial_bytes_consumed;

I believe the header should be updated. This is supposed to be similar
to the system call:

man readv -> "on success, the readv() function returns the number of
bytes read;"

https://codereview.chromium.org/11300020/

[j...@chromium.org]

This is the second half of the comments on this patch set.

As per discussion, please upstream any resulting changes, but LGTM to land.


https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc
File net/quic/quic_stream_sequencer_test.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode37
net/quic/quic_stream_sequencer_test.cc:37: const char* data,
nit: indent

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode50
net/quic/quic_stream_sequencer_test.cc:50: ReliableQuicStream* stream()
{ return stream_; }
nit: most of these should be const methods. line 52 might need a const
return type.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode129
net/quic/quic_stream_sequencer_test.cc:129: .WillOnce(Return(3));
nit: indent

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode138
net/quic/quic_stream_sequencer_test.cc:138: .WillOnce(Return(2));
nit: indent (happens several more times).

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode263
net/quic/quic_stream_sequencer_test.cc:263: iovec iov[5];
nit: move closer to first use.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode397
net/quic/quic_stream_sequencer_test.cc:397: iovec iov = { &buffer[0], 3
};
nit: move closer to first use. (next test as well)

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode457
net/quic/quic_stream_sequencer_test.cc:457: int idx = payload_size -
remaining_payload;
nit: (assuming you went with the longer spelling in source code)
idx-->index (throughout this file)

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode504
net/quic/quic_stream_sequencer_test.cc:504: list_[idx].first,
list_[idx].second.c_str(),
nit: probably avoid c_str() (in favor of data()) unless really needed,
to avoid accidental mutation of string buffer.

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.cc
File net/quic/reliable_quic_stream.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.cc#newcode61
net/quic/reliable_quic_stream.cc:61:
sequencer_.CloseStreamAtOffset(offset, false); // Full close
nit: period at end

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.h
File net/quic/reliable_quic_stream.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.h#newcode50
net/quic/reliable_quic_stream.h:50: virtual bool HasBytesToRead();
nit: both above are probably const methods.

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.h#newcode63
net/quic/reliable_quic_stream.h:63: QuicSession* session() { return
session_; }
nit: const method

https://codereview.chromium.org/11300020/

[r...@chromium.org]

All but a few of these comments will be addressed when internal change
37570979
lands. Please review my responses for those which are not addressed.

Thanks for the review!

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream.h
File net/quic/quic_crypto_stream.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream.h#newcode54
net/quic/quic_crypto_stream.h:54: };

On 2012/10/31 22:37:37, jar wrote:
> nit: NO_COPY_AND_ASSIGN macro.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream_test.cc
File net/quic/quic_crypto_stream_test.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream_test.cc#newcode38
net/quic/quic_crypto_stream_test.cc:38: std::vector<CryptoTag>
message_tags_;

On 2012/10/31 22:37:37, jar wrote:
> nit: I think even here in test land, members should be private, and we
should
> use accessors.

> You can use protected in test fixtures, but I don't think this
qualifies.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream_test.cc#newcode40
net/quic/quic_crypto_stream_test.cc:40: };
On 2012/10/31 22:37:37, jar wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_stream_test.cc#newcode60
net/quic/quic_crypto_stream_test.cc:60: IPEndPoint addr_;

On 2012/10/31 22:37:37, jar wrote:
> These should probably be protected:

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_session.cc
File net/quic/quic_session.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_session.cc#newcode179
net/quic/quic_session.cc:179: // If the stream was locally initiated we
strictly in-order creation.

On 2012/10/31 22:37:37, jar wrote:
> nit: typo? "...we strictly in-order creation"

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_session.cc#newcode183
net/quic/quic_session.cc:183: } else {

On 2012/10/31 22:37:37, jar wrote:
> nit: with return on line 182, else and curlies are not needed.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_session.cc#newcode187
net/quic/quic_session.cc:187: implicitly_created_streams_.count(id) ==
0;

On 2012/10/31 22:37:37, jar wrote:
> I don't think there are every any entries in
implicitly_created_streams_ that
> are larger than the largest_peer_created_stream_id.

> Why are we checking in line 187?

We need IsClosed() to return true *only* when a stream has been closed.
So if IsClosed() is called with a stream id *larger* than the largest
peer created stream id, we need IsClosed() to return false.

The check on 186 will cause us to return false if the stream id is
larger than the largest peer created stream id.

The check on 187 will cause us to return false if the stream id has been
implicitly created.

Only if it has actually been closed, will these two checks allows us to
return true. (FWIW, there is some pretty rigorous testing (I hope :>)
in quic_session_test)

Added a more explicit comment in the .h file. (upstream)

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc
File net/quic/quic_stream_sequencer.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode45
net/quic/quic_stream_sequencer.cc:45: size_t data_len =
frame.data.size();

On 2012/10/31 22:37:37, jar wrote:
> nit: declare/init as close to first use as possible.

I assume you're referring to byte_offset, not data_len?

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode54
net/quic/quic_stream_sequencer.cc:54: // stream might only consume part
of it and we'd need a partial ack.

On 2012/10/31 22:37:37, jar wrote:
> nit: typo? "...can't pass it up the stream might..."

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode72
net/quic/quic_stream_sequencer.cc:72: size_t data_len =
frame.data.size();

On 2012/10/31 22:37:37, jar wrote:
> nit: define/init as close to first use as possible.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode75
net/quic/quic_stream_sequencer.cc:75: // This should not happen, as
WillAcceptFrame should be called before

On 2012/10/31 22:37:37, jar wrote:
> Shouldn't this be a DCHECK(), even if we do put this defensive code
here?

Could be a DCHECK, but that complicates unit tests. Leaving as is, as
per our offline discussion.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode142
net/quic/quic_stream_sequencer.cc:142: size_t idx = 0;

On 2012/10/31 22:37:37, jar wrote:
> nit: idx --> index

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode148
net/quic/quic_stream_sequencer.cc:148:
const_cast<char*>(it->second.c_str()));

On 2012/10/31 22:37:37, jar wrote:
> Should this be second.data()? I think the other call may instigate an
> allocation and strcpy just to make room for the null.

> The other uses of c_str() in this file might also need to be changed.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode159
net/quic/quic_stream_sequencer.cc:159: FrameMap::iterator it =
frames_.begin();

On 2012/10/31 22:37:37, jar wrote:
> nit: putting this after line 161 will mean that you don't have to
reset it in
> line 171 (and make the loop clearer that it is always pointing to
frames.begin()
> ).

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode162
net/quic/quic_stream_sequencer.cc:162: if (it->first !=
num_bytes_consumed_ || it == frames_.end()) {

On 2012/10/31 22:37:37, jar wrote:
> Shouldn't you first test for frames_.end()?

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode184
net/quic/quic_stream_sequencer.cc:184: size_t iov_idx = 0;

On 2012/10/31 22:37:37, jar wrote:
> nit: iov_index? (I found other folks using the "iov" abbreviation,
and figure
> maybe it is common in such code... but the idx might as well be
spelled out per
> style guide).

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode195
net/quic/quic_stream_sequencer.cc:195: char* iov_ptr =
static_cast<char*>(iov[iov_idx].iov_base) + iov_offset;

On 2012/11/01 17:43:47, alyssar wrote:
> I think there's some confusion about the memory model, which we should
clarify
> in comments in the .h file.

> In Readv we're memcpy-ing into caller supplied buffers.
> In GetReadableRegions we're providing access into the sequencer's
'buffers'

Changed to use data().

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode217
net/quic/quic_stream_sequencer.cc:217:
it->second.substr(frame_offset)));

On 2012/10/31 22:37:37, jar wrote:
> nit: indent

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode221
net/quic/quic_stream_sequencer.cc:221: return num_bytes_consumed_ -
initial_bytes_consumed;

On 2012/11/01 17:43:47, alyssar wrote:
> I believe the header should be updated. This is supposed to be
similar to the
> system call:

> man readv -> "on success, the readv() function returns the number of
bytes
> read;"

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode257
net/quic/quic_stream_sequencer.cc:257:
frames_.insert(make_pair(num_bytes_consumed_, new_data));

On 2012/10/31 22:37:37, jar wrote:
> I was curious why you used a slightly different pattern here, than you
did with
> seemingly similar code on line 216-218.

> I don't know that either is better, so I was curious (and I had to
reach each
> pattern carefully to make sense of it).

Do you think something should be changed here? Can you be explicit?

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.h
File net/quic/quic_stream_sequencer.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.h#newcode64
net/quic/quic_stream_sequencer.h:64: // iovs used. Any buffered data no
longer in use will be released.

On 2012/10/31 22:37:37, jar wrote:
> Reading the code, it appears to return the number of bytes
transferred, rather
> than the number of iovs used.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.h#newcode77
net/quic/quic_stream_sequencer.h:77: bool MaybeCloseStream();

On 2012/10/31 22:37:37, jar wrote:
> nit: methods after typedefs etc.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc
File net/quic/quic_stream_sequencer_test.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode37
net/quic/quic_stream_sequencer_test.cc:37: const char* data,

On 2012/11/01 22:21:10, jar wrote:
> nit: indent

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode50
net/quic/quic_stream_sequencer_test.cc:50: ReliableQuicStream* stream()
{ return stream_; }

On 2012/11/01 22:21:10, jar wrote:
> nit: most of these should be const methods. line 52 might need a const
return
> type.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode129
net/quic/quic_stream_sequencer_test.cc:129: .WillOnce(Return(3));

On 2012/11/01 22:21:10, jar wrote:
> nit: indent

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode138
net/quic/quic_stream_sequencer_test.cc:138: .WillOnce(Return(2));

On 2012/11/01 22:21:10, jar wrote:
> nit: indent (happens several more times).

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode263
net/quic/quic_stream_sequencer_test.cc:263: iovec iov[5];

On 2012/11/01 22:21:10, jar wrote:
> nit: move closer to first use.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode397
net/quic/quic_stream_sequencer_test.cc:397: iovec iov = { &buffer[0], 3
};

On 2012/11/01 22:21:10, jar wrote:
> nit: move closer to first use. (next test as well)

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode457
net/quic/quic_stream_sequencer_test.cc:457: int idx = payload_size -
remaining_payload;

On 2012/11/01 22:21:10, jar wrote:
> nit: (assuming you went with the longer spelling in source code)
idx-->index
> (throughout this file)

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer_test.cc#newcode504
net/quic/quic_stream_sequencer_test.cc:504: list_[idx].first,
list_[idx].second.c_str(),

On 2012/11/01 22:21:10, jar wrote:
> nit: probably avoid c_str() (in favor of data()) unless really needed,
to avoid
> accidental mutation of string buffer.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.cc
File net/quic/reliable_quic_stream.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.cc#newcode61
net/quic/reliable_quic_stream.cc:61:
sequencer_.CloseStreamAtOffset(offset, false); // Full close

On 2012/11/01 22:21:10, jar wrote:
> nit: period at end

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.h
File net/quic/reliable_quic_stream.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.h#newcode50
net/quic/reliable_quic_stream.h:50: virtual bool HasBytesToRead();

On 2012/11/01 22:21:10, jar wrote:
> nit: both above are probably const methods.

Fixed upstream.

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_stream.h#newcode63
net/quic/reliable_quic_stream.h:63: QuicSession* session() { return
session_; }

On 2012/11/01 22:21:10, jar wrote:
> nit: const method

It's gauche to return mutable pointers to members in a const method.
IIRC, there was a recent thread about this on chromium-dev.

https://codereview.chromium.org/11300020/

[j...@chromium.org]

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc
File net/quic/quic_stream_sequencer.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_sequencer.cc#newcode45
net/quic/quic_stream_sequencer.cc:45: size_t data_len =
frame.data.size();

I think my comment was mostly bogus. I saw this problem around line 72,
and then glanced up the page to see if there were other similar items
(and this looked similar enough at the time to make me add a comment...
but it is IMO not that similar.). You could move line 44 as you
noted... and that would be good... but not as notable as the line 72
example, where I needed to search for the declaration.

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/r...@chromium.org/11300020/10002


https://chromiumcodereview.appspot.com/11300020/

[commi...@chromium.org]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number=15026
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

https://chromiumcodereview.appspot.com/11300020/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/r...@chromium.org/11300020/4002


https://chromiumcodereview.appspot.com/11300020/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/r...@chromium.org/11300020/4004


https://chromiumcodereview.appspot.com/11300020/

[commi...@chromium.org]

Sorry for I got bad news for ya.

Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number=35355

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/r...@chromium.org/11300020/3019


https://chromiumcodereview.appspot.com/11300020/

[commi...@chromium.org]

Change committed as 165858

https://chromiumcodereview.appspot.com/11300020/