Add EffectiveConnectionType enum to the system profile proto

chrome/browser/metrics/chrome_metrics_service_client.cc

Index: chrome/browser/metrics/chrome_metrics_service_client.cc
diff --git a/chrome/browser/metrics/chrome_metrics_service_client.cc b/chrome/browser/metrics/chrome_metrics_service_client.cc
index fa618404fe9cbdf83657f32b48140bd05bf1638b..6e6f8e0ef085d49aff1fe1dbdf3ef71af561cfec 100644
--- a/chrome/browser/metrics/chrome_metrics_service_client.cc
+++ b/chrome/browser/metrics/chrome_metrics_service_client.cc
@@ -28,6 +28,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/google/google_brand.h"
+#include "chrome/browser/io_thread.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/metrics/chrome_stability_metrics_provider.h"
 #include "chrome/browser/metrics/https_engagement_metrics_provider.h"
@@ -122,6 +123,15 @@
 
 namespace {
 
+// Returns the network quality estimator. Runs on IO thread.
+net::NetworkQualityEstimator* GetNetworkQualityEstimatorOnIOThread(
+    IOThread* io_thread) {
+  if (!io_thread)

[Alexei Svitkine (slow), 2017/01/03 17:48:48]

In what cases would this be hit? Add a comment or remove if it's not possible.

[tbansal1, 2017/01/03 18:55:43]

Done.

+    return nullptr;
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  return io_thread->globals()->network_quality_estimator.get();
+}
+
 // This specifies the amount of time to wait for all renderers to send their
 // data.
 const int kMaxHistogramGatheringWaitDuration = 60000;  // 60 seconds.
@@ -283,6 +293,7 @@ ChromeMetricsServiceClient::ChromeMetricsServiceClient(
       drive_metrics_provider_(nullptr),
       start_time_(base::TimeTicks::Now()),
       has_uploaded_profiler_data_(false),
+      network_metrics_provider_(nullptr),
       weak_ptr_factory_(this) {
   DCHECK(thread_checker_.CalledOnValidThread());
   RecordCommandLineMetrics();
@@ -570,10 +581,24 @@ void ChromeMetricsServiceClient::Initialize() {
       std::unique_ptr<metrics::MetricsProvider>(
           new ExtensionsMetricsProvider(metrics_state_manager_)));
 #endif
+
+  std::unique_ptr<metrics::NetworkMetricsProvider> network_metrics_provider(
+      new metrics::NetworkMetricsProvider(
+          content::BrowserThread::GetBlockingPool()));
+
+  network_metrics_provider_ = network_metrics_provider.get();
+
+  // Get the network quality estimator on the IO thread, and provide it to the
+  // |network_metrics_provider|.
+  content::BrowserThread::PostTaskAndReplyWithResult(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(GetNetworkQualityEstimatorOnIOThread,
+                 g_browser_process->io_thread()),
+      base::Bind(&ChromeMetricsServiceClient::ProvideNetworkQualityEstimator,
+                 weak_ptr_factory_.GetWeakPtr()));

[Alexei Svitkine (slow), 2017/01/03 17:48:48]

Can all of this be internally in NetworksMetricsProvider? Why does it have it be
in chrome_metrics_service_client.cc? Is it because of the IO thread reference?
If so, perhaps you can just pass in a Callback object to the relevant function
to NetworkMetricsProvider ctor and it does the rest with it?

[tbansal1, 2017/01/03 18:55:43]

Yes, IO thread is in chrome/browser, so IOThread or IOThread::Globals() can't be
referenced from components. That means I have to call
"io_thread->globals()->network_quality_estimator.get()" somewhere in
chrome/browser. Also, that call can only be done on IO thread. So, the only
option is to do the PostTask to IO thread here, and get the reference to NQE.
Probably not.

[asvitkine_google, 2017/01/03 19:09:50]

Just to be clear, I would like there to be as little code as possible in this
file - as adding more code like this doesn't scale well.

So let's find a solution that achieves that. One option is to have an interface
defined in network_metrics_provider.h that has functions to get the provider.
And then you can have a class implementing that interface in
chrome/browser/metrics and in this code you can create that object and pass it
to the network_metrics_provider ctor.

+
   metrics_service_->RegisterMetricsProvider(
-      std::unique_ptr<metrics::MetricsProvider>(
-          new metrics::NetworkMetricsProvider(
-              content::BrowserThread::GetBlockingPool())));
+      std::move(network_metrics_provider));
 
   // Currently, we configure OmniboxMetricsProvider to not log events to UMA
   // if there is a single incognito session visible. In the future, it may
@@ -901,3 +926,13 @@ void ChromeMetricsServiceClient::OnURLOpenedFromOmnibox(OmniboxLog* log) {
 bool ChromeMetricsServiceClient::IsUMACellularUploadLogicEnabled() {
   return metrics::IsCellularLogicEnabled();
 }
+
+void ChromeMetricsServiceClient::ProvideNetworkQualityEstimator(
+    net::NetworkQualityEstimator* network_quality_estimator) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Provide the |network_quality_estimator| and the taskrunner for the thread
+  // on which |network_quality_estimator| runs to |network_metrics_provider_|.
+  network_metrics_provider_->ProvideNetworkQualityEstimator(
+      network_quality_estimator, content::BrowserThread::GetTaskRunnerForThread(
+                                     content::BrowserThread::IO));
+}