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..d2af26b450b94967733b558044b9aba4cd92ae30 100644
--- a/base/metrics/user_metrics.cc
+++ b/base/metrics/user_metrics.cc
@@ -8,70 +8,67 @@
 
 #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()) {

[Alexei Svitkine (slow), 2016/04/13 14:43:12]

Nit: space after if. Maybe run git cl format?

[beaudoin, 2016/04/13 16:19:36]

Done.

+    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) {

[Alexei Svitkine (slow), 2016/04/13 14:43:12]

Nit: Use the following if it works:

for (const ActionCallback& callback : g_callbacks.Get()) {

}

[beaudoin, 2016/04/13 16:19:36]

Done.

+    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();

[Alexei Svitkine (slow), 2016/04/13 14:43:12]

Nit: non-const refs are discouraged - suggest changing this to a pointer.

[beaudoin, 2016/04/13 16:19:36]

Done.

[Ilya Sherman, 2016/04/13 19:23:14]

Hmm, I'm not aware of non-const refs being discouraged within function
implementations.  (Note: I'm fine with the pointer as well, so there's no need
to change anything here.  I'd just be interested to know whether the style guide
actually says something about this.)

+  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) {
+  DCHECK(!g_task_runner.Get() || g_task_runner.Get()->BelongsToCurrentThread());

[Ilya Sherman, 2016/04/13 01:06:27]

This DCHECK is a little bit odd to read right now, because it checks that the
|g_task_runner| belongs to the current thread, but that might not be the same
thread as the |task_runner|.  I think it would make sense to either test that
|g_task_runner| belongs to the same thread as |task_runner|, or to indirectly
test this by also adding a DCHECK to verify that the |task_runner| belongs to
the current thread.

[beaudoin, 2016/04/13 16:19:36]

That last DCHECK makes sense. It sounds like a bad idea to use thread A to set a
task_runner that would post to thread B.

Done.

+  g_task_runner.Get() = task_runner;
 }
 
 }  // namespace base