Add QuicStream and friends to QUIC code.

Add QuicStream and friends to QUIC code.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165858

[Ryan Hamilton, 2012-10-31 17:11:47]

friendly ping

[jar (doing other things), 2012-10-31 22:37:37]

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_strea...
net/quic/quic_crypto_stream.h:54: };
nit: NO_COPY_AND_ASSIGN macro.

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

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_strea...
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_strea...
net/quic/quic_crypto_stream_test.cc:40: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_strea...
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#n...
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#n...
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#n...
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_seque...
File net/quic/quic_stream_sequencer.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
File net/quic/quic_stream_sequencer.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_seque...
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_seque...
net/quic/quic_stream_sequencer.h:77: bool MaybeCloseStream();
nit: methods after typedefs etc.

[alyssar, 2012-11-01 17:43:47]

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

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_seque...
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_seque...
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_seque...
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_seque...
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;"

[jar (doing other things), 2012-11-01 22:21:10]

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_seque...
File net/quic/quic_stream_sequencer_test.cc (right):

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

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_seque...
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_seque...
net/quic/quic_stream_sequencer_test.cc:129: .WillOnce(Return(3));
nit: indent

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_str...
File net/quic/reliable_quic_stream.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_str...
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_str...
File net/quic/reliable_quic_stream.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_str...
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_str...
net/quic/reliable_quic_stream.h:63: QuicSession* session() { return session_; }
nit: const method

[Ryan Hamilton, 2012-11-01 22:52:20]

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_strea...
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_strea...
File net/quic/quic_crypto_stream_test.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_crypto_strea...
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_strea...
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_strea...
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#n...
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#n...
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#n...
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_seque...
File net/quic/quic_stream_sequencer.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
File net/quic/quic_stream_sequencer.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_seque...
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_seque...
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_seque...
File net/quic/quic_stream_sequencer_test.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_seque...
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_str...
File net/quic/reliable_quic_stream.cc (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_str...
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_str...
File net/quic/reliable_quic_stream.h (right):

https://codereview.chromium.org/11300020/diff/5001/net/quic/reliable_quic_str...
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_str...
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.

[jar (doing other things), 2012-11-01 23:20:38]

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

https://codereview.chromium.org/11300020/diff/5001/net/quic/quic_stream_seque...
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.



On 2012/11/01 22:52:20, Ryan Hamilton wrote:
> 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?

[commit-bot: I haz the power, 2012-11-03 03:13:59]

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

[commit-bot: I haz the power, 2012-11-03 03:32:17]

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...
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.

[commit-bot: I haz the power, 2012-11-03 03:54:48]

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

[commit-bot: I haz the power, 2012-11-03 04:22:53]

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

[commit-bot: I haz the power, 2012-11-03 04:58:06]

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...
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.

[commit-bot: I haz the power, 2012-11-03 14:06:26]

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

[commit-bot: I haz the power, 2012-11-03 16:55:34]

Change committed as 165858