Setup ProxyConfigServiceAndroid in Cronet

components/cronet/android/url_request_context_adapter.cc

Index: components/cronet/android/url_request_context_adapter.cc
diff --git a/components/cronet/android/url_request_context_adapter.cc b/components/cronet/android/url_request_context_adapter.cc
index 026e39d0b1524e30d1fb4f8fc5cd31232514cd14..230efcdc6382b3fdae9724aa1e21bec048bb8457 100644
--- a/components/cronet/android/url_request_context_adapter.cc
+++ b/components/cronet/android/url_request_context_adapter.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
+#include "base/message_loop/message_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "components/cronet/url_request_context_config.h"
 #include "net/base/net_errors.h"
@@ -14,7 +15,7 @@
 #include "net/http/http_auth_handler_factory.h"
 #include "net/http/http_network_layer.h"
 #include "net/http/http_server_properties.h"
-#include "net/proxy/proxy_config_service_fixed.h"
+#include "net/proxy/proxy_config_service.h"
 #include "net/proxy/proxy_service.h"
 #include "net/ssl/ssl_config_service_defaults.h"
 #include "net/url_request/static_http_user_agent_settings.h"
@@ -108,6 +109,9 @@ class BasicNetworkDelegate : public net::NetworkDelegate {
   DISALLOW_COPY_AND_ASSIGN(BasicNetworkDelegate);
 };
 
+// Android specific UI MessageLoop.

[mmenke, 2014/10/15 17:44:17]

Maybe "UI MessageLoop" -> "MessageLoop on main thread, which is where objects
that receive Java notifications generally live."

[xunjieli, 2014/10/15 19:48:08]

Done.

+base::MessageLoop* g_main_message_loop = nullptr;

[mmenke, 2014/10/15 17:44:17]

optional:  Suggest putting this at the top of the anonymous namespace (My
general feeling is that globals/static variables/constants should go before
class declarations in anonymous namespaces, unless they're strongly associated
with one of those classes, in which case is makes sense for them to be declared
just above/below that class).

[xunjieli, 2014/10/15 19:48:08]

Done.

+
 }  // namespace
 
 namespace cronet {
@@ -125,22 +129,34 @@ void URLRequestContextAdapter::Initialize(
   base::Thread::Options options;
   options.message_loop_type = base::MessageLoop::TYPE_IO;
   network_thread_->StartWithOptions(options);
+  config_ = config.Pass();
+}
+
+void URLRequestContextAdapter::InitRequestContextOnUIThread() {

[mmenke, 2014/10/15 17:44:18]

DCHECK(config_)?

[xunjieli, 2014/10/15 19:48:09]

Done.

+  if (!base::MessageLoop::current()) {

[mmenke, 2014/10/15 17:44:17]

We have no tests where this check is false, I believe, as it requires multiple
contexts/factories.  Maybe add two simple tests for this case?

One where we make a second context at the same time as the main one, and send
requests to both contexts at once.

One where we send a request on the main context, wait for it to complete, and
then make and send a request on a second context.

[xunjieli, 2014/10/15 19:48:08]

Done.

+    DCHECK(!g_main_message_loop);
+    g_main_message_loop = new base::MessageLoopForUI;

[mmenke, 2014/10/15 17:44:17]

optional nit:  Seems more common to use "()" with 0-argument constructors.

[xunjieli, 2014/10/15 19:48:08]

Done.

+    base::MessageLoopForUI::current()->Start();
+  }

[mmenke, 2014/10/15 17:44:17]

DCHECK_EQ(g_main_message_loop, base::MessageLoop::current());?

[xunjieli, 2014/10/15 19:48:08]

Done.

 
+  net::ProxyConfigService* service =

[mmenke, 2014/10/15 17:44:17]

Should generally avoid using raw pointers, except when passing around stuff you
don't own.  Should stick this directly in a scoped_ptr.

[xunjieli, 2014/10/15 19:48:09]

Done.

+      net::ProxyService::CreateSystemProxyConfigService(GetNetworkTaskRunner(),
+                                                        NULL);
+  proxy_config_service_.reset(service);
   GetNetworkTaskRunner()->PostTask(
       FROM_HERE,
-      base::Bind(&URLRequestContextAdapter::InitializeURLRequestContext,
+      base::Bind(&URLRequestContextAdapter::InitRequestContextOnNetworkThread,
                  this,
-                 Passed(&config)));
+                 Passed(&config_)));

[mmenke, 2014/10/15 17:44:17]

I don't think we get anything from passing the config here, as we muck with
proxy_config_service_ here as well as in InitRequestContextOnNetworkThread.

Should either pass the ProxyConfigService here as well, also using Passed, or
get rid of the config_ argument and delete it in
InitRequestContextOnNetworkThread.

I slightly prefer the former, as it stays more in line with the whole "only muck
with the Context on the network thread" thing, but can live with either.

[xunjieli, 2014/10/15 19:48:09]

Done. Sounds good. I adopted the second approach. I can't get the first working,
three tests out of 31 tests failed with a seg fault when I tried
Passed(&proxy_config_service_) to InitRequestContextOnNetworkThread. Not sure
why..

 }
 
-void URLRequestContextAdapter::InitializeURLRequestContext(
+void URLRequestContextAdapter::InitRequestContextOnNetworkThread(
     scoped_ptr<URLRequestContextConfig> config) {
   DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread());
   // TODO(mmenke):  Add method to have the builder enable SPDY.
   net::URLRequestContextBuilder context_builder;
   context_builder.set_network_delegate(new BasicNetworkDelegate());
-  context_builder.set_proxy_config_service(
-      new net::ProxyConfigServiceFixed(net::ProxyConfig()));
+  context_builder.set_proxy_config_service(proxy_config_service_.get());
   config->ConfigureURLRequestContextBuilder(&context_builder);
 
   context_.reset(context_builder.Build());
@@ -179,11 +195,6 @@ void URLRequestContextAdapter::InitializeURLRequestContext(
           net::AlternateProtocol::QUIC,
           1.0f);
     }
-    is_context_initialized_ = true;
-    while (!tasks_waiting_for_context_.empty()) {
-      tasks_waiting_for_context_.front().Run();
-      tasks_waiting_for_context_.pop();
-    }
   }
 
   if (VLOG_IS_ON(2)) {
@@ -192,6 +203,12 @@ void URLRequestContextAdapter::InitializeURLRequestContext(
                                                net::NetLog::LOG_ALL_BUT_BYTES);
   }
 
+  is_context_initialized_ = true;
+  while (!tasks_waiting_for_context_.empty()) {
+    tasks_waiting_for_context_.front().Run();
+    tasks_waiting_for_context_.pop();
+  }
+
   delegate_->OnContextInitialized(this);
 }