Land Recent QUIC Changes.

Land Recent QUIC Changes.

Stub for BBRv2, based on TCP congestion feedback frames.

Merge internal change: 67154078
https://codereview.chromium.org/283333008/


Update QuicSentPacketManager to sample a new recent min rtt within the
first 2 rtt samples after quiescence.

Merge internal change: 67149500
https://codereview.chromium.org/286143007/


Change the test QUIC stream id to be the first non reserved stream ID
(and also to use the right typedef).

Merge internal change: 67146383
https://codereview.chromium.org/292453003/


Adds dynamic FEC on/off switch in packet creator. Tightens use of
max_packets_per_fec_group through use of a setter method across the QUIC
code. FEC operations are now decided based on should_fec_protect_, which
is expected to be a dynamic on/off control in the packet creator,
instead of max_packets_per_fec_group.

Adds dynamic FEC on/off switch in packet_creator.

Merge internal change: 67144415
https://codereview.chromium.org/286153003/


Minor cleanup to simplify QuicSentPacketManager's OnPacketSent and
always reset the retransmission alarm anytime a new pending packet is
sent.

Always setting the retransmission alarm is only a simplification of the
approach, and not intended to fix any issues.

Merge internal change: 67143274
https://codereview.chromium.org/288333002/


Don't set QUIC's write alarm if we are connection flow control blocked.

Added QUIC_VERSION_19 to kSupportedQuicVersions.

Merge internal change: 67141668
https://codereview.chromium.org/285193006/


Minor cleanup of QuicUnackedPacketMap to simplify the implementation of
HasPendingPackets and move a test only method,
GetNumRetransmittablePackets, into QuicSentPacketManagerPeer.

Merge internal change: 67123054
https://codereview.chromium.org/282323003/


Fix a QUIC bug where a crypto packet was never removed from the
UnackedPacketMap if it was not acked and spuriously retransmitted at
least twice.

Merge internal change: 67050631
https://codereview.chromium.org/284273002/


Refactor: move flow controller from QuicConnection to QuicSession.
No behavior change intended.

Merge internal change: 67036889
https://codereview.chromium.org/286213002/


Fix a QUIC bug where a packet could remain in the UnackedPacketMap
indefinitely. This can cause a memory leak when tracking entropy.

Merge internal change: 67028206
https://codereview.chromium.org/285233006/


Delete dead code used only in tests.

Merge internal change: 66938996
https://codereview.chromium.org/288303002/


Add handling + parsing for ALTSVC frame. Also change frame type number
and add an extra version check for BLOCKED frame. SPDY4/HTTP2 only.

Merge internal change: 66925490
https://codereview.chromium.org/286173002/


QUIC loadtest fixes:
- Wait for the QUIC handshake to complete before saying that the
QuicTestClient is connected.
- Force connect at client creation time when talking HTTP/HTTPS/SPDY
for consistency.

Merge internal change: 66855236
https://codereview.chromium.org/282153004/


QUIC now respects configured SPDY stream limits.

Merge internal change: 66831620
https://codereview.chromium.org/286113004/

R=rch@chromium.org

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

[Ryan Hamilton, 2014-05-16 18:13:18]

lgtm

[ramant (doing other things), 2014-05-16 18:23:13]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2014-05-16 18:23:34]

The CQ bit was unchecked by rtenneti@chromium.org

[ramant (doing other things), 2014-05-16 18:36:42]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-05-16 18:37:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/288313003/20001

[commit-bot: I haz the power, 2014-05-17 05:19:07]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-17 15:51:35]

Change committed as 271211

[wtc, 2014-05-19 18:58:39]

Patch set 2 LGTM.

https://codereview.chromium.org/288313003/diff/20001/net/quic/congestion_cont...
File net/quic/congestion_control/send_algorithm_interface.cc (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/congestion_cont...
net/quic/congestion_control/send_algorithm_interface.cc:33: LOG(DFATAL) <<
"BbrTcpSender is not supported.";

Nit: Is the class named BbrTcpSender or TcpBbrSender?

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:25: using
net::test::kInitialFlowControlWindowForTest;

Nit: since the entire file is inside the net::test namespace, this using
statement isn't needed.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_connection...
net/quic/quic_connection.h:97: // both has pending data to write, and is able to
write (e.g. based on flow

Nit: has both has => both has

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_packet_cre...
File net/quic/quic_packet_creator.h (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_packet_cre...
net/quic/quic_packet_creator.h:61: // be enabled first
(set_max_packets_per_fec_group should be non-zero) for

Typo: It seems that "set_max_packets_per_fec_group" should be
"max_packets_per_fec_group".

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_packet_cre...
net/quic/quic_packet_creator.h:129: // Caller must ensure that any open FEC
group are closed before calling this

Nit: the subject "any open FEC group" (singular) and the verb "are" (plural)
don't match.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_packet_cre...
net/quic/quic_packet_creator.h:139: // still return NOT_IN_FEC_GROUP, and
fec_group_.get() may still be NULL.

Nit: "IsFecProtected() may still return NOT_IN_FEC_GROUP" doesn't match the code
because IsFecProtected() returns a bool. Did you mean "MaybeStartFec" instead of
"IsFecProtected"?

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_sent_packe...
File net/quic/quic_sent_packet_manager.cc (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_sent_packe...
net/quic/quic_sent_packet_manager.cc:177: // the high water mark to be raised if
necessary.

I'm wondering if this comment needs to be updated, now that the function has
been changed from IsSentAndNotPending to IsForRttOnly.

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

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_session.cc...
net/quic/quic_session.cc:302:
write_blocked_streams_.HasWriteBlockedDataStreams())) {

Nit: I would simplify this expression with De Morgan's laws. Up to you.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_session_te...
File net/quic/quic_session_test.cc (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_session_te...
net/quic/quic_session_test.cc:117: explicit TestSession(QuicConnection*
connection,

Nit: this "explicit" keyword can be removed now.

[Ian Swett, 2014-05-19 20:20:40]

lgtm

A minor suggestion on a better comment if you'd like to change it.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_sent_packe...
File net/quic/quic_sent_packet_manager.cc (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_sent_packe...
net/quic/quic_sent_packet_manager.cc:177: // the high water mark to be raised if
necessary.
On 2014/05/19 18:58:40, wtc wrote:
> 
> I'm wondering if this comment needs to be updated, now that the function has
> been changed from IsSentAndNotPending to IsForRttOnly.

It could be updated.  A better comment might be "Remove any rtt only packets
less than or equal to the largest observed, since they will not produce an RTT
measurement."

[ramant (doing other things), 2014-05-20 03:22:31]

Fixed the comments in the following CL:

https://codereview.chromium.org/292113002/

https://codereview.chromium.org/288313003/diff/20001/net/quic/congestion_cont...
File net/quic/congestion_control/send_algorithm_interface.cc (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/congestion_cont...
net/quic/congestion_control/send_algorithm_interface.cc:33: LOG(DFATAL) <<
"BbrTcpSender is not supported.";
On 2014/05/19 18:58:40, wtc wrote:
> 
> Nit: Is the class named BbrTcpSender or TcpBbrSender?

It is called BbrTcpSender (internally).

Ian: Should we rename kTCPBBR to kBBRTCP or should we BbrTcpSender to
TcpBbrSender.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:25: using
net::test::kInitialFlowControlWindowForTest;
On 2014/05/19 18:58:40, wtc wrote:
> 
> Nit: since the entire file is inside the net::test namespace, this using
> statement isn't needed.

Done.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_connection...
net/quic/quic_connection.h:97: // both has pending data to write, and is able to
write (e.g. based on flow
On 2014/05/19 18:58:40, wtc wrote:
> 
> Nit: has both has => both has

Done.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_packet_cre...
File net/quic/quic_packet_creator.h (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_packet_cre...
net/quic/quic_packet_creator.h:61: // be enabled first
(set_max_packets_per_fec_group should be non-zero) for
On 2014/05/19 18:58:40, wtc wrote:
> 
> Typo: It seems that "set_max_packets_per_fec_group" should be
> "max_packets_per_fec_group".

Done.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_packet_cre...
net/quic/quic_packet_creator.h:129: // Caller must ensure that any open FEC
group are closed before calling this
On 2014/05/19 18:58:40, wtc wrote:
> 
> Nit: the subject "any open FEC group" (singular) and the verb "are" (plural)
> don't match.

Done.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_packet_cre...
net/quic/quic_packet_creator.h:139: // still return NOT_IN_FEC_GROUP, and
fec_group_.get() may still be NULL.
On 2014/05/19 18:58:40, wtc wrote:
> 
> Nit: "IsFecProtected() may still return NOT_IN_FEC_GROUP" doesn't match the
code
> because IsFecProtected() returns a bool. Did you mean "MaybeStartFec" instead
of
> "IsFecProtected"?

jri and I went through the code and updated the comment per jri.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_sent_packe...
File net/quic/quic_sent_packet_manager.cc (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_sent_packe...
net/quic/quic_sent_packet_manager.cc:177: // the high water mark to be raised if
necessary.
On 2014/05/19 20:20:41, Ian Swett wrote:
> On 2014/05/19 18:58:40, wtc wrote:
> > 
> > I'm wondering if this comment needs to be updated, now that the function has
> > been changed from IsSentAndNotPending to IsForRttOnly.
> 
> It could be updated.  A better comment might be "Remove any rtt only packets
> less than or equal to the largest observed, since they will not produce an RTT
> measurement."

Done.

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_sent_packe...
net/quic/quic_sent_packet_manager.cc:177: // the high water mark to be raised if
necessary.
On 2014/05/19 18:58:40, wtc wrote:
> 
> I'm wondering if this comment needs to be updated, now that the function has
> been changed from IsSentAndNotPending to IsForRttOnly.

Updated the comment per Ian.

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

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_session.cc...
net/quic/quic_session.cc:302:
write_blocked_streams_.HasWriteBlockedDataStreams())) {
On 2014/05/19 18:58:40, wtc wrote:
> 
> Nit: I would simplify this expression with De Morgan's laws. Up to you.

rjshade: wdyt?

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_session_te...
File net/quic/quic_session_test.cc (right):

https://codereview.chromium.org/288313003/diff/20001/net/quic/quic_session_te...
net/quic/quic_session_test.cc:117: explicit TestSession(QuicConnection*
connection,
On 2014/05/19 18:58:40, wtc wrote:
> 
> Nit: this "explicit" keyword can be removed now.

Done.