Decouple Metrics Initialization from IO Thread Initialization

chrome/browser/profiles/profile_impl_io_data.cc

 // Copyright (c) 2012 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 "chrome/browser/profiles/profile_impl_io_data.h"
 
 #include <memory>
 #include <set>
 #include <string>
 #include <utility>
@@ skipping 29 matching lines @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "components/cookie_config/cookie_store_util.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
 #include "components/data_reduction_proxy/core/browser/data_store_impl.h"
 #include "components/domain_reliability/monitor.h"
+#include "components/metrics/data_use_tracker.h"
+#include "components/metrics/metrics_service.h"
 #include "components/net_log/chrome_net_log.h"
 #include "components/prefs/json_pref_store.h"
 #include "components/prefs/pref_filter.h"
 #include "components/prefs/pref_member.h"
 #include "components/prefs/pref_service.h"
 #include "components/previews/core/previews_io_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/cookie_store_factory.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/resource_context.h"
@@ skipping 44 matching lines @@
   }
 #endif  // #if !defined(OS_ANDROID)
 
 #if defined(OS_ANDROID) || defined(OS_LINUX) || defined(OS_CHROMEOS)
   return net::CACHE_BACKEND_SIMPLE;
 #else
   return net::CACHE_BACKEND_BLOCKFILE;
 #endif
 }
 
+metrics::UpdateUsagePrefCallbackType GetMetricsDataUseForwarder() {
+  // Some unit tests use IOThread but do not initialize MetricsService. In that
+  // case it is fine not to return a real forwarder.
+  if (g_browser_process->metrics_service()) {
+    return g_browser_process->metrics_service()->GetDataUseForwardingCallback();
+  }
+  return metrics::UpdateUsagePrefCallbackType();
+}
+
 }  // namespace
 
 using content::BrowserThread;
 
 ProfileImplIOData::Handle::Handle(Profile* profile)
     : io_data_(new ProfileImplIOData),
       profile_(profile),
       initialized_(false) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(profile);
@@ skipping 296 matching lines @@
       media_cache_max_size(0),
       session_cookie_mode(
           content::CookieStoreConfig::EPHEMERAL_SESSION_COOKIES) {}
 
 ProfileImplIOData::LazyParams::~LazyParams() {}
 
 ProfileImplIOData::ProfileImplIOData()
     : ProfileIOData(Profile::REGULAR_PROFILE),
       http_server_properties_manager_(NULL),
       app_cache_max_size_(0),
-      app_media_cache_max_size_(0) {
+      app_media_cache_max_size_(0),
+      metrics_data_use_forwarder_(GetMetricsDataUseForwarder()) {
 }
 
 ProfileImplIOData::~ProfileImplIOData() {
   DestroyResourceContext();
 
   if (media_request_context_)
     media_request_context_->AssertNoURLRequests();
 }
 
 void ProfileImplIOData::InitializeInternal(
@@ skipping 37 matching lines @@
 
   main_context->set_transport_security_state(transport_security_state());
   main_context->set_ct_policy_enforcer(
       io_thread_globals->ct_policy_enforcer.get());
 
   main_context->set_net_log(io_thread->net_log());
 
   main_context_storage->set_network_delegate(
       data_reduction_proxy_io_data()->CreateNetworkDelegate(
           io_thread_globals->data_use_ascriber->CreateNetworkDelegate(
-              std::move(chrome_network_delegate),
-              io_thread->GetMetricsDataUseForwarder()),
+              std::move(chrome_network_delegate), metrics_data_use_forwarder_),

[Alexei Svitkine (slow), 2016/11/28 23:04:18]

Why can't it get it from IO thread like before?

It can still be the wrapper callback that's implemented by that file.

[robliao, 2016/11/29 18:17:43]

This changes the relationship from
1. MetricsService <- IOThread <- ProfileImpl[IOData] to

1. MetricsService <- IOThread
2. MetricsService <- ProfileImpl[IOData] 

The IO Thread only holds this callback as a convenience for ProfileImplIOData.
Having this as a direct relationship simplifies IOThread and allows
MetricsService to be the source of truth.

IOThread can do the dance it needs because it calls back from the IO thread.

[Alexei Svitkine (slow), 2016/11/29 18:34:34]

So my concern with this is now it's duplicating some logic between the two.
Specifically, you now need to have "if metrics service is not null" check in
both places.

I think if you go with my other suggestion of not having MetricsService expose a
callback anymore and for the callback to exist only to the new IOThread function
you added - then it would make sense to use this same callback here that you get
from IOThread.

[robliao, 2016/11/30 02:22:46]

Acknowledged. This is no longer an issue as I've pursued a minimal change given
I'm now changing the metrics service.

           true));
 
   main_context->set_host_resolver(
       io_thread_globals->host_resolver.get());
 
   main_context->set_http_auth_handler_factory(
       io_thread_globals->http_auth_handler_factory.get());
 
   main_context->set_proxy_service(proxy_service());
 
@@ skipping 306 matching lines @@
     const base::Closure& completion) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(initialized());
 
   DCHECK(transport_security_state());
   // Completes synchronously.
   transport_security_state()->DeleteAllDynamicDataSince(time);
   DCHECK(http_server_properties_manager_);
   http_server_properties_manager_->Clear(completion);
 }