Allow QUIC's BBR congestion control to be negotiated in the crypto

Allow QUIC's BBR congestion control to be negotiated in the crypto
handshake.

Adding an implementation of QuicFixedTagVector to QuicConfig to enable
multiple congestion control options per connection.

Merge internal change: 68114869

R=rch@chromium.org

[Ryan Hamilton, 2014-06-02 15:32:48]

lgtm

[wtc, 2014-06-03 02:00:53]

Patch set 1 LGTM.

I have some questions and suggested changes for the QuicFixedTagVector class in
this CL. I may have misunderstood the code, so please forgive me if my comments
are off the mark.

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

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.cc#newc...
net/quic/quic_config.cc:364: QuicTagVector QuicFixedTagVector::GetSendValues()
const {

Another design is to make the getter method returns bool:

  bool QuicFixedTagVector::GetSendValues(QuicTagVector* out) const {
    if (!has_send_values_) {
      return false;
    }
    *out = send_values_;
    return true;
  }

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.cc#newc...
net/quic/quic_config.cc:365: LOG_IF(DFATAL, !has_send_values_) << "No send
values to get for tag:" << tag_;

This check seems too strict. If presence_ == PRESENCE_OPTIONAL, it seems that we
should allow !has_send_values.

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.cc#newc...
net/quic/quic_config.cc:412: has_receive_values_ = true;

Nit: should we do receive_values_.clear() first?

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.h
File net/quic/quic_config.h (right):

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.h#newco...
net/quic/quic_config.h:218: // Stores tag from CHLO or SHLO messages that are
not negotiated.

1. If I only read this class declaration, I have no idea how to use it. Below I
suggest some potential improvements.

2. Nit: "tag" is confusing because the value of this tag is a vector of tags.
It's not clear whether "tag" refers to the name or the value.

Also, this comment should point out that the config value is a tag vector, in
addition to the fixed, non-negotiated aspect.

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.h#newco...
net/quic/quic_config.h:234: void SetReceivedValues(const QuicTagVector& values);

Nit: document "send values" and "receive values"

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.h#newco...
net/quic/quic_config.h:236: // If has_send_value is true, serialises
|tag_vector_| and |send_value_| to

Nit: this comment seems stale.

"has_send_value" is missing the trailing '_'.

There is no member by the name "tag_vector_".

send_value_ => send_values_

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.h#newco...
net/quic/quic_config.h:240: // Sets |receive_values_| to the corresponding value
from |client_hello_| if

1. Typo: client_hello_ => peer_hello

2. "if it exists" probably should be deleted.

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.h#newco...
net/quic/quic_config.h:244: HelloType hello_type,

Nit: I believe |hello_type| can be inferred from peer_hello.tag(). So this
parameter doesn't seem necessary.

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.h#newco...
net/quic/quic_config.h:340: // Congestion control option.

Nit: option => options

[Ian Swett, 2014-06-03 13:31:29]

lgtm

Good points Wan-Teh about the class being a bit confusing.  I think it makes
more sense in the context of the other config classes, but I'm sure it could be
improved further, particularly in terms of comments.

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

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.cc#newc...
net/quic/quic_config.cc:364: QuicTagVector QuicFixedTagVector::GetSendValues()
const {
On 2014/06/03 02:00:53, wtc wrote:
> 
> Another design is to make the getter method returns bool:
> 
>   bool QuicFixedTagVector::GetSendValues(QuicTagVector* out) const {
>     if (!has_send_values_) {
>       return false;
>     }
>     *out = send_values_;
>     return true;
>   }

I prefer get methods which have return values, but if we're going to change it,
we should make sure every config class follows the same pattern.

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.cc#newc...
net/quic/quic_config.cc:365: LOG_IF(DFATAL, !has_send_values_) << "No send
values to get for tag:" << tag_;
On 2014/06/03 02:00:53, wtc wrote:
> 
> This check seems too strict. If presence_ == PRESENCE_OPTIONAL, it seems that
we
> should allow !has_send_values.

The contract is that you should never call GetSendValues without calling
HasSendValues first.  Again, that should be true for all of the fixed fields,
but isn't documented.

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.cc#newc...
net/quic/quic_config.cc:412: has_receive_values_ = true;
On 2014/06/03 02:00:53, wtc wrote:
> 
> Nit: should we do receive_values_.clear() first?

We could, though we never process the same tag twice.

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.h
File net/quic/quic_config.h (right):

https://codereview.chromium.org/308273004/diff/1/net/quic/quic_config.h#newco...
net/quic/quic_config.h:218: // Stores tag from CHLO or SHLO messages that are
not negotiated.
On 2014/06/03 02:00:53, wtc wrote:
> 
> 1. If I only read this class declaration, I have no idea how to use it. Below
I
> suggest some potential improvements.
> 
> 2. Nit: "tag" is confusing because the value of this tag is a vector of tags.
> It's not clear whether "tag" refers to the name or the value.
> 
> Also, this comment should point out that the config value is a tag vector, in
> addition to the fixed, non-negotiated aspect.

Agreed on documentation.  This class follows the pattern of classes in
QuicConfig in general.  I'm not sure if we want to document every one separately
or not?  In practice, they're pretty straightforward in context, but more
comments are fine by me.