Decouple Metrics Initialization from IO Thread Initialization

chrome/browser/io_thread.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/io_thread.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #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"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 369 matching lines @@
   if (value)
     value->GetAsBoolean(&is_quic_allowed_by_policy_);
 
   value = policy_service
               ->GetPolicies(policy::PolicyNamespace(
                   policy::POLICY_DOMAIN_CHROME, std::string()))
               .GetValue(policy::key::kHttp09OnNonDefaultPortsEnabled);
   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()));
 
   BrowserThread::SetDelegate(BrowserThread::IO, this);
 }
 
 IOThread::~IOThread() {
   // This isn't needed for production code, but in tests, IOThread may
   // be multiply constructed.
   BrowserThread::SetDelegate(BrowserThread::IO, NULL);
@@ skipping 645 matching lines @@
 
   context->set_job_factory(
       globals->proxy_script_fetcher_url_request_job_factory.get());
 
   // TODO(rtenneti): We should probably use HttpServerPropertiesManager for the
   // system URLRequestContext too. There's no reason this should be tied to a
   // profile.
   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_;
 }