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..68fb9a2ba4f7bfe0d78331e14000a92b5459bda1 100644
--- a/base/metrics/user_metrics.cc
+++ b/base/metrics/user_metrics.cc
@@ -8,9 +8,10 @@
 
 #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 {
@@ -22,7 +23,6 @@ class 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);
     }
@@ -30,14 +30,12 @@ class Callbacks {
 
   // 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);
@@ -47,31 +45,59 @@ class Callbacks {
   }
 
  private:
-  base::ThreadChecker thread_checker_;
   std::vector<ActionCallback> callbacks_;
 
   DISALLOW_COPY_AND_ASSIGN(Callbacks);
 };

[Ilya Sherman, 2016/04/06 02:36:28]

I don't think that it's appropriate, or necessary, to remove the thread checking
from this class.  Actions and callbacks should still be recorded on a single
thread.  Only the Record(Computed)Action functions should need the thread hop.

[beaudoin, 2016/04/06 15:06:13]

Ok. I've removed the thread hop for Add/Remove and added back the
thread_checker_. I'm a bit annoyed by the fact that we have two "thread
trackers" though: thread_checker_ and g_task_runner. If you're fine doing the
DCHECK only in the exposed functions I could do DCHECK(g_task_runner.Get() &&
g_task_runner.Get()->BelongsToCurrentThread());

Truth is, we could get rid of Callbacks entirely and just have a global
std::vector<ActionCallback>...

[Ilya Sherman, 2016/04/06 21:48:48]

Yeah, I'd be fine with the DCHECKs you described, and getting rid of the
Callbacks wrapper class.

[beaudoin, 2016/04/07 18:12:34]

Done.

 
 base::LazyInstance<Callbacks> 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) {
+  DCHECK(g_task_runner.Get());
+  if (!g_task_runner.Get()->BelongsToCurrentThread()) {
+    g_task_runner.Get()->PostTask(FROM_HERE,
+        base::Bind(&RecordComputedAction, action));
+    return;
+  }
   g_callbacks.Get().Record(action);
 }
 
 void AddActionCallback(const ActionCallback& callback) {
+  DCHECK(g_task_runner.Get());
+  if (!g_task_runner.Get()->BelongsToCurrentThread()) {
+    g_task_runner.Get()->PostTask(FROM_HERE,
+        base::Bind(&AddActionCallback, callback));
+    return;
+  }
   g_callbacks.Get().AddCallback(callback);
 }
 
 void RemoveActionCallback(const ActionCallback& callback) {
+  DCHECK(g_task_runner.Get());
+  if (!g_task_runner.Get()->BelongsToCurrentThread()) {
+    g_task_runner.Get()->PostTask(FROM_HERE,
+        base::Bind(&RemoveActionCallback, callback));
+    return;
+  }
   g_callbacks.Get().RemoveCallback(callback);
+}
 
+void SetRecordActionTaskRunner(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+  if (g_task_runner.Get() && !g_task_runner.Get()->BelongsToCurrentThread()) {
+    g_task_runner.Get()->PostTask(FROM_HERE,
+        base::Bind(&SetRecordActionTaskRunner, task_runner));

[Ilya Sherman, 2016/04/06 02:36:28]

The task runner should only be set once, IMO, so I don't really understand what
the purpose of this code is.

[beaudoin, 2016/04/06 15:06:13]

Done.

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