Notfiy NQE of QUIC RTT

Notfiy NQE of QUIC RTT

NQE is going to expose QUIC RTT to Cronet. Some of the
Cronet apps plan to use QUIC RTT for their own network
quality estimation. For more details, please see the design
doc in #10 in the attached bug.

BUG=497899
Committed: https://crrev.com/fdf5665b2c4d9aa3b421a833bc7bf5330454faf3
Cr-Commit-Position: refs/heads/master@{#350053}

[tbansal1, 2015-09-09 01:00:03]

tbansal@chromium.org changed reviewers:
+ rch@chromium.org

[tbansal1, 2015-09-09 01:00:04]

rch: PTAL.

I have added one test. I plan to add couple of more tests. For now, it would be
great if you can give me some feedback on the code path.

No need to look at the test code right now. It is sort of messy right now. I
will work more on it once I get l-g-t-m on the code path and the general
approach. Thanks.

[tbansal1, 2015-09-09 01:00:04]

tbansal@chromium.org changed reviewers:
+ rch@chromium.org

[tbansal1, 2015-09-09 01:00:04]

rch: PTAL.

I have added one test. I plan to add couple of more tests. For now, it would be
great if you can give me some feedback on the code path.

No need to look at the test code right now. It is sort of messy right now. I
will work more on it once I get l-g-t-m on the code path and the general
approach. Thanks.

[tbansal1, 2015-09-09 20:14:09]

Patchset #1 (id:1) has been deleted

[tbansal1, 2015-09-09 20:16:13]

On 2015/09/09 01:00:04, tbansal1 wrote:
> rch: PTAL.
> 
> I have added one test. I plan to add couple of more tests. For now, it would
be
> great if you can give me some feedback on the code path.
> 
> No need to look at the test code right now. It is sort of messy right now. I
> will work more on it once I get l-g-t-m on the code path and the general
> approach. Thanks.

Added a patchset (deleted original patchset) with more comments and tests. CL is
in much better condition now. You may now look at the tests too :) Thanks.

[Ryan Hamilton, 2015-09-10 19:32:05]

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

https://codereview.chromium.org/1305293004/diff/20001/net/quic/quic_connectio...
net/quic/quic_connection.h:297: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher);
quic_connection.h is shared code with the internal repository. as such, it
should not contain chromium-specific code/logic. Can this move into, say
QuicConnectionLogger?

https://codereview.chromium.org/1305293004/diff/20001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1305293004/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1235: if (socket_performance_watcher_factory_) {
When do we expect this to be null?

https://codereview.chromium.org/1305293004/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1237: ->CreateUDPSocketPerformanceWatcher();
Is this git cl format'd?

[tbansal1, 2015-09-11 19:37:51]

Patchset #2 (id:40001) has been deleted

[tbansal1, 2015-09-11 19:38:35]

rch: PTAL. Thanks.

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

https://codereview.chromium.org/1305293004/diff/20001/net/quic/quic_connectio...
net/quic/quic_connection.h:297: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher);
On 2015/09/10 19:32:04, Ryan Hamilton wrote:
> quic_connection.h is shared code with the internal repository. as such, it
> should not contain chromium-specific code/logic. Can this move into, say
> QuicConnectionLogger?

Done.

https://codereview.chromium.org/1305293004/diff/20001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1305293004/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1235: if (socket_performance_watcher_factory_) {
On 2015/09/10 19:32:04, Ryan Hamilton wrote:
> When do we expect this to be null?

incognito and some tests.

https://codereview.chromium.org/1305293004/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1237: ->CreateUDPSocketPerformanceWatcher();
On 2015/09/10 19:32:04, Ryan Hamilton wrote:
> Is this git cl format'd?

Yes.

[tbansal1, 2015-09-16 15:54:32]

rch: friendly ping :) PTAL when you have time. Thanks.

[Ryan Hamilton, 2015-09-17 01:59:49]

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.h (right):

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:124:
scoped_ptr<SocketPerformanceWatcher> socket_performance_watcher);
nit: by convention, net_log usually comes last.

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection_logger.cc:468: if (socket_performance_watcher_ &&
connection_) {
Why would we not have a connection?

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection_logger.cc:501: }
I think smoothed_rtt() can return Zero. Is that OK?

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection_logger.cc:502: }
Looking at this more closely, I see that you're re-fetching RTT on every packet
sent and received. I wonder if it might be a better idea to do this only when
the RTT changes. To do this, I propose you add a new method to
QuicConnectionDebugVisitorInterface:

void OnRttChanged(QuicTime::Delta rtt) {}

You can then call this method from:

https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_conn...

And then implement it in QuicConnectionLogger to do the logic you have here.

Does that sound workable?  Looks good otherwise...

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
File net/quic/quic_connection_logger.h (right):

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection_logger.h:184: const QuicConnection* connection_;  //
Unowned. May be null.
|session_| already has a connection() accessor so I don't think we need a new
member for this.

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_network_t...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_network_t...
net/quic/quic_network_transaction_unittest.cc:624:
DCHECK(socket_performance_watcher_factory_
s/DCHECK/EXPECT_TRUE/ ?

[tbansal1, 2015-09-17 22:22:28]

Patchset #3 (id:80001) has been deleted

[tbansal1, 2015-09-17 22:22:34]

Patchset #3 (id:100001) has been deleted

[tbansal1, 2015-09-17 22:23:42]

rch: PTAL.
Rebased, Now using QuicConnectionDebugVisitorInterface. Also, made some changes
to the tests, and added new tests. Thanks.

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.h (right):

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:124:
scoped_ptr<SocketPerformanceWatcher> socket_performance_watcher);
On 2015/09/17 01:59:49, Ryan Hamilton wrote:
> nit: by convention, net_log usually comes last.

Done.

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection_logger.cc:468: if (socket_performance_watcher_ &&
connection_) {
On 2015/09/17 01:59:49, Ryan Hamilton wrote:
> Why would we not have a connection?

Removed, I was just not sure, so tried to be on the safer side.

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection_logger.cc:501: }
On 2015/09/17 01:59:49, Ryan Hamilton wrote:
> I think smoothed_rtt() can return Zero. Is that OK?

Added check for zero.

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection_logger.cc:502: }
On 2015/09/17 01:59:49, Ryan Hamilton wrote:
> Looking at this more closely, I see that you're re-fetching RTT on every
packet
> sent and received. I wonder if it might be a better idea to do this only when
> the RTT changes. To do this, I propose you add a new method to
> QuicConnectionDebugVisitorInterface:
> 
> void OnRttChanged(QuicTime::Delta rtt) {}
> 
> You can then call this method from:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_conn...
> 
> And then implement it in QuicConnectionLogger to do the logic you have here.
> 
> Does that sound workable?  Looks good otherwise...

Done. That's much cleaner :)

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
File net/quic/quic_connection_logger.h (right):

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection_logger.h:184: const QuicConnection* connection_;  //
Unowned. May be null.
On 2015/09/17 01:59:49, Ryan Hamilton wrote:
> |session_| already has a connection() accessor so I don't think we need a new
> member for this.

Done. Removed.

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_network_t...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/60001/net/quic/quic_network_t...
net/quic/quic_network_transaction_unittest.cc:624:
DCHECK(socket_performance_watcher_factory_
On 2015/09/17 01:59:49, Ryan Hamilton wrote:
> s/DCHECK/EXPECT_TRUE/ ?

Done.

[tbansal1, 2015-09-17 22:26:13]

CC'ing rsleevi because CL is related to socket performance watcher.

[Ryan Hamilton, 2015-09-18 15:56:38]

Almost there, just a few minor comments. Are you planning to wait for sleevi to
review?

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.h (right):

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_connecti...
net/quic/quic_connection_logger.h:35: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher);
nit: by convention net_log should come last.

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:163: };
Instead of creating this class which exists simply to outlive the actual
PerformanceWatcher an alternative would be to EXPECT_TRUE(have_rtt_update_) in
the TestWatcher destructor. I think that would avoid some extra code.

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:181: class
TestNetworkQualityEstimator : public NetworkQualityEstimator {
Instead of a full blown NQE, how 'bout just a  SocketPerformanceWatcherFactory
since that's all you need?

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:1879: EXPECT_EQ(1U,
test_network_quality_estimator_->GetWatchersCreated());
I doesn't seem like we need to test this behavior in every test. That just makes
the tests harder to read. In any case, you could move this code into
SendRequestAndExpectQuicResponse, I suspect.

[tbansal1, 2015-09-18 17:21:20]

PTAL. Thanks. This is not blocked on sleevi. He asked me to keep him cc'ed. Not
block the CL.

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.h (right):

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_connecti...
net/quic/quic_connection_logger.h:35: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher);
On 2015/09/18 15:56:38, Ryan Hamilton wrote:
> nit: by convention net_log should come last.

Done.

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:163: };
On 2015/09/18 15:56:38, Ryan Hamilton wrote:
> Instead of creating this class which exists simply to outlive the actual
> PerformanceWatcher an alternative would be to EXPECT_TRUE(have_rtt_update_) in
> the TestWatcher destructor. I think that would avoid some extra code.

Removed the class. I am not sure if EXPECT_TRUE(have_rtt_update_) is the right
approach. In some tests, RTT update is not received, and that is verified.

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:181: class
TestNetworkQualityEstimator : public NetworkQualityEstimator {
On 2015/09/18 15:56:38, Ryan Hamilton wrote:
> Instead of a full blown NQE, how 'bout just a  SocketPerformanceWatcherFactory
> since that's all you need?

Yeah, I had TestSocketPerformanceWatcherFactory before, but thought it might be
better to test with NQE because that's the one implementing. It seems like a
more realistic test. wdyt?

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:1879: EXPECT_EQ(1U,
test_network_quality_estimator_->GetWatchersCreated());
On 2015/09/18 15:56:38, Ryan Hamilton wrote:
> I doesn't seem like we need to test this behavior in every test. That just
makes
> the tests harder to read.
Agreed. Removed some of the tests.
> In any case, you could move this code into
> SendRequestAndExpectQuicResponse, I suspect.
Fails some tests e.g., SendRequestAndExpectHttpResponse which actually uses
non-QUIC connection.

[Ryan Sleevi, 2015-09-18 17:36:43]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-09-18 17:36:46]

Mostly style and comment nits - design seems sound :)

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:66:
scoped_ptr<SocketPerformanceWatcher>(),
You can just pass nullptr here. You'd still need the include for the implicit
cast though

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection.cc:1831: if (debug_visitor_ != nullptr)
if (debug_visitor_)

It's cleaner that way, plus works across all pointer and smart pointer types :)

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection.cc:1832: debug_visitor_->OnRttChanged(rtt);
It doesn't seem like it really makes sense to couple this with the the debug
visitor - this isn't necessarily a debug event. Shouldn't this have a proper
interface from the Quicconnection?

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection_logger.cc:797: long microseconds =
rtt.ToMicroseconds();
BUG: This should be int64

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.h (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection_logger.h:184: // Watches the performance of the UDP
socket underlying this QUIC connection.
Wording nit:
// Receives notifications regarding the performance of the underlying socket for
the QUIC connection.

It doesn't watch (implies proactive observation), since it's in fact notified by
this class. Also, slightly rewords the latter part to avoid the explicit mention
of both UDP and this (these are more pet peeves :P - the former being
over-specifying the implementation, the latter being avoiding self-references
when possible)

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
File net/quic/quic_connection_logger_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection_logger_unittest.cc:38:
scoped_ptr<SocketPerformanceWatcher>(),
nullptr

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_http_str...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_http_str...
net/quic/quic_http_stream_test.cc:228: scoped_ptr<SocketPerformanceWatcher>(),
nullptr));
nullptr

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:182: return
scoped_ptr<SocketPerformanceWatcher>();
nullptr

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:187: if (watcher &&
watcher->IsRTTAvailableRTTNotificationReceived())
When is a null watcher in watchers_ valid?

[tbansal1, 2015-09-18 17:59:59]

Addressed sleevi comments. PTAL. Thanks.

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:66:
scoped_ptr<SocketPerformanceWatcher>(),
On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> You can just pass nullptr here. You'd still need the include for the implicit
> cast though

Done.

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection.cc:1831: if (debug_visitor_ != nullptr)
On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> if (debug_visitor_)
> 
> It's cleaner that way, plus works across all pointer and smart pointer types
:)

Done.

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection.cc:1832: debug_visitor_->OnRttChanged(rtt);
On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> It doesn't seem like it really makes sense to couple this with the the debug
> visitor - this isn't necessarily a debug event. Shouldn't this have a proper
> interface from the Quicconnection?

Not sure what is the right design here. Initially, I had the plumbing in
QuicConnection. rch@ mentioned it is discouraged to have Chromium specific code
in QuicConnection because it needs to be synced with the backend implementation.
wdyt?

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection_logger.cc:797: long microseconds =
rtt.ToMicroseconds();
On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> BUG: This should be int64

Done. Thanks for catching that. This is what I get for working on Java (upstream
code) and C++ at the same time.

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.h (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection_logger.h:184: // Watches the performance of the UDP
socket underlying this QUIC connection.
On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> Wording nit:
> // Receives notifications regarding the performance of the underlying socket
for
> the QUIC connection.
> 
> It doesn't watch (implies proactive observation), since it's in fact notified
by
> this class. Also, slightly rewords the latter part to avoid the explicit
mention
> of both UDP and this (these are more pet peeves :P - the former being
> over-specifying the implementation, the latter being avoiding self-references
> when possible)

Done. That makes sense, I need to be more careful when naming things. Hope I can
get better :)

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
File net/quic/quic_connection_logger_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection_logger_unittest.cc:38:
scoped_ptr<SocketPerformanceWatcher>(),
On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_http_str...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_http_str...
net/quic/quic_http_stream_test.cc:228: scoped_ptr<SocketPerformanceWatcher>(),
nullptr));
On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:182: return
scoped_ptr<SocketPerformanceWatcher>();
On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:187: if (watcher &&
watcher->IsRTTAvailableRTTNotificationReceived())
On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> When is a null watcher in watchers_ valid?

Removed the check.

[Ryan Hamilton, 2015-09-18 22:01:26]

Thanks for the fixes. A few more small comments.

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:181: class
TestNetworkQualityEstimator : public NetworkQualityEstimator {
On 2015/09/18 17:21:20, tbansal1 wrote:
> On 2015/09/18 15:56:38, Ryan Hamilton wrote:
> > Instead of a full blown NQE, how 'bout just a 
SocketPerformanceWatcherFactory
> > since that's all you need?
> 
> Yeah, I had TestSocketPerformanceWatcherFactory before, but thought it might
be
> better to test with NQE because that's the one implementing. It seems like a
> more realistic test. wdyt?

Are you actually testing any methods of the NQE beyond those used in this test?

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/1305293004/diff/140001/net/quic/quic_connecti...
net/quic/quic_connection.cc:1832: debug_visitor_->OnRttChanged(rtt);
On 2015/09/18 17:59:59, tbansal1 wrote:
> On 2015/09/18 17:36:45, Ryan Sleevi wrote:
> > It doesn't seem like it really makes sense to couple this with the the debug
> > visitor - this isn't necessarily a debug event. Shouldn't this have a proper
> > interface from the Quicconnection?
> 
> Not sure what is the right design here. Initially, I had the plumbing in
> QuicConnection. rch@ mentioned it is discouraged to have Chromium specific
code
> in QuicConnection because it needs to be synced with the backend
implementation.
> wdyt?

I think this current approach is correct. The "real" visitor is used to
implement the core QUIC protocol, i.e. things which the QuicSession needs to
implement. The debug visitor is for pulling various metrics out of the
Connection. Some of these metrics may end up having "real" effects, but I'm fine
with this being tied to the debug visitor.

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:66: /*SocketPerformanceWatcher*/
nullptr,
/*socket_performance_watcher=*/

(Use the name of the argument, not the argument's type.

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_connecti...
File net/quic/quic_connection_logger_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_connecti...
net/quic/quic_connection_logger_unittest.cc:38: /*SocketPerformanceWatcher*/
nullptr,
ditto

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_http_str...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_http_str...
net/quic/quic_http_stream_test.cc:228: /*SocketPerformanceWatcher*/ nullptr,
nullptr));
ditto

[tbansal1, 2015-09-18 22:36:22]

PTAL. Thanks a lot for taking time to review this.

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/120001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:181: class
TestNetworkQualityEstimator : public NetworkQualityEstimator {
On 2015/09/18 22:01:26, Ryan Hamilton wrote:
> On 2015/09/18 17:21:20, tbansal1 wrote:
> > On 2015/09/18 15:56:38, Ryan Hamilton wrote:
> > > Instead of a full blown NQE, how 'bout just a 
> SocketPerformanceWatcherFactory
> > > since that's all you need?
> > 
> > Yeah, I had TestSocketPerformanceWatcherFactory before, but thought it might
> be
> > better to test with NQE because that's the one implementing. It seems like a
> > more realistic test. wdyt?
> 
> Are you actually testing any methods of the NQE beyond those used in this
test?

Not right now. That's because NQE is not doing anything with the RTT data. Once
https://codereview.chromium.org/1273173002/ lands, it will start notifying
observers. Then, I can add an observer, and see if it gets notified or not.

For now, if you think it is better to use SocketPerformanceWatcherFactory, I can
do that. Later, I can change it to use TestNQE, once NQE starts doing something
with the data.

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:66: /*SocketPerformanceWatcher*/
nullptr,
On 2015/09/18 22:01:26, Ryan Hamilton wrote:
> /*socket_performance_watcher=*/
> 
> (Use the name of the argument, not the argument's type.

Done.

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_connecti...
File net/quic/quic_connection_logger_unittest.cc (right):

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_connecti...
net/quic/quic_connection_logger_unittest.cc:38: /*SocketPerformanceWatcher*/
nullptr,
On 2015/09/18 22:01:26, Ryan Hamilton wrote:
> ditto

Done.

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_http_str...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/1305293004/diff/160001/net/quic/quic_http_str...
net/quic/quic_http_stream_test.cc:228: /*SocketPerformanceWatcher*/ nullptr,
nullptr));
On 2015/09/18 22:01:26, Ryan Hamilton wrote:
> ditto

Done.

[Ryan Hamilton, 2015-09-21 17:46:42]

lgtm

[tbansal1, 2015-09-21 22:03:21]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2015-09-21 22:03:21]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1305293004/#ps220001
(title: "Removed a test")

[commit-bot: I haz the power, 2015-09-21 22:05:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1305293004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1305293004/220001

[commit-bot: I haz the power, 2015-09-21 22:46:47]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2015-09-21 22:47:24]

Patchset 8 (id:??) landed as
https://crrev.com/fdf5665b2c4d9aa3b421a833bc7bf5330454faf3
Cr-Commit-Position: refs/heads/master@{#350053}