Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a Test…

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 fc88f1b673f2fae0d33f884a7931b7fb20748d12..24b6202b043c12f2fa2912618ce3b19dfb718bab 100644
--- a/components/cronet/android/cronet_url_request_context_adapter.cc
+++ b/components/cronet/android/cronet_url_request_context_adapter.cc
@@ -29,6 +29,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/single_thread_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/cronet/android/cert/cert_verifier_cache_serializer.h"
@@ -593,7 +594,7 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
     std::unique_ptr<net::HttpServerPropertiesManager>
         http_server_properties_manager(new net::HttpServerPropertiesManager(
             new PrefServiceAdapter(pref_service_.get()),
-            GetNetworkTaskRunner()));
+            base::ThreadTaskRunnerHandle::Get(), GetNetworkTaskRunner()));

[xunjieli, 2016/12/14 01:47:11]

Hmm.. This method is invoked on the network thread.
base::ThreadTaskRunnerHandle::Get() is essentially GetNetworkTaskRunner(). I
know this CL doesn't introduce any functional change, but my question is what
should we use for |pref_task_runner_|? Is it bad for it to be the same as
|network_task_runner_|?

[Zhongyi Shi, 2016/12/14 04:20:43]

Yeah, this CL shouldn't introduce functional change. If I read the code
correctly (you might also want to confirm), in src/chrome/browser, the
HttpServerPropertiesManager is created by ProfileImplIOData::Handle::Init where
it should be running on the BrowserThread::UI thread. I agreed that
base::ThreaTaskRunnerHanlde::Get() is actually getting a network task runner as
it's running on the network thread. Do you think this is a bug? 

[xunjieli, 2016/12/14 18:49:38]

I think the bug might be in HttpServerPropertiesManager. It's not clear to me
why HttpServerPropertiesManager needs to receive notifications on UI thread.
Maybe the PrefService has such limitation? I found one line on thread safety in
pref_value_store.h but not anywhere else. It will be good to find out why we
have this separation (pref_task_runner_ vs network_task_runner_).

I talked to Bence offline as well. He wasn't sure about reason behind this
setup.

In Cronet, this will basically mean that we are posting tasks from the network
thread to itself and back, which seems wasteful (but not dangerous).

However, in Chrome, if this class starts to post too many tasks to the
pref_task_runner_, we will be causing UI jank which seems dangerous and
possible. 

Since this CL introduces no functional change, lgtm.

[Zhongyi Shi, 2016/12/14 19:47:24]

If you look at the HttpServerPropertiesManager documentation, it interacts with
pref thread(on desktop, it's the UI thread) and network thread. I have also
updated the cl description explaining why there're two task_runners in this
class. 

However, note it's also required that "HttpServerPropertiesManager is
constructed by on the pref thread, to set up |pref_task_runner_| and the prefs
listeners". In cronet case, it's just all handed to networking thread and all
the data are accessible by the networking thread. I talked to Ryan offline, and
he felt it should be okay. 
We used timer to schedule posts every 60s so that we don't post too often.
Agreed that this CL shouldn't be blocked by the discussion here. It might
get your attention that in cronet, network_task_runner and pref_task_runner
are the same though. 

     http_server_properties_manager->InitializeOnNetworkThread();
     http_server_properties_manager_ = http_server_properties_manager.get();
     context_builder.SetHttpServerProperties(