[fuchsia] Clean up LegacyMetricsUserEventRecorder.

Addresses some post-LGTM feedback items that narrowly missed the
initial landing of LegacyMetricsUserEventRecorder.

TBR=ddorwin@chromium.org

Bug: 1060768
Change-Id: I9f20281b11585439947de89eb71fa15aa6750a93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2107712Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751092}

fuchsia/base/legacymetrics_user_event_recorder.cc

@@ -31,11 +31,10 @@ LegacyMetricsUserActionRecorder::TakeEvents() {
   return std::move(events_);
 }
 
-void LegacyMetricsUserActionRecorder::OnUserAction(
-    const std::string& event_name,
-    base::TimeTicks time) {
+void LegacyMetricsUserActionRecorder::OnUserAction(const std::string& action,
+                                                   base::TimeTicks time) {
   fuchsia::legacymetrics::UserActionEvent fidl_event;
-  fidl_event.set_name(event_name);
+  fidl_event.set_name(action);
   fidl_event.set_time(time.ToZxTime());
   events_.push_back(std::move(fidl_event));
 }

fuchsia/base/legacymetrics_user_event_recorder.h

@@ -29,8 +29,8 @@ class LegacyMetricsUserActionRecorder {
   std::vector<fuchsia::legacymetrics::UserActionEvent> TakeEvents();
 
  private:
-  // base::UserActionCallback implementation.
-  void OnUserAction(const std::string& event_name, base::TimeTicks time);
+  // base::ActionCallback implementation.
+  void OnUserAction(const std::string& action, base::TimeTicks time);
 
   std::vector<fuchsia::legacymetrics::UserActionEvent> events_;
   const base::ActionCallback on_event_callback_;

fuchsia/base/legacymetrics_user_event_recorder_unittest.cc

@@ -11,45 +11,54 @@
 namespace cr_fuchsia {
 namespace {
 
-TEST(LegacyMetricsUserActionRecorderTest, ProduceAndConsume) {
-  constexpr char kExpectedUserAction1[] = "Hello";
-  constexpr char kExpectedUserAction2[] = "There";
+class LegacyMetricsUserActionRecorderTest : public testing::Test {
+ public:
+  LegacyMetricsUserActionRecorderTest() = default;
+  ~LegacyMetricsUserActionRecorderTest() override = default;
 
+  void SetUp() override {
+    base::SetRecordActionTaskRunner(base::ThreadTaskRunnerHandle::Get());
+  }
+
+ private:
   base::test::SingleThreadTaskEnvironment task_environment;
-  base::SetRecordActionTaskRunner(base::ThreadTaskRunnerHandle::Get());
+};
+
+TEST_F(LegacyMetricsUserActionRecorderTest, ProduceAndConsume) {
+  constexpr char kExpectedUserAction1[] = "Hello";
+  constexpr char kExpectedUserAction2[] = "There";
 
   zx_time_t time_start = base::TimeTicks::Now().ToZxTime();
-  auto buffer = std::make_unique<LegacyMetricsUserActionRecorder>();
+  LegacyMetricsUserActionRecorder buffer;
   base::RecordComputedAction(kExpectedUserAction1);
-  EXPECT_TRUE(buffer->HasEvents());
+  EXPECT_TRUE(buffer.HasEvents());
   base::RecordComputedAction(kExpectedUserAction2);
 
-  auto events = buffer->TakeEvents();
-  EXPECT_FALSE(buffer->HasEvents());
+  auto events = buffer.TakeEvents();
+  EXPECT_FALSE(buffer.HasEvents());
   EXPECT_EQ(2u, events.size());
 
+  // Verify the contents of the buffer are as expected.
   EXPECT_EQ(kExpectedUserAction1, events[0].name());
   EXPECT_GE(events[0].time(), time_start);
-
   EXPECT_EQ(kExpectedUserAction2, events[1].name());
   EXPECT_GE(events[1].time(), time_start);
-
   EXPECT_GE(events[1].time(), events[0].time());
 
-  EXPECT_TRUE(buffer->TakeEvents().empty());
+  // Verify that the buffer is now empty.
+  EXPECT_TRUE(buffer.TakeEvents().empty());
 
+  // Add more data to the buffer, and then verify it, to ensure that recording
+  // continues to work.
   base::RecordComputedAction(kExpectedUserAction2);
-  EXPECT_TRUE(buffer->HasEvents());
-  events = buffer->TakeEvents();
-  EXPECT_FALSE(buffer->HasEvents());
+  EXPECT_TRUE(buffer.HasEvents());
+  events = buffer.TakeEvents();
+  EXPECT_FALSE(buffer.HasEvents());
   EXPECT_EQ(1u, events.size());
   EXPECT_EQ(kExpectedUserAction2, events[0].name());
 }
 
-TEST(LegacyMetricsUserActionRecorderTest, RecorderDeleted) {
-  base::test::SingleThreadTaskEnvironment task_environment;
-  base::SetRecordActionTaskRunner(base::ThreadTaskRunnerHandle::Get());
-
+TEST_F(LegacyMetricsUserActionRecorderTest, RecorderDeleted) {
   auto buffer = std::make_unique<LegacyMetricsUserActionRecorder>();
   buffer.reset();
 
@@ -58,11 +67,9 @@ TEST(LegacyMetricsUserActionRecorderTest, RecorderDeleted) {
   base::RecordComputedAction("NoCrashingPlz");
 }
 
-TEST(LegacyMetricsUserActionRecorderTest, EmptyBuffer) {
-  base::test::SingleThreadTaskEnvironment task_environment;
-  base::SetRecordActionTaskRunner(base::ThreadTaskRunnerHandle::Get());
-  auto buffer = std::make_unique<LegacyMetricsUserActionRecorder>();
-  EXPECT_FALSE(buffer->HasEvents());
+TEST_F(LegacyMetricsUserActionRecorderTest, EmptyBuffer) {
+  LegacyMetricsUserActionRecorder buffer;
+  EXPECT_FALSE(buffer.HasEvents());
 }
 
 }  // namespace