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"
@@ skipping 233 matching lines @@
 int GetSwitchValueAsInt(const base::CommandLine& command_line,
                         const std::string& switch_name) {
   int value;
   if (!base::StringToInt(command_line.GetSwitchValueASCII(switch_name),
                          &value)) {
     return 0;
   }
   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 {
  public:
   explicit SystemURLRequestContextGetter(IOThread* io_thread);
 
   // Implementation for net::UrlRequestContextGetter.
   net::URLRequestContext* GetURLRequestContext() override;
   scoped_refptr<base::SingleThreadTaskRunner> GetNetworkTaskRunner()
       const override;
@@ skipping 134 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 98 matching lines @@
 #if BUILDFLAG(ANDROID_JAVA_UI)
   globals_->external_data_use_observer.reset(
       new chrome::android::ExternalDataUseObserver(
           globals_->data_use_aggregator.get(),
           BrowserThread::GetTaskRunnerForThread(BrowserThread::IO),
           BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)));
 #endif
 
   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_);
 
   std::map<std::string, std::string> network_quality_estimator_params;
   variations::GetVariationParams(kNetworkQualityEstimatorFieldTrialName,
                                  &network_quality_estimator_params);
 
   std::unique_ptr<net::ExternalEstimateProvider> external_estimate_provider;
 #if BUILDFLAG(ANDROID_JAVA_UI)
   external_estimate_provider.reset(
@@ skipping 525 matching lines @@
       std::move(job_factory);
 
   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;
 }
-
-const metrics::UpdateUsagePrefCallbackType&
-IOThread::GetMetricsDataUseForwarder() {
-  return metrics_data_use_forwarder_;
-}