Queueing QUIC frames to be resent instead of packets and packing RST frames with acks and congestio…

Queueing QUIC frames to be resent instead of packets and packing RST frames with acks and congestion info frames.

Merge internal change: 40834055


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

[jar (doing other things), 2013-01-16 23:59:11]

LGTM

Two nits below for Ian to consider upstreaming.

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

https://codereview.chromium.org/11958018/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:117: for (UnackedPacketMap::iterator u =
unacked_packets_.begin();
nit: avoid single letter variable names.  Use of "it" as an iterator appears
commonly enough, but "u" is not so good.

https://codereview.chromium.org/11958018/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:121: STLDeleteValues(&unacked_packets_);
nit: It seems strange to iterate over the map starting at line 117, and then
call for another iteration and delete.  I had to look closely to convince myself
that you were doing a correct destructor call.  

I sure wish we had a destructor-like method (Ryan pointed out we can't use a
real destructor, since this is a copyable struct :-()

Nicer might be just add after line 119: 
delete u->second;

To be honest, I'm not sure what the best pattern is... but this seemed more
complex to read than I would have liked.

https://codereview.chromium.org/11958018/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:123: for (QueuedPacketList::iterator q =
queued_packets_.begin();
nit: "q" should be a  better name (...yeah... ya' can't use "it" in both cases
:-/ ).

[commit-bot: I haz the power, 2013-01-17 02:28:41]

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

[commit-bot: I haz the power, 2013-01-17 04:51:55]

Change committed as 177348

[Ian Swett, 2013-01-17 15:33:27]

Changes made in a new CL.

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

https://codereview.chromium.org/11958018/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:117: for (UnackedPacketMap::iterator u =
unacked_packets_.begin();
On 2013/01/16 23:59:11, jar wrote:
> nit: avoid single letter variable names.  Use of "it" as an iterator appears
> commonly enough, but "u" is not so good.
Agreed, that is a terrible name.  I have no idea why I changed it.

https://codereview.chromium.org/11958018/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:121: STLDeleteValues(&unacked_packets_);
On 2013/01/16 23:59:11, jar wrote:
> nit: It seems strange to iterate over the map starting at line 117, and then
> call for another iteration and delete.  I had to look closely to convince
myself
> that you were doing a correct destructor call.  
> 
> I sure wish we had a destructor-like method (Ryan pointed out we can't use a
> real destructor, since this is a copyable struct :-()
> 
> Nicer might be just add after line 119: 
> delete u->second;
> 
> To be honest, I'm not sure what the best pattern is... but this seemed more
> complex to read than I would have liked.

I hated writing it.  I actually made a destructor, realized copy was being used,
contemplating not using it anymore, then decided that was rather ugly as well. 
I'm going to add a comment above the loop to clarify why it's doing this.

What about renaming DeleteUnackedPacket() to DeleteEnclosedFrames()?

https://codereview.chromium.org/11958018/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:123: for (QueuedPacketList::iterator q =
queued_packets_.begin();
On 2013/01/16 23:59:11, jar wrote:
> nit: "q" should be a  better name (...yeah... ya' can't use "it" in both cases
> :-/ ).

Done.

[jar (doing other things), 2013-01-17 16:14:32]

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

https://codereview.chromium.org/11958018/diff/1/net/quic/quic_connection.cc#n...
net/quic/quic_connection.cc:121: STLDeleteValues(&unacked_packets_);
On 2013/01/17 15:33:27, ianswett wrote:
> 
> What about renaming DeleteUnackedPacket() to DeleteEnclosedFrames()?

+1  That would certainly help IMO.