Decouple Metrics Initialization from IO Thread Initialization

chrome/browser/io_thread.cc

Index: chrome/browser/io_thread.cc
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index 662c9e7b349f2cd1359adea8257f4aa60cd90f37..8e8bf5012a5a19268a01ef3d90e5f1d217889e3b 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -251,6 +251,25 @@ int GetSwitchValueAsInt(const base::CommandLine& command_line,
   return value;
 }
 
+void MetricsDataUseForwarderCallback(
+    const std::string& service_name, int message_size, bool is_cellular) {
+  BrowserThread::PostTask(
+      BrowserThread::UI,
+      FROM_HERE,
+      base::Bind([](
+          const std::string& service_name, int message_size, bool is_cellular) {
+        // Some unit tests use IOThread but do not initialize MetricsService. In
+        // that case it's fine to skip the callback.
+        auto metrics_service = g_browser_process->metrics_service();
+        if (metrics_service) {
+          metrics::UpdateUsagePrefCallbackType callback =
+              metrics_service->GetDataUseForwardingCallback();

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

If you're doing like this, then maybe you don't need the intermediate callback?

i.e. you could just call the thing from this point directly instead of get the
callback and call that?

Because I definitely don't love this double callback stuff.

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

Because we're doing a IO->UI hop, if we wanted to save off the callback we would
need to do another UI->IO hop to safely save the value (along with dealing with
any inflight calls that could be in progress). Given that these calls don't
block the critical path of anything (they're async), this seemed preferable.

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

Sorry, let me clarify - I'm not suggesting to save off the callback.

I'm suggesting instead of calling
"metrics_service->GetDataUseForwardingCallback();", to just call:

metrics_service->OnDataUse(service_name, message_size, is_cellular);

And then not to have a need for an API on metrics service to get the callback.
(Where you would replace the metrics service GetDataUseForwardingCallback API
with OnDataUse function.)

Basically the current complexity of the callback stuff comes from the fact that
it needs to deal with both being called from non-UI thread and the relevant
object in metrics service not existing (on some platforms).

If you're already solving the threading stuff by ensuring we're executing on the
UI thread (via the code you wrote here), then we can clean up a lot of this
complexity and not have any functions that return callbacks in the metrics
service and related code.

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

Gotcha. That works for me. I didn't know if you folks were planning on using
this callback elsewhere. Changed.

+          if (!callback.is_null())
+            callback.Run(service_name, message_size, is_cellular);
+        }
+      }, service_name, message_size, is_cellular));
+}
+
 }  // namespace
 
 class SystemURLRequestContextGetter : public net::URLRequestContextGetter {
@@ -405,14 +424,6 @@ IOThread::IOThread(
   if (value)
     value->GetAsBoolean(&http_09_on_non_default_ports_enabled_);
 
-  // Some unit tests use IOThread but do not initialize MetricsService. In that
-  // case it is fine not to have |metrics_data_use_forwarder_|.
-  if (g_browser_process->metrics_service()) {
-    // Callback for updating data use prefs should be obtained on UI thread.
-    metrics_data_use_forwarder_ =
-        g_browser_process->metrics_service()->GetDataUseForwardingCallback();
-  }
-
   chrome_browser_net::SetGlobalSTHDistributor(
       std::unique_ptr<net::ct::STHDistributor>(new net::ct::STHDistributor()));
 
@@ -531,7 +542,8 @@ void IOThread::Init() {
 
   globals_->system_network_delegate =
       globals_->data_use_ascriber->CreateNetworkDelegate(
-          std::move(chrome_network_delegate), metrics_data_use_forwarder_);
+          std::move(chrome_network_delegate),
+          base::Bind(&MetricsDataUseForwarderCallback));
 
   globals_->host_resolver = CreateGlobalHostResolver(net_log_);
 
@@ -1077,8 +1089,3 @@ net::URLRequestContext* IOThread::ConstructProxyScriptFetcherContext(
   // profile.
   return context;
 }
-
-const metrics::UpdateUsagePrefCallbackType&
-IOThread::GetMetricsDataUseForwarder() {
-  return metrics_data_use_forwarder_;
-}