Notify NQE of TCP RTT values

Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params
-> ClientSocketPoolManagerImpl
-> TransportClientSocketPool
-> TransportConnectJobFactory
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the
CreateTransportClientSocket. From there it is passed to
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket
object is reused for a different connection (by first
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423
TBR=pfeldman@chromium.org, frankf@chromium.org
(for chrome/browser/devtools/device/* and
chrome/test/chromedriver/net/* since the changes are
mechanical).

Committed: https://crrev.com/7b403bcc8080792387a99f869285cd00f36b6167
Cr-Commit-Position: refs/heads/master@{#387128}

[tbansal1, 2015-09-28 23:12:09]

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

[tbansal1, 2015-09-28 23:12:09]

Ryan: Can you PTAL at the architecture and layering? I have not added any tests
yet. I am going to do that once the plumbing looks okay. Thanks!

[tbansal1, 2015-09-28 23:13:18]

I did build Chrome on Android, and this change does work: It reads tcp_info
struct and notifies NQE of the RTT. So, at least we know it is working. thanks!!

[Ryan Sleevi, 2015-10-20 00:10:36]

Is this something that we want to force callers to be aware of for all sockets,
or is this something that should be opted-in to? I suppose I'm largely
'wondering outloud' if this should be something like
SetSocketPerformanceFactory(), a param in Connect(), or in the ctor as you have
it.

For example, if extensions are using the chrome.sockets.tcp APIs, should it be
participating in the socket performance factory? why or why not? How does that
use case influence the design here.

Otherwise it mostly mechnically LG, with a few comments about comments.

https://codereview.chromium.org/1376473003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_socket_net.cc (right):

https://codereview.chromium.org/1376473003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_socket_net.cc:120: new
net::TCPSocket(/*socket_performance_watcher_factory=*/nullptr,
Don't do this. We don't generally do it in Chrome.

https://codereview.chromium.org/1376473003/diff/1/net/base/socket_performance...
File net/base/socket_performance_watcher_factory.h (right):

https://codereview.chromium.org/1376473003/diff/1/net/base/socket_performance...
net/base/socket_performance_watcher_factory.h:11: #include
"net/base/socket_performance_watcher.h"
Unnecessary / forward declare.

https://codereview.chromium.org/1376473003/diff/1/net/dns/dns_socket_pool.cc
File net/dns/dns_socket_pool.cc (right):

https://codereview.chromium.org/1376473003/diff/1/net/dns/dns_socket_pool.cc#...
net/dns/dns_socket_pool.cc:66: /*socket_performance_watcher_factory=*/nullptr,
net_log_, source));
ditto; let's just say the remarks apply throughout this CL.

https://codereview.chromium.org/1376473003/diff/1/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/1376473003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager_impl.cc:224: socket_factory_,
socket_performance_watcher_factory_, net_log_)));
This does make me 'somewhat uneasy' about future privacy issues that might arise
from all of the pools sharing the same socket performance watcher factory.

I'm not sure if I can articulate a more reasoned argument for why, but if we
expose per-host information to origins, it might allow them to infer information
about proxies, which would be a privacy concern.

It may be that this is misguided; the onus is on you to justify why it's OK (and
it may be, but just write it up :P)

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_client_socket.h
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_client_socke...
net/socket/tcp_client_socket.h:29: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher,
Note that |addresses| is plural; this API contract necessarily implies that the
socket_performance_watcher is aggregated based on host, and not IP, which is a
somewhat surprising design requirement. Is that the natural implication you
want?

What about situations of Connect(), Disconnect(), and Connect()? Is that also
correct?

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_socket_posix.cc
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_socket_posix...
net/socket/tcp_socket_posix.cc:718: if (!socket_performance_watcher_)
Is there a reason not to wrap the entire method body inside an #if
defined(TCP_INFO), which gives a much stronger signal to the compiler about the
no-op behaviour of this method on platforms that it's not defined on (allowing
them to ideally elide this method entirely)

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_socket_posix...
net/socket/tcp_socket_posix.cc:721: bool getsockopt_success = false;
I'd expect this will result in a compiler warning about an unused variable on
platforms where TCP_INFO is not defined.

[Ryan Sleevi, 2015-12-08 01:38:45]

Ping

[tbansal1, 2015-12-08 01:50:11]

On 2015/12/08 01:38:45, Ryan Sleevi wrote:
> Ping

Thanks for pinging. I am going to do this sometime this month. This got
deprioritized due to data use accounting.

[bengr, 2016-02-05 00:20:10]

On 2015/12/08 01:50:11, tbansal1 wrote:
> On 2015/12/08 01:38:45, Ryan Sleevi wrote:
> > Ping
> 
> Thanks for pinging. I am going to do this sometime this month. This got
> deprioritized due to data use accounting.

Ryan, I'm happy to do a first pass on the review. Let me know if I should.
cbentzel@ and I discussed me taking on some of the net/ reviewing load to share
some of the burden.

[Ryan Sleevi, 2016-02-05 00:26:15]

On 2016/02/05 00:20:10, bengr wrote:
> Ryan, I'm happy to do a first pass on the review. Let me know if I should.
> cbentzel@ and I discussed me taking on some of the net/ reviewing load to
share
> some of the burden.

I'm not sure what prompted this comment? I'm not swamped with reviews - and this
doesn't even seem ready for review? If anything, though, it seems that it would
benefit from an up-front, thorough review - I have yet to have very positive
experiences with "first pass" reviews.

[tbansal1, 2016-02-05 00:47:35]

On 2016/02/05 00:26:15, Ryan Sleevi wrote:
> On 2016/02/05 00:20:10, bengr wrote:
> > Ryan, I'm happy to do a first pass on the review. Let me know if I should.
> > cbentzel@ and I discussed me taking on some of the net/ reviewing load to
> share
> > some of the burden.
> 
> I'm not sure what prompted this comment? I'm not swamped with reviews - and
this
> doesn't even seem ready for review? If anything, though, it seems that it
would
> benefit from an up-front, thorough review - I have yet to have very positive
> experiences with "first pass" reviews.

I am going to update this CL (probably today or tomorrow),
hence I asked this question to bengr@. It seems to me that
the review protocol (as bengr@ pointed out) changed since
I started this CL.

[tbansal1, 2016-02-05 16:50:28]

Description was changed from

==========
Notify NQE of TCP RTT values

No tests added yet.

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params -> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool -> TransportConnectJobfactory
-> CreateTransportClientSocket 

CreateTransportClientSocket() creates a 
SocketPerformanceWatcher which is then passed to 
TCPClientSocket -> TCPSocketPosix

BUG=498380,502419,502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> net::ClientSocketPoolBase<net::TransportSocketParams>
-> ClientSocketPoolBaseHelper
-> ConnectJobFactory

TransportConnectJobFactory overrides ConnectJobFactory
which passes SocketPerformanceWatcherFactory to
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

BUG=498380, 502419, 502423
==========

[bengr, 2016-02-05 18:49:03]

On 2016/02/05 00:26:15, Ryan Sleevi wrote:
> On 2016/02/05 00:20:10, bengr wrote:
> > Ryan, I'm happy to do a first pass on the review. Let me know if I should.
> > cbentzel@ and I discussed me taking on some of the net/ reviewing load to
> share
> > some of the burden.
> 
> I'm not sure what prompted this comment? I'm not swamped with reviews - and
this
> doesn't even seem ready for review? If anything, though, it seems that it
would
> benefit from an up-front, thorough review - I have yet to have very positive
> experiences with "first pass" reviews.

By "first pass" I meant "up-front and thorough." I agree that a single pass by
someone else doesn't help. I'm willing to review this until I think it looks
good and then hand it off to you, or I can just stay out of it. It's up to you,
but my goal is to provide some relief to bottleneck net reviewers. Let me know.

[tbansal1, 2016-02-05 18:58:19]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2016-02-05 19:12:02]

Patchset #2 (id:40001) has been deleted

[Ryan Sleevi, 2016-02-05 19:12:15]

On 2016/02/05 18:49:03, bengr wrote:
> By "first pass" I meant "up-front and thorough." I agree that a single pass by
> someone else doesn't help. I'm willing to review this until I think it looks
> good and then hand it off to you, or I can just stay out of it. It's up to
you,
> but my goal is to provide some relief to bottleneck net reviewers. Let me
know.

Usually that approach only helps with fresh committers working out style and
design idioms :) I think Tarun's been around long enough. But yes, more thorough
reviews the better.

[tbansal1, 2016-02-05 19:25:05]

Patchset #2 (id:60001) has been deleted

[tbansal1, 2016-02-05 19:37:10]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2016-02-05 19:44:14]

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

[tbansal1, 2016-02-05 19:44:15]

bengr/rsleevi: ptal. Thanks. This CL touches a lot of files but most of the
changes are benign. Please see the CL
description for how the SocketPerformanceWatcher object
flows down the layers.

On 2015/10/20 00:10:36, Ryan Sleevi wrote:
> Is this something that we want to force callers to be aware of for all
sockets,
> or is this something that should be opted-in to? I suppose I'm largely
> 'wondering outloud' if this should be something like
> SetSocketPerformanceFactory(), a param in Connect(), or in the ctor as you
have
> it.
My guess is that "opted-in to" model can break easily if the future
implementations
forget to opt-in. By making it a part of constructor, we are forcing the future
implementations to make a cautious decision about whether they should pass a
nullptr or should implement it. wdyt?

Passing SocketPerformanceWatcher (SPW) in TcpClientSocket::Connect() is probably
not much different than passing it in TcpClientSocket constructor. I do not feel
stronglRyan Sleey about either of them.
> 
> For example, if extensions are using the chrome.sockets.tcp APIs, should it be
> participating in the socket performance factory? why or why not? How does that
> use case influence the design here.
> 
In this CL, it is not participating. I am not sure how that affects the design.
We still need to make a decision on a case-by-case basis.

https://codereview.chromium.org/1376473003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_socket_net.cc (right):

https://codereview.chromium.org/1376473003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_socket_net.cc:120: new
net::TCPSocket(/*socket_performance_watcher_factory=*/nullptr,
On 2015/10/20 00:10:36, Ryan Sleevi wrote:
> Don't do this. We don't generally do it in Chrome.

Done.

https://codereview.chromium.org/1376473003/diff/1/net/base/socket_performance...
File net/base/socket_performance_watcher_factory.h (right):

https://codereview.chromium.org/1376473003/diff/1/net/base/socket_performance...
net/base/socket_performance_watcher_factory.h:11: #include
"net/base/socket_performance_watcher.h"
On 2015/10/20 00:10:36, Ryan Sleevi wrote:
> Unnecessary / forward declare.

Done.

https://codereview.chromium.org/1376473003/diff/1/net/dns/dns_socket_pool.cc
File net/dns/dns_socket_pool.cc (right):

https://codereview.chromium.org/1376473003/diff/1/net/dns/dns_socket_pool.cc#...
net/dns/dns_socket_pool.cc:66: /*socket_performance_watcher_factory=*/nullptr,
net_log_, source));
On 2015/10/20 00:10:36, Ryan Sleevi wrote:
> ditto; let's just say the remarks apply throughout this CL.

Done.

https://codereview.chromium.org/1376473003/diff/1/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/1376473003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager_impl.cc:224: socket_factory_,
socket_performance_watcher_factory_, net_log_)));
On 2015/10/20 00:10:36, Ryan Sleevi wrote:
> This does make me 'somewhat uneasy' about future privacy issues that might
arise
> from all of the pools sharing the same socket performance watcher factory.
> 
> I'm not sure if I can articulate a more reasoned argument for why, but if we
> expose per-host information to origins, it might allow them to infer
information
> about proxies, which would be a privacy concern.
> 
> It may be that this is misguided; the onus is on you to justify why it's OK
(and
> it may be, but just write it up :P)

I agree that exposing the raw numerical estimates of RTT or bandwidth to remote
origins is a privacy risk. But, I predict that we will never expose those. In
future, a more likely scenario is that we expose a enum value (dog slow, slow,
not slow, fast etc.).

And that enum value would be computed from multiple factors: bandwidth, RTT,
signal strength, packet loss rate etc (not all are yet 
implemented).

In the unlikely case that a decision is made to expose the raw values,  we would
need to revisit this architecture.

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_client_socket.h
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_client_socke...
net/socket/tcp_client_socket.h:29: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher,
On 2015/10/20 00:10:36, Ryan Sleevi wrote:
> Note that |addresses| is plural; this API contract necessarily implies that
the
> socket_performance_watcher is aggregated based on host, and not IP, which is a
> somewhat surprising design requirement. Is that the natural implication you
> want?
> 
> What about situations of Connect(), Disconnect(), and Connect()? Is that also
> correct?

Good point. Added a Reset() function to SocketPerformanceWatcher which is called
before Connect(). Right now, it is not used by NQE but eventually it would clear
up the state pertaining to that watcher.

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_socket_posix.cc
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_socket_posix...
net/socket/tcp_socket_posix.cc:718: if (!socket_performance_watcher_)
On 2015/10/20 00:10:36, Ryan Sleevi wrote:
> Is there a reason not to wrap the entire method body inside an #if
> defined(TCP_INFO), which gives a much stronger signal to the compiler about
the
> no-op behaviour of this method on platforms that it's not defined on (allowing
> them to ideally elide this method entirely)

Done, I did not think of the optimization.

https://codereview.chromium.org/1376473003/diff/1/net/socket/tcp_socket_posix...
net/socket/tcp_socket_posix.cc:721: bool getsockopt_success = false;
On 2015/10/20 00:10:36, Ryan Sleevi wrote:
> I'd expect this will result in a compiler warning about an unused variable on
> platforms where TCP_INFO is not defined.

Obsolete.

[bengr, 2016-02-08 18:51:14]

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/device/adb/adb_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/device/adb/adb_client_socket.cc:184: socket_.reset(new
net::TCPClientSocket(address_list, NULL, NULL,
There aren't a lot of uses of NULL in this file so consider replacing all of
them with nullptr. Consider this for all file where you use NULL instead of
nullptr.

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/device/adb/adb_client_socket.cc:185:
net::NetLog::Source()));
Also, /* inline comments */ would be helpful next to each nullptr parameter so
we know what they are.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator.cc:916:
DCHECK(thread_checker_.CalledOnValidThread());
Add a comment that nothing needs to be reset for RTT observations.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:7: #include <stdint.h>
Add a blank line.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1114: // Tests that TCP RTTs are
notified by the TCP socket to the Network quality
Capitalize "Quality Estimator."

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1118: std::map<std::string,
std::string> variation_params;
#include <string>

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1126:
scoped_ptr<HttpNetworkSession::Params> params(new HttpNetworkSession::Params);
#include "net/http/http_network_session.h"

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1141:
before_count_tcp_rtt_observations++;
++before_count_tcp_rtt_observations;

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1142: }
Can you expect also that at some point, rtt_observer.observations().size() == 2?
Can you write a test where there's a deterministic number of observations?

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1146: request->Start();
#include "net/url_request/url_request.h"

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1152:
after_count_tcp_rtt_observations++;
++after_count_tcp_rtt_observations;

https://codereview.chromium.org/1376473003/diff/100001/net/dns/address_sorter...
File net/dns/address_sorter_posix_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/dns/address_sorter...
net/dns/address_sorter_posix_unittest.cc:111: SocketPerformanceWatcherFactory*,
Forward declare?

https://codereview.chromium.org/1376473003/diff/100001/net/dns/dns_session_un...
File net/dns/dns_session_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/dns/dns_session_un...
net/dns/dns_session_unittest.cc:38: SocketPerformanceWatcherFactory*,
Forward declare?

https://codereview.chromium.org/1376473003/diff/100001/net/socket/client_sock...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:95: if (pool_type !=
HttpNetworkSession::WEBSOCKET_SOCKET_POOL) {
Do you think you should instead check that it is a NORMAL_SOCKET_POOL? I'm
concerned that this will break if another socket pool type is added.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:270: TransportClientSocketPool*
transport_client_socket_pool_http =
Right now it's easy enough to see that ownership get passed to
transport_socket_pools_for_http_proxies_, but that relationship could get
fuzzier if anyone adds code between the here and the pass. Consider storing this
as a scoped_ptr and then releasing it when you pass. 

Same for line 281.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:274:
socket_performance_watcher_factory_);
Why isn't this part of the constructor?

https://codereview.chromium.org/1376473003/diff/100001/net/socket/socket_test...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/socket_test...
net/socket/socket_test_util.h:517: SocketPerformanceWatcherFactory*
socket_performance_watcher_factory,
Forward declare.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:731:
base::TimeDelta::FromMicroseconds(info.tcpi_rtt));
#include "base/time/time.h"

Also, is tcpi_rtt guaranteed to be valid? How do you tell?

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.h (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.h:18: #include
"net/base/socket_performance_watcher.h"
Can this be a forward declaration?

[tbansal1, 2016-02-08 21:24:36]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> net::ClientSocketPoolBase<net::TransportSocketParams>
-> ClientSocketPoolBaseHelper
-> ConnectJobFactory

TransportConnectJobFactory overrides ConnectJobFactory
which passes SocketPerformanceWatcherFactory to
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-08 21:33:26]

bengr: ptal.

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/device/adb/adb_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/device/adb/adb_client_socket.cc:184: socket_.reset(new
net::TCPClientSocket(address_list, NULL, NULL,
On 2016/02/08 18:51:14, bengr wrote:
> There aren't a lot of uses of NULL in this file so consider replacing all of
> them with nullptr. Consider this for all file where you use NULL instead of
> nullptr.

This CL is already huge :(. So, I would prefer not to change other code.

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/device/adb/adb_client_socket.cc:185:
net::NetLog::Source()));
On 2016/02/08 18:51:14, bengr wrote:
> Also, /* inline comments */ would be helpful next to each nullptr parameter so
> we know what they are.

I think inline comments are discouraged in Chromium net stack code:
https://codereview.chromium.org/1376473003/diff/1/device/bluetooth/bluetooth_...

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator.cc:916:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/02/08 18:51:14, bengr wrote:
> Add a comment that nothing needs to be reset for RTT observations.

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:7: #include <stdint.h>
On 2016/02/08 18:51:14, bengr wrote:
> Add a blank line.

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1114: // Tests that TCP RTTs are
notified by the TCP socket to the Network quality
On 2016/02/08 18:51:14, bengr wrote:
> Capitalize "Quality Estimator."

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1118: std::map<std::string,
std::string> variation_params;
On 2016/02/08 18:51:14, bengr wrote:
> #include <string>

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1126:
scoped_ptr<HttpNetworkSession::Params> params(new HttpNetworkSession::Params);
On 2016/02/08 18:51:14, bengr wrote:
> #include "net/http/http_network_session.h"

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1141:
before_count_tcp_rtt_observations++;
On 2016/02/08 18:51:14, bengr wrote:
> ++before_count_tcp_rtt_observations;

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1142: }
On 2016/02/08 18:51:14, bengr wrote:
> Can you expect also that at some point, rtt_observer.observations().size() ==
2?
> Can you write a test where there's a deterministic number of observations?

I do not think it is possible to write a test with deterministic number of
observations. Reason being that TCP may break the payload arbitrarily. It might
break a 100 byte webpage into 100 packets (each of 1 byte). There are no
guarantees.

It is possible to write test that generates more than 1 RTT observation for a
single URL request. e.g., by creating URL request that fetches a large webpage
from the embedded test web server.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1146: request->Start();
On 2016/02/08 18:51:14, bengr wrote:
> #include "net/url_request/url_request.h"

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1152:
after_count_tcp_rtt_observations++;
On 2016/02/08 18:51:14, bengr wrote:
> ++after_count_tcp_rtt_observations;

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/dns/address_sorter...
File net/dns/address_sorter_posix_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/dns/address_sorter...
net/dns/address_sorter_posix_unittest.cc:111: SocketPerformanceWatcherFactory*,
On 2016/02/08 18:51:14, bengr wrote:
> Forward declare?

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/dns/dns_session_un...
File net/dns/dns_session_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/dns/dns_session_un...
net/dns/dns_session_unittest.cc:38: SocketPerformanceWatcherFactory*,
On 2016/02/08 18:51:14, bengr wrote:
> Forward declare?

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/client_sock...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:95: if (pool_type !=
HttpNetworkSession::WEBSOCKET_SOCKET_POOL) {
On 2016/02/08 18:51:14, bengr wrote:
> Do you think you should instead check that it is a NORMAL_SOCKET_POOL? I'm
> concerned that this will break if another socket pool type is added.

Obsolete.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:270: TransportClientSocketPool*
transport_client_socket_pool_http =
On 2016/02/08 18:51:14, bengr wrote:
> Right now it's easy enough to see that ownership get passed to
> transport_socket_pools_for_http_proxies_, but that relationship could get
> fuzzier if anyone adds code between the here and the pass. Consider storing
this
> as a scoped_ptr and then releasing it when you pass. 
> 
> Same for line 281.

Obsolete.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:274:
socket_performance_watcher_factory_);
On 2016/02/08 18:51:14, bengr wrote:
> Why isn't this part of the constructor?

Done. It required changed some more files. :)

https://codereview.chromium.org/1376473003/diff/100001/net/socket/socket_test...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/socket_test...
net/socket/socket_test_util.h:517: SocketPerformanceWatcherFactory*
socket_performance_watcher_factory,
On 2016/02/08 18:51:14, bengr wrote:
> Forward declare.

Done.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:731:
base::TimeDelta::FromMicroseconds(info.tcpi_rtt));
On 2016/02/08 18:51:14, bengr wrote:
> #include "base/time/time.h"
It is already there in the .tcp_socket_posix.h file
> 
> Also, is tcpi_rtt guaranteed to be valid? How do you tell?

Return value of GetTcpInfo() is expected to be false in the struct is invalid.
But, I added another check just to be sure that I do not report invalid values
(although I have not seen invalid values on linux or on android).

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.h (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.h:18: #include
"net/base/socket_performance_watcher.h"
On 2016/02/08 18:51:14, bengr wrote:
> Can this be a forward declaration?

No, that gives compiler error:
gnu/4.6/../../../../include/c++/4.6/bits/unique_ptr.h:61:16: error: invalid
application of 'sizeof' to an incomplete type 'net::SocketPerformanceWatcher'

[bengr, 2016-02-08 23:42:07]

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/device/adb/adb_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/device/adb/adb_client_socket.cc:184: socket_.reset(new
net::TCPClientSocket(address_list, NULL, NULL,
On 2016/02/08 21:33:26, tbansal1 wrote:
> On 2016/02/08 18:51:14, bengr wrote:
> > There aren't a lot of uses of NULL in this file so consider replacing all of
> > them with nullptr. Consider this for all file where you use NULL instead of
> > nullptr.
> 
> This CL is already huge :(. So, I would prefer not to change other code.

Acknowledged.

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/device/adb/adb_client_socket.cc:185:
net::NetLog::Source()));
On 2016/02/08 21:33:26, tbansal1 wrote:
> On 2016/02/08 18:51:14, bengr wrote:
> > Also, /* inline comments */ would be helpful next to each nullptr parameter
so
> > we know what they are.
> 
> I think inline comments are discouraged in Chromium net stack code:
>
https://codereview.chromium.org/1376473003/diff/1/device/bluetooth/bluetooth_...

I've seen it both ways in Chromium, but I'll defer to the comment from the net/
owner.

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1142: }
On 2016/02/08 21:33:26, tbansal1 wrote:
> On 2016/02/08 18:51:14, bengr wrote:
> > Can you expect also that at some point, rtt_observer.observations().size()
==
> 2?
> > Can you write a test where there's a deterministic number of observations?
> 
> I do not think it is possible to write a test with deterministic number of
> observations. Reason being that TCP may break the payload arbitrarily. It
might
> break a 100 byte webpage into 100 packets (each of 1 byte). There are no
> guarantees.
> 
> It is possible to write test that generates more than 1 RTT observation for a
> single URL request. e.g., by creating URL request that fetches a large webpage
> from the embedded test web server.

Acknowledged.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:731:
base::TimeDelta::FromMicroseconds(info.tcpi_rtt));
On 2016/02/08 21:33:26, tbansal1 wrote:
> On 2016/02/08 18:51:14, bengr wrote:
> > #include "base/time/time.h"
> It is already there in the .tcp_socket_posix.h file
> > 
> > Also, is tcpi_rtt guaranteed to be valid? How do you tell?
> 
> Return value of GetTcpInfo() is expected to be false in the struct is invalid.
> But, I added another check just to be sure that I do not report invalid values
> (although I have not seen invalid values on linux or on android).

Actually, isn't 0 a valid RTT? Perhaps leave it the way you had it.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.h (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.h:18: #include
"net/base/socket_performance_watcher.h"
On 2016/02/08 21:33:26, tbansal1 wrote:
> On 2016/02/08 18:51:14, bengr wrote:
> > Can this be a forward declaration?
> 
> No, that gives compiler error:
> gnu/4.6/../../../../include/c++/4.6/bits/unique_ptr.h:61:16: error: invalid
> application of 'sizeof' to an incomplete type 'net::SocketPerformanceWatcher'

Acknowledged.

https://codereview.chromium.org/1376473003/diff/120001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1376473003/diff/120001/net/base/network_quali...
net/base/network_quality_estimator.cc:917: // Nothing needs to be done for RTT
observations.
Can you say why?

[tbansal1, 2016-02-09 17:11:22]

bengr: ptal. thanks.

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
File chrome/browser/devtools/device/adb/adb_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/chrome/browser/devtool...
chrome/browser/devtools/device/adb/adb_client_socket.cc:185:
net::NetLog::Source()));
On 2016/02/08 23:42:06, bengr wrote:
> On 2016/02/08 21:33:26, tbansal1 wrote:
> > On 2016/02/08 18:51:14, bengr wrote:
> > > Also, /* inline comments */ would be helpful next to each nullptr
parameter
> so
> > > we know what they are.
> > 
> > I think inline comments are discouraged in Chromium net stack code:
> >
>
https://codereview.chromium.org/1376473003/diff/1/device/bluetooth/bluetooth_...
> 
> I've seen it both ways in Chromium, but I'll defer to the comment from the
net/
> owner.

Acknowledged.

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/100001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:731:
base::TimeDelta::FromMicroseconds(info.tcpi_rtt));
On 2016/02/08 23:42:06, bengr wrote:
> On 2016/02/08 21:33:26, tbansal1 wrote:
> > On 2016/02/08 18:51:14, bengr wrote:
> > > #include "base/time/time.h"
> > It is already there in the .tcp_socket_posix.h file
> > > 
> > > Also, is tcpi_rtt guaranteed to be valid? How do you tell?
> > 
> > Return value of GetTcpInfo() is expected to be false in the struct is
invalid.
> > But, I added another check just to be sure that I do not report invalid
values
> > (although I have not seen invalid values on linux or on android).
> 
> Actually, isn't 0 a valid RTT? Perhaps leave it the way you had it.

Probably not. If RTT computed after doing EWMA is 0, then every TCP packet would
time out as soon as it is sent.

https://codereview.chromium.org/1376473003/diff/120001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1376473003/diff/120001/net/base/network_quali...
net/base/network_quality_estimator.cc:917: // Nothing needs to be done for RTT
observations.
On 2016/02/08 23:42:06, bengr wrote:
> Can you say why?

Done.

[bengr, 2016-02-16 16:41:17]

https://codereview.chromium.org/1376473003/diff/140001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/140001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1118: // Tests that TCP RTTs are
notified by the TCP socket to the Network Quality
// Tests that the TCP socket notifies the Network Quality Estimator of TCP RTTs,
which in turn notifies registered RTT observers.

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:131: socket_performance_watcher_->OnReset();
Does this mean you reset on the first connect?

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:721: #if defined(TCP_INFO)
Should this match network_quality_estimator_unittests.cc? In that file you
condition on OS_LINUX being defined, but here you condition on TCP_INFO.

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:726: bool getsockopt_success =
GetTcpInfo(socket_->socket_fd(), &info);
How about:

if (!GetTcpInfo(...))
  return;

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:731: // Zero RTT value indicates that tcp_info
struct was not properly populated.
Really? What's reported for a local socket?

[Ryan Sleevi, 2016-02-16 16:50:08]

Drive-by for catching up on review: What's the recommended review order to see
your design, and then the mechanical changes? That is, what represents the core
interfaces (headers), then implementation? And I'm assuming everything else is
incidental

[Ryan Sleevi, 2016-02-16 16:51:07]

(For example, I'm trying to make sense of why OnReset() exists, as it seems
counter-intuitive and not ideal, but I don't want to say that until I've taken
the time to understand it)

[tbansal1, 2016-02-17 02:53:34]

bengr: ptal.
rsleevi: Please feel free to review but I believe the contract is that bengr@
gives l-g-t-m first, and then I add other reviewers from net-stack. This CL got
a bit messed up because it started before that contract was in place. Sorry
about that.

https://codereview.chromium.org/1376473003/diff/140001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/140001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1118: // Tests that TCP RTTs are
notified by the TCP socket to the Network Quality
On 2016/02/16 16:41:17, bengr wrote:
> // Tests that the TCP socket notifies the Network Quality Estimator of TCP
RTTs,
> which in turn notifies registered RTT observers.

Done.

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:131: socket_performance_watcher_->OnReset();
On 2016/02/16 16:41:17, bengr wrote:
> Does this mean you reset on the first connect? 

Yes.

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:721: #if defined(TCP_INFO)
On 2016/02/16 16:41:17, bengr wrote:
> Should this match network_quality_estimator_unittests.cc? In that file you
> condition on OS_LINUX being defined, but here you condition on TCP_INFO.

Updated nqe_unittest.cc

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:726: bool getsockopt_success =
GetTcpInfo(socket_->socket_fd(), &info);
On 2016/02/16 16:41:17, bengr wrote:
> How about:
> 
> if (!GetTcpInfo(...))
>   return;

Done.

https://codereview.chromium.org/1376473003/diff/140001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:731: // Zero RTT value indicates that tcp_info
struct was not properly populated.
On 2016/02/16 16:41:17, bengr wrote:
> Really? What's reported for a local socket?

Non-zero value. The test in nqe_unittest.cc tests exactly this. It fetches a
HTTP URL from an embedded test server.

[Ryan Sleevi, 2016-02-17 15:19:23]

On 2016/02/17 02:53:34, tbansal1 wrote:
> bengr: ptal.
> rsleevi: Please feel free to review but I believe the contract is that bengr@
> gives l-g-t-m first, and then I add other reviewers from net-stack. This CL
got
> a bit messed up because it started before that contract was in place. Sorry
> about that.

I explicitly suggested otherwise for this review. I also think it's important,
since my questions are fundamentally about the design of the approach, to make
sure that all of this code that bengr is reviewing is actually the right
direction.

To that end, if you have a chance to reply to my previous design-level comments,
I would appreciate it :)

[bengr, 2016-02-17 15:57:31]

On 2016/02/17 15:19:23, Ryan Sleevi (Slow to 2-21) wrote:
> On 2016/02/17 02:53:34, tbansal1 wrote:
> > bengr: ptal.
> > rsleevi: Please feel free to review but I believe the contract is that
bengr@
> > gives l-g-t-m first, and then I add other reviewers from net-stack. This CL
> got
> > a bit messed up because it started before that contract was in place. Sorry
> > about that.
> 
> I explicitly suggested otherwise for this review. I also think it's important,
> since my questions are fundamentally about the design of the approach, to make
> sure that all of this code that bengr is reviewing is actually the right
> direction.
> 
> To that end, if you have a chance to reply to my previous design-level
comments,
> I would appreciate it :)

I agree that Tarun should answer the questions in comment #4. (For some reason I
thought you two were in agreement on design, but having just scanned the
comments in this CL, I don't think that's the case.) I'll hold off on the
review.

[tbansal1, 2016-02-17 16:42:49]

n 2015/10/20 00:10:36, Ryan Sleevi wrote:
> Is this something that we want to force callers to be aware of for all
sockets,
> or is this something that should be opted-in to? I suppose I'm largely
> 'wondering outloud' if this should be something like
> SetSocketPerformanceFactory(), a param in Connect(), or in the ctor as you
have
> it.
My guess is that "opted-in to" model can break easily if the future
implementations
forget to opt-in. By making it a part of constructor, we are forcing the future
implementations to make a cautious decision about whether they should pass a
nullptr or should implement it. wdyt?

Passing SocketPerformanceWatcher (SPW) in TcpClientSocket::Connect() is probably
not much different than passing it in TcpClientSocket constructor. I do not feel
strongly about either of them.
> 
> For example, if extensions are using t
he chrome.sockets.tcp APIs, should it be
> participating in the socket performance factory? why or why not? How does that
> use case influence the design here.
> 
In this CL, it is not participating. I am not sure how that affects the design.
We still need to make a decision on a case-by-case basis.

On 2016/02/16 16:51:07, Ryan Sleevi (Slow to 2-21) wrote:
> (For example, I'm trying to make sense of why OnReset() exists, as it seems
> counter-intuitive and not ideal, but I don't want to say that until I've taken
> the time to understand it)

It exists because the same socket can be used to connect to multiple addresses
(one after another). 
When this happens, we should let NQE (aka SocketPerformanceWatcherFactory) know
that since
NQE may use that information (it currently does not). Did you had an alternate
design in mind?
One alternate design is to create a new SocketPerformanceWatcher every time
socket is reused.

[bengr, 2016-02-21 21:20:32]

On 2016/02/17 16:42:49, tbansal1 wrote:
> n 2015/10/20 00:10:36, Ryan Sleevi wrote:
> > Is this something that we want to force callers to be aware of for all
> sockets,
> > or is this something that should be opted-in to? I suppose I'm largely
> > 'wondering outloud' if this should be something like
> > SetSocketPerformanceFactory(), a param in Connect(), or in the ctor as you
> have
> > it.
> My guess is that "opted-in to" model can break easily if the future
> implementations
> forget to opt-in. By making it a part of constructor, we are forcing the
future
> implementations to make a cautious decision about whether they should pass a
> nullptr or should implement it. wdyt?
> 
> Passing SocketPerformanceWatcher (SPW) in TcpClientSocket::Connect() is
probably
> not much different than passing it in TcpClientSocket constructor. I do not
feel
> strongly about either of them.
> > 
> > For example, if extensions are using t
> he chrome.sockets.tcp APIs, should it be
> > participating in the socket performance factory? why or why not? How does
that
> > use case influence the design here.
> > 
> In this CL, it is not participating. I am not sure how that affects the
design.
> We still need to make a decision on a case-by-case basis.
> 
> On 2016/02/16 16:51:07, Ryan Sleevi (Slow to 2-21) wrote:
> > (For example, I'm trying to make sense of why OnReset() exists, as it seems
> > counter-intuitive and not ideal, but I don't want to say that until I've
taken
> > the time to understand it)
> 
> It exists because the same socket can be used to connect to multiple addresses
> (one after another). 
> When this happens, we should let NQE (aka SocketPerformanceWatcherFactory)
know
> that since
> NQE may use that information (it currently does not). Did you had an alternate
> design in mind?
> One alternate design is to create a new SocketPerformanceWatcher every time
> socket is reused.

sleevi@: Let me know when you are happy the design approach and then I'll
continue my review.

[Ryan Sleevi, 2016-02-23 00:05:33]

On 2016/02/17 16:42:49, tbansal1 wrote:
> My guess is that "opted-in to" model can break easily if the future
> implementations
> forget to opt-in. By making it a part of constructor, we are forcing the
future
> implementations to make a cautious decision about whether they should pass a
> nullptr or should implement it. wdyt?

I don't think the Constructor-vs-Connect really changes that in any meaningful
way. I guess I'm having trouble understanding the scenario you're concerned
about?

> Passing SocketPerformanceWatcher (SPW) in TcpClientSocket::Connect() is
probably
> not much different than passing it in TcpClientSocket constructor. I do not
feel
> strongly about either of them.

Well, yes and no - it's obviously different for the case of a single
ClientSocket being reused. That said, I was able to find
https://bugs.chromium.org/p/chromium/issues/detail?id=499289 which is davidben@
wanting to get rid of that contract entirely.

> It exists because the same socket can be used to connect to multiple addresses
> (one after another). 

Well, what do you mean, "same socket" - same TCP socket or same OS socket
handle?

> One alternate design is to create a new SocketPerformanceWatcher every time
> socket is reused.

Right, and I think that's perhaps clearer semantics, and related to single
responsibility. That is, it seems odd to have one object that watches multiple
(logically independent) connections. Plus, once the Connect()/Disconnect()
contract goes away, perhaps it will be simpler.

[Ryan Sleevi, 2016-02-23 00:14:03]

On 2016/02/21 21:20:32, bengr wrote:
> sleevi@: Let me know when you are happy the design approach and then I'll
> continue my review.

I would encourage you to keep iterating :)

Note that the question from comment #25 still applies (and if I missed it,
apologies - can someone point out the review order)

[tbansal1, 2016-02-23 00:22:32]

On 2016/02/23 00:05:33, Ryan Sleevi wrote:
> On 2016/02/17 16:42:49, tbansal1 wrote:
> > My guess is that "opted-in to" model can break easily if the future
> > implementations
> > forget to opt-in. By making it a part of constructor, we are forcing the
> future
> > implementations to make a cautious decision about whether they should pass a
> > nullptr or should implement it. wdyt?
> 
> I don't think the Constructor-vs-Connect really changes that in any meaningful
> way. I guess I'm having trouble understanding the scenario you're concerned
> about?
> 
Sorry, I was saying that SetSocketPerformanceWatcher() kind of opted-in model
may break because
callers may create the socket, but later forget to call
SetSocketPerformanceWatcher() on the created socket.

I am okay with passing SPW either in the socket constructor, or as a param in
Connect().
> > Passing SocketPerformanceWatcher (SPW) in TcpClientSocket::Connect() is
> probably
> > not much different than passing it in TcpClientSocket constructor. I do not
> feel
> > strongly about either of them.
> 
> Well, yes and no - it's obviously different for the case of a single
> ClientSocket being reused. That said, I was able to find
> https://bugs.chromium.org/p/chromium/issues/detail?id=499289 which is
davidben@
> wanting to get rid of that contract entirely.
Got it. I can keep what I have now (passing SPW in the constructor and calling
OnReset() later)
or pass SPW in the connect(). I think passing SPW in the connect() is cleaner.
WDYT?
> 
> > It exists because the same socket can be used to connect to multiple
addresses
> > (one after another). 
> 
> Well, what do you mean, "same socket" - same TCP socket or same OS socket
> handle?
>
I meant TCP socket (TCPSocketPosix to be precise). 
> > One alternate design is to create a new SocketPerformanceWatcher every time
> > socket is reused.
> 
> Right, and I think that's perhaps clearer semantics, and related to single
> responsibility. That is, it seems odd to have one object that watches multiple
> (logically independent) connections. Plus, once the Connect()/Disconnect()
> contract goes away, perhaps it will be simpler.
Right. I will update the CL to create and pass SPW in the connect() instead of
constructor.

[Ryan Sleevi, 2016-02-23 01:00:54]

On 2016/02/23 00:22:32, tbansal1 wrote:
> Right. I will update the CL to create and pass SPW in the connect() instead of
> constructor.

Well, you asked a question about keeping it as-is, but then in the same message
conclude to change :)

Let me check with mmenke for thoughts; this whole problem goes away if
crbug.com/499289 goes away, but the take-away with that bug should have been "It
may be OK to have OnReset() now, since the whole problem goes away".

[Ryan Sleevi, 2016-02-23 01:01:27]

Also, to reiterate: The most important thing you can provide is a high-level
review guidance for reading this. That's still missing from the bug and CL
description, and that's the most useful thing for future readers.

[tbansal1, 2016-02-23 18:18:03]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every TcpSocketPosix. If a
TcpSocketPosix object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-23 18:18:18]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every TcpSocketPosix. If a
TcpSocketPosix object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocketPosix. If a TcpSocketPosix object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-23 18:19:44]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocketPosix. If a TcpSocketPosix object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets.

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-23 18:27:16]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets.

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.cc).

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-23 18:30:50]

> Well, you asked a question about keeping it as-is, but then in the same
message
> conclude to change :)

> Let me check with mmenke for thoughts; this whole problem goes away if
> crbug.com/499289 goes away, but the take-away with that bug should have been
"It
> may be OK to have OnReset() now, since the whole problem goes away".

Sure, please let me know what you/mmenke think is the right approach.

On 2016/02/23 01:01:27, Ryan Sleevi wrote:
> Also, to reiterate: The most important thing you can provide is a high-level
> review guidance for reading this. That's still missing from the bug and CL
> description, and that's the most useful thing for future readers.

I have added more to the CL description.

[tbansal1, 2016-02-23 18:30:50]

> Well, you asked a question about keeping it as-is, but then in the same
message
> conclude to change :)

> Let me check with mmenke for thoughts; this whole problem goes away if
> crbug.com/499289 goes away, but the take-away with that bug should have been
"It
> may be OK to have OnReset() now, since the whole problem goes away".

Sure, please let me know what you/mmenke think is the right approach.

On 2016/02/23 01:01:27, Ryan Sleevi wrote:
> Also, to reiterate: The most important thing you can provide is a high-level
> review guidance for reading this. That's still missing from the bug and CL
> description, and that's the most useful thing for future readers.

I have added more to the CL description.

[Ryan Sleevi, 2016-02-23 21:07:03]

On 2016/02/23 18:30:50, tbansal1 wrote:
> On 2016/02/23 01:01:27, Ryan Sleevi wrote:
> > Also, to reiterate: The most important thing you can provide is a high-level
> > review guidance for reading this. That's still missing from the bug and CL
> > description, and that's the most useful thing for future readers.
> 
> I have added more to the CL description.

I'm sorry that we're having trouble communicating. I appreciate the effort you
put in to document, but I'm still struggling to take the right approach to this
review.

A common pattern that we take, when sending large reviews here that incorporate
elements of design with elements of boiler plate, is something like

"The main interface is in file X. You can see how that's consumed in File Y and
Z; Y invokes Foo, and passes the result to Z, which is then in charge of
frobbing the cogs. What's tricky about this is handling when the flanger gets
stuck, which you can see in file W - that's why X has the weirdness here.

I'd recommend you look at x.h, then y.cc, then look at Z & W to see the edge
cases. The rest is boiler plate to thread it through - the object is created in
A, and lives there, and is just passed through down to X"

Does that help explain a bit?

The challenge here is that you're not adding any files - you're modifying
existing ones and wiring it up - and so it's hard to know where the interesting
stuff "starts" (and cascades out), and what's just boiler plate.

[Ryan Sleevi, 2016-02-23 21:19:10]

From reading this, it seems like

"Start with TCPClientSocket.cc - that's where TCP RTT values are glued up to the
watcher, using the interface from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() sequences, I needed
to add a way to signal when a new socket was being established - that's why
SocketPerformanceWatcher got an OnReset() method. OnReset() is signalled right
before any connect attempt, to allow the SPW to reset internal state.

Most of the rest was just wiring it up through the Socket Pools - the
transport_socket_pool got a new member to handle it, and so all the other
ConnectJobs were updated to pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to take a null SPW,
since we're not interested in the performance of any of those sockets right now"
(why? I don't know. Shouldn't Pepper matter? *shrug*)

That gives a bit of a better idea how to read this change.

In re-reviewing the design, the biggest thing that strikes me as design weird is
the OnReset() in the ClientSocket's Connect() method - that implies that an SPW
will always have OnReset() called immediately after it's constructed. I think
that strikes me as a weird design contract - because what's the initial state
you're resetting from?

The OnReset() naming also leaves me challenged, because it's hard to indicate
who has agency. When I think of "OnFoo" methods, I think of "Foo happening on
this object due to external events" - but really, OnReset() would be better
named as Reset(), since it's a directive from the underlying ClientSocket to
"Reset any internal state, because I'm about to go Connect() again".

The concrete suggestions:
- Rename OnReset() to Reset()
- Move OnReset() from being invoked during Connect() to being called during
Disconnect(), since it reflects more an act of "tear down" than of "preparation
for the next connection"

I'm concerned that there's not a systematic answer to why the sockets should or
shouldn't be monitored outside of //net, and feel that gives a weird set of API
guarantees for the data used, but that's for you and Ben to work out. I have
trouble determining whether the desired end goal is to support a null SPW or not
- and if it's not, whether we'd be better off with adding another CTOR (and
deprecating the old one), so that it's easier to weed out when a null SPW is
supplied.

https://codereview.chromium.org/1376473003/diff/160001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/160001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:726: if (!GetTcpInfo(socket_->socket_fd(),
&info))
And you're sure the syscall overhead here is not going to destroy performance?

Do we have metrics tracking how much time we spend in this call? Some basic UMA?

[mmenke, 2016-02-23 21:28:38]

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

[mmenke, 2016-02-23 21:28:39]

https://codereview.chromium.org/1376473003/diff/160001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1376473003/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:925:
OnRTTObservation(observation.value.InMilliseconds(),
Drive-by comment (More on pre-existing stuff) calling observers synchronously
from the bottom of the network stack seems like a really, really bad idea.

Suppose I'm an observer...I have a URLRequest that's low priority, but active,
because bandwidth is plentiful, and not much is going on.  Suddenly, I see
everything become sluggish because of a notification, so let's cancel the
request!  But the request I closed was the one that caused the notification in
the first place....

For that reason, I think we really want all callbacks here to be async.

[mmenke, 2016-02-23 21:31:31]

https://codereview.chromium.org/1376473003/diff/160001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/160001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:726: if (!GetTcpInfo(socket_->socket_fd(),
&info))
On 2016/02/23 21:19:10, Ryan Sleevi wrote:
> And you're sure the syscall overhead here is not going to destroy performance?
> 
> Do we have metrics tracking how much time we spend in this call? Some basic
UMA?

Also, calling this on every read or write seems unnecessarily aggressive.  If
I'm downloading a 5 GB file, do we really want to call this 230,000 times?

[Ryan Sleevi, 2016-02-23 21:34:04]

On 2016/02/23 21:28:39, mmenke wrote:
> Suppose I'm an observer...I have a URLRequest that's low priority, but active,
> because bandwidth is plentiful, and not much is going on.  Suddenly, I see
> everything become sluggish because of a notification, so let's cancel the
> request!  But the request I closed was the one that caused the notification in
> the first place....
> 
> For that reason, I think we really want all callbacks here to be async.

I'm actually torn on this - as I fear we could end up causing Message Posting
storms instead (and, indeed, we certainly see these for events like scrolling
and various GPU tasks, which also trigger IPC storms).

It's unclear if your goal here is clearer lifetime semantics, or if it's
strictly async. But yes, I agree that either way, we need to tightly define some
of this API contract.

[tbansal1, 2016-02-23 21:39:55]

On 2016/02/23 21:28:39, mmenke wrote:
>
https://codereview.chromium.org/1376473003/diff/160001/net/base/network_quali...
> File net/base/network_quality_estimator.cc (right):
> 
>
https://codereview.chromium.org/1376473003/diff/160001/net/base/network_quali...
> net/base/network_quality_estimator.cc:925:
> OnRTTObservation(observation.value.InMilliseconds(),
> Drive-by comment (More on pre-existing stuff) calling observers synchronously
> from the bottom of the network stack seems like a really, really bad idea.
> 
> Suppose I'm an observer...I have a URLRequest that's low priority, but active,
> because bandwidth is plentiful, and not much is going on.  Suddenly, I see
> everything become sluggish because of a notification, so let's cancel the
> request!  But the request I closed was the one that caused the notification in
> the first place....
> 
> For that reason, I think we really want all callbacks here to be async.

I do not understand this comment. The RTT is computed from the time request is
sent
to the time response headers are received. If the request was low-priority, and
was
just hanging around, then it would not be penalized. Also, once the response
headers
are received, it is really impossible to close that request because the server
has
probably started sending the response body too. Am I missing something?

[mmenke, 2016-02-23 22:09:00]

On 2016/02/23 21:39:55, tbansal1 wrote:
> On 2016/02/23 21:28:39, mmenke wrote:
> >
>
https://codereview.chromium.org/1376473003/diff/160001/net/base/network_quali...
> > File net/base/network_quality_estimator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1376473003/diff/160001/net/base/network_quali...
> > net/base/network_quality_estimator.cc:925:
> > OnRTTObservation(observation.value.InMilliseconds(),
> > Drive-by comment (More on pre-existing stuff) calling observers
synchronously
> > from the bottom of the network stack seems like a really, really bad idea.
> > 
> > Suppose I'm an observer...I have a URLRequest that's low priority, but
active,
> > because bandwidth is plentiful, and not much is going on.  Suddenly, I see
> > everything become sluggish because of a notification, so let's cancel the
> > request!  But the request I closed was the one that caused the notification
in
> > the first place....
> > 
> > For that reason, I think we really want all callbacks here to be async.
> 
> I do not understand this comment. The RTT is computed from the time request is
> sent
> to the time response headers are received. If the request was low-priority,
and
> was
> just hanging around, then it would not be penalized. Also, once the response
> headers
> are received, it is really impossible to close that request because the server
> has
> probably started sending the response body too. Am I missing something?

That's not the issue.  The issue is that an observer could, in response to a
message, for whatever reason, delete the request that triggered the message.  As
you unwind the stack, you end up at a deleted object, which results in a crash.

[tbansal1, 2016-02-24 18:25:41]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.cc).

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the socket is
disconnected, to allow the SPW to reset internal state.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW, since we're not interested in the 
performance of any of those sockets right now

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-24 18:26:13]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the socket is
disconnected, to allow the SPW to reset internal state.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW, since we're not interested in the 
performance of any of those sockets right now

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the socket is
disconnected, to allow the SPW to reset internal 
state.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW, since we're not interested in the 
performance of any of those sockets right now

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-24 18:26:32]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the socket is
disconnected, to allow the SPW to reset internal 
state.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW, since we're not interested in the 
performance of any of those sockets right now

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the 
socket is disconnected, to allow the SPW to reset internal 
state.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW, since we're not interested in the 
performance of any of those sockets right now

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-24 18:30:54]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the 
socket is disconnected, to allow the SPW to reset internal 
state.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW, since we're not interested in the 
performance of any of those sockets right now

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the 
socket is disconnected, to allow the SPW to reset internal 
state before the socket is reused for a different 
connection.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW or SPWF, since we're not interested in the 
performance of any of those sockets right now.

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-24 18:30:55]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the 
socket is disconnected, to allow the SPW to reset internal 
state.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW, since we're not interested in the 
performance of any of those sockets right now

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the 
socket is disconnected, to allow the SPW to reset internal 
state before the socket is reused for a different 
connection.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW or SPWF, since we're not interested in the 
performance of any of those sockets right now.

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-24 23:03:32]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the 
socket is disconnected, to allow the SPW to reset internal 
state before the socket is reused for a different 
connection.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW or SPWF, since we're not interested in the 
performance of any of those sockets right now.

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h. A SPW is notified of TCP
RTT at most once every 10 seconds. A later CL will make this
more exact (see TODO in socket_performance_watcher.h).

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the 
socket is disconnected, to allow the SPW to reset internal 
state before the socket is reused for a different 
connection.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW or SPWF, since we're not interested in the 
performance of any of those sockets right now.

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-24 23:04:08]

rsleevi: PTAL.
I have updated the CL description with the suggestion. Thanks for the help since
I was unsure of how to describe the reviewer guideline in the best way. I had
added how the objects flow from one-layer to another but the guideline suggested
by you is definitely more useful.


> In re-reviewing the design, the biggest thing that strikes me as design weird
is
> the OnReset() in the ClientSocket's Connect() method - that implies that an
SPW
> will always have OnReset() called immediately after it's constructed. I think
> that strikes me as a weird design contract - because what's the initial state
> you're resetting from?
> 
> The OnReset() naming also leaves me challenged, because it's hard to indicate
> who has agency. When I think of "OnFoo" methods, I think of "Foo happening on
> this object due to external events" - but really, OnReset() would be better
> named as Reset(), since it's a directive from the underlying ClientSocket to
> "Reset any internal state, because I'm about to go Connect() again".
> 
> The concrete suggestions:
> - Rename OnReset() to Reset()
Done.
> - Move OnReset() from being invoked during Connect() to being called during
> Disconnect(), since it reflects more an act of "tear down" than of
"preparation
> for the next connection"
Done.



> And you're sure the syscall overhead here is not going to destroy performance?

> Do we have metrics tracking how much time we spend in this call? Some basic
UMA?

> Also, calling this on every read or write seems unnecessarily aggressive.  If
> I'm downloading a 5 GB file, do we really want to call this 230,000 times?

Added UMA to record time taken for the syscall. In my tests, I saw the delay to
be 20 microseconds. I have also reduced the frequency of notifications.

> It's unclear if your goal here is clearer lifetime semantics, or if it's
> strictly async. But yes, I agree that either way, we need to tightly define
some
> of this API contract.
I agree. For now, the consumers of NQE do not destroy/cancel any request. But,
we
should tighten the API to make it enforcable. Part of the challenge is that
it is unclear how the observers would use the observations provided by NQE.

[Ryan Sleevi, 2016-02-25 22:48:47]

I apologize, we seem to be having some trouble communicating. It might be easier
if Ben took a stab at the CL description.

The goal of the CL description is to ideally:

- Provide one sentence summary
- Provide high-level overview
- Call out any special/tricky bits / known issues
- boilerplate (BUG, TEST, etc)

You might find http://chris.beams.io/posts/git-commit/ helpful

*Separately*, when requesting review, you want to provide guidance on how to
review *in the CL*. This compliments, but doesn't replace, the commit message -
it's how your reviewer(s) should match what you say (in the commit message) to
what you do (in the code), and how to do that as efficiently as possible.

The "how to review" is best done as a review message (not part of the CL
description)

My biggest question on the design is below, figuring out how the SPWF interacts
with the SPW, and some of the abstractions therein. Hopefully I explained my
concerns clearly, but if you're not sure / confused, let me know.

https://codereview.chromium.org/1376473003/diff/180001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/180001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1111: // enabled only on the
Linux platform.
This comment does not match line 1112 :)

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
File net/base/socket_performance_watcher.cc (right):

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
net/base/socket_performance_watcher.cc:15:
rtt_notification_interval_(base::TimeDelta::FromMilliseconds(10000)),
Why not ::FromSeconds(10) ?

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:41: // has received 15 packets.
Afterwards, it should return 2 seconds.
Why not return this as a "const base::TimeDelta&" ?

Also, this comment seems like it belongs in a crbug.com, not here.

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
File net/base/socket_performance_watcher_factory.h (right):

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
net/base/socket_performance_watcher_factory.h:47: virtual void WatcherReset() =
0;
1) Who guarantees that it's called by this? Same with line 37? That seems to be
the responsibility of the SocketPerformanceWatcher, which may be a subclass

2) If it's called, have it On*; if it's directing an action ("You go Reset
this"), call it Reset

3) In order to provide for the invariants you document on line 37 and 45 (e.g.
that these methods are *only* called then), they should be protected methods for
subclasses to override. If your goal is to allow SocketPerformanceWatcher to
call back in to the Factory, then you're fundamentally talking about a design
circular loop (SPWF -> SocketPerformanceWatcher, SPW is required to call
SPWF->On*). You need to break that loop through delegation - SPW defines a
delegate that receives notifications of interesting events, SPWF provides (but
*does not* directly implement) that delegate, and the SPWF delegate then
triggers the SPWF's (protected) methods.

Let me know if you're not clear on these concerns and I can explain them more,
since that'll be easier before you start coding.

https://codereview.chromium.org/1376473003/diff/180001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/180001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:729: }
Why is this a function of the socket - and not of the watcher?

Is this a performance optimization? If so, document it.

Nothing about this code explains apriori about lines 725-729, so you should try
to provide clarifying guidance.

https://codereview.chromium.org/1376473003/diff/180001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:738: // Zero RTT value indicates that tcp_info
struct was not properly populated.
COMMENT NIT: This comment reads weird, because it does not logically flow with
how the condition is evaluated. Further, it raises the question of "What about
less than zero? Is that possible? What does it indicate?"

// Only notify the SocketPerformanceWatcher when the tcp_info struct
// was fully populated.
if (info.tcpi_rtt > 0) {
  socket_performance_watcher_->...
}

https://codereview.chromium.org/1376473003/diff/180001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:742:
UMA_HISTOGRAM_TIMES("SPW.TCPRTT.NotifyDelay", base::TimeTicks::Now() - now);
Wow, this seems like a really gross histogram name; do you have to abbreviate
it?

https://codereview.chromium.org/1376473003/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1376473003/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:47334: +    the tcp_info struct.
This entire histogram description seems wrong - because it means, based on your
documentation, you'll never log it on Windows, even if you implemented it on
Windows.

I'm sorry I wasn't clearer, but I don't think you have to do it via UMA. For
example

{
  tracked_objects::ScopedTracker tracking_profile(FROM_HERE);
  // ... Do stuff
}

Then you can use the existing jank infrastructure to measure this. The Chrome
perf folks can certainly help explain this, as this is how we quantified the
perf code yellow (and these metrics are uploaded to UMA, IIRC)

[tbansal1, 2016-02-26 19:00:56]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory from
HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher from
SocketPerformanceWatcherFactory, and passes it to TCPClientSocket.
-> TCPSocketPosix.

A new SocketPerformanceWatcher is created for every 
TcpSocket. If a TcpSocket object is reused for a
different connection (by first disconnecting and then
reconnecting), then SocketPerformanceWatcher is reset.

Currently, RTT is only reported for Posix TCP Sockets
(net/socket/tcp_socket_posix.

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the watcher, using the interface 
from socket_performance_watcher.h. A SPW is notified of TCP
RTT at most once every 10 seconds. A later CL will make this
more exact (see TODO in socket_performance_watcher.h).

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why
SocketPerformanceWatcher got an Reset() method in 
tcp_client_socket.cc. Reset() is signalled right after the 
socket is disconnected, to allow the SPW to reset internal 
state before the socket is reused for a different 
connection.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWFactory through.

The rest is just boilerplate - updating the constructor to 
take a null SPW or SPWF, since we're not interested in the 
performance of any of those sockets right now.

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the TCPClientSocket, which passes it to
TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-02-26 20:20:54]

Patchset #7 (id:200001) has been deleted

[tbansal1, 2016-02-26 23:40:10]

Patchset #7 (id:220001) has been deleted

[tbansal1, 2016-02-26 23:43:35]

rsleevi: PTAL. Thanks for taking time to help me improve.

https://codereview.chromium.org/1376473003/diff/180001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/180001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1111: // enabled only on the
Linux platform.
On 2016/02/25 22:48:46, Ryan Sleevi wrote:
> This comment does not match line 1112 :)

Thanks, fixed.

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
File net/base/socket_performance_watcher.cc (right):

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
net/base/socket_performance_watcher.cc:15:
rtt_notification_interval_(base::TimeDelta::FromMilliseconds(10000)),
On 2016/02/25 22:48:46, Ryan Sleevi wrote:
> Why not ::FromSeconds(10) ?

Done. Just changed it to a very large value until crbug.com/590300 is fixed.

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:41: // has received 15 packets.
Afterwards, it should return 2 seconds.
On 2016/02/25 22:48:46, Ryan Sleevi wrote:
> Why not return this as a "const base::TimeDelta&" ?
I think mmenke said at one point that it is okay to return TimeDelta by value,
because that class only contains an int64 member. I can't find that comment in
the review history, so I may be wrong. Also, codesearch shows it not returned by
reference 2200 times
(https://code.google.com/p/chromium/codesearch#search/&q=TimeDelta%5C%20%5BA-Z...),
and by reference 500 times
(https://code.google.com/p/chromium/codesearch#search/&q=TimeDelta&%5C%20%...).


> 
> Also, this comment seems like it belongs in a http://crbug.com, not here.
I was going to fix that in the next CL but filed a new bug (crbug.com/590300).

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
File net/base/socket_performance_watcher_factory.h (right):

https://codereview.chromium.org/1376473003/diff/180001/net/base/socket_perfor...
net/base/socket_performance_watcher_factory.h:47: virtual void WatcherReset() =
0;
On 2016/02/25 22:48:46, Ryan Sleevi wrote:
> 1) Who guarantees that it's called by this? Same with line 37? That seems to
be
> the responsibility of the SocketPerformanceWatcher, which may be a subclass
> 
> 2) If it's called, have it On*; if it's directing an action ("You go Reset
> this"), call it Reset
Done.
> 
> 3) In order to provide for the invariants you document on line 37 and 45 (e.g.
> that these methods are *only* called then), they should be protected methods
for
> subclasses to override. If your goal is to allow SocketPerformanceWatcher to
> call back in to the Factory, then you're fundamentally talking about a design
> circular loop (SPWF -> SocketPerformanceWatcher, SPW is required to call
> SPWF->On*). You need to break that loop through delegation - SPW defines a
> delegate that receives notifications of interesting events, SPWF provides (but
> *does not* directly implement) that delegate, and the SPWF delegate then
> triggers the SPWF's (protected) methods.
> 
This is a good suggestion. I filed crbug.com/590265 to track this. I would
prefer to fix this in a separate CL, since this CL has already become too
unwieldy.

> Let me know if you're not clear on these concerns and I can explain them more,
> since that'll be easier before you start coding.

https://codereview.chromium.org/1376473003/diff/180001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/180001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:729: }
On 2016/02/25 22:48:47, Ryan Sleevi wrote:
> Why is this a function of the socket - and not of the watcher?
Changed it to make it cleaner. Also, moved the logic to SPW.
> 
> Is this a performance optimization? If so, document it.
Done.
> 
> Nothing about this code explains apriori about lines 725-729, so you should
try
> to provide clarifying guidance.
Done. Added couple of comments.

https://codereview.chromium.org/1376473003/diff/180001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:738: // Zero RTT value indicates that tcp_info
struct was not properly populated.
On 2016/02/25 22:48:47, Ryan Sleevi wrote:
> COMMENT NIT: This comment reads weird, because it does not logically flow with
> how the condition is evaluated. Further, it raises the question of "What about
> less than zero? Is that possible? What does it indicate?"
> 
> // Only notify the SocketPerformanceWatcher when the tcp_info struct
> // was fully populated.
> if (info.tcpi_rtt > 0) {
>   socket_performance_watcher_->...
> }

Done.

https://codereview.chromium.org/1376473003/diff/180001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:742:
UMA_HISTOGRAM_TIMES("SPW.TCPRTT.NotifyDelay", base::TimeTicks::Now() - now);
On 2016/02/25 22:48:47, Ryan Sleevi wrote:
> Wow, this seems like a really gross histogram name; do you have to abbreviate
> it?

This is now obsolete. Sorry, I did not know abbreviation in histogram names was
a bad idea. There are plenty of histograms with abbreviated names (SB2.*, PLT.*)

https://codereview.chromium.org/1376473003/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1376473003/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:47334: +    the tcp_info struct.
On 2016/02/25 22:48:47, Ryan Sleevi wrote:
> This entire histogram description seems wrong - because it means, based on
your
> documentation, you'll never log it on Windows, even if you implemented it on
> Windows.
> 
> I'm sorry I wasn't clearer, but I don't think you have to do it via UMA. For
> example
> 
> {
>   tracked_objects::ScopedTracker tracking_profile(FROM_HERE);
>   // ... Do stuff
> }
> 
> Then you can use the existing jank infrastructure to measure this. The Chrome
> perf folks can certainly help explain this, as this is how we quantified the
> perf code yellow (and these metrics are uploaded to UMA, IIRC)

Thanks,.removed the UMA. Now, I am using the ScopedTracker.

[tbansal1, 2016-02-26 23:43:57]

Guideline for review:
Start with tcp_socket_posix.cc - that's where TCP RTT 
values are glued up to the SPW, using the interface 
from socket_performance_watcher.h. A SPW is notified of TCP
RTT at most once every 10 hours. A later CL
(crbug.com/590300) will make this more exact.

Because TCPSocket supports multiple Connect()/Disconnect() 
sequences, I needed to add a way to signal when a new 
socket was being established - that's why SPW got an Reset()
method in  tcp_client_socket.cc. Reset() is signalled right
after the socket is disconnected, to allow the SPW to reset
internal state before the socket is reused for a different 
connection. This Reset() signal is sent by SPW to
SocketPerformanceWatcherFactory (SPWF), which is currently
implemented by NQE.

Most of the rest was just wiring it up through the Socket 
Pools - the transport_socket_pool got a new member to 
handle it, and so all the other ConnectJobs were updated to 
pass through the pool's SPWF through.

The rest is just boilerplate - updating the constructor to 
take a null SPW or SPWF, since we're not interested in the 
performance of any of those sockets right now.

[Ryan Sleevi, 2016-03-02 19:56:58]

This review is very large and has been going on for a very long time, so it's
hard for me to keep reviewing and offering good feedback.

I'm still concerned about the layering and dependencies here, which really gets
to the heart of the class design of the SocketPerformanceWatcher and the
Factory. That is, I feel that we've got it wrong, but getting it right is going
to be complex, and I'm not sure I'd be able to review it in a timely and
meaningful fashion if you did it in this CL.

My concerns for layering center around the unclear
abstraction-vs-concrete-interface of the SocketPerformanceWatcher and the
SocketPerformanceWatcherFactory, and how events are signaled between the two.

SocketPerformanceWatcher *SHOULD NOT* depend on SocketPerformanceWatcherFactory.
Further, if SocketPerformanceWatcherFactory is a pure-virtual interface (as it
is today), then it SHOULD NOT make guarantees about how/when methods are called.

However, resolving those concerns is tricky and complex, and to be honest, I've
had trouble wrapping my head around the code a few times.

Here's some design questions for you and Ben to noodle about, that may help
explain the concern:
1) Do you anticipate there ever being any other type of concrete
SocketPerformanceWatcher?
2) Does SocketPerformanceWatcher belong in //net/socket (my instinct is that it
probably does?)
3) Is SocketPerformanceWatcherFactory pure virtual to enable returning other
types of SocketPerformanceWatchers (FooSocketPerformanceWatcher)? Or is it
merely pure virtual to receive *notifications* from the concrete
SocketPerformanceWatcher?


Let's say that the answers were that SPWF is only pure-virtual to enable event
signaling, and SPW is the only concrete implementation. One way to solve the
design tension would be:

class SocketPerformanceWatcher {
 public:
  class Delegate {
   public:
    void OnInterestingEvent() = 0;
  };

  explicit SocketPerformanceWatcher(scoped_ptr<Delegate> delegate);

  void SignalInterestingEvent(...) {
    // Any special logic such as rate limiting here
    delegate_->OnInterestingEvent();
  }
};

class SocketPerformanceWatcherFactory {
 public:
  class Delegate {
   public:
    void OnSomethingInteresting(...) = 0;
  };

  explicit SocketPerformanceWatcherFactory(scoped_ptr<Delegate> delegate);

  scoped_ptr<SocketPerformanceWatcher> CreateFoo(...);

 private:
  friend class SomeHelperWithAGoodName;

  void OnInterestingEvent(...);
};

and then in the .cc
class SomeHelperWithAGoodName
    : public SocketPerformanceWatcher::Delegate {
 public:
  SomeHelperWithAGoodName(const WeakPtr<SocketPerformanceWatcherFactory>&
weak_factory);

  // SPW::Delegate implementation
  void OnInterestingEvent() {
    if (!weak_factory_.IsValid())
      return;
    weak_factory_->OnSomethingInteresting(...);
  }
};

void SocketPerformanceWatcherFactory::OnSomethingInteresting(...) {
  // Do aggregate bookkeeping if relevant to base class
  // or whatever else
  delegate_->OnSomethingInteresting(...);
}

scoped_ptr<SocketPerformanceWatcher>
SocketPerformanceWatcherFactory::CreateFoo(...) {
  // stuff
  return new SocketPerformanceWatcher(new
SomeHelperWithAGoodName(weak_factory_.get());
}


It really depends on:
- How much concrete logic is provided by the SPW
- How much concrete logic is provided by the SPWF
- Whether you see multiple classes coming into play

The double-delegate (SPW and SPWF) may be overkill, and it may be that the SPWF
just fowards an SPW::Delegate, and the SPW::Delegate may be NQE - I don't know,
because it really depends on how much logic is concrete for all instances and
how much is distinct.

But note that the above approach favors a core Chromium tenet - favor
composition over inheritance. Mixing pure-virtuals (or virtuals) with concrete
classes is generally a sign of an anti-pattern, which is why I've tried to
separate out the concrete behaviour from the delegated behaviours.

Note that in the above suggestion, SPW *never* directly depends on SPWF.
Instead, SPW interacts with SPWF via SPW::Delegate. That is, if you visualize
out the dependencies, rather than SPWF depending on SPW (to create) and SPW
depending on SPWF (to signal events), SPW depends on itself (::Delegate), and
SPWF depends on SPW (via ::Delegate) - breaking the dependency cycle.

https://codereview.chromium.org/1376473003/diff/240001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/240001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1147: for (auto observation :
rtt_observer.observations()) {
s/auto/const auto&/

https://codereview.chromium.org/1376473003/diff/240001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1158: for (auto observation :
rtt_observer.observations()) {
s/auto/const auto&/

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
File net/base/socket_performance_watcher.cc (right):

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
net/base/socket_performance_watcher.cc:42: void
SocketPerformanceWatcher::Reset() const {
Shouldn't you also reset last_rtt_notification_ to the default value of the
ctor?

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:38: // Returns true if |this| can be
notified of a new RTT value.
I'm not sure what this means?

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:42: // new transport layer connection.
https://google.github.io/styleguide/cppguide.html#Function_Comments

You should define the operation, not the use

// Resets the internal state of this SocketPerformanceWatcher in
// preparation for observing a new socket.
// Note: The new socket MUST share the same protocol as the previously
// observed socket.

[tbansal1, 2016-03-03 02:09:09]

I agree that the circular dependency between SPW and SPWF should be removed. I
also agree with the approach that you proposed (thanks for the details!), and
that we can probably get away with just one delegate class.

However, removing this dependency will require changes across other unrelated
files (net/quic/ etc.).  Is it okay if I do that in a separate CL? I did file
crbug.com/590265 to track this.

https://codereview.chromium.org/1376473003/diff/240001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/240001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1147: for (auto observation :
rtt_observer.observations()) {
On 2016/03/02 19:56:57, Ryan Sleevi wrote:
> s/auto/const auto&/

Done, also changed at one other place in this file.

https://codereview.chromium.org/1376473003/diff/240001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1158: for (auto observation :
rtt_observer.observations()) {
On 2016/03/02 19:56:57, Ryan Sleevi wrote:
> s/auto/const auto&/

Done.

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
File net/base/socket_performance_watcher.cc (right):

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
net/base/socket_performance_watcher.cc:42: void
SocketPerformanceWatcher::Reset() const {
On 2016/03/02 19:56:57, Ryan Sleevi wrote:
> Shouldn't you also reset last_rtt_notification_ to the default value of the
> ctor?

I was going to replace last_rtt_notification_ with a counter that counts how
many RTT notifications have been received so far. But, it makes sense to reset
the last_rtt_notification_ to the default value here for consistency.

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:38: // Returns true if |this| can be
notified of a new RTT value.
On 2016/03/02 19:56:57, Ryan Sleevi wrote:
> I'm not sure what this means?

I renamed the function name, and edited the comment to make it clearer.

https://codereview.chromium.org/1376473003/diff/240001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:42: // new transport layer connection.
On 2016/03/02 19:56:57, Ryan Sleevi wrote:
> https://google.github.io/styleguide/cppguide.html#Function_Comments
> 
> You should define the operation, not the use
> 
> // Resets the internal state of this SocketPerformanceWatcher in
> // preparation for observing a new socket.
> // Note: The new socket MUST share the same protocol as the previously
> // observed socket.

Done.

[Ryan Sleevi, 2016-03-04 01:31:20]

On 2016/03/03 02:09:09, tbansal1 wrote:
> I agree that the circular dependency between SPW and SPWF should be removed. I
> also agree with the approach that you proposed (thanks for the details!), and
> that we can probably get away with just one delegate class.
> 
> However, removing this dependency will require changes across other unrelated
> files (net/quic/ etc.).  Is it okay if I do that in a separate CL? I did file
> crbug.com/590265 to track this.

Yeah, I'm torn on whether we should do that work before this or after this.

[Ryan Sleevi, 2016-03-04 01:38:38]

LGTM % nit

https://codereview.chromium.org/1376473003/diff/260001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/260001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:39: bool ShouldNotifyUpdatedRTT() const
WARN_UNUSED_RESULT;
I still don't know how to make sense of this - or how to use it properly.

Are you saying that every call should be

if (ShouldNotifyUpdatedRTT())
  OnUpdatedRTTAvailable(...) ?

Or something else?

If that's correct (and it looks like it is)

// Returns |true| if the current SocketPerformanceWatcher is interested
// in receiving a new RTT estimate (via OnUpdatedRTTAvailable).
// Callers may use this to avoid doing expensive work computing the
// RTT when the SocketPerformanceWatcher is not interested in such
// updates.

That feels klunkily worded, but maybe that gets more to the "Here's how you can
hold this / why this exists" sort of explanation.

https://codereview.chromium.org/1376473003/diff/260001/net/base/socket_perfor...
File net/base/socket_performance_watcher_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/260001/net/base/socket_perfor...
net/base/socket_performance_watcher_unittest.cc:50: // Tests if
SocketPerformanceWatcher computes CanNotifyRTT() correctly.
Out of date comment (CanNotifyRTT)

https://codereview.chromium.org/1376473003/diff/260001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/260001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:732: "590254
TCPSocketPosix::NotifySocketPerformanceWatcher"));
Nit: You should move this to cover the full function, since that short-circuit
on 724-726 will affect performance too.

[tbansal1, 2016-03-04 02:37:54]

https://codereview.chromium.org/1376473003/diff/260001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/260001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:39: bool ShouldNotifyUpdatedRTT() const
WARN_UNUSED_RESULT;
On 2016/03/04 01:38:38, Ryan Sleevi - OOO until 3-10 wrote:
> I still don't know how to make sense of this - or how to use it properly.
> 
> Are you saying that every call should be
> 
> if (ShouldNotifyUpdatedRTT())
>   OnUpdatedRTTAvailable(...) ?
Yes, that's what I meant.
> 
> Or something else?
> 
> If that's correct (and it looks like it is)
> 
> // Returns |true| if the current SocketPerformanceWatcher is interested
> // in receiving a new RTT estimate (via OnUpdatedRTTAvailable).
> // Callers may use this to avoid doing expensive work computing the
> // RTT when the SocketPerformanceWatcher is not interested in such
> // updates.
> 
> That feels klunkily worded, but maybe that gets more to the "Here's how you
can
> hold this / why this exists" sort of explanation.
Done.

https://codereview.chromium.org/1376473003/diff/260001/net/base/socket_perfor...
File net/base/socket_performance_watcher_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/260001/net/base/socket_perfor...
net/base/socket_performance_watcher_unittest.cc:50: // Tests if
SocketPerformanceWatcher computes CanNotifyRTT() correctly.
On 2016/03/04 01:38:38, Ryan Sleevi - OOO until 3-10 wrote:
> Out of date comment (CanNotifyRTT)

Done.

https://codereview.chromium.org/1376473003/diff/260001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/260001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:732: "590254
TCPSocketPosix::NotifySocketPerformanceWatcher"));
On 2016/03/04 01:38:38, Ryan Sleevi - OOO until 3-10 wrote:
> Nit: You should move this to cover the full function, since that short-circuit
> on 724-726 will affect performance too.

Done.

[tbansal1, 2016-03-04 17:28:24]

Patchset #10 (id:300001) has been deleted

[tbansal1, 2016-03-04 17:33:22]

tbansal@chromium.org changed reviewers:
+ bbudge@chromium.org, pfeldman@chromium.org, wez@chromium.org, zea@chromium.org

[tbansal1, 2016-03-04 17:33:23]

PTAL:
pfeldman:  chrome/browser/devtools/*
zea: jingle/ and google_apis/gcm/
bbudge: content/browser/renderer_host/pepper/*
wez: extensions/browser/api/cast_channel/*

Thanks!

[Wez, 2016-03-04 18:50:27]

Given that socket performance watching is very much an optional add-on to the
socket, not something that all callers will find useful, you've ended up with a
lot of nullptr ctor parameters - have you considered instead having a
SetPerformanceWatcher() method on the socket API, so callers can add it after
construction, and thereby avoid polluting the call-sites that have no need of
this feature?

https://codereview.chromium.org/1376473003/diff/340001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/340001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:30: virtual ~SocketPerformanceWatcher();
Why is this dtor virtual?  Nothing seems to inherit from this, nor does it have
virtual methods.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/client_sock...
File net/socket/client_socket_factory.h (right):

https://codereview.chromium.org/1376473003/diff/340001/net/socket/client_sock...
net/socket/client_socket_factory.h:44: SocketPerformanceWatcherFactory*
socket_performance_watcher_factory,
Is there any reason to pass a *factory* in here, rather than
scoped_ptr<Watcher>? The latter would make lifetime/ownership clearer[1] and it
seems that all any implementation will do is take an instance from the factory
anyway.

[1] Since with your change this call has two bare-pointer parameters, with
different lifetime requirements.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/340001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:523: new TCPSocketPosix(nullptr,
net_log_.net_log(), net_log_.source()));
IIUC this means that you can't associated a socket-performance-watcher with a
server socket (one received via accept())?  If you passed the factory all the
way down to the TCPSocket ctor then you could allow that to work, since
BuildTcpSocketPosix could just create a new watcher instance.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:718: // Check if |socket_performance_watcher_|
can be notified of a RTT value.
nit: "can" -> "must" or reword to "...watcher_| is interested in..."

https://codereview.chromium.org/1376473003/diff/340001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:730: // SocketPerformanceWatcher in order to
maintain high accuracy.
nit: Not clear from the comment why omitting valid data improves accuracy.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/transport_c...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/1376473003/diff/340001/net/socket/transport_c...
net/socket/transport_client_socket_pool.h:164: SocketPerformanceWatcherFactory*
socket_performance_watcher_factory,
The comment on this class needs to clarify the lifetime/ownership requirements
of this (and the other bare-pointer parameters) that are supplied to the ctor.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/websocket_t...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/1376473003/diff/340001/net/socket/websocket_t...
net/socket/websocket_transport_client_socket_pool.cc:244: NULL,
nullptr

https://codereview.chromium.org/1376473003/diff/340001/net/socket/websocket_t...
File net/socket/websocket_transport_connect_sub_job.cc (right):

https://codereview.chromium.org/1376473003/diff/340001/net/socket/websocket_t...
net/socket/websocket_transport_connect_sub_job.cc:140: one_address, NULL,
net_log().net_log(), net_log().source());
nullptr

[bbudge, 2016-03-07 17:49:03]

content/browser/renderer_host/pepper LGTM

[Nicolas Zea, 2016-03-08 00:51:03]

Could you change all new NULL's to nullptrs? (recommended for all new code, see
http://chromium-cpp.appspot.com/)

[Ryan Sleevi, 2016-03-08 00:53:36]

On 2016/03/08 00:51:03, Nicolas Zea wrote:
> Could you change all new NULL's to nullptrs? (recommended for all new code,
see
> http://chromium-cpp.appspot.com/)

As a reviewer on this, I'd like to strongly push back on this request.

I think the right balance was struck with NULL for existing code, nullptr for
the new code, and nullptr where nullptr was the existing stance.

If this was new code (added files), that request is entirely reasonable. But we
also value _consistency_ with existing style when there's existing style and
both are permitted - as is the case of NULL v nullptr - and the places left as
NULL are done so to preserve consistency.

It would look very weird to see NULL, nullptr, NULL, in one line, and it'd be
very weird to see nullptr, nullptr, nullptr in one line and NULL, NULL, NULL in
another line in the same file.

[Nicolas Zea, 2016-03-08 01:08:26]

On 2016/03/08 00:53:36, Ryan Sleevi - OOO until 3-10 wrote:
> On 2016/03/08 00:51:03, Nicolas Zea wrote:
> > Could you change all new NULL's to nullptrs? (recommended for all new code,
> see
> > http://chromium-cpp.appspot.com/)
> 
> As a reviewer on this, I'd like to strongly push back on this request.
> 
> I think the right balance was struck with NULL for existing code, nullptr for
> the new code, and nullptr where nullptr was the existing stance.
> 
> If this was new code (added files), that request is entirely reasonable. But
we
> also value _consistency_ with existing style when there's existing style and
> both are permitted - as is the case of NULL v nullptr - and the places left as
> NULL are done so to preserve consistency.
> 
> It would look very weird to see NULL, nullptr, NULL, in one line, and it'd be
> very weird to see nullptr, nullptr, nullptr in one line and NULL, NULL, NULL
in
> another line in the same file.

I'm not sure seeing nullptr's on one line, and NULLs on another line is that
strange (any more than seeing auto's in one function, but not in another is that
strange). But if you already had that discussion, I'm fine with favoring
consistency. gcm/jingle LGTM then.

[tbansal1, 2016-03-08 22:51:51]

Patchset #12 (id:360001) has been deleted

[tbansal1, 2016-03-08 22:55:03]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()
-> TransportConnectJob::CreateTransportClientSocket
-> DefaultClientSocketFactory::CreateTransportClientSocket()

CreateTransportClientSocket() creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the TCPClientSocket, which passes it to
TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-03-09 00:08:56]

Patchset #12 (id:380001) has been deleted

[tbansal1, 2016-03-09 00:20:03]

On 2016/03/04 18:50:27, Wez wrote:
> Given that socket performance watching is very much an optional add-on to the
> socket, not something that all callers will find useful, you've ended up with
a
> lot of nullptr ctor parameters - have you considered instead having a
> SetPerformanceWatcher() method on the socket API, so callers can add it after
> construction, and thereby avoid polluting the call-sites that have no need of
> this feature?
If I added a Set*() method, it would be added to the StreamSocket class. That
class is implemented by 21 other classes. So, it would not simplify things by a
lot. Also, the plumbing up to the socket factory layer would still remain the
same.

Passing the watcher in the ctor also makes it easier to add watcher to FTP
requests/WebSockets/Extensions etc.

https://codereview.chromium.org/1376473003/diff/340001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/340001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:30: virtual ~SocketPerformanceWatcher();
On 2016/03/04 18:50:26, Wez wrote:
> Why is this dtor virtual?  Nothing seems to inherit from this, nor does it
have
> virtual methods.

Done.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/client_sock...
File net/socket/client_socket_factory.h (right):

https://codereview.chromium.org/1376473003/diff/340001/net/socket/client_sock...
net/socket/client_socket_factory.h:44: SocketPerformanceWatcherFactory*
socket_performance_watcher_factory,
On 2016/03/04 18:50:26, Wez wrote:
> Is there any reason to pass a *factory* in here, rather than
> scoped_ptr<Watcher>? The latter would make lifetime/ownership clearer[1] and
it
> seems that all any implementation will do is take an instance from the factory
> anyway.
Right, done.
> 
> [1] Since with your change this call has two bare-pointer parameters, with
> different lifetime requirements.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/340001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:523: new TCPSocketPosix(nullptr,
net_log_.net_log(), net_log_.source()));
On 2016/03/04 18:50:26, Wez wrote:
> IIUC this means that you can't associated a socket-performance-watcher with a
> server socket (one received via accept())?
SocketPerformanceWatcher only expects RTT values from connections that are
initiated by Chrome since these connections are more likely to be from Chrome to
a web server (that's what Socket Performance Watcher is interested in).
>  If you passed the factory all the
> way down to the TCPSocket ctor then you could allow that to work, since
> BuildTcpSocketPosix could just create a new watcher instance.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:718: // Check if |socket_performance_watcher_|
can be notified of a RTT value.
On 2016/03/04 18:50:26, Wez wrote:
> nit: "can" -> "must" or reword to "...watcher_| is interested in..."

Done.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:730: // SocketPerformanceWatcher in order to
maintain high accuracy.
On 2016/03/04 18:50:26, Wez wrote:
> nit: Not clear from the comment why omitting valid data improves accuracy.

I expanded the comment to make it clearer.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/websocket_t...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/1376473003/diff/340001/net/socket/websocket_t...
net/socket/websocket_transport_client_socket_pool.cc:244: NULL,
On 2016/03/04 18:50:26, Wez wrote:
> nullptr

I am going to leave it as NULL since this file using NULL elsewhere at 2 places.

https://codereview.chromium.org/1376473003/diff/340001/net/socket/websocket_t...
File net/socket/websocket_transport_connect_sub_job.cc (right):

https://codereview.chromium.org/1376473003/diff/340001/net/socket/websocket_t...
net/socket/websocket_transport_connect_sub_job.cc:140: one_address, NULL,
net_log().net_log(), net_log().source());
On 2016/03/04 18:50:26, Wez wrote:
> nullptr

Done.

[Wez, 2016-03-09 01:19:27]

On 2016/03/09 00:20:03, tbansal1 wrote:
> On 2016/03/04 18:50:27, Wez wrote:
> > Given that socket performance watching is very much an optional add-on to
the
> > socket, not something that all callers will find useful, you've ended up
with
> a
> > lot of nullptr ctor parameters - have you considered instead having a
> > SetPerformanceWatcher() method on the socket API, so callers can add it
after
> > construction, and thereby avoid polluting the call-sites that have no need
of
> > this feature?
> If I added a Set*() method, it would be added to the StreamSocket class. That
> class is implemented by 21 other classes. So, it would not simplify things by
a
> lot. Also, the plumbing up to the socket factory layer would still remain the
> same.

It could be a default no-op setter on the base-class, though, similar to the API
that enables TCP Fast Open, for example.

Anyway, not a blocker!  cast_channel/ LGTM :)

[tbansal1, 2016-03-09 06:25:26]

Patchset #13 (id:420001) has been deleted

[tbansal1, 2016-03-09 07:12:28]

Patchset #13 (id:440001) has been deleted

[tbansal1, 2016-03-09 07:22:47]

Patchset #13 (id:460001) has been deleted

[tbansal1, 2016-03-09 07:49:12]

Patchset #13 (id:480001) has been deleted

[bengr, 2016-03-09 17:58:04]

LGTM with a couple of nits on comments.

https://codereview.chromium.org/1376473003/diff/500001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1376473003/diff/500001/net/base/network_quali...
net/base/network_quality_estimator.cc:917: // Nothing needs to be done for RTT
observations since NetworkQualityEstimator
What about for packet loss and bandwidth? Does the watcher not apply to those
metrics?

https://codereview.chromium.org/1376473003/diff/500001/net/base/socket_perfor...
File net/base/socket_performance_watcher.cc (right):

https://codereview.chromium.org/1376473003/diff/500001/net/base/socket_perfor...
net/base/socket_performance_watcher.cc:37: // Currently using a small value
until crbug.com/590300 is fixed.
Could you elaborate in this comment?

[tbansal1, 2016-03-09 18:14:21]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423
TBR=rockot@chromium.org, pfeldman@chromium.org
==========

[tbansal1, 2016-03-09 18:15:06]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423
TBR=rockot@chromium.org, pfeldman@chromium.org
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423
==========

[tbansal1, 2016-03-09 19:13:12]

https://codereview.chromium.org/1376473003/diff/500001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1376473003/diff/500001/net/base/network_quali...
net/base/network_quality_estimator.cc:917: // Nothing needs to be done for RTT
observations since NetworkQualityEstimator
On 2016/03/09 17:58:04, bengr wrote:
> What about for packet loss and bandwidth? Does the watcher not apply to those
> metrics?

Not right now. I will need to do something once I start using TCP watcher for
packet loss or bandwidth.

https://codereview.chromium.org/1376473003/diff/500001/net/base/socket_perfor...
File net/base/socket_performance_watcher.cc (right):

https://codereview.chromium.org/1376473003/diff/500001/net/base/socket_perfor...
net/base/socket_performance_watcher.cc:37: // Currently using a small value
until crbug.com/590300 is fixed.
On 2016/03/09 17:58:04, bengr wrote:
> Could you elaborate in this comment?

Done.

[tbansal1, 2016-03-09 19:45:51]

Patchset #14 (id:520001) has been deleted

[tbansal1, 2016-03-10 00:12:24]

Patchset #14 (id:540001) has been deleted

[tbansal1, 2016-03-10 03:19:54]

Patchset #15 (id:580001) has been deleted

[tbansal1, 2016-03-10 19:23:26]

Patchset #16 (id:620001) has been deleted

[tbansal1, 2016-03-10 21:53:11]

Patchset #16 (id:640001) has been deleted

[tbansal1, 2016-03-10 21:53:22]

Patchset #15 (id:600001) has been deleted

[tbansal1, 2016-03-11 21:48:57]

Patchset #16 (id:680001) has been deleted

[tbansal1, 2016-03-11 21:51:13]

Patchset #15 (id:660001) has been deleted

[Ryan Sleevi, 2016-03-21 23:25:33]

https://codereview.chromium.org/1376473003/diff/700001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1376473003/diff/700001/net/base/network_quali...
net/base/network_quality_estimator.h:236: private:
Unrelated to this CL but mentioning it because the "Right people" are on this
CL.

1) This class is arguably way too large to be reasonable at this point. It seems
there's quite a bit that could be hidden either in the implementation or which
should be separated out.
2) This is way too many FRIEND_TEST_ALL_PREFIXES. You should just friend the
fixture, and whatever private methods your tests are accessing would be
handled/abstracted through that fixture, which loosens the coupling AND makes
this more readable.

https://codereview.chromium.org/1376473003/diff/700001/net/base/network_quali...
net/base/network_quality_estimator.h:527: base::WeakPtr<NetworkQualityEstimator>
GetWeakPtr();
DESIGN: No external class should *ever* get access to your classes WeakPtr as a
WeakPtr. It should always be encapsulated via callback binding / handled by this
class, never by the other class (SPW)

https://codereview.chromium.org/1376473003/diff/700001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/700001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:35: socket_performance_watcher_factory,
DESIGN: You should *NEVER* pass a WeakPtr<Foo> to anything but a
base::Callback<>

[tbansal1, 2016-03-23 20:01:33]

sleevi: ptal. Thanks.
Most of the meaty changes are in socket_performance_watcher.cc.

Some of the code has been moved out of network_quality_estimator.cc to 
net/base/network_quality_estimator_watcher_factory.cc.
- NQE no longer implements SPWF. That's now done by nqe_watcher_factory.

Rest are mechanical changes. e.g.:
- Updated tests in quic_network_transaction_unittest.cc
- Updated profile_io_data.cc to pass nqe_watcher_factory

https://codereview.chromium.org/1376473003/diff/700001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1376473003/diff/700001/net/base/network_quali...
net/base/network_quality_estimator.h:527: base::WeakPtr<NetworkQualityEstimator>
GetWeakPtr();
On 2016/03/21 23:25:32, Ryan Sleevi wrote:
> DESIGN: No external class should *ever* get access to your classes WeakPtr as
a
> WeakPtr. It should always be encapsulated via callback binding / handled by
this
> class, never by the other class (SPW)

Done.

https://codereview.chromium.org/1376473003/diff/700001/net/base/socket_perfor...
File net/base/socket_performance_watcher.h (right):

https://codereview.chromium.org/1376473003/diff/700001/net/base/socket_perfor...
net/base/socket_performance_watcher.h:35: socket_performance_watcher_factory,
On 2016/03/21 23:25:32, Ryan Sleevi wrote:
> DESIGN: You should *NEVER* pass a WeakPtr<Foo> to anything but a
> base::Callback<>

Done.

[tbansal1, 2016-03-24 16:24:45]

Patchset #16 (id:720001) has been deleted

[scheib, 2016-03-24 18:10:25]

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

[scheib, 2016-03-24 18:10:27]

device/bluetooth/* LGTM

Nit on constructor:

https://codereview.chromium.org/1376473003/diff/740001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1376473003/diff/740001/net/socket/tcp_client_...
net/socket/tcp_client_socket.h:33: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher,
Naming or comments should indicate that socket_performance_watcher is optional.

[Ryan Sleevi, 2016-03-25 01:45:34]

At this point, I feel I'm struggling to effectively review this. Rebasing in the
midst of a patchset addressing feedback makes it extremely difficult for your
reviewers to review what you're changing. In the future, please do not rebase
and change stuff in the same patchset; generally, address the feedback, upload,
rebase, upload (this allows side-by-side comments to continue to work)

As there's been a separate email thread going back and forth on this, would it
be possible for you to include what your next steps are, so that the design
smells being addressed in a *series* of smaller CLs can be addressed, and
include them on the CL as just a reply (e.g. no need for the description update,
just trying to make sure we're on the same page)?

The reason I bring this up is I'm struggling to understand the rationale of some
of the design changes in Patchset 16, and whether they're things you plan to fix
immediately or not. I've tried to highlight the chief concern, which is
understanding how the NetworkQualityEstimatorWatcherFactory (which is way more a
mouthful than the WatcherHelper inner class I suggested on email) fits, why it's
a separate file, and why the new virtual factory function thing exists, when it
doesn't seem used.

Given that this CL has gone well beyond the trivial change it started out on, I
do want to stress that my strong tendencies are to 'Not-LG' this and work on
splitting it up into smaller CLs that can be reviewed, but I also know how
frustrating that can be at what feels like the 95% line. It just feels that with
every patchset, this code is getting harder to read, understand, and review -
and that's concerning.

https://codereview.chromium.org/1376473003/diff/700001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1376473003/diff/700001/net/base/network_quali...
net/base/network_quality_estimator.cc:18: #include
"base/thread_task_runner_handle.h"
It seems like a bug that you removed this, given line 173-174?

https://codereview.chromium.org/1376473003/diff/700001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1376473003/diff/700001/net/base/network_quali...
net/base/network_quality_estimator.h:527: base::WeakPtr<NetworkQualityEstimator>
GetWeakPtr();
On 2016/03/23 20:01:33, tbansal1 wrote:
> On 2016/03/21 23:25:32, Ryan Sleevi wrote:
> > DESIGN: No external class should *ever* get access to your classes WeakPtr
as
> a
> > WeakPtr. It should always be encapsulated via callback binding / handled by
> this
> > class, never by the other class (SPW)
> 
> Done.

I'm not sure what was done? In Patchset 16, you still haven't addressed this.

https://codereview.chromium.org/1376473003/diff/740001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1376473003/diff/740001/net/base/network_quali...
net/base/network_quality_estimator.h:233: // notification from a socket
performance watcher.
This comment is leaking the abstraction details into the header; it's especially
at conflict with the comments on 180-182 (which implies a different factory can
be set, which may not call this method)

https://codereview.chromium.org/1376473003/diff/740001/net/base/network_quali...
net/base/network_quality_estimator.h:239: // Allows
|NetworkQualityEstimatorWatcherFactory| to call protected functions.
You can drop this comment. Per
https://google.github.io/styleguide/cppguide.html#Implementation_Comments , this
is the "Do not state the obvious" aspect, as that's given to a reader who
understands C++ well.

https://codereview.chromium.org/1376473003/diff/740001/net/base/network_quali...
File net/base/network_quality_estimator_watcher_factory.h (right):

https://codereview.chromium.org/1376473003/diff/740001/net/base/network_quali...
net/base/network_quality_estimator_watcher_factory.h:26: class
NET_EXPORT_PRIVATE NetworkQualityEstimatorWatcherFactory
Why is this a separate class? That is, why isn't it a helper scoped solely to
the implementation of the NQE?

[Ryan Sleevi, 2016-03-25 20:53:02]

To follow-up: It looks like you've taken the good path of splitting this CL into
several smaller ones, which will hopefully make it easier to review the meat,
and leave the existing reviewer's mechanical LGTMs in place.

1) Splitting out a CL that fixes the cyclic dependency issues between NQE & SPW
2) Splitting out a CL that implements the major logic (including necessary
thread-safety aspects), but doesn't actually do the mechanical wiring up
3) Rebasing this CL on #1/#2 so that the "Plumb the SPW down" works as expected.

It looks like https://codereview.chromium.org/1831383002/ may be #2 (or might be
#1 + #2), and so we'll follow-up on that there, and once those reviews are done,
we can double check with your existing reviewers on carrying forward this CL as
the mechanical wiring CL.

[tbansal1, 2016-04-11 16:52:15]

Patchset #18 (id:780001) has been deleted

[tbansal1, 2016-04-11 16:57:04]

Patchset #18 (id:800001) has been deleted

[tbansal1, 2016-04-11 17:17:36]

Patchset #18 (id:820001) has been deleted

[tbansal1, 2016-04-11 17:17:45]

Patchset #18 (id:820001) has been deleted

[tbansal1, 2016-04-11 17:41:45]

Patchset #18 (id:840001) has been deleted

[tbansal1, 2016-04-11 17:57:58]

Patchset #19 (id:880001) has been deleted

[tbansal1, 2016-04-11 17:59:07]

rsleevi: PTAL! I have rebased (PS # 17), and added notification throttling (PS #
18) to the TCP sockets. Now, a socket notifies the watcher at most once every
second. Thanks. 

Some other changes:
* Moved the NotifySocketPerformanceWatcher() call in TCPSocketPosix to
Handle(Read|Write|Connect)Completed() methods because those methods are called
if the (Read|Write|Connect) calls finish synchronously.
* Guarded the tests with additional OS_LINUX test because the tests were not
running on my desktop.

https://codereview.chromium.org/1376473003/diff/740001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.h (right):

https://codereview.chromium.org/1376473003/diff/740001/net/socket/tcp_client_...
net/socket/tcp_client_socket.h:33: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher,
On 2016/03/24 18:10:27, scheib wrote:
> Naming or comments should indicate that socket_performance_watcher is
optional.

Done.

[Ryan Sleevi, 2016-04-11 22:06:57]

Most of my main objections were resolved in the other CLs, so thanks for taking
those on.

What remains is mostly nits about the tests.

https://codereview.chromium.org/1376473003/diff/860001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1154: const size_t kNumRequests =
2;
Doesn't seem like you need to use a constant here, especially since it's coupled
with the comment on 1156/1157

for (size_t i = 0; i < 2; ++i) {

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:27: socket_(new
TCPSocket(std::move(socket_performance_watcher),
This is an intrinsically dangerous pattern (the .get() plus the move()), because
it can complicate reasoning about lifetimes.

Let's discuss this separately, to see if we can't work out an approach. I'm fine
with this for now, however, in this limited case.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:46:
notify_reset_socket_performance_watcher_(false) {
This is really just

is_first_connection_ isn't it?

Is there any other time or reasons we wouldn't notify? The way it's named now
hides that relationship, so perhaps this would make it clearer.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
File net/socket/tcp_client_socket_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket_unittest.cc:142: const size_t kNumIPs = 2;
Seems like this doesn't need to be a constant, given the test. Why would you
change this (to 3, 17, or 63?)

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket_unittest.cc:145: ip_list.push_back(IPAddress(72,
14, 213, 105));
Seems like you should make these different IPs to be more reflective of a 'real'
test case.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket_unittest.cc:160: result =
connect_callback.WaitForResult();
This three-line pattern is a bit of an anti-pattern we've been trying to
correct.

int result =
connect_callback.GetResult(socket.Connect(connect_callback.callback()));

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_server_...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_server_...
net/socket/tcp_server_socket.cc:18: : socket_(NULL, net_log, source),
pending_accept_(false) {}
nullptr

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:487: tick_clock_.swap(tick_clock);
I believe this should be

tick_clock_.swap(std::move(tick_clock));

since tick_clock is named and thus may not be inferred as an rvalue reference.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.h (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.h:219: const base::TimeDelta
rtt_notifications_minimum_interval_;
This seems like a very odd ordering to add this member.

Wouldn't it make more sense between 228 and 232?

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:146: connecting_socket.Connect(local_address_,
connect_callback.callback());
You never seem to check the result here? Seems like it should be around line 154

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:165: // Use a 1 byte message so that it is
written and read in one attempt.
There's no guarantee that this statement will apply.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:167: std::vector<char> buffer(message.size());
This just seems wasteful; why not

std::vector<char> buffer('t', 1); ?

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:168: ASSERT_EQ(1u, message.size());
Seems... unnecessary?

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:179: write_result =
write_callback.GetResult(write_result);
Combine
write_callback.GetResult(accepted_socket->Write(...));

Also, not guaranteed it will synchronously complete. You should order it as

- Start async write operation
- Start async read operation
- Ensure both are completed and have results.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:180: ASSERT_TRUE(write_result >= 0);
ASSERT_GE

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:187: new IOBufferWithSize(message.size() -
bytes_read));
bytes_read is always 0 (see line 185)

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:192: ASSERT_TRUE(read_result >= 0);
GE

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:193: ASSERT_TRUE(bytes_read + read_result <=
message.size());
LE

buffer.size()

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:194: memmove(&buffer[bytes_read],
read_buffer->data(), read_result);
Seems unnecessary

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:195: bytes_read += read_result;
Seems unnecessary given the above?

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:198: std::string
received_message(buffer.begin(), buffer.end());
You copy back into a string, which is also unnecessary, given that vector has an
operator== defined.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_win.h (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_win.h:135: // |socket_performance_watcher_|. May be
nullptr.
The first sentence doesn't add value; that's documenting
SocketPerformanceWatcher.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_win.h:136: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher_;
Given that this isn't used, this seems pointless to store for now. The only
reason I can conceive is to keep the lifetime tied to the socket object, but is
that part of the explicit API contract? If so, document it as such.

[tbansal1, 2016-04-12 01:25:30]

Patchset #19 (id:900001) has been deleted

[tbansal1, 2016-04-12 16:14:34]

rseleevi: ptal. Thanks.

https://codereview.chromium.org/1376473003/diff/860001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:1154: const size_t kNumRequests =
2;
On 2016/04/11 22:06:55, Ryan Sleevi wrote:
> Doesn't seem like you need to use a constant here, especially since it's
coupled
> with the comment on 1156/1157
> 
> for (size_t i = 0; i < 2; ++i) {

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:27: socket_(new
TCPSocket(std::move(socket_performance_watcher),
On 2016/04/11 22:06:55, Ryan Sleevi wrote:
> This is an intrinsically dangerous pattern (the .get() plus the move()),
because
> it can complicate reasoning about lifetimes.
> 
> Let's discuss this separately, to see if we can't work out an approach. I'm
fine
> with this for now, however, in this limited case.

I agree. I am not sure what's the best approach here but another possible
solution is to have the |socket_| expose the watcher (this will possibly need
friend'ing TCPClientSocket).

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:46:
notify_reset_socket_performance_watcher_(false) {
On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> This is really just
> 
> is_first_connection_ isn't it?
> 
> Is there any other time or reasons we wouldn't notify? The way it's named now
> hides that relationship, so perhaps this would make it clearer.
Removed the variable.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
File net/socket/tcp_client_socket_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket_unittest.cc:142: const size_t kNumIPs = 2;
On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> Seems like this doesn't need to be a constant, given the test. Why would you
> change this (to 3, 17, or 63?)

I kept it as a constant so that the relation between # of IPs pushed (Line 145)
and the number of connection notifications (Line 164) is clearer.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket_unittest.cc:145: ip_list.push_back(IPAddress(72,
14, 213, 105));
On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> Seems like you should make these different IPs to be more reflective of a
'real'
> test case.

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket_unittest.cc:160: result =
connect_callback.WaitForResult();
On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> This three-line pattern is a bit of an anti-pattern we've been trying to
> correct.
> 
> int result =
> connect_callback.GetResult(socket.Connect(connect_callback.callback()));
> 

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_server_...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_server_...
net/socket/tcp_server_socket.cc:18: : socket_(NULL, net_log, source),
pending_accept_(false) {}
On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.cc:487: tick_clock_.swap(tick_clock);
On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> I believe this should be
> 
> tick_clock_.swap(std::move(tick_clock));
This gave compilation error:
error: non-const lvalue reference to type 'unique_ptr<...>' cannot bind to a
temporary of type 'unique_ptr<...>'
> 
> since tick_clock is named and thus may not be inferred as an rvalue reference.

Done. I updated it to how this is done elsewhere in //net
https://code.google.com/p/chromium/codesearch#chromium/src/net/sdch/sdch_owne...

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_posix.h (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_posix.h:219: const base::TimeDelta
rtt_notifications_minimum_interval_;
On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> This seems like a very odd ordering to add this member.
> 
> Wouldn't it make more sense between 228 and 232?

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:146: connecting_socket.Connect(local_address_,
connect_callback.callback());
On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> You never seem to check the result here? Seems like it should be around line
154
Not sure if I understand the comment.

Result is checked below (Connect never succeeds synchronously) on Line 160:
EXPECT_EQ(OK, connect_callback.WaitForResult());

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:165: // Use a 1 byte message so that it is
written and read in one attempt.
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> There's no guarantee that this statement will apply.

I do not understand this comment. 

Is it possible for a 1 byte message to be broken down to smaller messages by
TCP? Also, since the buffer is empty, the message should be written. Right?

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:167: std::vector<char> buffer(message.size());
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> This just seems wasteful; why not
> 
> std::vector<char> buffer('t', 1); ?

I simplified the test significantly.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:168: ASSERT_EQ(1u, message.size());
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> Seems... unnecessary?

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:179: write_result =
write_callback.GetResult(write_result);
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> Combine
> write_callback.GetResult(accepted_socket->Write(...));
> 
> Also, not guaranteed it will synchronously complete. You should order it as
> 
> - Start async write operation
> - Start async read operation
> - Ensure both are completed and have results.

This test is similar to "ReadWrite" below. Also, would not GetResult() block
until the data has been written to the socket?

Since the socket write buffer is empty, what else could be the reason for this
to not succeed?

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:180: ASSERT_TRUE(write_result >= 0);
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> ASSERT_GE

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:187: new IOBufferWithSize(message.size() -
bytes_read));
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> bytes_read is always 0 (see line 185)

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:192: ASSERT_TRUE(read_result >= 0);
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> GE

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:193: ASSERT_TRUE(bytes_read + read_result <=
message.size());
On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> LE
> 
> buffer.size()

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:194: memmove(&buffer[bytes_read],
read_buffer->data(), read_result);
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> Seems unnecessary

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:195: bytes_read += read_result;
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> Seems unnecessary given the above?

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:198: std::string
received_message(buffer.begin(), buffer.end());
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> You copy back into a string, which is also unnecessary, given that vector has
an
> operator== defined.

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_win.h (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_win.h:135: // |socket_performance_watcher_|. May be
nullptr.
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> The first sentence doesn't add value; that's documenting
> SocketPerformanceWatcher.

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_win.h:136: scoped_ptr<SocketPerformanceWatcher>
socket_performance_watcher_;
On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> Given that this isn't used, this seems pointless to store for now. The only
> reason I can conceive is to keep the lifetime tied to the socket object, but
is
> that part of the explicit API contract? If so, document it as such.

Added comment in tcp_client_socket.h

[Ryan Sleevi, 2016-04-12 21:08:32]

I'm going to go ahead and re-give an LGTM, but one thing that MUST be fixed
before landing.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:27: socket_(new
TCPSocket(std::move(socket_performance_watcher),
On 2016/04/12 16:14:33, tbansal1 wrote:
> On 2016/04/11 22:06:55, Ryan Sleevi wrote:
> > This is an intrinsically dangerous pattern (the .get() plus the move()),
> because
> > it can complicate reasoning about lifetimes.
> > 
> > Let's discuss this separately, to see if we can't work out an approach. I'm
> fine
> > with this for now, however, in this limited case.
> 
> I agree. I am not sure what's the best approach here but another possible
> solution is to have the |socket_| expose the watcher (this will possibly need
> friend'ing TCPClientSocket).

Friending is almost always the wrong decision. Certainly, if it isn't test code,
and it isn't an implementation detail, they shouldn't be friends. Anyways, we
should follow-up separately.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:146: connecting_socket.Connect(local_address_,
connect_callback.callback());
On 2016/04/12 16:14:33, tbansal1 wrote:
> On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> > You never seem to check the result here? Seems like it should be around line
> 154
> Not sure if I understand the comment.
> 
> Result is checked below (Connect never succeeds synchronously) on Line 160:
> EXPECT_EQ(OK, connect_callback.WaitForResult());

Ah, I missed that.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:160: EXPECT_EQ(OK,
connect_callback.WaitForResult());
Should it be an ASSERT?

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:165: // Use a 1 byte message so that it is
written and read in one attempt.
On 2016/04/12 16:14:34, tbansal1 wrote:
> Is it possible for a 1 byte message to be broken down to smaller messages by
> TCP?

It's possible that you'll get 0 bytes written with an asynchronous completion.

> Also, since the buffer is empty, the message should be written. Right?

No, that's not guaranteed here. That is, don't depend/assume on kernel buffering
semantics (we've had other tests that failed here)

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:179: write_result =
write_callback.GetResult(write_result);
On 2016/04/12 16:14:34, tbansal1 wrote:
> On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> > Combine
> > write_callback.GetResult(accepted_socket->Write(...));
> > 
> > Also, not guaranteed it will synchronously complete. You should order it as
> > 
> > - Start async write operation
> > - Start async read operation
> > - Ensure both are completed and have results.
> 
> This test is similar to "ReadWrite" below. Also, would not GetResult() block
> until the data has been written to the socket?
> 
> Since the socket write buffer is empty, what else could be the reason for this
> to not succeed?

You can't assume that the underlying implementation will do this. That's what is
"not guaranteed"

For example, Windows implements local sockets (127.0.0.1) as a series of named
pipes, and the first write may not complete until there's also a read operation
enqueued. While I realize this is "Linux Only", it's about writing to the
interface. There's no guarantee that the kernel buffer won't be, say, 0 bytes -
so you need to handle it as an asynchronous operation, and not just assume
synchronicity.

https://codereview.chromium.org/1376473003/diff/920001/net/socket/tcp_socket_...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/920001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:182: read_result =
read_callback.GetResult(read_result);
You combined 173-174 but not 180-182. We really do want consistency.

To be explicit on structure

TestCompletionCallback write_callback;
int write_result = accepted_socket->Write(...);

scoped_refptr<IOBufferWithSize> read_buffer(...);
TestCompletionCallback read_callback;
int read_result = connecting_socket.Read(...);

ASSERT_EQ(1, write_callback.GetResult(write_result));
ASSERT_EQ(1, read_callback.GetResult(read_result));

[tbansal1, 2016-04-12 22:39:17]

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

[tbansal1, 2016-04-12 22:39:19]

reillyg: PTAL at chrome/browser/*. Thanks.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:27: socket_(new
TCPSocket(std::move(socket_performance_watcher),
On 2016/04/12 21:08:31, Ryan Sleevi wrote:
> On 2016/04/12 16:14:33, tbansal1 wrote:
> > On 2016/04/11 22:06:55, Ryan Sleevi wrote:
> > > This is an intrinsically dangerous pattern (the .get() plus the move()),
> > because
> > > it can complicate reasoning about lifetimes.
> > > 
> > > Let's discuss this separately, to see if we can't work out an approach.
I'm
> > fine
> > > with this for now, however, in this limited case.
> > 
> > I agree. I am not sure what's the best approach here but another possible
> > solution is to have the |socket_| expose the watcher (this will possibly
need
> > friend'ing TCPClientSocket).
> 
> Friending is almost always the wrong decision. Certainly, if it isn't test
code,
> and it isn't an implementation detail, they shouldn't be friends. Anyways, we
> should follow-up separately.

Acknowledged.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:146: connecting_socket.Connect(local_address_,
connect_callback.callback());
On 2016/04/12 21:08:31, Ryan Sleevi wrote:
> On 2016/04/12 16:14:33, tbansal1 wrote:
> > On 2016/04/11 22:06:56, Ryan Sleevi wrote:
> > > You never seem to check the result here? Seems like it should be around
line
> > 154
> > Not sure if I understand the comment.
> > 
> > Result is checked below (Connect never succeeds synchronously) on Line 160:
> > EXPECT_EQ(OK, connect_callback.WaitForResult());
> 
> Ah, I missed that.

Acknowledged.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:160: EXPECT_EQ(OK,
connect_callback.WaitForResult());
On 2016/04/12 21:08:31, Ryan Sleevi wrote:
> Should it be an ASSERT?

Done.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:165: // Use a 1 byte message so that it is
written and read in one attempt.
On 2016/04/12 21:08:31, Ryan Sleevi wrote:
> On 2016/04/12 16:14:34, tbansal1 wrote:
> > Is it possible for a 1 byte message to be broken down to smaller messages by
> > TCP?
> 
> It's possible that you'll get 0 bytes written with an asynchronous completion.
> 
> > Also, since the buffer is empty, the message should be written. Right?
> 
> No, that's not guaranteed here. That is, don't depend/assume on kernel
buffering
> semantics (we've had other tests that failed here)

Acknowledged. I have rewritten the comment.

https://codereview.chromium.org/1376473003/diff/860001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:179: write_result =
write_callback.GetResult(write_result);
On 2016/04/12 21:08:31, Ryan Sleevi wrote:
> On 2016/04/12 16:14:34, tbansal1 wrote:
> > On 2016/04/11 22:06:57, Ryan Sleevi wrote:
> > > Combine
> > > write_callback.GetResult(accepted_socket->Write(...));
> > > 
> > > Also, not guaranteed it will synchronously complete. You should order it
as
> > > 
> > > - Start async write operation
> > > - Start async read operation
> > > - Ensure both are completed and have results.
> > 
> > This test is similar to "ReadWrite" below. Also, would not GetResult() block
> > until the data has been written to the socket?
> > 
> > Since the socket write buffer is empty, what else could be the reason for
this
> > to not succeed?
> 
> You can't assume that the underlying implementation will do this. That's what
is
> "not guaranteed"
> 
> For example, Windows implements local sockets (127.0.0.1) as a series of named
> pipes, and the first write may not complete until there's also a read
operation
> enqueued. While I realize this is "Linux Only", it's about writing to the
> interface. There's no guarantee that the kernel buffer won't be, say, 0 bytes
-
> so you need to handle it as an asynchronous operation, and not just assume
> synchronicity.

Thanks. I did not know that some operating systems may special case local
sockets and may not write to socket until a read operation is enqueued. That
sounds broken because on real networks there is no way to know if a read
operation is enqueued or not on the remote endpoint.

https://codereview.chromium.org/1376473003/diff/920001/net/socket/tcp_socket_...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/1376473003/diff/920001/net/socket/tcp_socket_...
net/socket/tcp_socket_unittest.cc:182: read_result =
read_callback.GetResult(read_result);
On 2016/04/12 21:08:32, Ryan Sleevi wrote:
> You combined 173-174 but not 180-182. We really do want consistency.
> 
> To be explicit on structure
> 
> TestCompletionCallback write_callback;
> int write_result = accepted_socket->Write(...);
> 
> scoped_refptr<IOBufferWithSize> read_buffer(...);
> TestCompletionCallback read_callback;
> int read_result = connecting_socket.Read(...);
> 
> ASSERT_EQ(1, write_callback.GetResult(write_result));
> ASSERT_EQ(1, read_callback.GetResult(read_result));

Done.

[tbansal1, 2016-04-12 22:41:28]

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

[tbansal1, 2016-04-12 22:41:29]

frankf: PTAL at chrome/test/*. Thanks.

[Reilly Grant (use Gerrit), 2016-04-13 03:23:45]

//*/extensions/* and //device/bluetooth lgtm

[tbansal1, 2016-04-13 17:52:34]

Patchset #21 (id:960001) has been deleted

[tbansal1, 2016-04-13 20:22:25]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org,frankf@chromium.org
==========

[tbansal1, 2016-04-13 20:23:29]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org,frankf@chromium.org
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org,frankf@chromium.org
(for chrome/browser/devtools/device/* and 
chrome/test/chromedriver/net/* since the changes are
mechanical).
==========

[tbansal1, 2016-04-13 20:24:00]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org,frankf@chromium.org
(for chrome/browser/devtools/device/* and 
chrome/test/chromedriver/net/* since the changes are
mechanical).
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org, frankf@chromium.org
(for chrome/browser/devtools/device/* and 
chrome/test/chromedriver/net/* since the changes are
mechanical).
==========

[tbansal1, 2016-04-13 20:24:27]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-04-13 20:24:28]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
zea@chromium.org, wez@chromium.org, bengr@chromium.org, scheib@chromium.org,
rsleevi@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/1376473003/#ps980001
(title: "rebased")

[commit-bot: I haz the power, 2016-04-13 20:24:48]

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

[Nico, 2016-04-13 21:22:50]

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

[Nico, 2016-04-13 21:22:52]

(rietveld doesn't display it for some reason, but this failed on the win_clang
bot:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds...)

[tbansal1, 2016-04-13 22:25:18]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-04-13 22:25:19]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
reillyg@chromium.org, wez@chromium.org, rsleevi@chromium.org,
scheib@chromium.org, bengr@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1376473003/#ps1000001
(title: "Reorder initialization in constructor")

[commit-bot: I haz the power, 2016-04-13 22:25:43]

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

[commit-bot: I haz the power, 2016-04-13 22:33:39]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org, frankf@chromium.org
(for chrome/browser/devtools/device/* and 
chrome/test/chromedriver/net/* since the changes are
mechanical).
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org, frankf@chromium.org
(for chrome/browser/devtools/device/* and 
chrome/test/chromedriver/net/* since the changes are
mechanical).
==========

[commit-bot: I haz the power, 2016-04-13 22:33:41]

Committed patchset #22 (id:1000001)

[commit-bot: I haz the power, 2016-04-13 22:34:48]

Description was changed from

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params 
-> ClientSocketPoolManagerImpl 
-> TransportClientSocketPool 
-> TransportConnectJobFactory 
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the 
CreateTransportClientSocket. From there it is passed to 
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket 
object is reused for a different connection (by first 
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org, frankf@chromium.org
(for chrome/browser/devtools/device/* and 
chrome/test/chromedriver/net/* since the changes are
mechanical).
==========

to

==========
Notify NQE of TCP RTT values

This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params
-> ClientSocketPoolManagerImpl
-> TransportClientSocketPool
-> TransportConnectJobFactory
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the
CreateTransportClientSocket. From there it is passed to
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket
object is reused for a different connection (by first
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org, frankf@chromium.org
(for chrome/browser/devtools/device/* and
chrome/test/chromedriver/net/* since the changes are
mechanical).

Committed: https://crrev.com/7b403bcc8080792387a99f869285cd00f36b6167
Cr-Commit-Position: refs/heads/master@{#387128}
==========

[commit-bot: I haz the power, 2016-04-13 22:34:51]

Patchset 22 (id:??) landed as
https://crrev.com/7b403bcc8080792387a99f869285cd00f36b6167
Cr-Commit-Position: refs/heads/master@{#387128}