[Cronet] Add network change events to Cronet's NetLog

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 9730c101626fa0bd2f6af3859523707c1088a08f..ff43ad57386853f9d3ad7863f51b3705274c4303 100644
--- a/components/cronet/android/cronet_url_request_context_adapter.cc
+++ b/components/cronet/android/cronet_url_request_context_adapter.cc
@@ -37,6 +37,7 @@
 #include "components/prefs/pref_service_factory.h"
 #include "jni/CronetUrlRequestContext_jni.h"
 #include "net/base/load_flags.h"
+#include "net/base/logging_network_change_observer.h"
 #include "net/base/net_errors.h"
 #include "net/base/network_delegate_impl.h"
 #include "net/base/url_util.h"
@@ -60,8 +61,22 @@
 
 namespace {
 
+// This class wraps a NetLog that also contains network change events.
+class NetLogWithNetworkChangeEvents {
+ public:
+  NetLogWithNetworkChangeEvents() : net_log_(), net_change_logger_(&net_log_) {}
+
+  net::NetLog* net_log() { return &net_log_; }
+
+ private:
+  net::NetLog net_log_;

[xunjieli, 2016/05/13 13:32:00]

Can we make these into std::unique_ptr so we know that they are owned by this
class?

[pauljensen, 2016/05/13 16:16:18]

Isn't having a definition of a variable even stronger ownership than a
unique_ptr?  A unique_ptr is an unnecessary layer of abstraction in this case I
believe, for the same reason we write "int i" rather than
"std::unique_ptr<Integer> i(new Integer)"

[xunjieli, 2016/05/13 18:09:23]

Acknowledged. Matt says if there isn't a use of unique_ptr, don't use one. I
thought smart pointers are always superior :\

+  net::LoggingNetworkChangeObserver net_change_logger_;

[xunjieli, 2016/05/13 13:32:00]

Can we add a comment on why this is global?

[pauljensen, 2016/05/13 16:16:18]

I don't think it is global, it's a member of a class type.  g_net_log is global.

[xunjieli, 2016/05/13 18:09:23]

Yea. Semantically, it isn't a global. But it is essentially a global, since we
have a global NetLog. Could you comment on why we have it here instead of making
a new one for every URLRequestContextAdapter?

[pauljensen, 2016/05/13 18:13:56]

Well we have one LoggingNetworkChangeObserver for each NetLog because you only
want the events logged once.  Is that what you want me to make the comment say?
g_net_log already has a comment explaining why we use a global one.

[xunjieli, 2016/05/13 18:19:07]

Yes. It isn't obvious why LoggingNetworkChangeObserver needs to be boundled with
the global object. Of course, if I do a codesearch and know what
LoggingNetworkChangeObserver does, then sure. On the other hand, we do not
bundle WriteToFileNetLogObserver. For someone who isn't familiar with these two
classes, from the first glance, it looks like they are both observers and are
related to logging, but they have different life cycles. This can be lead to
confusion down the road. A quick comment along the line that you just mentioned
will do.

[pauljensen, 2016/05/21 01:26:31]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(NetLogWithNetworkChangeEvents);
+};
+
 // Use a global NetLog instance. See crbug.com/486120.
-static base::LazyInstance<net::NetLog>::Leaky g_net_log =
+static base::LazyInstance<NetLogWithNetworkChangeEvents>::Leaky g_net_log =
     LAZY_INSTANCE_INITIALIZER;
 
 const char kHttpServerProperties[] = "net.http_server_properties";
@@ -491,7 +506,7 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
         config->data_reduction_proxy_key, config->data_reduction_primary_proxy,
         config->data_reduction_fallback_proxy,
         config->data_reduction_secure_proxy_check_url, config->user_agent,
-        GetNetworkTaskRunner(), g_net_log.Pointer()));
+        GetNetworkTaskRunner(), g_net_log.Get().net_log()));
     network_delegate = data_reduction_proxy_->CreateNetworkDelegate(
         std::move(network_delegate));
     context_builder.set_proxy_delegate(
@@ -502,17 +517,18 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
   }
 #endif  // defined(DATA_REDUCTION_PROXY_SUPPORT)
   context_builder.set_network_delegate(std::move(network_delegate));
-  context_builder.set_net_log(g_net_log.Pointer());
+  context_builder.set_net_log(g_net_log.Get().net_log());
 
   // Android provides a local HTTP proxy server that handles proxying when a PAC
   // URL is present. Create a proxy service without a resolver and rely on this
   // local HTTP proxy. See: crbug.com/432539.
   context_builder.set_proxy_service(
       net::ProxyService::CreateWithoutProxyResolver(
-          std::move(proxy_config_service_), g_net_log.Pointer()));
+          std::move(proxy_config_service_), g_net_log.Get().net_log()));
 
-  config->ConfigureURLRequestContextBuilder(
-      &context_builder, g_net_log.Pointer(), GetFileThread()->task_runner());
+  config->ConfigureURLRequestContextBuilder(&context_builder,
+                                            g_net_log.Get().net_log(),
+                                            GetFileThread()->task_runner());
 
   // Set up pref file if storage path is specified.
   if (!config->storage_path.empty()) {