Add functionality for embedders to configure NQE

components/cronet/android/cronet_url_request_context_adapter.cc

Index: components/cronet/android/cronet_url_request_context_adapter.cc
diff --git a/components/cronet/android/cronet_url_request_context_adapter.cc b/components/cronet/android/cronet_url_request_context_adapter.cc
index a01ee30cd363f8602ad4fe803ae4f2735e8ce836..0f56c3fc27f37030e91af50bffcd7375744b4d87 100644
--- a/components/cronet/android/cronet_url_request_context_adapter.cc
+++ b/components/cronet/android/cronet_url_request_context_adapter.cc
@@ -677,22 +677,15 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
 
   if (config->enable_network_quality_estimator) {
     DCHECK(!network_quality_estimator_);
-    std::map<std::string, std::string> variation_params;
-    // Configure network quality estimator: Specify the algorithm that should
-    // be used for computing the effective connection type. The algorithm
-    // is specified using the key-value pairs defined in
-    // //net/nqe/network_quality_estimator.cc.
-    // TODO(tbansal): Investigate a more robust way of configuring the network
-    // quality estimator.
-    variation_params["effective_connection_type_algorithm"] =
-        "TransportRTTOrDownstreamThroughput";
+    // It's ok to clear thread association for |config->nqe_params| since the
+    // object is being moved, and so can no longer be modified on the other
+    // thread.
+    config->nqe_params->DetachFromThread();

[mgersh, 2017/06/28 18:26:50]

This shouldn't be necessary, since experimental option parsing happens on the
network sequence.

[tbansal1, 2017/06/29 01:17:58]

Done.

     network_quality_estimator_ = base::MakeUnique<net::NetworkQualityEstimator>(
         std::unique_ptr<net::ExternalEstimateProvider>(),
-        base::MakeUnique<net::NetworkQualityEstimatorParams>(variation_params),
-        g_net_log.Get().net_log());
+        std::move(config->nqe_params), g_net_log.Get().net_log());

[mgersh, 2017/06/28 18:26:50]

Moving a pointer not owned by this object defeats the point of ownership, but I
think that's more of a problem with the way the existing Cronet code is
structured than with your CL. It might be better to store the individual options
in the URLRequestContextConfig and create the Params object here. I don't feel
strongly about that, though. What do you think?

[tbansal1, 2017/06/29 01:17:58]

Done.

     network_quality_estimator_->AddEffectiveConnectionTypeObserver(this);
     network_quality_estimator_->AddRTTAndThroughputEstimatesObserver(this);
-
     // Set up network quality prefs if the storage path is specified.
     if (!config->storage_path.empty()) {
       DCHECK(!network_qualities_prefs_manager_);
@@ -700,8 +693,6 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
           base::MakeUnique<net::NetworkQualitiesPrefsManager>(
               base::MakeUnique<NetworkQualitiesPrefDelegateImpl>(
                   pref_service_.get()));
-      network_qualities_prefs_manager_->InitializeOnNetworkThread(
-          network_quality_estimator_.get());
     }
     context_builder.set_network_quality_estimator(
         network_quality_estimator_.get());
@@ -795,6 +786,15 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
                                                  jcronet_url_request_context);
 
   is_context_initialized_ = true;
+  if (config->enable_network_quality_estimator &&
+      !config->storage_path.empty()) {

[mgersh, 2017/06/28 18:26:50]

This block logically goes together with HostCachePersistenceManager
initialization and it would be nice to combine them. That's in a CL that I sent
to the CQ right when you sent this, so you'll need to rebase again to pick it
up. There might be some merge conflicts. Sorry about the race condition :)

[tbansal1, 2017/06/29 01:17:58]

This code block should run after |jcronet_url_request_context_| has been
initialized. So, IIUC, it's not possible to move this up.

[mgersh, 2017/06/29 19:02:31]

Ah, ok, I misread which object needs to be initialized and why. I think it would
still be safe to do it earlier, even where it was before this CL, because all of
these things run on the same task runner (prefs and network task runners are the
same in Cronet) so the posted task won't run until this one finishes.

[tbansal1, 2017/06/29 22:45:05]

The problem is that it is not a posted task. I am using a post task now.
Hopefully that's cleaner.

+    // Initializing |network_qualities_prefs_manager_| may post a callback to
+    // |this|. So, initialize it after |jcronet_url_request_context_| has been
+    // constructed.
+    network_qualities_prefs_manager_->InitializeOnNetworkThread(
+        network_quality_estimator_.get());
+  }
+
   while (!tasks_waiting_for_context_.empty()) {
     tasks_waiting_for_context_.front().Run();
     tasks_waiting_for_context_.pop();