Delivering the FIRST_NONEMPTY_PAINT phase changing event to base/

components/metrics/profiler/tracking_synchronizer_unittest.cc

Index: components/metrics/profiler/tracking_synchronizer_unittest.cc
diff --git a/components/metrics/profiler/tracking_synchronizer_unittest.cc b/components/metrics/profiler/tracking_synchronizer_unittest.cc
index a597a477c64a0f5a8cdb6303bd24231149c8928a..89278451af4c34cb3af1e9f1d93be082cd6a94cd 100644
--- a/components/metrics/profiler/tracking_synchronizer_unittest.cc
+++ b/components/metrics/profiler/tracking_synchronizer_unittest.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/memory/ref_counted.h"
+#include "base/test/simple_test_tick_clock.h"
 #include "base/tracked_objects.h"
 #include "components/metrics/profiler/tracking_synchronizer.h"
 #include "components/metrics/profiler/tracking_synchronizer_observer.h"
@@ -77,9 +78,24 @@ class TestObserver : public TrackingSynchronizerObserver {
   DISALLOW_COPY_AND_ASSIGN(TestObserver);
 };
 
-base::TimeTicks TestTimeFromMs(int64 ms) {
-  return base::TimeTicks() + base::TimeDelta::FromMilliseconds(ms);
-}
+class TestTrackingSynchronizer : public TrackingSynchronizer {
+ public:
+  explicit TestTrackingSynchronizer(base::TickClock* clock)
+      : TrackingSynchronizer(clock) {}
+
+  ~TestTrackingSynchronizer() {}
+
+  void RegisterPhaseCompletion(
+      ProfilerEventProto::ProfilerEvent profiling_event) {
+    TrackingSynchronizer::RegisterPhaseCompletion(profiling_event);
+  }
+
+  void SendData(const tracked_objects::ProcessDataSnapshot& profiler_data,
+                content::ProcessType process_type,
+                TrackingSynchronizerObserver* observer) const {
+    TrackingSynchronizer::SendData(profiler_data, process_type, observer);
+  }
+};

[Ilya Sherman, 2015/04/07 22:58:35]

Can you test the public API of the class, rather than exposing these
implementation details for the test to call directly?

[vadimt, 2015/04/08 01:09:50]

Not sure it's worth the effort. The purpose of the test if to test the
non-trivial logic of TrackingSynchronizer::SendData rather than the boilerplate
logic of collecting snapshots from the system. I'd need to mock the snapshot
collection (which at the first sight requires significant effort) for
essentially same result. So, testing SendData directly looks like a reasonable
shortcut for me...

 
 }  // namespace
 
@@ -88,13 +104,17 @@ TEST(TrackingSynchronizerTest, ProfilerData) {
 #if !defined(OS_IOS)
   content::TestBrowserThreadBundle thread_bundle;
 #endif
-  scoped_refptr<TrackingSynchronizer> tracking_synchronizer =
-      new TrackingSynchronizer(TestTimeFromMs(111));
 
-  // Mimic a phase change event.
-  tracking_synchronizer->phase_completion_events_sequence_.push_back(
+  auto clock = new base::SimpleTestTickClock();

[Ilya Sherman, 2015/04/07 22:58:35]

Please document why this isn't leaked, since it looks a lot like a memory leak.

[vadimt, 2015/04/08 01:09:50]

Done.

+  clock->Advance(base::TimeDelta::FromMilliseconds(111));
+
+  scoped_refptr<TestTrackingSynchronizer> tracking_synchronizer =
+      new TestTrackingSynchronizer(clock);
+
+  clock->Advance(base::TimeDelta::FromMilliseconds(222));
+
+  tracking_synchronizer->RegisterPhaseCompletion(
       ProfilerEventProto::EVENT_FIRST_NONEMPTY_PAINT);
-  tracking_synchronizer->phase_start_times_.push_back(TestTimeFromMs(333));
 
   tracked_objects::ProcessDataSnapshot profiler_data;
   ProcessDataPhaseSnapshot snapshot0;
@@ -109,10 +129,10 @@ TEST(TrackingSynchronizerTest, ProfilerData) {
   profiler_data.phased_process_data_snapshots[1] = snapshot1;
   profiler_data.process_id = 239;
 
+  clock->Advance(base::TimeDelta::FromMilliseconds(444));
   TestObserver test_observer;
-  tracking_synchronizer->SendData(profiler_data,
-                                  content::ProcessType::PROCESS_TYPE_PLUGIN,
-                                  TestTimeFromMs(777), &test_observer);
+  tracking_synchronizer->SendData(
+      profiler_data, content::ProcessType::PROCESS_TYPE_PLUGIN, &test_observer);
 }
 
 }  // namespace metrics