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

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 19 matching lines @@
 #include "base/values.h"
 #include "components/cronet/histogram_manager.h"
 #include "components/cronet/url_request_context_config.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_filter.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #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"
 #include "net/cert/cert_verifier.h"
 #include "net/cookies/cookie_monster.h"
 #include "net/http/http_auth_handler_factory.h"
 #include "net/http/http_server_properties_manager.h"
 #include "net/log/write_to_file_net_log_observer.h"
 #include "net/nqe/external_estimate_provider.h"
 #include "net/proxy/proxy_config_service_android.h"
 #include "net/proxy/proxy_service.h"
 #include "net/sdch/sdch_owner.h"
 #include "net/ssl/channel_id_service.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_builder.h"
 #include "net/url_request/url_request_interceptor.h"
 
 #if defined(DATA_REDUCTION_PROXY_SUPPORT)
 #include "components/cronet/android/cronet_data_reduction_proxy.h"
 #endif
 
 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";
 // Current version of disk storage.
 const int32_t kStorageVersion = 1;
 // Version number used when the version of disk storage is unknown.
 const uint32_t kStorageVersionUnknown = 0;
 // Name of preference directory.
 const char kPrefsDirectoryName[] = "prefs";
 // Name of preference file.
@@ skipping 409 matching lines @@
       new BasicNetworkDelegate());
 #if defined(DATA_REDUCTION_PROXY_SUPPORT)
   DCHECK(!data_reduction_proxy_);
   // For now, the choice to enable the data reduction proxy happens once,
   // at initialization. It cannot be disabled thereafter.
   if (!config->data_reduction_proxy_key.empty()) {
     data_reduction_proxy_.reset(new CronetDataReductionProxy(
         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(
         data_reduction_proxy_->CreateProxyDelegate());
     std::vector<std::unique_ptr<net::URLRequestInterceptor>> interceptors;
     interceptors.push_back(data_reduction_proxy_->CreateInterceptor());
     context_builder.SetInterceptors(std::move(interceptors));
   }
 #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()) {
     base::FilePath storage_path(config->storage_path);
     // Make sure storage directory has correct version.
     InitializeStorageDirectory(storage_path);
     base::FilePath filepath =
         storage_path.Append(FILE_PATH_LITERAL(kPrefsDirectoryName))
             .Append(FILE_PATH_LITERAL(kPrefsFileName));
     json_pref_store_ =
@@ skipping 359 matching lines @@
     JNIEnv* env,
     const JavaParamRef<jclass>& jcaller) {
   base::StatisticsRecorder::Initialize();
   std::vector<uint8_t> data;
   if (!HistogramManager::GetInstance()->GetDeltas(&data))
     return ScopedJavaLocalRef<jbyteArray>();
   return base::android::ToJavaByteArray(env, &data[0], data.size());
 }
 
 }  // namespace cronet