[Autofill Assistant] Addressed minor nits regarding the stopwatch.

Bug: b/170722915
Change-Id: Ide6bebda310e2c361f117988af701f28cc9bc02a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485510
Commit-Queue: Michele Mancina <micantox@google.com>
Reviewed-by: Sandro Maggi <sandromaggi@google.com>
Auto-Submit: Michele Mancina <micantox@google.com>
Cr-Commit-Position: refs/heads/master@{#819362}

components/autofill_assistant/browser/actions/stopwatch.cc

 // Copyright 2020 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
+
 #include "components/autofill_assistant/browser/actions/stopwatch.h"
 
 #include <ostream>
@@ -15,21 +16,17 @@ Stopwatch::Stopwatch() {
 }
 
 bool Stopwatch::Start() {
-  if (!running_) {
-    running_ = true;
-    start_time_ = base::TimeTicks::Now();
-    return true;
+  if (running_) {
+    return false;
   }
-  return false;
+  running_ = true;
+  start_time_ = base::TimeTicks::Now();
+  return true;
 }
 
-bool Stopwatch::StartAt(base::TimeTicks start_time) {
-  if (!running_) {
-    running_ = true;
-    start_time_ = start_time;
-    return true;
-  }
-  return false;
+void Stopwatch::StartAt(base::TimeTicks start_time) {
+  running_ = true;
+  start_time_ = start_time;
 }
 
 bool Stopwatch::Stop() {

components/autofill_assistant/browser/actions/stopwatch.h

@@ -17,18 +17,25 @@ class Stopwatch {
  public:
   Stopwatch();
   // Start measuring the time when the method is called.
+  // Returns true if the stopwatch was not already running.
   bool Start();
-  // Starts measuring the time from `start_time`.
-  bool StartAt(base::TimeTicks start_time);
-  // Stops the stopwatch and, if it was running, adds to the elapsed time.
+  // Starts measuring the time from `start_time`, if the stopwatch was already
+  // running, it simply updates the recorded start time of the current run.
+  void StartAt(base::TimeTicks start_time);
+  // Stops the stopwatch and, if it was running, adds to the cumulative elapsed
+  // time. If the stopwatch was not running, it has no effect and returns false.
   bool Stop();
-  // Same as above, but at `stop_time` rather than the current time.
+  // Stops the stopwatch at `stop_time` and, if it was running, adds to the
+  // cumulative elapsed time. If the stopwatch was not running, it has no effect
+  // and returns false.
   bool StopAt(base::TimeTicks stop_time);
   // Adds `time` to the cumulative elapsed time held by this stopwatch.
   void AddTime(base::TimeDelta time);
   // Remove `time` from the cumulative elapsed time held by this stopwatch.
   void RemoveTime(base::TimeDelta time);
+  // Returns the total time accumulated by this stopwatch.
   base::TimeDelta TotalElapsed() const;
+  // Whether the stopwatch is running or not.
   bool IsRunning() const;
 
   friend std::ostream& operator<<(std::ostream& out, const Stopwatch& action);

components/autofill_assistant/browser/actions/stopwatch_unittest.cc

@@ -26,7 +26,7 @@ class StopwatchTest : public testing::Test {
   Stopwatch stopwatch_;
 };
 
-TEST_F(StopwatchTest, StopwatchSimpleRun) {
+TEST_F(StopwatchTest, SimpleRun) {
   base::subtle::ScopedTimeClockOverrides overrides(
       nullptr, &TimeTicksOverride::Now, nullptr);
   EXPECT_TRUE(stopwatch_.Start());
@@ -50,7 +50,7 @@ TEST_F(StopwatchTest, StopwatchSimpleRun) {
   EXPECT_EQ(base::TimeDelta::FromSeconds(5), stopwatch_.TotalElapsed());
 }
 
-TEST_F(StopwatchTest, StopwatchAddTime) {
+TEST_F(StopwatchTest, AddTime) {
   base::subtle::ScopedTimeClockOverrides overrides(
       nullptr, &TimeTicksOverride::Now, nullptr);
   stopwatch_.Start();
@@ -60,7 +60,7 @@ TEST_F(StopwatchTest, StopwatchAddTime) {
   EXPECT_EQ(base::TimeDelta::FromSeconds(3), stopwatch_.TotalElapsed());
 }
 
-TEST_F(StopwatchTest, StopwatchRemoveTime) {
+TEST_F(StopwatchTest, RemoveTime) {
   base::subtle::ScopedTimeClockOverrides overrides(
       nullptr, &TimeTicksOverride::Now, nullptr);
   stopwatch_.Start();
@@ -70,7 +70,7 @@ TEST_F(StopwatchTest, StopwatchRemoveTime) {
   EXPECT_EQ(base::TimeDelta::FromSeconds(1), stopwatch_.TotalElapsed());
 }
 
-TEST_F(StopwatchTest, StopwatchRemoveGreaterThanElapsed) {
+TEST_F(StopwatchTest, RemoveGreaterThanElapsed) {
   base::subtle::ScopedTimeClockOverrides overrides(
       nullptr, &TimeTicksOverride::Now, nullptr);
   stopwatch_.Start();
@@ -80,6 +80,10 @@ TEST_F(StopwatchTest, StopwatchRemoveGreaterThanElapsed) {
   EXPECT_EQ(base::TimeDelta::FromSeconds(0), stopwatch_.TotalElapsed());
 }
 
+// This parameterized test uses 4 parameters: time to start at, time to stop at,
+// expected time before the stop is invoked and expected time at the end.
+// If start at or stop at times are 0, the regular Start() and Stop() methods
+// are used instead of the StartAt(time) or StopAt(time).
 class StopwatchStartStopTest
     : public StopwatchTest,
       public testing::WithParamInterface<std::tuple<long, long, long, long>> {};