Add QuicFramer and friends.

Add QuicFramer and friends.

Also fixes an unsigned constant problem from 162259.

Revert 162263 - Revert 162259 - Add QuicFramer and friends.


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

[Ryan Hamilton, 2012-10-16 22:24:04]

Annoyingly, this broke the build despite succeeding on a whole pile of try bots.
 Patch set 1 is the original CL (which you already reviewed).  Patch set 2 is
the tiny delta.

[jar (doing other things), 2012-10-16 23:39:01]

lgtm

[Ryan Sleevi, 2012-10-17 04:27:13]

ChromeOS takes too dang long to compile, so some free nits for your
consideration, since you mentioned this CL on chromium-dev :)

I'm not sure which side of the fence you'll want to address them, but I did want
to bring some visibility.

https://codereview.chromium.org/11188017/diff/5001/net/quic/crypto/crypto_pro...
File net/quic/crypto/crypto_protocol.cc (right):

https://codereview.chromium.org/11188017/diff/5001/net/quic/crypto/crypto_pro...
net/quic/crypto/crypto_protocol.cc:10:
CryptoHandshakeMessage::~CryptoHandshakeMessage() {}
nit: newline

https://codereview.chromium.org/11188017/diff/5001/net/quic/crypto/crypto_pro...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/11188017/diff/5001/net/quic/crypto/crypto_pro...
net/quic/crypto/crypto_protocol.h:19: struct NET_EXPORT_PRIVATE
CryptoHandshakeMessage {
style nit: newline between these types

https://codereview.chromium.org/11188017/diff/5001/net/quic/crypto/null_decry...
File net/quic/crypto/null_decrypter.h (right):

https://codereview.chromium.org/11188017/diff/5001/net/quic/crypto/null_decry...
net/quic/crypto/null_decrypter.h:16: // verify a hash (fnv128) over both the
payload and associated data.
nit: comment update to explain that the hash is not for cryptographic protection
(at least AIUI, it's for error control)

https://codereview.chromium.org/11188017/diff/5001/net/quic/crypto/quic_encry...
File net/quic/crypto/quic_encrypter.h (right):

https://codereview.chromium.org/11188017/diff/5001/net/quic/crypto/quic_encry...
net/quic/crypto/quic_encrypter.h:33: 
nit: whitespace

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

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_data_reader....
net/quic/quic_data_reader.h:60: bool ReadUInt128(uint128* result);
drive by review: I don't see you actually making use of the fact that the ReadX
functions return booleans, and instead acting as if they always succeed
(typically combined with explicitly checking if .BytesRemaining() > some_size

This means that there are now TWO ways to check for success (the method
returning false or BytesRemaining < X). It seems like you should consider making
these functions WARN_UNUSED_RESULT, since if they do fail (for whatever API
reason that is not specified in this header), they could leave uninitialized
members in |result|, and lead to protocol exploits.

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

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_data_writer....
net/quic/quic_data_writer.cc:14: using std::numeric_limits;
nit: Chromium style consensus "Using is bad, mkay", although this has not been
codified. It's extremely rare in Chromium, outside of WebKit, and even in
src/chrome there has been a move away from it.

Last time it was raised -
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/qSrl_UAqAkQ/dis...

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_data_writer....
net/quic/quic_data_writer.cc:33: #ifdef ARCH_CPU_64_BITS
nit: #if defined(...), as per Chromium style guide

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

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_data_writer....
net/quic/quic_data_writer.h:57: uint32 lo = value & 0x00000000FFFFFFFF;
Possible BUG: I believe you *may* need to wrap this in GG_ULONGLONG to properly
have MSVC recognize this as a 64-bit constant. At least, I'd suggest checking to
make sure this will do the right thing - MSVC gets picky about 64-bit constants.

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

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:32: QuicPacket** packet) {
Future point of consideration:
https://sites.google.com/a/chromium.org/dev/developers/smart-pointer-guidelin...

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

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_framer_test....
net/quic/quic_framer_test.cc:110: fec_count_++;
nit: pre-increment

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

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_protocol.h#n...
net/quic/quic_protocol.h:176: base::hash_set<QuicPacketSequenceNumber>
missing_packets;
nit: Reading this without a colorized terminal (eg: in Codereview), the lack of
vertical whitespace throughout this file makes it much harder to separate out
comments, methods, and members. While
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic...
discourages excessive use, typical Chromium style has been to include whitespace
between methods and (less commonly but still dominant) between members, when
members include supporting commentary.

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_protocol.h#n...
net/quic/quic_protocol.h:207: uint16 delta_time;  // delta time is described
below.
nit; describe above?

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_protocol.h#n...
net/quic/quic_protocol.h:255: friend std::ostream& operator<<(std::ostream& os,
const QuicAckFragment& s) {
style nit: methods before members
style nit: When possible, do not put this in the header, because this forces
<ostream> to be included, which brings in static initializers
style nit: IWYU: Missing <ostream>
style nit: If done for purposes of GTest, consider overloading PrintTo
privately/in a test file, as per
http://code.google.com/p/googletest/wiki/AdvancedGuide#Teaching_Google_Test_H...

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_protocol.h#n...
net/quic/quic_protocol.h:330: bool operator==(const QuicFecData& other) const {
style nit: Do not overload operator==, as per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...
, or document why needed (only a few, specific algorithms require this, and I
haven't seen them being used.)

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_protocol.h#n...
net/quic/quic_protocol.h:354: owns_buffer_(false) { }
nit: You inconsistently have constructors using { } or {
} form (eg: compare with lines 296-310)

The canonical example of the two acceptable formats,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...
, shows the use of multi-line braces when using multi-line initializer lists,
and that is the dominant pattern in Chromium in my experience.

You also inconsistently include whitespace between the braces for the one-liners
(the canonical example includes no whitespace, your implementation files include
no whitespace, but you occassionally include whitespace, such as here)

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_protocol.h#n...
net/quic/quic_protocol.h:386: length() - kStartOfFecProtectedData);
DESIGN: There's no checking here in QuicPacket that |length| (line 380) is
either > kStartOfFecProtectedData, > kStartOfHashData (line 390), or >
kStartOfEncryptedData (line 394).

It would be a safer API to provide some checks at construction, perhaps through
making the constructor private and using a factory method that can return NULL
on bad input.

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

https://codereview.chromium.org/11188017/diff/5001/net/quic/quic_utils.h#newc...
net/quic/quic_utils.h:20: }
BUG? o_O

https://codereview.chromium.org/11188017/diff/5001/net/quic/test_tools/quic_t...
File net/quic/test_tools/quic_test_utils.cc (right):

https://codereview.chromium.org/11188017/diff/5001/net/quic/test_tools/quic_t...
net/quic/test_tools/quic_test_utils.cc:6: #include
"net/quic/crypto/crypto_framer.h"
style nit: Chromium tends to observe the canonical example from
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
, which ensures newlines between each ordered section (eg: between 5 and 6)

https://codereview.chromium.org/11188017/diff/5001/net/quic/test_tools/quic_t...
net/quic/test_tools/quic_test_utils.cc:10: using std::string;
style nit: see previous comments about using.

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

https://codereview.chromium.org/11188017/diff/5001/net/quic/uint128.h#newcode26
net/quic/uint128.h:26: return lhs ^ rhs;
maybe add a NOTIMPLEMENTED so that it's obvious?

[commit-bot: I haz the power, 2012-10-17 15:29:47]

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

[Ryan Hamilton, 2012-10-17 15:47:51]

sleevi:  These look like good comments.  I will create an upstream CL to address
them.  FWIW, you're the 5th person to review this code.

[commit-bot: I haz the power, 2012-10-17 18:09:05]

Change committed as 162462

[Vitaly Buka (NO REVIEWS), 2012-10-17 18:28:05]

https://chromiumcodereview.appspot.com/11188017/diff/5001/net/quic/quic_utils.cc
File net/quic/quic_utils.cc (right):

https://chromiumcodereview.appspot.com/11188017/diff/5001/net/quic/quic_utils...
net/quic/quic_utils.cc:26: static uint128 kOffset(780984778246553632u,
4400696054689967450u);
u -> ull?