Issue 471613002: Change how QUIC negotiates pacing from congestion feedback to QUIC

Issue 471613002: Change how QUIC negotiates pacing from congestion feedback to QUIC (Closed)

Created:
6 years, 4 months ago by ramant (doing other things)

Modified:
6 years, 4 months ago

Reviewers:
Ian Swett, Ryan Hamilton

CC:
chromium-reviews, cbentzel+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@Add_max_bandwidth_max_bandwidth_timestamp_73055131

Project:
chromium

Visibility:
Public.

More Reviews

Description

Change how QUIC negotiates pacing from congestion feedback to QUIC connection option. Merge internal change: 73061068 R=ianswett@google.com, rch@chromium.org

Patch Set 1 #

Total comments: 5

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+34 lines, -48 lines)			Patch
M	chrome/browser/io_thread.h	View	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/io_thread.cc	View	4 chunks	+5 lines, -5 lines	0 comments	Download
M	chrome/browser/io_thread_unittest.cc	View	4 chunks	+9 lines, -4 lines	0 comments	Download
M	net/http/http_network_session.h	View	1 chunk	+0 lines, -1 line	0 comments	Download
M	net/http/http_network_session.cc	View	4 chunks	+2 lines, -3 lines	2 comments	Download
M	net/quic/crypto/crypto_protocol.h	View	1 chunk	+1 line, -1 line	0 comments	Download
M	net/quic/quic_config.h	View	1 chunk	+0 lines, -3 lines	0 comments	Download
M	net/quic/quic_config.cc	View	2 chunks	+0 lines, -12 lines	0 comments	Download
M	net/quic/quic_config_test.cc	View	1 chunk	+2 lines, -3 lines	0 comments	Download
M	net/quic/quic_connection.cc	View	1 chunk	+3 lines, -0 lines	3 comments	Download
M	net/quic/quic_connection_test.cc	View	1 chunk	+1 line, -1 line	0 comments	Download
M	net/quic/quic_crypto_client_stream_test.cc	View	1 chunk	+1 line, -2 lines	0 comments	Download
M	net/quic/quic_sent_packet_manager.cc	View	1 chunk	+2 lines, -3 lines	0 comments	Download
M	net/quic/quic_stream_factory.h	View	1 chunk	+0 lines, -1 line	0 comments	Download
M	net/quic/quic_stream_factory.cc	View	3 chunks	+2 lines, -6 lines	0 comments	Download
M	net/quic/quic_stream_factory_test.cc	View	1 chunk	+1 line, -2 lines	0 comments	Download
M	net/tools/quic/end_to_end_test.cc	View	1 chunk	+5 lines, -0 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

ramant (doing other things)

https://codereview.chromium.org/471613002/diff/1/net/quic/quic_connection.cc File net/quic/quic_connection.cc (right): https://codereview.chromium.org/471613002/diff/1/net/quic/quic_connection.cc#newcode241 net/quic/quic_connection.cc:241: // TODO(rtenneti): Should we enable this code in chromium? ...

6 years, 4 months ago (2014-08-13 16:12:02 UTC) #2

Ryan Hamilton

lgtm https://codereview.chromium.org/471613002/diff/1/net/http/http_network_session.cc File net/http/http_network_session.cc (right): https://codereview.chromium.org/471613002/diff/1/net/http/http_network_session.cc#newcode254 net/http/http_network_session.cc:254: ContainsQuicTag(params_.quic_connection_options, kPACE)); Consider removing "enable_quic_pacing" and replacing it ...

6 years, 4 months ago (2014-08-13 16:33:41 UTC) #3

ramant (doing other things)

6 years, 4 months ago (2014-08-14 02:45:48 UTC) #4

https://codereview.chromium.org/471613002/diff/1/net/http/http_network_sessio...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/471613002/diff/1/net/http/http_network_sessio...
net/http/http_network_session.cc:254:
ContainsQuicTag(params_.quic_connection_options, kPACE));
On 2014/08/13 16:33:40, Ryan Hamilton wrote:
> Consider removing "enable_quic_pacing" and replacing it with
> "quic_connection_options".

Fixed it in CL: https://chromiumcodereview.appspot.com/470493004/

Done.

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

https://codereview.chromium.org/471613002/diff/1/net/quic/quic_connection.cc#...
net/quic/quic_connection.cc:241: // TODO(rtenneti): Should we enable this code
in chromium?
On 2014/08/13 16:33:40, Ryan Hamilton wrote:
> On 2014/08/13 16:12:02, ramant wrote:
> > Hi Ryan and Ian,
> >   Do we need this code in chromium? We are passing pacing as connection
> options.
> > sent_packet_manager_ uses enable pacing flags.
> > 
> > thanks
> > raman
> 
> Hm. Good question. I think we can probably leave this commented out until
we're
> happy that pacing works as expected. Ian, what do you think?

Acknowledged.

Expand Messages | Collapse Messages