Change the wire format of the ack frame to include a compressed version

Change the wire format of the ack frame to include a compressed version
of the previous CongestionFeedback Timestamp frame.
Adds QUIC_VERSION_23. Deprecate CongestionFeedbackFrame

1 bytes - num_received_packets
First packet:
1 byte - distance from the largest observed in sequence number space.
4 bytes - mod of microseconds since the connection's creation when the
  packet arrived.  (1.1hrs in us)
Every other packet:
1 byte - distance from the largest observed in sequence number space.
2 bytes - mod of microseconds since the previous timestamp

Merge internal change: 73740829

The following are chromium specific changes:
+ Changed ACK frame not to send CongestionFeedback frame
+ Added received_packet_times to ACK frame.
+ The timestamp that is sent is relative to creation time, thus passed
  MockClock's now() as argument to AckFrame and it is used to pass
  QuicFramer's creation time and received_packet_times.

R=cyr@google.com, ianswett@google.com, rch@chromium.org

Committed: https://crrev.com/4b06ae7cc281996b6a6e5989ef70a54ee3b4ed79
Cr-Commit-Position: refs/heads/master@{#291830}

[ramant (doing other things), 2014-08-23 18:35:32]

https://codereview.chromium.org/478153003/diff/1/net/quic/quic_network_transa...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/478153003/diff/1/net/quic/quic_network_transa...
net/quic/quic_network_transaction_unittest.cc:147: clock_->Now());
Hi Ryan,
  In QuicFramer::AppendTimestampToAckFrame, the time sent is relative to
framer's creation time (which is based on MockClock).

  In Client code, framer was created with 20000 ms as the creation time and we
send zero for received_packet_times in actual data.

  In the quic_test_packet_maker.cc, framer was created with zero time and thus
we were passing ' N' in the expected data instead of zero (and that's why the
chromium's specific tests were failing).

  Not 100% sure if "timestamp" is the right name. For Version_23 also, we say
"timestamp" (a better description could be better).

  The following are the files that are chromium specific:

net/quic/quic_http_stream_test.cc
net/quic/quic_network_transaction_unittest.cc
net/quic/test_tools/quic_test_packet_maker.cc
net/quic/test_tools/quic_test_packet_maker.h

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

https://codereview.chromium.org/478153003/diff/1/net/quic/quic_protocol.cc#ne...
net/quic/quic_protocol.cc:393: os << " is_truncated: " <<
ack_frame.is_truncated;
minor change to print the data in the same order as it was declared in
QuicAckFrame.

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

https://codereview.chromium.org/478153003/diff/20001/net/quic/quic_network_tr...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/478153003/diff/20001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:147: clock_->Now());
Hi Ryan,
  In QuicFramer::AppendTimestampToAckFrame, the time sent is relative to
framer's creation time (which is based on MockClock).

  In Client code, framer was created with 20000 ms as the creation time and we
send zero for received_packet_times in actual data.

  In the quic_test_packet_maker.cc, framer was created with zero time and thus
we were passing ' N' in the expected data instead of zero (and that's why the
chromium's specific tests were failing).

  Not 100% sure if "timestamp" is the right name. For Version_23 also, we say
"timestamp" (a better description could be better).

  The following are the files that are chromium specific:

net/quic/quic_http_stream_test.cc
net/quic/quic_network_transaction_unittest.cc
net/quic/test_tools/quic_test_packet_maker.cc
net/quic/test_tools/quic_test_packet_maker.h

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

https://codereview.chromium.org/478153003/diff/20001/net/quic/quic_protocol.c...
net/quic/quic_protocol.cc:392: os << " ] is_truncated: " <<
ack_frame.is_truncated;
minor change to print the data in the same order as it was declared in
QuicAckFrame.

[Ryan Hamilton, 2014-08-25 22:07:51]

LGTM. One possibility to consider. I'm not sure it's better; your call.

Thanks so much for getting to the bottom of this!!

https://codereview.chromium.org/478153003/diff/20001/net/quic/quic_network_tr...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/478153003/diff/20001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:147: clock_->Now());
Alternatively, consider passing a clock in to the
QuicTestPacketMakerConstructor.

[ramant (doing other things), 2014-08-25 23:39:35]

PTAL.

Thanks much Ryan,
raman

https://codereview.chromium.org/478153003/diff/20001/net/quic/quic_network_tr...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/478153003/diff/20001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:147: clock_->Now());
On 2014/08/25 22:07:51, Ryan Hamilton wrote:
> Alternatively, consider passing a clock in to the
> QuicTestPacketMakerConstructor.

Thanks for the above suggestion.

Done.

[ramant (doing other things), 2014-08-26 00:41:07]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-08-26 00:41:49]

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

[commit-bot: I haz the power, 2014-08-26 03:43:52]

Committed patchset #3 (60001) as a7d27122744fbbdee720511cc670908ddd5da13a

[Mostyn Bramley-Moore, 2014-09-09 23:36:37]

This CL added a c++11 lambda expression in quic_received_packet_manager.cc -
would you accept a patch that removes this?

[Ryan Hamilton, 2014-09-10 01:44:37]

Definitely! Feel free to just cook up a CL.

(Can't wait until we get C++11 for real :>)


On Tue, Sep 9, 2014 at 4:36 PM, <mostynb@opera.com> wrote:

> This CL added a c++11 lambda expression in quic_received_packet_manager.cc
> -
> would you accept a patch that removes this?
>
> https://codereview.chromium.org/478153003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-09-10 02:40:05]

Patchset 3 (id:??) landed as
https://crrev.com/4b06ae7cc281996b6a6e5989ef70a54ee3b4ed79
Cr-Commit-Position: refs/heads/master@{#291830}