Setup ProxyConfigServiceAndroid in Cronet

components/cronet/android/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/url_request_context_adapter.h"
 
 #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"
 #include "net/base/net_log_logger.h"
 #include "net/cert/cert_verifier.h"
 #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"
 #include "net/url_request/url_request_context_builder.h"
 #include "net/url_request/url_request_context_storage.h"
 #include "net/url_request/url_request_job_factory_impl.h"
 
 namespace {
 
 class BasicNetworkDelegate : public net::NetworkDelegate {
@@ skipping 73 matching lines @@
 
   virtual int OnBeforeSocketStreamConnect(
       net::SocketStream* stream,
       const net::CompletionCallback& callback) override {
     return net::OK;
   }
 
   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 {
 
 URLRequestContextAdapter::URLRequestContextAdapter(
     URLRequestContextAdapterDelegate* delegate,
     std::string user_agent) {
   delegate_ = delegate;
   user_agent_ = user_agent;
 }
 
 void URLRequestContextAdapter::Initialize(
     scoped_ptr<URLRequestContextConfig> config) {
   network_thread_ = new base::Thread("network");
   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());
 
   // Currently (circa M39) enabling QUIC requires setting probability threshold.
   if (config->enable_quic) {
     context_->http_server_properties()
         ->SetAlternateProtocolProbabilityThreshold(0.0f);
     for (size_t hint = 0; hint < config->quic_hints.size(); ++hint) {
       const URLRequestContextConfig::QuicHint& quic_hint =
@@ skipping 18 matching lines @@
       }
 
       net::HostPortPair quic_hint_host_port_pair(quic_hint.host,
                                                  quic_hint.port);
       context_->http_server_properties()->SetAlternateProtocol(
           quic_hint_host_port_pair,
           static_cast<uint16>(quic_hint.alternate_port),
           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)) {
     net_log_observer_.reset(new NetLogObserver());
     context_->net_log()->AddThreadSafeObserver(net_log_observer_.get(),
                                                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);
 }
 
 void URLRequestContextAdapter::PostTaskToNetworkThread(
     const tracked_objects::Location& posted_from,
     const RunAfterContextInitTask& callback) {
   GetNetworkTaskRunner()->PostTask(
       posted_from,
       base::Bind(
           &URLRequestContextAdapter::RunTaskAfterContextInitOnNetworkThread,
@@ skipping 75 matching lines @@
     net_log_logger_.reset();
   }
 }
 
 void NetLogObserver::OnAddEntry(const net::NetLog::Entry& entry) {
   VLOG(2) << "Net log entry: type=" << entry.type()
           << ", source=" << entry.source().type << ", phase=" << entry.phase();
 }
 
 }  // namespace cronet