Move the thread hop for UMA user actions from content:: to base::.

base/metrics/user_metrics.cc

Index: base/metrics/user_metrics.cc
diff --git a/base/metrics/user_metrics.cc b/base/metrics/user_metrics.cc
index 55467e66eebba0f4a84f99444a08be935020e99b..6033cc17c26aff11123b60e8b9b4e15b074d94b1 100644
--- a/base/metrics/user_metrics.cc
+++ b/base/metrics/user_metrics.cc
@@ -8,70 +8,66 @@
 
 #include <vector>
 
+#include "base/bind.h"
 #include "base/lazy_instance.h"
+#include "base/location.h"
 #include "base/macros.h"
 #include "base/threading/thread_checker.h"
 
 namespace base {
 namespace {
 
-// A helper class for tracking callbacks and ensuring thread-safety.
-class Callbacks {
- public:
-  Callbacks() {}
-
-  // Records the |action|.
-  void Record(const std::string& action) {
-    DCHECK(thread_checker_.CalledOnValidThread());
-    for (size_t i = 0; i < callbacks_.size(); ++i) {
-      callbacks_[i].Run(action);
-    }
-  }
-
-  // Adds |callback| to the list of |callbacks_|.
-  void AddCallback(const ActionCallback& callback) {
-    DCHECK(thread_checker_.CalledOnValidThread());
-    callbacks_.push_back(callback);
-  }
-
-  // Removes the first instance of |callback| from the list of |callbacks_|, if
-  // there is one.
-  void RemoveCallback(const ActionCallback& callback) {
-    DCHECK(thread_checker_.CalledOnValidThread());
-    for (size_t i = 0; i < callbacks_.size(); ++i) {
-      if (callbacks_[i].Equals(callback)) {
-        callbacks_.erase(callbacks_.begin() + i);
-        return;
-      }
-    }
-  }
-
- private:
-  base::ThreadChecker thread_checker_;
-  std::vector<ActionCallback> callbacks_;
-
-  DISALLOW_COPY_AND_ASSIGN(Callbacks);
-};
-
-base::LazyInstance<Callbacks> g_callbacks = LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<std::vector<ActionCallback>> g_callbacks =
+    LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<scoped_refptr<base::SingleThreadTaskRunner>> g_task_runner =
+    LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
 void RecordAction(const UserMetricsAction& action) {
-  g_callbacks.Get().Record(action.str_);
+  RecordComputedAction(action.str_);
 }
 
 void RecordComputedAction(const std::string& action) {
-  g_callbacks.Get().Record(action);
+  if(!g_task_runner.Get()) {
+    DCHECK(g_callbacks.Get().empty());
+    return;
+  }
+
+  if (!g_task_runner.Get()->BelongsToCurrentThread()) {
+    g_task_runner.Get()->PostTask(FROM_HERE,
+        base::Bind(&RecordComputedAction, action));
+    return;
+  }
+
+  const std::vector<ActionCallback>& callbacks = g_callbacks.Get();
+  for (size_t i = 0; i < callbacks.size(); ++i) {
+    callbacks[i].Run(action);
+  }
 }
 
 void AddActionCallback(const ActionCallback& callback) {
-  g_callbacks.Get().AddCallback(callback);
+  // Only allow adding a callback if the task runner is set.
+  DCHECK(g_task_runner.Get());
+  DCHECK(g_task_runner.Get()->BelongsToCurrentThread());
+  g_callbacks.Get().push_back(callback);
 }
 
 void RemoveActionCallback(const ActionCallback& callback) {
-  g_callbacks.Get().RemoveCallback(callback);
+  DCHECK(g_task_runner.Get());
+  DCHECK(g_task_runner.Get()->BelongsToCurrentThread());
+  std::vector<ActionCallback>& callbacks = g_callbacks.Get();
+  for (size_t i = 0; i < callbacks.size(); ++i) {
+    if (callbacks[i].Equals(callback)) {
+      callbacks.erase(callbacks.begin() + i);
+      return;
+    }
+  }
+}
 
+void SetRecordActionTaskRunner(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+  g_task_runner.Get() = task_runner;

[Ilya Sherman, 2016/04/07 23:49:51]

Hmm, I don't like removing the DCHECK here to ensure that a task runner is only
set once in production code.  ResetRecordActionTaskRunnerForTest() is one
(potentially good) option.  Another would be to ignore any task runners that
belong to the current thread, since the task runner is only used for
thread-hopping.

[beaudoin, 2016/04/08 13:56:00]

That second approach is clever, but unfortunately it will break the assumption
(above) that RecordComputedAction is only called when either the g_task_runner
is set or g_callbacks is empty.

I'll go with SetRecordActionTaskRunnerForTesting().

[Ilya Sherman, 2016/04/09 07:06:10]

Hmm, maybe I didn't phrase my suggestion very clearly.  I meant, in the case
that a task runner is already set, it would be fine to ignore subsequent calls
to SetRecordActionTaskRunner() in the case that the new and the old task runner
run on the same thread.  (If that still breaks the assumption that
RecordComputedAction is only called when either the g_task_runner is set or
g_callbacks is empty, then I'm missing something, and would very much appreciate
a clarification =))

[beaudoin1, 2016/04/09 19:07:35]

Ah! I thought you meant to ignore it even the first time it's set (if it's on
the same thread), but it would not make much sense.

I like that approach better, I'll migrate to it.

 }
 
 }  // namespace base