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..590ae38bdda803c534cb96a104e7aaa11318908a 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -15,7 +15,6 @@
 #include "base/debug/leak_tracker.h"
 #include "base/environment.h"
 #include "base/files/file_path.h"
-#include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
@@ -405,14 +404,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()));
 
@@ -1078,7 +1069,25 @@ net::URLRequestContext* IOThread::ConstructProxyScriptFetcherContext(
   return context;
 }
 
+void IOThread::InitMetricsDataUseForwarder() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+#if DCHECK_IS_ON()
+  metrics_data_use_forwarder_set_ = true;
+#endif
+  // 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();
+  }
+}
+
 const metrics::UpdateUsagePrefCallbackType&
 IOThread::GetMetricsDataUseForwarder() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+#if DCHECK_IS_ON()

[Alexei Svitkine (slow), 2016/11/22 16:29:46]

Nit: I'd remove these ifdefs.

I think the readability gains from not having them (in 3 places) outweigh the
extra 4 byte overhead in release builds.

[robliao, 2016/11/22 18:02:37]

Let's keep them for now. 4 bytes saved here and there adds up. If the owner has
strong feelings about this, than I'll remove them.

[mmenke, 2016/11/22 18:09:06]

Why is this even needed?  Can't we just do
DCHECK(!metrics_data_use_forwarder_.foo()), and get rid of the bool, where foo
is one of is_null/is_empty/empty (Not sure which it is)?

[robliao, 2016/11/22 18:10:27]

metrics_data_use_forwarder_ will be null/empty on non-Android builds. We can't
distinguish between an empty callback, a real callback, and a callback that was
never set.

[mmenke, 2016/11/22 18:20:58]

I don't think that's document in either io_thread.h or metrics_service.h. 
Document it in both places?

And yea, I'd rather just get rid of the ifdefs, too, even if it means keeping
the bool.

[robliao, 2016/11/22 19:00:57]

Done and done.

+  DCHECK(metrics_data_use_forwarder_set_);
+#endif
   return metrics_data_use_forwarder_;
 }