Add functionality for embedders to configure NQE

components/cronet/android/cronet_url_request_context_adapter.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/cronet/android/cronet_url_request_context_adapter.h"
 
 #include <limits.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 659 matching lines @@
   // Explicitly disable the persister for Cronet to avoid persistence of dynamic
   // HPKP. This is a safety measure ensuring that nobody enables the persistence
   // of HPKP by specifying transport_security_persister_path in the future.
   context_builder.set_transport_security_persister_path(base::FilePath());
 
   // Disable net::CookieStore and net::ChannelIDService.
   context_builder.SetCookieAndChannelIdStores(nullptr, nullptr);
 
   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_);
       network_qualities_prefs_manager_ =
           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());
   }
 
   context_ = context_builder.Build();
 
   context_->set_check_cleartext_permitted(true);
   context_->set_enable_brotli(config->enable_brotli);
 
@@ skipping 73 matching lines @@
   context_->transport_security_state()
       ->SetEnablePublicKeyPinningBypassForLocalTrustAnchors(
           config->bypass_public_key_pinning_for_local_trust_anchors);
 
   JNIEnv* env = base::android::AttachCurrentThread();
   jcronet_url_request_context_.Reset(env, jcronet_url_request_context.obj());
   Java_CronetUrlRequestContext_initNetworkThread(env,
                                                  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();
   }
 }
 
 void CronetURLRequestContextAdapter::Destroy(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller) {
   DCHECK(!GetNetworkTaskRunner()->BelongsToCurrentThread());
@@ skipping 363 matching lines @@
     JNIEnv* env,
     const JavaParamRef<jclass>& jcaller) {
   DCHECK(base::StatisticsRecorder::IsActive());
   std::vector<uint8_t> data;
   if (!HistogramManager::GetInstance()->GetDeltas(&data))
     return ScopedJavaLocalRef<jbyteArray>();
   return base::android::ToJavaByteArray(env, &data[0], data.size());
 }
 
 }  // namespace cronet