Report user actions when gesture starts and stops in user_model.

components/scheduler/renderer/user_model_unittest.cc

Index: components/scheduler/renderer/user_model_unittest.cc
diff --git a/components/scheduler/renderer/user_model_unittest.cc b/components/scheduler/renderer/user_model_unittest.cc
index ebf3ff345c7bbbc282029049de09f4e450d548be..0425486a166ffc05268486c8f3da36f3e190ecd3 100644
--- a/components/scheduler/renderer/user_model_unittest.cc
+++ b/components/scheduler/renderer/user_model_unittest.cc
@@ -4,7 +4,11 @@
 
 #include "components/scheduler/renderer/user_model.h"
 
+#include "base/metrics/field_trial.h"
+#include "base/metrics/user_metrics.h"
+#include "base/metrics/user_metrics_action.h"
 #include "base/test/simple_test_tick_clock.h"
+#include "base/test/test_simple_task_runner.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -12,14 +16,22 @@ namespace scheduler {
 
 class UserModelTest : public testing::Test {
  public:
-  UserModelTest() {}
-  ~UserModelTest() override {}
+  UserModelTest() : mock_task_runner_(new base::TestSimpleTaskRunner()) {}
+  ~UserModelTest() override { base::RemoveActionCallback(action_callback_); }
 
   void SetUp() override {
     clock_.reset(new base::SimpleTestTickClock());
     clock_->Advance(base::TimeDelta::FromMicroseconds(5000));
 
-    user_model_.reset(new UserModel());
+    user_model_.reset(new UserModel(mock_task_runner_));
+
+    // Register a mock user action callback for testing.
+    num_actions_recorded_ = 0;
+    last_action_recorded_ = "";
+    base::RemoveActionCallback(action_callback_);

[Ilya Sherman, 2016/03/21 08:09:44]

Why is this line needed here?  Seems like something that should be done in
teardown.  (FWIW, I also find it pretty confusing that you both have a
constructor and a setup method, both of which seem to do initialization work. 
Is there any particular reason not to do all of the work in the constructor?)

[beaudoin, 2016/03/23 02:46:42]

SetUp is the standard ways tests are initialized around this area, so I'll leave
it as that. I've moved the new base::TestSimpleTaskRunner() to the SetUp
function. I've removed the tear down part, it's no longer needed with
UserActionTester.

+    action_callback_ = base::Bind(&UserModelTest::RecordComputedAction,
+                                  base::Unretained(this));
+    base::AddActionCallback(action_callback_);
   }
 
  protected:
@@ -33,8 +45,19 @@ class UserModelTest : public testing::Test {
         UserModel::kExpectSubsequentGestureMillis);
   }
 
+  void RecordComputedAction(const std::string& action) {

[Ilya Sherman, 2016/03/21 08:09:44]

nit: What does the "Computed" part of this name refer to?

[beaudoin, 2016/03/23 02:46:41]

Gone with UserActionTester.

+    num_actions_recorded_++;

[Ilya Sherman, 2016/03/21 08:09:44]

nit: preincrement

[beaudoin, 2016/03/23 02:46:42]

Gone.

+    last_action_recorded_ = action;
+  }
+
   scoped_ptr<base::SimpleTestTickClock> clock_;
   scoped_ptr<UserModel> user_model_;
+  scoped_refptr<base::TestSimpleTaskRunner> mock_task_runner_;
+  base::ActionCallback action_callback_;
+
+  // Tracks user actions that were recorded.
+  int num_actions_recorded_;
+  std::string last_action_recorded_;
 };
 
 TEST_F(UserModelTest, TimeLeftInUserGesture_NoInput) {
@@ -237,18 +260,35 @@ TEST_F(UserModelTest, IsGestureExpectedToContinue_ShortlyAfterGestureStarted) {
             prediction_valid_duration);
 }
 
-TEST_F(UserModelTest, IsGestureExpectedToContinue_LongAfterGestureStarted) {
+TEST_F(UserModelTest, AreActionsLogged) {
+  // Add the action logging feature.
+  base::FeatureList::ClearInstanceForTesting();
+  scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
+  feature_list->InitializeFromCommandLine(features::kRecordGestureAction.name,
+                                          "");

[Ilya Sherman, 2016/03/21 08:09:44]

nit: Prefer std::string() to an empty string literal.

[beaudoin, 2016/03/23 02:46:42]

Done.

+  base::FeatureList::SetInstance(std::move(feature_list));
+
+  // Reset user model so it uses the new feature list.
+  user_model_.reset(new UserModel(mock_task_runner_));
+
+  EXPECT_EQ(0, num_actions_recorded_);
+
   user_model_->DidStartProcessingInputEvent(
       blink::WebInputEvent::Type::GestureScrollBegin, clock_->NowTicks());
+  mock_task_runner_->RunPendingTasks();
 
-  base::TimeDelta delta(base::TimeDelta::FromMilliseconds(
-      UserModel::kMedianGestureDurationMillis * 2));
+  EXPECT_EQ(1, num_actions_recorded_);
+  EXPECT_EQ("RendererScheduler.UserModel.GestureStart", last_action_recorded_);
+
+  base::TimeDelta delta(base::TimeDelta::FromMilliseconds(500));
   clock_->Advance(delta);
 
-  base::TimeDelta prediction_valid_duration;
-  EXPECT_FALSE(user_model_->IsGestureExpectedToContinue(
-      clock_->NowTicks(), &prediction_valid_duration));
-  EXPECT_EQ(base::TimeDelta(), prediction_valid_duration);
+  user_model_->DidStartProcessingInputEvent(
+      blink::WebInputEvent::Type::GestureScrollEnd, clock_->NowTicks());
+  mock_task_runner_->RunPendingTasks();
+
+  EXPECT_EQ(2, num_actions_recorded_);
+  EXPECT_EQ("RendererScheduler.UserModel.GestureEnd", last_action_recorded_);

[Ilya Sherman, 2016/03/21 08:09:44]

It would probably be better to use the code from base/test/user_action_tester.h
rather than re-implementing it, if possible.  I'm not 100% sure whether the code
would Just Work in the renderer process, though.

[beaudoin, 2016/03/23 02:46:41]

Looks like it's almost a drop-in replacement for what I did. Thanks for the
pointer!

 }
 
 }  // namespace scheduler