Collect stats to investigate the viability of UDP

Collect stats to investigate the viability of UDP
connectivity from the browser (first cut). Collect
stats for TCP connectivity also.

- What percentage of users can get a message end-to-end
  to an TCP and UDP server.
- What is the latency for TCP and UDP messages.

Added TCP and UDP echo servers to testserver.py
for unittests.

BUG=82565
TEST=udp tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88495

[ramant (doing other things), 2011-05-27 07:02:46]

Hi Mike,Willchan and Jim,
  This is a first cut at sending a small packet for UDP and TCP servers. The
server names were left empty and this code won't send the echo message. Once we
have the servers up and running, will update the server names. We will contact
our servers for 1 in 1000 users who have UMA upload enabled.

  Would appreciate your opinion and comments.

thanks much,
raman

[Mike Belshe, 2011-05-27 21:27:00]

Thanks Raman -

A few comments.  Codewise, checking the return value of Send is important. 
Also, checking for network down.

I'll think a little about which test cases we want to run, and then see what you
think.

Mike

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:1034:
chrome_browser_net::CollectNetworkStats(network_stats_server_);
How often do you want to run this test?  I'm not sure how often the
MetricsService::OnHistogramSynchronizationDone() is called?  Is this once per
browser lifetime?  Or is it repeated?

My thoughts for when to schedule a test:
   - field trial selects 0.5% of users
   - randomly pick a time between 10 and 20 minutes after browser start
   - run test once

But I made that up - other suggestions welcome.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:102: const int kWriteChunkSize = 777;  //
777 is more abusive.
Do you need this chunking?  I had that in the unittest for fun, but its not
appropriate here?

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:117: 
rv could be <0, indicating an error.  It should be rare.  You probably want this
function to return an int, which is either the status or the error.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:143: bytes_to_read_ = bytes_to_send_ =
bytes_to_send;
nit:  I think these compound assignments are against style?

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:148: this->Finish(IP_STRING_PARSE_FAIL,
net::ERR_INVALID_ARGUMENT);
nit:  generally we don't do 'this->' in our code.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:167: this->SendData();
I think you'll need:

if (SendData() < 0) {
   Finish(SEND_FAILED, rv);
   return false;
}

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:251: this->SendData();
Same concern about Send() failing here.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:310: 
Is there a way to add a check to see if the network is actually alive here?  

I believe this was a flaw with the WebSockets test - the test could kick off
when the user is offline (not connected to wifi on a laptop, for instance) and
it skewed the answers.  It would be fine to have an error state "NETWORK_DOWN"
if you want to track it.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:317:
udp_stats_client->Start(network_stats_server, 90, 5, NULL);
Port 90 is an IANA registered port for dnsix.  
http://www.iana.org/assignments/port-numbers

Recommend:
  HTTP:  use ports 80 (http), 6121 (spdy)
  UDP:   use ports 6121

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:91: net::TestDataStream received_stream_;
BTW - I think you only need one stream of test data.

Since it's an echo server, you can call stream_.GetBytes() to generate the data
to send, then call stream_.VerifyBytes() on the data you received.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
File chrome/browser/net/network_stats_unittest.cc (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats_unittest.cc:50: // RunUDPEchoTest(5);
Can't the test work now since you're using loopback?

Nevermind - I get it.  I guess the test is self-reporting to the server.   But
for testing, you should be able to set a flag which says "don't report results"
or somehow Mocks that...

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats_unittest.cc:55: // RunTCPEchoTest(5);
BTW - I'd like to see tests which send more than just 5 bytes.

Maybe do a 1K test too?

http://codereview.chromium.org/7056031/diff/25014/net/base/test_data_stream.h
File net/base/test_data_stream.h (left):

http://codereview.chromium.org/7056031/diff/25014/net/base/test_data_stream.h...
net/base/test_data_stream.h:24: void GetBytes(char* buffer, int length) {
BTW - I think clank is going to bitch at you when you check this in. 
Previously, this code was just in a unittest, now it gets linked into chrome.

They're going to want all of the function bodies moved to a .cc file.

[ramant (doing other things), 2011-05-31 21:25:36]

Hi Mike,
  Many many thanks for your comments. Made all the changes you and jar had
suggested. Would appreciate if you could look at the changes again.

thanks much,
raman

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:1034:
chrome_browser_net::CollectNetworkStats(network_stats_server_);
On 2011/05/27 21:27:00, Mike Belshe wrote:
> How often do you want to run this test?  I'm not sure how often the
> MetricsService::OnHistogramSynchronizationDone() is called?  Is this once per
> browser lifetime?  Or is it repeated?
> 
> My thoughts for when to schedule a test:
>    - field trial selects 0.5% of users
>    - randomly pick a time between 10 and 20 minutes after browser start
>    - run test once
> 
> But I made that up - other suggestions welcome.

Made changes per jar and your talks. CollectStats after successful UMA upload.
We upload UMA stats 1 minute after startup and every 30 mins after that.

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:102: const int kWriteChunkSize = 777;  //
777 is more abusive.
On 2011/05/27 21:27:00, Mike Belshe wrote:
> Do you need this chunking?  I had that in the unittest for fun, but its not
> appropriate here?

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:117: 
On 2011/05/27 21:27:00, Mike Belshe wrote:
> rv could be <0, indicating an error.  It should be rare.  You probably want
this
> function to return an int, which is either the status or the error.

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:143: bytes_to_read_ = bytes_to_send_ =
bytes_to_send;
On 2011/05/27 21:27:00, Mike Belshe wrote:
> nit:  I think these compound assignments are against style?

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:148: this->Finish(IP_STRING_PARSE_FAIL,
net::ERR_INVALID_ARGUMENT);
On 2011/05/27 21:27:00, Mike Belshe wrote:
> nit:  generally we don't do 'this->' in our code.

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:167: this->SendData();
On 2011/05/27 21:27:00, Mike Belshe wrote:
> I think you'll need:
> 
> if (SendData() < 0) {
>    Finish(SEND_FAILED, rv);
>    return false;
> }

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:251: this->SendData();
On 2011/05/27 21:27:00, Mike Belshe wrote:
> Same concern about Send() failing here.

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:310: 
On 2011/05/27 21:27:00, Mike Belshe wrote:
> Is there a way to add a check to see if the network is actually alive here?  
> 
> I believe this was a flaw with the WebSockets test - the test could kick off
> when the user is offline (not connected to wifi on a laptop, for instance) and
> it skewed the answers.  It would be fine to have an error state "NETWORK_DOWN"
> if you want to track it.

UMA calls only after it has successfully uploaded to the server. Added
net::NetworkChangeNotifier::IsOffline check.

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:317:
udp_stats_client->Start(network_stats_server, 90, 5, NULL);
On 2011/05/27 21:27:00, Mike Belshe wrote:
> Port 90 is an IANA registered port for dnsix.  
> http://www.iana.org/assignments/port-numbers
> 
> Recommend:
>   HTTP:  use ports 80 (http), 6121 (spdy)
>   UDP:   use ports 6121

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:91: net::TestDataStream received_stream_;
On 2011/05/27 21:27:00, Mike Belshe wrote:
> BTW - I think you only need one stream of test data.
> 
> Since it's an echo server, you can call stream_.GetBytes() to generate the
data
> to send, then call stream_.VerifyBytes() on the data you received.

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
File chrome/browser/net/network_stats_unittest.cc (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats_unittest.cc:50: // RunUDPEchoTest(5);
On 2011/05/27 21:27:00, Mike Belshe wrote:
> Can't the test work now since you're using loopback?
> 
> Nevermind - I get it.  I guess the test is self-reporting to the server.   But
> for testing, you should be able to set a flag which says "don't report
results"
> or somehow Mocks that...

Done.

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats_unittest.cc:55: // RunTCPEchoTest(5);
On 2011/05/27 21:27:00, Mike Belshe wrote:
> BTW - I'd like to see tests which send more than just 5 bytes.
> 
> Maybe do a 1K test too?

Done.

http://codereview.chromium.org/7056031/diff/25014/net/base/test_data_stream.h
File net/base/test_data_stream.h (left):

http://codereview.chromium.org/7056031/diff/25014/net/base/test_data_stream.h...
net/base/test_data_stream.h:24: void GetBytes(char* buffer, int length) {
On 2011/05/27 21:27:00, Mike Belshe wrote:
> BTW - I think clank is going to bitch at you when you check this in. 
> Previously, this code was just in a unittest, now it gets linked into chrome.
> 
> They're going to want all of the function bodies moved to a .cc file.

Done.

[Mike Belshe, 2011-06-03 18:25:32]

LGTM

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:314: tcp_stats_client->Start(server_address,
5, NULL);
One last thing - what do you think of doing a small write test and a large write
test?

I think 5 bytes is too small to be interesting.  Maybe 100 bytes for the small
test, 1200 bytes for the large test.

[willchan no longer on Chromium, 2011-06-04 19:26:51]

http://codereview.chromium.org/7056031/diff/57057/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7056031/diff/57057/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:75: virtual void Finish(Status status, int
result) {}
I think this is the only method that needs to be virtual. Why are the other ones
virtual? Also, why is this protected? Do subclasses need to call it? If not,
make it private. I'd also encourage subclasses not to call the parent
implementation, as that leads to implementation inheritance and the
http://en.wikipedia.org/wiki/Fragile_base_class problem.

http://codereview.chromium.org/7056031/diff/57057/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:77: // The socket handle for this session.
All these member variables should be private:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access....

http://codereview.chromium.org/7056031/diff/57057/chrome/installer/util/chrom...
File chrome/installer/util/chrome_frame_distribution.h (right):

http://codereview.chromium.org/7056031/diff/57057/chrome/installer/util/chrom...
chrome/installer/util/chrome_frame_distribution.h:46: virtual std::string
GetNetworkStatsServer() OVERRIDE;
I don't know why these methods aren't const. I guess if you made this one const
it'd stick out. Sorta annoys me to get constness wrong like this.

[ramant (doing other things), 2011-06-05 17:54:12]

Many many thanks Mike and William for your comments (made all the changes you
had suggested).

Had compiled it multiple times. There were no leaks or race conditions with the
in NetworkStats from linux_valgrind and linux_tsan (this code is not enabled yet
in production because we are not ready with google3 servers).

Would appreciate if you could review the changes one more time.

thanks very much,
raman

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7056031/diff/25014/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:314: tcp_stats_client->Start(server_address,
5, NULL);
On 2011/06/03 18:25:32, Mike Belshe wrote:
> One last thing - what do you think of doing a small write test and a large
write
> test?
> 
> I think 5 bytes is too small to be interesting.  Maybe 100 bytes for the small
> test, 1200 bytes for the large test.

Done.

http://codereview.chromium.org/7056031/diff/57057/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7056031/diff/57057/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:75: virtual void Finish(Status status, int
result) {}
On 2011/06/04 19:26:51, willchan wrote:
> I think this is the only method that needs to be virtual. Why are the other
ones
> virtual? Also, why is this protected? Do subclasses need to call it? If not,
> make it private. I'd also encourage subclasses not to call the parent
> implementation, as that leads to implementation inheritance and the
> http://en.wikipedia.org/wiki/Fragile_base_class problem.

unittests provide finish_callback_. If we couldn't parse the ip address or host
resolver fails then sub classes Finish() method which calls finish_callback_.

Added helper methods so that sub classes don't access private data.

Done.

http://codereview.chromium.org/7056031/diff/57057/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:77: // The socket handle for this session.
On 2011/06/04 19:26:51, willchan wrote:
> All these member variables should be private:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access....

Done.

http://codereview.chromium.org/7056031/diff/57057/chrome/installer/util/chrom...
File chrome/installer/util/chrome_frame_distribution.h (right):

http://codereview.chromium.org/7056031/diff/57057/chrome/installer/util/chrom...
chrome/installer/util/chrome_frame_distribution.h:46: virtual std::string
GetNetworkStatsServer() OVERRIDE;
On 2011/06/04 19:26:51, willchan wrote:
> I don't know why these methods aren't const. I guess if you made this one
const
> it'd stick out. Sorta annoys me to get constness wrong like this.

Done.

[willchan no longer on Chromium, 2011-06-06 09:25:49]

Am I right that this does blocking IO on the IO thread? If so, this is important
to fix.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:286: int rv = host_resolver.Resolve(request,
&addresses, NULL, net::BoundNetLog());
Wait, is this a synchronous HostResolver call? I don't see a callback. This will
block the IO thread, right? And why aren't we using the HostResolverImpl in the
IOThread? We do our prioritization and scheduling of requests by using the same
HostResolver instance for everything, so creating your own here bypasses those
restrictions.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:337: Release();  // Acquired on Start().
Why is this refcounted? Who is acquiring a reference? It looks like it just
AddRef()s itself and then Release()s itself. Why not just delete itself in
Finish()?

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:357: // Collect network stats is there is a
network connection.
if there is

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:358: if
(net::NetworkChangeNotifier::IsOffline())
How often is this called? I've noticed that IsOffline() can actually block for a
few seconds on Windows while it makes an RPC call. This sucks and we need to fix
it, but it's generally only called on a network change. Or you going to be
calling this often? If so, then it could be an issue.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:387: static const size_t kMaxNumberOfTests =
INT_MAX;
No need for the static. Just let the compiler allocate this value as an
intermediate value in a register.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:397: network_stats_server, 6121,
kSmallTestBytesToSend, NULL);
Use a named constant for 6121. Perhaps something like kUDPTestingPort. Document
that its value is not significant (or if it is significant, explain).

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.h:77: const scoped_ptr<net::Socket>& socket() {
return socket_; }
I think you should just return socket_.get();, unless you need the scoped_ptr
for anything?

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.h:84: static const int kMaxMessage = 2048;
If this is only used privately, this it's an implementation detail that should
be kept in the .cc file.

[ramant (doing other things), 2011-06-07 20:54:00]

Hi willchan,
  Made all the changes you had mentioned and we had talked about. sync'ed all
the changes from trunk as of this morning.

  There were no new leaks or races due to network_stats.cc or
network_stats_unittest.cc (valgrind and tsan errors are in WebKit).

  Would appreciate your comments.

thanks much,
raman

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:286: int rv = host_resolver.Resolve(request,
&addresses, NULL, net::BoundNetLog());
On 2011/06/06 09:25:49, willchan wrote:
> Wait, is this a synchronous HostResolver call? I don't see a callback. This
will
> block the IO thread, right? And why aren't we using the HostResolverImpl in
the
> IOThread? We do our prioritization and scheduling of requests by using the
same
> HostResolver instance for everything, so creating your own here bypasses those
> restrictions.

Many many thanks for pointing this out. Added the callback and used the host
resolver from io_thread.

Done.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:337: Release();  // Acquired on Start().
On 2011/06/06 09:25:49, willchan wrote:
> Why is this refcounted? Who is acquiring a reference? It looks like it just
> AddRef()s itself and then Release()s itself. Why not just delete itself in
> Finish()?

Done.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:357: // Collect network stats is there is a
network connection.
On 2011/06/06 09:25:49, willchan wrote:
> if there is

Done.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:358: if
(net::NetworkChangeNotifier::IsOffline())
On 2011/06/06 09:25:49, willchan wrote:
> How often is this called? I've noticed that IsOffline() can actually block for
a
> few seconds on Windows while it makes an RPC call. This sucks and we need to
fix
> it, but it's generally only called on a network change. Or you going to be
> calling this often? If so, then it could be an issue.

We call this once in 30 mins and only when UMA upload to the server has
succeeded. We know there is a connection because UMA upload was successful. Thus
changed it to a DCHECK.

We could limit the number of times we perform the UDP test to only once by
changing kMaxNumberOfTests to 1.

Done.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:387: static const size_t kMaxNumberOfTests =
INT_MAX;
On 2011/06/06 09:25:49, willchan wrote:
> No need for the static. Just let the compiler allocate this value as an
> intermediate value in a register.

Done.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.cc:397: network_stats_server, 6121,
kSmallTestBytesToSend, NULL);
On 2011/06/06 09:25:49, willchan wrote:
> Use a named constant for 6121. Perhaps something like kUDPTestingPort.
Document
> that its value is not significant (or if it is significant, explain).

Done.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.h:77: const scoped_ptr<net::Socket>& socket() {
return socket_; }
On 2011/06/06 09:25:49, willchan wrote:
> I think you should just return socket_.get();, unless you need the scoped_ptr
> for anything?

Done.

http://codereview.chromium.org/7056031/diff/24/chrome/browser/net/network_sta...
chrome/browser/net/network_stats.h:84: static const int kMaxMessage = 2048;
On 2011/06/06 09:25:49, willchan wrote:
> If this is only used privately, this it's an implementation detail that should
> be kept in the .cc file.

Done.

[willchan no longer on Chromium, 2011-06-08 14:07:02]

LGTM, just nits left.

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:442: io_thread_(NULL),
Is this on the UI thread? If so, why not just initialize io_thread_ here?

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.h:368: // network_stats_server_ host.
Maybe you should document that this is only safe to access on the IO thread?

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:54: read_buffer_ = NULL;
Is this necessary? We already use a scoped_refptr.

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:105: bool NetworkStats::ReadComplete(int
result) {
Add a DCHECK_NE(net::ERR_IO_PENDING, result);

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:133: void NetworkStats::OnWriteComplete(int
result) {
DCHECK_NE(net::ERR_IO_PENDING, result)

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:398: size_t kMaxNumberOfTests = INT_MAX;
const size_t

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:102: net::Socket* socket_;
Why isn't this a scoped_ptr?

http://codereview.chromium.org/7056031/diff/70004/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1421: 'browser/net/network_stats.cc',
Sort alphabetically

http://codereview.chromium.org/7056031/diff/70004/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1471: 'browser/net/network_stats_unittest.cc',
sort alphabetically

[ramant (doing other things), 2011-06-08 17:47:17]

Hi willchan,
  Many many thanks for the code review. Made all the changes you had suggested
(except initializing io_thread in metrics_service constructor).

thanks much,
raman

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:442: io_thread_(NULL),
On 2011/06/08 14:07:02, willchan wrote:
> Is this on the UI thread? If so, why not just initialize io_thread_ here?

metrics_service is started before other threads are created. When I tried to
access io_thread, it started creating io_thread and we run into DCHECK because
io_thread is being started out of order.

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.h:368: // network_stats_server_ host.
On 2011/06/08 14:07:02, willchan wrote:
> Maybe you should document that this is only safe to access on the IO thread?

Done.

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:54: read_buffer_ = NULL;
On 2011/06/08 14:07:02, willchan wrote:
> Is this necessary? We already use a scoped_refptr.

Done.

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:105: bool NetworkStats::ReadComplete(int
result) {
On 2011/06/08 14:07:02, willchan wrote:
> Add a DCHECK_NE(net::ERR_IO_PENDING, result);

Done.

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:133: void NetworkStats::OnWriteComplete(int
result) {
On 2011/06/08 14:07:02, willchan wrote:
> DCHECK_NE(net::ERR_IO_PENDING, result)

Done.

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:398: size_t kMaxNumberOfTests = INT_MAX;
On 2011/06/08 14:07:02, willchan wrote:
> const size_t

Done.

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:102: net::Socket* socket_;
On 2011/06/08 14:07:02, willchan wrote:
> Why isn't this a scoped_ptr?

Done.

http://codereview.chromium.org/7056031/diff/70004/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1421: 'browser/net/network_stats.cc',
On 2011/06/08 14:07:02, willchan wrote:
> Sort alphabetically

Done.

http://codereview.chromium.org/7056031/diff/70004/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7056031/diff/70004/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1471: 'browser/net/network_stats_unittest.cc',
On 2011/06/08 14:07:02, willchan wrote:
> sort alphabetically

Done.