Add QuicFramer and friends.

Add QuicFramer and friends. 

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

[Ryan Hamilton, 2012-10-12 19:08:57]

Hi Jim,

Here's the initial CL for QUIC.  It's not yet *used*, but the framer, et. al.
are there.

[jar (doing other things), 2012-10-14 23:04:38]

Mostly a bunch of nits, and a few questions.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
File net/quic/crypto/crypto_framer.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:26: }
I can live with (and think the network stack may have originally had this
policy)...  Do you want all if statements to have curlies?  even one-liners??

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:33: if (reader_->BytesRemaining() <
sizeof(uint32)) {
nit: better would be sizeof(message_tag_)

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:34: break;
Should this check for BytesRemaining == 0, and return false if not? .... or do
we have a stream with an unaligned end?

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:39: if (reader_->BytesRemaining() <
sizeof(uint16)) {
nit: sizeof(num_entries_)

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:63: if (reader_->BytesRemaining() <
num_entries_ * sizeof(uint16)) {
nit: it is always suggested that we use sizeof(variable) rather than
sizeof(typename).  

Could you consider moving line 68 out of the loop, and in front of line 63, so
that we can do sizeof(len)?

Same thing with line 53 moving up for use in line 49?

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:85: tag_value_map_[tags_[i]] = value;
Since StringPiece has a length, why do we want to keep a separate
tag_length_map?

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:96: buffer_ =
reader_->PeekRemainingPayload().as_string();
Is it valuable to preserve the reader_ here?  It almost seems like it could have
been a local variable (or pointed at via a scoped_ptr), and been discarded.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:103: size_t len = sizeof(uint32);  // message
tag
nit: Comments are sentences with Capital letters starting, and period at the
end.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:104: len += sizeof(uint16);  // number of map
entries
nit: declare/init as close to use as possible.  Move lines 103-104 to after 107.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:111: len += sizeof(uint16);  // value len
nit: how about using members, rather than types in most of these sizeof() calls?

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:153: state_ = STATE_READING_TAG;
Why is reader_ not discarded here?  ...but perhaps it shouldn't be saved ever as
mentioned above.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
File net/quic/crypto/crypto_framer.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:15: #include "net/quic/crypto/crypto_protocol.h"
nit: alphabetize

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:35: class CryptoFramer {
It might be helpful to have a class comment, indicating what this represents.  I
started to figure it out reading the member variables... but a header comment
would help.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:46: visitor_ = visitor;
Perhaps it is worth a comment that the ownership of the visitor is passed
irrevocably to the CryptoFramer.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:57: // Serializes |message| into |packet|. 
Returns fase if there was an
nit: fase-->false

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:97: // Length of the data associated with each
tag in the message currently
This says "Length" just like line 94, but it looks to be the actual map.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
File net/quic/crypto/crypto_framer_test.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:161: }
nit: maybe delete data;

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:206:
EXPECT_TRUE(framer.ProcessInput(StringPiece(AsChars(input), arraysize(input))));
nit: wrap line.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:266:
EXPECT_FALSE(framer.ProcessInput(StringPiece(AsChars(input),
arraysize(input))));
nit: wrap

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:306:
EXPECT_FALSE(framer.ProcessInput(StringPiece(AsChars(input),
arraysize(input))));
nit: wrap

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/null_decr...
File net/quic/crypto/null_decrypter.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/null_decr...
net/quic/crypto/null_decrypter.cc:16: LOG(INFO) << "here!";
nit: Do we want DLOG?

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/null_encr...
File net/quic/crypto/null_encrypter_test.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/null_encr...
net/quic/crypto/null_encrypter_test.cc:21: /* TODO(rch): use this when uint128
multiplication is implemented.
What is this all about?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader.cc
File net/quic/quic_data_reader.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.cc:19: if (!CanRead(2)) {
nit: probably better is to use:
CanRead(sizeof(*result))

Same in line 28 (and in similar functions).

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.cc:50: // Make sure that we have the whole uint48.
nit: you don't need lines 51-54, since the other reads will progressively check
for their bytes... and call OnFailure() if either part is not available.

(just like you did on lines 92-100)

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.cc:81: LOG(INFO) << "here";
nit: DLOG?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader.h
File net/quic/quic_data_reader.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.h:9: #include "net/quic/uint128.h"
nit: alphabetize

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.h:47: // Forwards the internal iterater on success.
nit: spelling: iterater--> iterator

This was inherited from the original SPDY file :-/.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.h:56: // Reads a 128-bit unsigned integer nito the
given output parameter.
nit: nito-->into

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer.cc
File net/quic/quic_data_writer.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.cc:19: : buffer_(new char[size]),
nit/comment: It is a bit tempting to have a fixed size character array that can
hold a UDP packet, if this is really meant to be the interior of a packet.  We'd
avoid secondary allocations,.. realloc, etc.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.cc:31: if (needed_size > capacity_) {
nit: to handle wrapping, perhaps better would be:

if (capacity_ - length_ < length)
  return NULL;

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.cc:39: return buffer_ + offset;
Why did you save length_ into offset?  Isn't this just
return buffer_ + length_;

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.cc:43: if (!BeginWrite(len)) return false;
nit: probably want to be consistent with braces and newline.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer.h
File net/quic/quic_data_writer.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.h:28: explicit QuicDataWriter(size_t length);
nit: constructors then destructors.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.h:35: char* rv = buffer_;
nit: Usually we try not to put much code in the header files.  I think the goal
is to reduce compile times.

Is there a good reason for doing this here?  It looks like optimization... but
most of these would be handled by the compiler optimizations (I think).

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.h:71: static void WriteUint128ToBuffer(uint128 value,
char* buffer);
This API is a bit scary (no way to check).  Is this used for writing back into
the quic_data_writer?... and if so... can we use a non-static method, and bounds
check the target?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.h:88: size_t length_;    // current length of the
buffer
nit: Capital and period.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc
File net/quic/quic_framer.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:33: // compute the length of the packet
nit: comments start with Upper-case, and end with periods.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:35: len += 1;  // fragment count
nit: IMO, nicer would be sizeof(some_named_member_)

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:48: if (!writer.WriteUInt8(fragments.size())) {
Do we have guarantees that framents.size() is less than 8 bits?  For consistency
in this method, it might be nice to assign the value to a uint8, and use that
local value. Line 52 made me wonder what would happen if the value was larger.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:126: DCHECK_GT(packet->length(), kPacketHeaderSize);
A CHECK is a bit tempting here.  I'm not sure about the context of this call...
but then again, it is called rarely at most, so a CHECK might be NBD.

...also... this stuff *might* go away if we don't retransmit.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:139: LOG(INFO) << "here!";
nit: DLOG

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:185: reader_.reset(NULL);
I wasn't clear about when you wanted to delete reader_, and when you returned
false, but didn't delete it. Why is it valuable to save it sometimes (in some
error conditions)?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:205: DLOG(WARNING) << "Unable to process fragment
data.";
This appears to be the only path where reader_ is not dropped.  How is it
further used in this case?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:370: return false;
nit: perhaps set detailed error?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:589: len += 4;  // stream id
nit: IMO, much better would be mostly sizeof(named_member_) all through here.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h
File net/quic/quic_framer.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:16: #include "base/string_piece.h"
nit: alphabetize last two lines

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:33: // Called when a new packet has been recieved.
Clarification: Is this called when the packet is received, but before
authentication etc?  After auth, but before processing? After processing?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:41: // |payload| is the non-encrypted FEC protected
payload of the packet.
Due to out-of-order receipt, even with FECs only across consecutive packets, it
is possible for participants for more than one FEC group to appear.  How is this
handled?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:58: // Called when FEC data has been parsed.
Same question about which FEC group is being handled.  Is that apparent in the
parameter?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:86: void set_visitor(QuicFramerVisitorInterface* visitor)
{
I wasn't familiar with the term visitor.  Is this kindred to an observer?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:90: // Set a builder to be called from the framer when
building FEC protected
It would be helpful to have the terms builder, framer, and visitor discussed in
a top level comment, so as to provide context. Is this actually done in some
other file?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:109: // Pass a revived data packet into the framer for
parsing.
I'm guessing that "revived" means "result of FEC based reconstruction," but it
would be good to clarify this.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:112: bool ProcessRevivedPacket(const IPEndPoint&
client_address,
I understood why the regular packet included IPEndPoints, but was not clear as
to how a reconstructed packet would also have such.  Note that it is possible
for the FEC contributors to have distinct IPEndPoints.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:118: // Returns true upon success.
Is it assumed that the input is sized perfectly to fit into this next next
packet?  What happens if the the input is too small, or too large?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:142: // to ciphertext no larger than |ciphertext_size|.
Is this for a single UDP packet?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:199: };
Should you have the macro to prevent copy constructors etc. from being
created?... or does this need to be handled by STL containers?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.cc
File net/quic/quic_protocol.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.cc...
net/quic/quic_protocol.cc:17: : stream_id(stream_id), fin(fin), offset(offset),
data(data) {
nit: one member init per line (once you can't fit everything on one line above).

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.cc...
net/quic/quic_protocol.cc:33: }
nit: silly(?) personal pref to assign null to buffer_... but you can take it or
leave it.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:16: #include "base/string_piece.h"
nit: alphabetize

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:35: const size_t kPacketHeaderSize = 25;
Do we really want these in the net namespace? Should we be in a net::quic
namespace here?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:73: PACKET_FLAGS_FEC = 1,  // Payload is FEC as opposed
to fragments
nit: periods at end of comments.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:253: friend std::ostream& operator<<(std::ostream& os,
const QuicAckFragment& s) {
I'm used to writing my own pretty prints, and avoiding operator overloads etc
(and usually avoiding streams).  Does net tend to do this as a special case?

I *think* the style guide says we're only supposed to use this for logging...
and is this what you're targetting?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:331: other.first_protected_packet_sequence_number)
return false;
nit: I think you standardized on curlies in if staments, but this one runs over
two lines, and needs curlies no matter what your standard.

Lines 329 and 332 should also probably use curlies.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:387: length() - kStartOfEncryptedData);
nit: indent

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:393: };
nit: put comment about why we don't disallow copy and assign (or add macro).

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.cc
File net/quic/quic_utils.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.cc#ne...
net/quic/quic_utils.cc:14: 1 +   // fragment count
nit: much better would be some sizeof(names) here.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.cc#ne...
net/quic/quic_utils.cc:22: static uint128 kPrime(16777216, 315);
nit: I like statics... but I think we're supposed to start using anonymous
namespaces.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.h
File net/quic/quic_utils.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.h#new...
net/quic/quic_utils.h:13: #include "net/quic/quic_protocol.h"
nit: alphabetize

https://codereview.chromium.org/11125002/diff/11031/net/quic/uint128.h
File net/quic/uint128.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/uint128.h#newcode30
net/quic/uint128.h:30: LOG(INFO) << "rhs: " << rhs.hi << " " << rhs.lo;
nit: DLOG

[Ryan Hamilton, 2012-10-15 21:22:08]

I think I've addressed most of your comment, albeit several with TODOs.  This
TODOs will result in subsequent CLs that will start life on the server since and
will then be migrated to Chrome.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
File net/quic/crypto/crypto_framer.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:26: }
On 2012/10/14 23:04:38, jar wrote:
> I can live with (and think the network stack may have originally had this
> policy)...  Do you want all if statements to have curlies?  even one-liners??

This is required Server-side style.  We have the same issue with the shared SPDY
code.  Minimizing diffs between Chrome and Server-side code is more important
than preserving this style in the network stack, at least in my experience
maintaining this shared code.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:33: if (reader_->BytesRemaining() <
sizeof(uint32)) {
On 2012/10/14 23:04:38, jar wrote:
> nit: better would be sizeof(message_tag_)

As per my C++ readability review, these sizeofs will change when I land
cl/35411506.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:34: break;
On 2012/10/14 23:04:38, jar wrote:
> Should this check for BytesRemaining == 0, and return false if not? .... or do
> we have a stream with an unaligned end?

I don't understand what this check would be doing?  BytesRemaing() == 0 does not
mean that we're at the end of the message, it just means we have not received
any more data.  But in order to have a valid method, more data better show up
later.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:85: tag_value_map_[tags_[i]] = value;
On 2012/10/14 23:04:38, jar wrote:
> Since StringPiece has a length, why do we want to keep a separate
> tag_length_map?

I'm not sure the best way to make use of that.   (And I think wtc is going to
change the wire format so this might change).  First we have a list of uint16
(lengths).  We need to store these so that when we process the value, we know
how much to read.  Are you proposing that when we read the lengths, we create a
string with length "N" and then we later overwrite that with a new string whose
values were found in the message?  That feels worse.  

I think wtc is going to change this to <len><value>* which will make the whole
issue largely moot.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:96: buffer_ =
reader_->PeekRemainingPayload().as_string();
On 2012/10/14 23:04:38, jar wrote:
> Is it valuable to preserve the reader_ here?  It almost seems like it could
have
> been a local variable (or pointed at via a scoped_ptr), and been discarded.

Changed to a local variable.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.cc:104: len += sizeof(uint16);  // number of map
entries
On 2012/10/14 23:04:38, jar wrote:
> nit: declare/init as close to use as possible.  Move lines 103-104 to after
107.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
File net/quic/crypto/crypto_framer.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:15: #include "net/quic/crypto/crypto_protocol.h"
On 2012/10/14 23:04:38, jar wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:35: class CryptoFramer {
On 2012/10/14 23:04:38, jar wrote:
> It might be helpful to have a class comment, indicating what this represents. 
I
> started to figure it out reading the member variables... but a header comment
> would help.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:46: visitor_ = visitor;
On 2012/10/14 23:04:38, jar wrote:
> Perhaps it is worth a comment that the ownership of the visitor is passed
> irrevocably to the CryptoFramer.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:46: visitor_ = visitor;
On 2012/10/14 23:04:38, jar wrote:
> Perhaps it is worth a comment that the ownership of the visitor is passed
> irrevocably to the CryptoFramer.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:57: // Serializes |message| into |packet|. 
Returns fase if there was an
On 2012/10/14 23:04:38, jar wrote:
> nit: fase-->false

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer.h:97: // Length of the data associated with each
tag in the message currently
On 2012/10/14 23:04:38, jar wrote:
> This says "Length" just like line 94, but it looks to be the actual map.

Will be fixed when the changes from cl/35411506 are merged.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
File net/quic/crypto/crypto_framer_test.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:161: }
On 2012/10/14 23:04:38, jar wrote:
> nit: maybe delete data;

Given that data will be uninitialized unless the test fails, I'm not sure this
is a great idea.  I could explicitly initialize data to NULL before the
Construct() call and then check to see if it's not NULL afterwards and then
delete, but that doesn't really seem useful.  I can do this if you feel
strongly.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:206:
EXPECT_TRUE(framer.ProcessInput(StringPiece(AsChars(input), arraysize(input))));
On 2012/10/14 23:04:38, jar wrote:
> nit: wrap line.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:266:
EXPECT_FALSE(framer.ProcessInput(StringPiece(AsChars(input),
arraysize(input))));
On 2012/10/14 23:04:38, jar wrote:
> nit: wrap

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:306:
EXPECT_FALSE(framer.ProcessInput(StringPiece(AsChars(input),
arraysize(input))));
On 2012/10/14 23:04:38, jar wrote:
> nit: wrap

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/null_decr...
File net/quic/crypto/null_decrypter.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/null_decr...
net/quic/crypto/null_decrypter.cc:16: LOG(INFO) << "here!";
On 2012/10/14 23:04:38, jar wrote:
> nit: Do we want DLOG?

Left over from some troubleshooting I'm doing.  Will remove.

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/null_encr...
File net/quic/crypto/null_encrypter_test.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/null_encr...
net/quic/crypto/null_encrypter_test.cc:21: /* TODO(rch): use this when uint128
multiplication is implemented.
On 2012/10/14 23:04:38, jar wrote:
> What is this all about?

As the comment suggests, we don't currently have int128 multiplication working
in Chrome, so this auth hash calculation is incorrect.  I need to get the
internal int128 implementation open sourced.  But I don't want to wait for that
to happen in order to start integrating quic.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader.cc
File net/quic/quic_data_reader.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.cc:19: if (!CanRead(2)) {
Added TODO.  Will address in a follow on CL once this lands.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.cc:50: // Make sure that we have the whole uint48.
On 2012/10/14 23:04:38, jar wrote:
> nit: you don't need lines 51-54, since the other reads will progressively
check
> for their bytes... and call OnFailure() if either part is not available.
> 
> (just like you did on lines 92-100)

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.cc:81: LOG(INFO) << "here";
On 2012/10/14 23:04:38, jar wrote:
> nit: DLOG?

Part of my on-going debugging.  will remove.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader.h
File net/quic/quic_data_reader.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.h:9: #include "net/quic/uint128.h"
On 2012/10/14 23:04:38, jar wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.h:47: // Forwards the internal iterater on success.
On 2012/10/14 23:04:38, jar wrote:
> nit: spelling: iterater--> iterator
> 
> This was inherited from the original SPDY file :-/.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_reader...
net/quic/quic_data_reader.h:56: // Reads a 128-bit unsigned integer nito the
given output parameter.
On 2012/10/14 23:04:38, jar wrote:
> nit: nito-->into

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer.cc
File net/quic/quic_data_writer.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.cc:19: : buffer_(new char[size]),
On 2012/10/14 23:04:38, jar wrote:
> nit/comment: It is a bit tempting to have a fixed size character array that
can
> hold a UDP packet, if this is really meant to be the interior of a packet. 
We'd
> avoid secondary allocations,.. realloc, etc.

But then the take() method could not be implemented.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.cc:31: if (needed_size > capacity_) {
On 2012/10/14 23:04:38, jar wrote:
> nit: to handle wrapping, perhaps better would be:
> 
> if (capacity_ - length_ < length)
>   return NULL;

Done.  FYI, This code was simply copied from the spdy variant.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.cc:39: return buffer_ + offset;
On 2012/10/14 23:04:38, jar wrote:
> Why did you save length_ into offset?  Isn't this just
> return buffer_ + length_;

This code was copied from the spdy variant.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.cc:43: if (!BeginWrite(len)) return false;
On 2012/10/14 23:04:38, jar wrote:
> nit: probably want to be consistent with braces and newline.

Done.  But just to be clear, this is perfectly acceptable style as is.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer.h
File net/quic/quic_data_writer.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.h:28: explicit QuicDataWriter(size_t length);
On 2012/10/14 23:04:38, jar wrote:
> nit: constructors then destructors.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.h:35: char* rv = buffer_;
On 2012/10/14 23:04:38, jar wrote:
> nit: Usually we try not to put much code in the header files.  I think the
goal
> is to reduce compile times.
> 
> Is there a good reason for doing this here?  It looks like optimization... but
> most of these would be handled by the compiler optimizations (I think).

This code mostly came from the SPDY equivalent and preserved the inlined
methods.  Added a TODO to move the code.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.h:71: static void WriteUint128ToBuffer(uint128 value,
char* buffer);
On 2012/10/14 23:04:38, jar wrote:
> This API is a bit scary (no way to check).  Is this used for writing back into
> the quic_data_writer?... and if so... can we use a non-static method, and
bounds
> check the target?

I believe these methods are used for tweaking fields in already constructed
packets.  It's possible that with the recent change to get rid of
re-transmission that they will not be needed, but currently they are used. 
Happy to revisit once we nuke retransmission.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_data_writer...
net/quic/quic_data_writer.h:88: size_t length_;    // current length of the
buffer
On 2012/10/14 23:04:38, jar wrote:
> nit: Capital and period.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc
File net/quic/quic_framer.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:33: // compute the length of the packet
On 2012/10/14 23:04:38, jar wrote:
> nit: comments start with Upper-case, and end with periods.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:35: len += 1;  // fragment count
On 2012/10/14 23:04:38, jar wrote:
> nit: IMO, nicer would be sizeof(some_named_member_)

That's a bit tricky because the sizeof(member_) is not guaranteed to be
sizeof(wire_format).  Added a number of comments.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:48: if (!writer.WriteUInt8(fragments.size())) {
On 2012/10/14 23:04:38, jar wrote:
> Do we have guarantees that framents.size() is less than 8 bits?  For
consistency
> in this method, it might be nice to assign the value to a uint8, and use that
> local value. Line 52 made me wonder what would happen if the value was larger.

Added DCHECK

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:126: DCHECK_GT(packet->length(), kPacketHeaderSize);
On 2012/10/14 23:04:38, jar wrote:
> A CHECK is a bit tempting here.  I'm not sure about the context of this
call...
> but then again, it is called rarely at most, so a CHECK might be NBD.
> 
> ...also... this stuff *might* go away if we don't retransmit.

In general we really don't want the server to crash in release mode.  DCHECK
(i.e. crash in debug mode) is totally fine.  But crashing in release mode is
highly undesirable.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:139: LOG(INFO) << "here!";
On 2012/10/14 23:04:38, jar wrote:
> nit: DLOG

Removed.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:185: reader_.reset(NULL);
On 2012/10/14 23:04:38, jar wrote:
> I wasn't clear about when you wanted to delete reader_, and when you returned
> false, but didn't delete it. Why is it valuable to save it sometimes (in some
> error conditions)?

I believe it is dropped everywhere.  Note the DCHECK at the start of
ProcessPacket:

  DCHECK(!reader_.get());

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:205: DLOG(WARNING) << "Unable to process fragment
data.";
On 2012/10/14 23:04:38, jar wrote:
> This appears to be the only path where reader_ is not dropped.  How is it
> further used in this case?

I believe it is dropped here because RaiseError is called from
ProcessFragmentData().

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:370: return false;
On 2012/10/14 23:04:38, jar wrote:
> nit: perhaps set detailed error?

Done.  I assume this code path will change at some point once we start bit
packing.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:589: len += 4;  // stream id
On 2012/10/14 23:04:38, jar wrote:
> nit: IMO, much better would be mostly sizeof(named_member_) all through here.

This is not guaranteed to work because the in-memory format is not equivalent to
the wire format.  Added a todo.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h
File net/quic/quic_framer.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:16: #include "base/string_piece.h"
On 2012/10/14 23:04:38, jar wrote:
> nit: alphabetize last two lines

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:33: // Called when a new packet has been recieved.
On 2012/10/14 23:04:38, jar wrote:
> Clarification: Is this called when the packet is received, but before
> authentication etc?  After auth, but before processing? After processing?

Commented.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:41: // |payload| is the non-encrypted FEC protected
payload of the packet.
On 2012/10/14 23:04:38, jar wrote:
> Due to out-of-order receipt, even with FECs only across consecutive packets,
it
> is possible for participants for more than one FEC group to appear.  How is
this
> handled?

In the code not committed as part of this CL, we maintain a collection of "open"
fec groups.  But this code is not being committed as part of this cl.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:58: // Called when FEC data has been parsed.
On 2012/10/14 23:04:38, jar wrote:
> Same question about which FEC group is being handled.  Is that apparent in the
> parameter?

It is in the packet header that was passed in during OnPacketHeader().

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:86: void set_visitor(QuicFramerVisitorInterface* visitor)
{
On 2012/10/14 23:04:38, jar wrote:
> I wasn't familiar with the term visitor.  Is this kindred to an observer?

Yes.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:90: // Set a builder to be called from the framer when
building FEC protected
On 2012/10/14 23:04:38, jar wrote:
> It would be helpful to have the terms builder, framer, and visitor discussed
in
> a top level comment, so as to provide context. Is this actually done in some
> other file?

Added a comment.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:109: // Pass a revived data packet into the framer for
parsing.
On 2012/10/14 23:04:38, jar wrote:
> I'm guessing that "revived" means "result of FEC based reconstruction," but it
> would be good to clarify this.

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:112: bool ProcessRevivedPacket(const IPEndPoint&
client_address,
On 2012/10/14 23:04:38, jar wrote:
> I understood why the regular packet included IPEndPoints, but was not clear as
> to how a reconstructed packet would also have such.  Note that it is possible
> for the FEC contributors to have distinct IPEndPoints.

If the FEC packet were the first packet received from a new IP address we need
to make sure that we take account of the new IP.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:118: // Returns true upon success.
On 2012/10/14 23:04:38, jar wrote:
> Is it assumed that the input is sized perfectly to fit into this next next
> packet?  What happens if the the input is too small, or too large?

If the packet can not be constructed as requested, this method will return
false.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:142: // to ciphertext no larger than |ciphertext_size|.
On 2012/10/14 23:04:38, jar wrote:
> Is this for a single UDP packet?

That is how this will be used, but this method does not explicitly encode that
knowledge.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:199: };
On 2012/10/14 23:04:38, jar wrote:
> Should you have the macro to prevent copy constructors etc. from being
> created?... or does this need to be handled by STL containers?

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.cc
File net/quic/quic_protocol.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.cc...
net/quic/quic_protocol.cc:17: : stream_id(stream_id), fin(fin), offset(offset),
data(data) {
On 2012/10/14 23:04:38, jar wrote:
> nit: one member init per line (once you can't fit everything on one line
above).

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.cc...
net/quic/quic_protocol.cc:33: }
On 2012/10/14 23:04:38, jar wrote:
> nit: silly(?) personal pref to assign null to buffer_... but you can take it
or
> leave it.

Leaving it.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:16: #include "base/string_piece.h"
On 2012/10/14 23:04:38, jar wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:35: const size_t kPacketHeaderSize = 25;
On 2012/10/14 23:04:38, jar wrote:
> Do we really want these in the net namespace? Should we be in a net::quic
> namespace here?

This is the same pattern we followed for SPDY.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:253: friend std::ostream& operator<<(std::ostream& os,
const QuicAckFragment& s) {
On 2012/10/14 23:04:38, jar wrote:
> I'm used to writing my own pretty prints, and avoiding operator overloads etc
> (and usually avoiding streams).  Does net tend to do this as a special case?
> 
> I *think* the style guide says we're only supposed to use this for logging...
> and is this what you're targetting?

Yes, this is targeting printing.  This code is shared between server and client
and having managed the SPDY code, I found it much more important to minimize
diffs between client and server.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:331: other.first_protected_packet_sequence_number)
return false;
On 2012/10/14 23:04:38, jar wrote:
> nit: I think you standardized on curlies in if staments, but this one runs
over
> two lines, and needs curlies no matter what your standard.
> 
> Lines 329 and 332 should also probably use curlies.

I love it when reviewers fight.  Oh wait, no I don't.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:387: length() - kStartOfEncryptedData);
On 2012/10/14 23:04:38, jar wrote:
> nit: indent

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:393: };
On 2012/10/14 23:04:38, jar wrote:
> nit: put comment about why we don't disallow copy and assign (or add macro).

Added TODO.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.cc
File net/quic/quic_utils.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.cc#ne...
net/quic/quic_utils.cc:14: 1 +   // fragment count
On 2012/10/14 23:04:38, jar wrote:
> nit: much better would be some sizeof(names) here.

added todo.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.cc#ne...
net/quic/quic_utils.cc:22: static uint128 kPrime(16777216, 315);
On 2012/10/14 23:04:38, jar wrote:
> nit: I like statics... but I think we're supposed to start using anonymous
> namespaces.

added todo.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.h
File net/quic/quic_utils.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_utils.h#new...
net/quic/quic_utils.h:13: #include "net/quic/quic_protocol.h"
On 2012/10/14 23:04:38, jar wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/uint128.h
File net/quic/uint128.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/uint128.h#newcode30
net/quic/uint128.h:30: LOG(INFO) << "rhs: " << rhs.hi << " " << rhs.lo;
On 2012/10/14 23:04:38, jar wrote:
> nit: DLOG

Removed.

[jar (doing other things), 2012-10-15 23:54:31]

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
File net/quic/crypto/crypto_framer_test.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:161: }
Not a big deal... but init to NULL, and then just call delete (without bothering
to test) would be a pitifully bit cleaner.  If you like it as is, then NBD.

On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> On 2012/10/14 23:04:38, jar wrote:
> > nit: maybe delete data;
> 
> Given that data will be uninitialized unless the test fails, I'm not sure this
> is a great idea.  I could explicitly initialize data to NULL before the
> Construct() call and then check to see if it's not NULL afterwards and then
> delete, but that doesn't really seem useful.  I can do this if you feel
> strongly.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc
File net/quic/quic_framer.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:126: DCHECK_GT(packet->length(), kPacketHeaderSize);
It sure seems evil to increment memory that is not really ours... even in a
server scenario.  That is the big fear here.  If you hate the CHECK, maybe a
test, and then noop if invalid?

On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> On 2012/10/14 23:04:38, jar wrote:
> > A CHECK is a bit tempting here.  I'm not sure about the context of this
> call...
> > but then again, it is called rarely at most, so a CHECK might be NBD.
> > 
> > ...also... this stuff *might* go away if we don't retransmit.
> 
> In general we really don't want the server to crash in release mode.  DCHECK
> (i.e. crash in debug mode) is totally fine.  But crashing in release mode is
> highly undesirable.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:185: reader_.reset(NULL);
On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> On 2012/10/14 23:04:38, jar wrote:
> > I wasn't clear about when you wanted to delete reader_, and when you
returned
> > false, but didn't delete it. Why is it valuable to save it sometimes (in
some
> > error conditions)?
> 
> I believe it is dropped everywhere.  Note the DCHECK at the start of
> ProcessPacket:
> 
>   DCHECK(!reader_.get());

There are a bunch of early returns that don't *seem* to reset reader_.  Line
158, 169 and 177 are examples.

[Ryan Hamilton, 2012-10-16 00:03:56]

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
File net/quic/crypto/crypto_framer_test.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/crypto/crypto_fr...
net/quic/crypto/crypto_framer_test.cc:161: }
On 2012/10/15 23:54:32, jar wrote:
> Not a big deal... but init to NULL, and then just call delete (without
bothering
> to test) would be a pitifully bit cleaner.  If you like it as is, then NBD.
> 
> On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> > On 2012/10/14 23:04:38, jar wrote:
> > > nit: maybe delete data;
> > 
> > Given that data will be uninitialized unless the test fails, I'm not sure
this
> > is a great idea.  I could explicitly initialize data to NULL before the
> > Construct() call and then check to see if it's not NULL afterwards and then
> > delete, but that doesn't really seem useful.  I can do this if you feel
> > strongly.
> 

Done.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc
File net/quic/quic_framer.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:126: DCHECK_GT(packet->length(), kPacketHeaderSize);
On 2012/10/15 23:54:32, jar wrote:
> It sure seems evil to increment memory that is not really ours... even in a
> server scenario.  That is the big fear here.  If you hate the CHECK, maybe a
> test, and then noop if invalid?
> 
> On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> > On 2012/10/14 23:04:38, jar wrote:
> > > A CHECK is a bit tempting here.  I'm not sure about the context of this
> > call...
> > > but then again, it is called rarely at most, so a CHECK might be NBD.
> > > 
> > > ...also... this stuff *might* go away if we don't retransmit.
> > 
> > In general we really don't want the server to crash in release mode.  DCHECK
> > (i.e. crash in debug mode) is totally fine.  But crashing in release mode is
> > highly undesirable.
> 

Done.  I'll attempt to land a CHECK on the server side an can hash out a better
alternative there.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:185: reader_.reset(NULL);
On 2012/10/15 23:54:32, jar wrote:
> On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> > On 2012/10/14 23:04:38, jar wrote:
> > > I wasn't clear about when you wanted to delete reader_, and when you
> returned
> > > false, but didn't delete it. Why is it valuable to save it sometimes (in
> some
> > > error conditions)?
> > 
> > I believe it is dropped everywhere.  Note the DCHECK at the start of
> > ProcessPacket:
> > 
> >   DCHECK(!reader_.get());
> 
> There are a bunch of early returns that don't *seem* to reset reader_.  Line
> 158, 169 and 177 are examples. 

You're talking about this, right:

	 158     return RaiseError(QUIC_PACKET_TOO_LARGE);

I think that's ok.  Look at the definition of RaiseError, which includes:

 787   reader_.reset(NULL);

[Ryan Hamilton, 2012-10-16 16:46:47]

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc
File net/quic/quic_framer.cc (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.cc#n...
net/quic/quic_framer.cc:185: reader_.reset(NULL);
On 2012/10/16 00:03:56, Ryan Hamilton wrote:
> On 2012/10/15 23:54:32, jar wrote:
> > On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> > > On 2012/10/14 23:04:38, jar wrote:
> > > > I wasn't clear about when you wanted to delete reader_, and when you
> > returned
> > > > false, but didn't delete it. Why is it valuable to save it sometimes (in
> > some
> > > > error conditions)?
> > > 
> > > I believe it is dropped everywhere.  Note the DCHECK at the start of
> > > ProcessPacket:
> > > 
> > >   DCHECK(!reader_.get());
> > 
> > There are a bunch of early returns that don't *seem* to reset reader_.  Line
> > 158, 169 and 177 are examples. 
> 
> You're talking about this, right:
> 
> 	 158     return RaiseError(QUIC_PACKET_TOO_LARGE);
> 
> I think that's ok.  Look at the definition of RaiseError, which includes:
> 
>  787   reader_.reset(NULL);

Looks like 166 and 177 do not RaiseError.  I'll fix and then upstream.  Thanks
for the catch.

[jar (doing other things), 2012-10-16 19:24:00]

Please at least answer the questions (and possibly todo comments, or an upstream
change), and then LGTM.

https://chromiumcodereview.appspot.com/11125002/diff/11031/net/quic/quic_fram...
File net/quic/quic_framer.h (right):

https://chromiumcodereview.appspot.com/11125002/diff/11031/net/quic/quic_fram...
net/quic/quic_framer.h:112: bool ProcessRevivedPacket(const IPEndPoint&
client_address,
On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> On 2012/10/14 23:04:38, jar wrote:
> > I understood why the regular packet included IPEndPoints, but was not clear
as
> > to how a reconstructed packet would also have such.  Note that it is
possible
> > for the FEC contributors to have distinct IPEndPoints.
> 
> If the FEC packet were the first packet received from a new IP address we need
> to make sure that we take account of the new IP.

hmm....

You're suggesting that the revived packet should have the IP address of the last
packet in an FEC group?  That might (for example) mean that a lost 2nd packet
among an FEC group of 6 would have a "new IP address" even if packets numbered
3-5 don't.  I'd expect this to cause confusion in the return address algorithm.

What am I missing?

https://chromiumcodereview.appspot.com/11125002/diff/11031/net/quic/quic_prot...
File net/quic/quic_protocol.h (right):

https://chromiumcodereview.appspot.com/11125002/diff/11031/net/quic/quic_prot...
net/quic/quic_protocol.h:35: const size_t kPacketHeaderSize = 25;
On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> On 2012/10/14 23:04:38, jar wrote:
> > Do we really want these in the net namespace? Should we be in a net::quic
> > namespace here?
> 
> This is the same pattern we followed for SPDY.

Too big to change now.... but this seems bad.  At a minimum the name shouldn't
be so generic, and not even mention QUIC.

https://chromiumcodereview.appspot.com/11125002/diff/25001/net/quic/quic_data...
File net/quic/quic_data_writer.cc (right):

https://chromiumcodereview.appspot.com/11125002/diff/25001/net/quic/quic_data...
net/quic/quic_data_writer.cc:29: if (length_ + length > capacity_) {
nit: To avoid integer overflow confusion, I think you really need to do this
test as:

if (capacity_ - length_ < length)

That approach trusts the slots to be reasonable, but then can handle any value
of |length|.

https://chromiumcodereview.appspot.com/11125002/diff/25001/net/quic/quic_fram...
File net/quic/quic_framer.h (right):

https://chromiumcodereview.appspot.com/11125002/diff/25001/net/quic/quic_fram...
net/quic/quic_framer.h:15: #include "net/base/ip_endpoint.h"
nit: alphebetize

[Ryan Hamilton, 2012-10-16 19:43:23]

Thanks for the review.

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h
File net/quic/quic_framer.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:112: bool ProcessRevivedPacket(const IPEndPoint&
client_address,
On 2012/10/16 19:24:00, jar wrote:
> On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> > On 2012/10/14 23:04:38, jar wrote:
> > > I understood why the regular packet included IPEndPoints, but was not
clear
> as
> > > to how a reconstructed packet would also have such.  Note that it is
> possible
> > > for the FEC contributors to have distinct IPEndPoints.
> > 
> > If the FEC packet were the first packet received from a new IP address we
need
> > to make sure that we take account of the new IP.
> 
> hmm....
> 
> You're suggesting that the revived packet should have the IP address of the
last
> packet in an FEC group?  That might (for example) mean that a lost 2nd packet
> among an FEC group of 6 would have a "new IP address" even if packets numbered
> 3-5 don't.  I'd expect this to cause confusion in the return address
algorithm.
> 
> What am I missing? 

In my example, I was suggesting that the last received packet in the group was
the FEC packet.  In this example, I was also suggesting that it was the first
packet from the new IP.  In the case, I wanted to make sure that that Visitor
got the new IP along with the revived packet.

Am I misunderstanding your question?

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/11125002/diff/11031/net/quic/quic_protocol.h#...
net/quic/quic_protocol.h:35: const size_t kPacketHeaderSize = 25;
On 2012/10/16 19:24:00, jar wrote:
> On 2012/10/15 21:22:08, Ryan Hamilton wrote:
> > On 2012/10/14 23:04:38, jar wrote:
> > > Do we really want these in the net namespace? Should we be in a net::quic
> > > namespace here?
> > 
> > This is the same pattern we followed for SPDY.
> 
> Too big to change now.... but this seems bad.  At a minimum the name shouldn't
> be so generic, and not even mention QUIC.

TODO added.

https://codereview.chromium.org/11125002/diff/25001/net/quic/quic_data_writer.cc
File net/quic/quic_data_writer.cc (right):

https://codereview.chromium.org/11125002/diff/25001/net/quic/quic_data_writer...
net/quic/quic_data_writer.cc:29: if (length_ + length > capacity_) {
On 2012/10/16 19:24:01, jar wrote:
> nit: To avoid integer overflow confusion, I think you really need to do this
> test as:
> 
> if (capacity_ - length_ < length)
> 
> That approach trusts the slots to be reasonable, but then can handle any value
> of |length|.

Ah, I see.  I didn't realize that was why you wrote the code that way (which
seemed convoluted.  Turns out it was ... for a reason).  Done.

https://codereview.chromium.org/11125002/diff/25001/net/quic/quic_framer.h
File net/quic/quic_framer.h (right):

https://codereview.chromium.org/11125002/diff/25001/net/quic/quic_framer.h#ne...
net/quic/quic_framer.h:15: #include "net/base/ip_endpoint.h"
On 2012/10/16 19:24:01, jar wrote:
> nit: alphebetize

Done.

[commit-bot: I haz the power, 2012-10-16 19:46:45]

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

[commit-bot: I haz the power, 2012-10-16 21:43:21]

Change committed as 162259