Land Recent QUIC Changes.

Land Recent QUIC Changes.

By mistake, changes from https://codereview.chromium.org/428253003/ (it was LGTM'ed) got merged into this CL (will be careful not to repeat it while merging changes)

QUIC - enable persisting QuicServerInfo to disk cache by default.

Deleted unused enable_quic_persist_server_info from params.

This change should improve success rate of zero RTT for QUIC.

Reviewed CL: https://codereview.chromium.org/428253003/


Add case for PING frame in QuicFrame <<.

Merge internal change: 71908470
https://codereview.chromium.org/421963006/


Add methods to the QuicConnectionDebug visitor for tracking various
events which cause a QuicConnection to discard a packet.

Merge internal change: 71871881
https://codereview.chromium.org/421913015/


Change ReliableQuicStream::OnStreamFrame to return void since the method
already closes the connection when there are errors and callers are not
checking the return value

Make QuicStreamSequencer::OnStreamFrame return void instead of bool
since it is already closing the connect or resetting the stream when
there is an error.

Make two QUIC methods return void instead of bool.

Merge internal change: 71870242
https://codereview.chromium.org/420393004/


Create a visitor which can allow using both a trace... visitor and the
existing stats visitor.

Changed QuicConnection's QuicConnectionDebugVisitor to a scoped_ptr.

Merge internal change: 71863508
https://codereview.chromium.org/429453003/


Use 1350 byte QUIC packets by default.

Merge internal change: 71837432
https://codereview.chromium.org/427673005/


Improve debug logging of QUIC crypto handshake.

Merge internal change: 71833151
https://codereview.chromium.org/428803002/


Fix a bug in QuicUnackedPacketMap where an in flight packet could be
removed before being removed from bytes_in_flight.

Merge internal change: 71783653
https://codereview.chromium.org/422123005/


Inline the members of QUIC's ReceivedPacketInfo into QuicAckFrame now
that version 15 is gone.

Merge internal change: 71763611
https://codereview.chromium.org/424003002/


Remove FixRate congestion frame type.

Merge internal change: 71746617
https://codereview.chromium.org/424903002/


Remove QUIC_VERSION_15 now that Chrome Stable supports QUIC_VERSION_16.

Merge internal change: 71718286
https://codereview.chromium.org/413403008/

R=rch@chromium.org
TBR=thestig@chromium.org

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

[ramant (doing other things), 2014-07-29 02:23:55]

Hi Ryan,
  Will land after landing the following CL (and after performance regression was
found):
  https://codereview.chromium.org/411823002/

thanks
raman

[Ryan Hamilton, 2014-07-29 19:17:47]

LGTM

But let's wait 24 hours for https://codereview.chromium.org/422623004/ to roll
out first to confirm that it fixes what we expect it to fix.

[ramant (doing other things), 2014-08-01 19:37:07]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-08-01 19:39:14]

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

[ramant (doing other things), 2014-08-01 22:26:03]

Added thestig@chromium.org for io_thread_unittest.cc.

Fixed the following unit test

IOThreadTest.EnableQuicFromFieldTrialGroup (run #1):
[ RUN      ] IOThreadTest.EnableQuicFromFieldTrialGroup
../../chrome/browser/io_thread_unittest.cc:132: Failure
Value of: params.quic_max_packet_length
  Actual: 1350
Expected: 1200u
Which is: 1200
[  FAILED  ] IOThreadTest.EnableQuicFromFieldTrialGroup

[ramant (doing other things), 2014-08-01 22:26:23]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-08-01 22:27:58]

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

[Lei Zhang, 2014-08-01 22:41:23]

chrome/ lgtm

[ramant (doing other things), 2014-08-01 22:43:53]

On 2014/08/01 22:41:23, Lei Zhang wrote:
> chrome/ lgtm

Thanks very much Lei,
raman

[commit-bot: I haz the power, 2014-08-02 06:15:41]

Change committed as 287168

[wtc, 2014-08-04 23:28:13]

Patch set 4 LGTM. I suggest some changes.

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.cc:1698: if (debug_visitor_ != NULL) {

debug_visitor_ => debug_visitor_.get()

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.cc:1700: debug_visitor_->OnUndecryptablePacket();

Nit: perhaps more efficient to pass the number of undecryptable packets as the
argument to OnUndecryptablePacket so that we just need to call
OnUndecryptablePacket once?

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

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.h:140: // Called when a packet is recived with a
connection id that does not

recived => received

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.h:370: void
set_debug_visitor(QuicConnectionDebugVisitor* debug_visitor) {

We should document that this method now takes ownership of debug_visitor.

This seems like a bad API design: set_visitor doesn't take ownership of the
argument, but set_debug_visitor does.

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_crypto_str...
File net/quic/quic_crypto_stream.cc (right):

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_crypto_str...
net/quic/quic_crypto_stream.cc:21: #define ENDPOINT (is_server_ ? "Server: " : "
Client: ")

Nit: It seems that we can just call session()->is_server() in this macro. Then
we don't need the is_server_ member.

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_received_p...
File net/quic/quic_received_packet_manager.h (right):

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_received_p...
net/quic/quic_received_packet_manager.h:117: // Update the |received_info| for
an outgoing ack.

|received_info| => |ack_frame|. Also fix line 134.

https://codereview.chromium.org/420313005/diff/80001/net/quic/test_tools/quic...
File net/quic/test_tools/quic_test_utils.h (right):

https://codereview.chromium.org/420313005/diff/80001/net/quic/test_tools/quic...
net/quic/test_tools/quic_test_utils.h:94: // from least_unacked to
largest_observed acked.

This comment needs updating because the "least_unacked" parameter is gone.

[ramant (doing other things), 2014-08-09 02:53:20]

Fixed the comments in internal source tree and chromium CL:
 https://codereview.chromium.org/454263002

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.cc:1698: if (debug_visitor_ != NULL) {
On 2014/08/04 23:28:12, wtc wrote:
> 
> debug_visitor_ => debug_visitor_.get()

Done.

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.cc:1700: debug_visitor_->OnUndecryptablePacket();
On 2014/08/04 23:28:12, wtc wrote:
> 
> Nit: perhaps more efficient to pass the number of undecryptable packets as the
> argument to OnUndecryptablePacket so that we just need to call
> OnUndecryptablePacket once?

Good point. We are not using this method yet. Added a TODO.

Acknowledged.

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

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.h:140: // Called when a packet is recived with a
connection id that does not
On 2014/08/04 23:28:13, wtc wrote:
> 
> recived => received

Done.

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_connection...
net/quic/quic_connection.h:370: void
set_debug_visitor(QuicConnectionDebugVisitor* debug_visitor) {
On 2014/08/04 23:28:13, wtc wrote:
> 
> We should document that this method now takes ownership of debug_visitor.

Done.

> 
> This seems like a bad API design: set_visitor doesn't take ownership of the
> argument, but set_debug_visitor does.

Acknowledged.

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_crypto_str...
File net/quic/quic_crypto_stream.cc (right):

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_crypto_str...
net/quic/quic_crypto_stream.cc:21: #define ENDPOINT (is_server_ ? "Server: " : "
Client: ")
On 2014/08/04 23:28:13, wtc wrote:
> 
> Nit: It seems that we can just call session()->is_server() in this macro. Then
> we don't need the is_server_ member.

Done.

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_received_p...
File net/quic/quic_received_packet_manager.h (right):

https://codereview.chromium.org/420313005/diff/80001/net/quic/quic_received_p...
net/quic/quic_received_packet_manager.h:117: // Update the |received_info| for
an outgoing ack.
On 2014/08/04 23:28:13, wtc wrote:
> 
> |received_info| => |ack_frame|. Also fix line 134.

Done.

https://codereview.chromium.org/420313005/diff/80001/net/quic/test_tools/quic...
File net/quic/test_tools/quic_test_utils.h (right):

https://codereview.chromium.org/420313005/diff/80001/net/quic/test_tools/quic...
net/quic/test_tools/quic_test_utils.h:94: // from least_unacked to
largest_observed acked.
On 2014/08/04 23:28:13, wtc wrote:
> 
> This comment needs updating because the "least_unacked" parameter is gone.

Done.