fix Visu's comments

Author: leotianlizhan

firebase-perf/src/main/java/com/google/firebase/perf/application/FrameMetricsRecorder.java

@@ -31,13 +31,20 @@
  *
  * <p>IMPORTANT: each recorder holds a reference to an Activity, so it is very important to
  * dereference each recorder at or before its Activity's onDestroy. Similar for Fragments.
+ *
+ * <p>Relationship of Fragment recording to Activity recording is like a stopwatch. The stopwatch
+ * itself is for the Activity, Fragments traces are laps in the stopwatch. Stopwatch can only ever
+ * be for the Activity by-definition because "frames" refer to a frame for a window, and
+ * FrameMetrics can only come from an Activity's Window. Fragment traces are laps in the stopwatch,
+ * because the frame metrics data is still for the Activity window, fragment traces are just
+ * intervals in the Activity frames recording that has the name "fragment X" attached to them.
  */
 public class FrameMetricsRecorder {
   private static final AndroidLogger logger = AndroidLogger.getInstance();
 
   private final Activity activity;
   private final FrameMetricsAggregator frameMetricsAggregator;
-  private final Map<Fragment, PerfFrameMetrics> subTraceMap;
+  private final Map<Fragment, PerfFrameMetrics> fragmentSnapshotMap;
 
   private boolean isRecording = false;
 
@@ -57,7 +64,7 @@ public FrameMetricsRecorder(Activity activity) {
       Map<Fragment, PerfFrameMetrics> subTraceMap) {
     this.activity = activity;
     this.frameMetricsAggregator = frameMetricsAggregator;
-    this.subTraceMap = subTraceMap;
+    this.fragmentSnapshotMap = subTraceMap;
   }
 
   /** Starts recording FrameMetrics for the activity window. */
@@ -81,9 +88,9 @@ public Optional<PerfFrameMetrics> stop() {
       logger.warn("Cannot stop because no recording was started");
       return Optional.absent();
     }
-    if (!subTraceMap.isEmpty()) {
+    if (!fragmentSnapshotMap.isEmpty()) {
       logger.warn(
-          "Sub-traces are still ongoing! Sub-traces should be stopped first before stopping Activity screen trace.");
+          "Sub-recordings are still ongoing! Sub-recordings should be stopped first before stopping Activity screen trace.");
     }
     Optional<PerfFrameMetrics> data = this.snapshot();
     try {
@@ -99,57 +106,61 @@ public Optional<PerfFrameMetrics> stop() {
   }
 
   /**
-   * Starts a sub-trace in the current recording.
+   * Starts a Fragment sub-recording in the current Activity recording. Fragments are sub-recordings
+   * due to the way frame metrics work: frame metrics can only comes from an activity's window. An
+   * analogy for sub-recording is a lap in a stopwatch.
    *
-   * @param key a UI state to associate this sub-trace with. e.g.) fragment
+   * @param fragment a Fragment to associate this sub-trace with.
    */
-  public void startSubTrace(Fragment key) {
+  public void startFragment(Fragment fragment) {
     if (!isRecording) {
-      logger.warn("Cannot start sub-trace because FrameMetricsAggregator is not recording");
+      logger.warn("Cannot start sub-recording because FrameMetricsAggregator is not recording");
       return;
     }
-    if (subTraceMap.containsKey(key)) {
+    if (fragmentSnapshotMap.containsKey(fragment)) {
       logger.warn(
-          "Cannot start sub-trace because one is already ongoing with the key %s",
-          key.getClass().getSimpleName());
+          "Cannot start sub-recording because one is already ongoing with the key %s",
+          fragment.getClass().getSimpleName());
       return;
     }
     Optional<PerfFrameMetrics> snapshot = this.snapshot();
     if (!snapshot.isAvailable()) {
-      logger.warn("startSubTrace(%s): snapshot() failed", key.getClass().getSimpleName());
+      logger.warn("startFragment(%s): snapshot() failed", fragment.getClass().getSimpleName());
       return;
     }
-    subTraceMap.put(key, snapshot.get());
+    fragmentSnapshotMap.put(fragment, snapshot.get());
   }
 
   /**
-   * Stops the sub-trace associated with the given UI state.
+   * Stops the sub-recording associated with the given Fragment. Fragments are sub-recordings due to
+   * the way frame metrics work: frame metrics can only comes from an activity's window. An analogy
+   * for sub-recording is a lap in a stopwatch.
    *
-   * @param key the UI state associated with the sub-trace to be stopped.
-   * @return FrameMetrics accumulated during this sub-trace.
+   * @param fragment the Fragment associated with the sub-recording to be stopped.
+   * @return FrameMetrics accumulated during this sub-recording.
    */
-  public Optional<PerfFrameMetrics> stopSubTrace(Fragment key) {
+  public Optional<PerfFrameMetrics> stopFragment(Fragment fragment) {
     if (!isRecording) {
-      logger.warn("Cannot stop sub-trace because FrameMetricsAggregator is not recording");
+      logger.warn("Cannot stop sub-recording because FrameMetricsAggregator is not recording");
       return Optional.absent();
     }
-    if (!subTraceMap.containsKey(key)) {
+    if (!fragmentSnapshotMap.containsKey(fragment)) {
       logger.warn(
-          "Sub-trace associated with key %s was not started or does not exist",
-          key.getClass().getSimpleName());
+          "Sub-recording associated with key %s was not started or does not exist",
+          fragment.getClass().getSimpleName());
       return Optional.absent();
     }
-    PerfFrameMetrics snapshotStart = subTraceMap.remove(key);
+    PerfFrameMetrics snapshotStart = fragmentSnapshotMap.remove(fragment);
     Optional<PerfFrameMetrics> snapshotEnd = this.snapshot();
     if (!snapshotEnd.isAvailable()) {
-      logger.warn("stopSubTrace(%s): snapshot() failed", key.getClass().getSimpleName());
+      logger.warn("stopFragment(%s): snapshot() failed", fragment.getClass().getSimpleName());
       return Optional.absent();
     }
-    return Optional.of(snapshotEnd.get().subtract(snapshotStart));
+    return Optional.of(snapshotEnd.get().deltaFrameMetricsFromSnapshot(snapshotStart));
   }
 
   /**
-   * Calculate total frames, slow frames, and frozen frames from SparseIntArray[] recorded by {@link
+   * Snapshots total frames, slow frames, and frozen frames from SparseIntArray[] recorded by {@link
    * FrameMetricsAggregator}.
    *
    * @return {@link PerfFrameMetrics} at the time of snapshot.

firebase-perf/src/main/java/com/google/firebase/perf/metrics/FrameMetricsCalculator.java

@@ -56,7 +56,7 @@ public int getTotalFrames() {
      * @param that the subtrahend PerfFrameMetrics object.
      * @return difference of this and the argument.
      */
-    public PerfFrameMetrics subtract(PerfFrameMetrics that) {
+    public PerfFrameMetrics deltaFrameMetricsFromSnapshot(PerfFrameMetrics that) {
       int newTotalFrames = this.totalFrames - that.getTotalFrames();
       int newSlowFrames = this.slowFrames - that.getSlowFrames();
       int newFrozenFrames = this.frozenFrames - that.getFrozenFrames();

firebase-perf/src/test/java/com/google/firebase/perf/application/FrameMetricsRecorderTest.java

@@ -82,8 +82,8 @@ public void startAndStop_calledTwice_ignoresSecondCall() {
     assertThat(result2.isAvailable()).isFalse();
 
     assertThat(result1.get().getTotalFrames()).isEqualTo(1 + 3 + 1);
-    assertThat(result1.get().getTotalFrames()).isEqualTo(3 + 1);
-    assertThat(result1.get().getTotalFrames()).isEqualTo(1);
+    assertThat(result1.get().getSlowFrames()).isEqualTo(3 + 1);
+    assertThat(result1.get().getFrozenFrames()).isEqualTo(1);
   }
 
   @Test
@@ -101,9 +101,9 @@ public void startAndStopSubTrace_activityRecordingNeverStarted_returnsEmptyResul
     Fragment fragment = new Fragment();
 
     stubFrameMetricsAggregatorData(fma, frameTimes1);
-    recorder.startSubTrace(fragment);
+    recorder.startFragment(fragment);
     stubFrameMetricsAggregatorData(fma, frameTimes2);
-    Optional<PerfFrameMetrics> result = recorder.stopSubTrace(fragment);
+    Optional<PerfFrameMetrics> result = recorder.stopFragment(fragment);
     assertThat(result.isAvailable()).isFalse();
   }
 
@@ -114,9 +114,9 @@ public void startAndStopSubTrace_activityRecordingHasEnded_returnsEmptyResult()
     recorder.start();
     recorder.stop();
     stubFrameMetricsAggregatorData(fma, frameTimes1);
-    recorder.startSubTrace(fragment);
+    recorder.startFragment(fragment);
     stubFrameMetricsAggregatorData(fma, frameTimes2);
-    Optional<PerfFrameMetrics> result = recorder.stopSubTrace(fragment);
+    Optional<PerfFrameMetrics> result = recorder.stopFragment(fragment);
     assertThat(result.isAvailable()).isFalse();
   }
 
@@ -131,13 +131,13 @@ public void startAndStopSubTrace_whenCalledTwice_ignoresSecondCall() {
     // comments are in this format: total frames, slow frames, frozen frames
     recorder.start();
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 1}, {17, 1}, {800, 1}}); // 3, 2, 1
-    recorder.startSubTrace(fragment);
+    recorder.startFragment(fragment);
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 2}, {17, 3}, {800, 2}}); // ignored
-    recorder.startSubTrace(fragment); // this call is ignored
+    recorder.startFragment(fragment); // this call is ignored
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 5}, {17, 4}, {800, 5}}); // 14, 9, 5
-    Optional<PerfFrameMetrics> result1 = recorder.stopSubTrace(fragment);
+    Optional<PerfFrameMetrics> result1 = recorder.stopFragment(fragment);
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 6}, {17, 5}, {800, 5}}); // ignored
-    Optional<PerfFrameMetrics> result2 = recorder.stopSubTrace(fragment); // this call is ignored
+    Optional<PerfFrameMetrics> result2 = recorder.stopFragment(fragment); // this call is ignored
 
     // total = 14 - 3 = 11, slow = 9 - 2 = 7, frozen = 5 - 1 = 4
     assertThat(result1.get().getTotalFrames()).isEqualTo(14 - 3);
@@ -153,8 +153,8 @@ public void stopAndStopSubTrace_whenNoSubTraceWithGivenKeyExists_returnsEmptyRes
 
     recorder.start();
 
-    recorder.startSubTrace(fragment1);
-    Optional<PerfFrameMetrics> result = recorder.stopSubTrace(fragment2);
+    recorder.startFragment(fragment1);
+    Optional<PerfFrameMetrics> result = recorder.stopFragment(fragment2);
 
     assertThat(result.isAvailable()).isFalse();
   }
@@ -164,9 +164,9 @@ public void startAndStopSubTrace_duringActivityRecording_returnsValidResult() {
     Fragment fragment = new Fragment();
     recorder.start();
     stubFrameMetricsAggregatorData(fma, frameTimes1);
-    recorder.startSubTrace(fragment);
+    recorder.startFragment(fragment);
     stubFrameMetricsAggregatorData(fma, frameTimes2);
-    Optional<PerfFrameMetrics> result = recorder.stopSubTrace(fragment);
+    Optional<PerfFrameMetrics> result = recorder.stopFragment(fragment);
     assertThat(result.isAvailable()).isTrue();
     // frameTimes2 - frameTimes1
     assertThat(result.get().getTotalFrames()).isEqualTo(9);
@@ -181,13 +181,13 @@ public void startAndStopSubTrace_whenTwoSubTracesOverlap_returnsCorrectResults()
     recorder.start();
     // comments are in this format: total frames, slow frames, frozen frames
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 1}, {17, 1}, {800, 1}}); // 3, 2, 1
-    recorder.startSubTrace(fragment1);
+    recorder.startFragment(fragment1);
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 2}, {17, 3}, {800, 2}}); // 7, 5, 2
-    recorder.startSubTrace(fragment2);
+    recorder.startFragment(fragment2);
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 5}, {17, 4}, {800, 5}}); // 14, 9, 5
-    Optional<PerfFrameMetrics> subTrace1 = recorder.stopSubTrace(fragment1);
+    Optional<PerfFrameMetrics> subTrace1 = recorder.stopFragment(fragment1);
     stubFrameMetricsAggregatorData(fma, new int[][] {{1, 6}, {17, 5}, {800, 5}}); // 16, 10, 5
-    Optional<PerfFrameMetrics> subTrace2 = recorder.stopSubTrace(fragment2);
+    Optional<PerfFrameMetrics> subTrace2 = recorder.stopFragment(fragment2);
 
     // 3rd snapshot - 1st snapshot
     assertThat(subTrace1.get().getTotalFrames()).isEqualTo(14 - 3);