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..23ca9fae671d97461898472f4fffbbf17de6a437 100644
--- a/components/metrics/profiler/tracking_synchronizer_unittest.cc
+++ b/components/metrics/profiler/tracking_synchronizer_unittest.cc
@@ -77,6 +77,27 @@ class TestObserver : public TrackingSynchronizerObserver {
   DISALLOW_COPY_AND_ASSIGN(TestObserver);
 };
 
+class TestTrackingSynchronizer : public TrackingSynchronizer {
+ public:
+  explicit TestTrackingSynchronizer(base::TimeTicks now)
+      : TrackingSynchronizer(now) {}
+
+  void RegisterPhaseCompletion(
+      ProfilerEventProto::ProfilerEvent profiling_event,
+      base::TimeTicks now) {
+    TrackingSynchronizer::RegisterPhaseCompletion(profiling_event, now);
+  }

[Ilya Sherman, 2015/04/07 01:15:30]

nit: Can this be written simply as "using
TrackingSynchronizer::RegisiterPhaseCompletion;"?

[vadimt, 2015/04/07 21:44:14]

Technically, this works, at least on Windows, but our guide allows using this
only for constructors. I'd not do this.

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

Can you please link to the section of the guide where this is described?  I've
seen this used fairly frequently in tests.

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

https://chromium-cpp.appspot.com/, Inherited Constructors.
BTW, I've misread this: inherited constructors are blacklisted.
Speaking of inherited methods, they are not present in the allowed features
list, which makes me think that the tests you've seen are wrong.

[Ilya Sherman, 2015/04/08 23:36:14]

This isn't a C++11 feature, so that list is not relevant.  You're welcome to
raise the issue on chromium-dev if you're really concerned.

[vadimt, 2015/04/09 00:15:03]

Done.

+
+  ~TestTrackingSynchronizer() {}

[Ilya Sherman, 2015/04/07 01:15:30]

nit: Please move this up to the constructor.

[vadimt, 2015/04/07 21:44:14]

Done.

+
+  void SendData(const tracked_objects::ProcessDataSnapshot& profiler_data,
+                content::ProcessType process_type,
+                base::TimeTicks now,
+                TrackingSynchronizerObserver* observer) const {
+    TrackingSynchronizer::SendData(profiler_data, process_type, now, observer);
+  }
+};

[Ilya Sherman, 2015/04/07 01:15:30]

I would strongly prefer to use a TickClock in the TrackingSynchronizer class,
rather than needing to jump through hoops to pass a test time to each method
that wants one.

[vadimt, 2015/04/07 21:44:14]

Done.

+
 base::TimeTicks TestTimeFromMs(int64 ms) {
   return base::TimeTicks() + base::TimeDelta::FromMilliseconds(ms);
 }
@@ -88,13 +109,12 @@ 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(
-      ProfilerEventProto::EVENT_FIRST_NONEMPTY_PAINT);
-  tracking_synchronizer->phase_start_times_.push_back(TestTimeFromMs(333));
+  scoped_refptr<TestTrackingSynchronizer> tracking_synchronizer =
+      new TestTrackingSynchronizer(TestTimeFromMs(111));
+
+  tracking_synchronizer->RegisterPhaseCompletion(
+      ProfilerEventProto::EVENT_FIRST_NONEMPTY_PAINT, TestTimeFromMs(333));
 
   tracked_objects::ProcessDataSnapshot profiler_data;
   ProcessDataPhaseSnapshot snapshot0;