Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin

content/browser/renderer_host/input/render_widget_host_latency_tracker.cc

Index: content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
diff --git a/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc b/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
index 17b396efc59e402a93ec18828278032bd22dc3ba..bdcd1722edd88e5631e02ca7633085d55f19b837 100644
--- a/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
+++ b/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc
@@ -92,6 +92,91 @@ void UpdateLatencyCoordinates(const WebInputEvent& event,
     (end.event_time - start.event_time).InMicroseconds(),          \
     1, 50000, 50)
 
+void ComputeFirstScrollLatencyHistograms(
+    const LatencyInfo::LatencyComponent& original_component,
+    const LatencyInfo::LatencyComponent& gpu_swap_begin_component,
+    const LatencyInfo::LatencyComponent& gpu_swap_end_component,
+    const LatencyInfo& latency,
+    bool is_running_navigation_hint_task) {
+  // This UMA metric tracks the time between the final frame swap for the
+  // first scroll event in a sequence and the original timestamp of that
+  // scroll event's underlying touch event.
+  for (size_t i = 0; i < original_component.event_count; i++) {
+    UMA_HISTOGRAM_TOUCH_TO_SCROLL_LATENCY(
+        "Event.Latency.TouchToFirstScrollUpdateSwapBegin", original_component,
+        gpu_swap_begin_component);
+  }
+  // TODO(horo): IsRunningNavigationHintTask UMAs are only for
+  // SpeculativeLaunchServiceWorker experimentation. So remove this UMA when
+  // the experimentation finished (crbug.com/638827).
+  if (is_running_navigation_hint_task) {
+    for (size_t i = 0; i < original_component.event_count; i++) {
+      UMA_HISTOGRAM_TOUCH_TO_SCROLL_LATENCY(
+          "Event.Latency.TouchToFirstScrollUpdateSwapBegin_"
+          "IsRunningNavigationHintTask",
+          original_component, gpu_swap_begin_component);
+    }
+  }
+
+  LatencyInfo::LatencyComponent rendering_scheduled_component;
+  bool rendering_scheduled_on_main = latency.FindLatency(
+      ui::INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_MAIN_COMPONENT, 0,
+      &rendering_scheduled_component);
+
+  if (!rendering_scheduled_on_main) {
+    if (!latency.FindLatency(
+            ui::INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_IMPL_COMPONENT, 0,
+            &rendering_scheduled_component)) {
+      return;
+    }
+  }
+
+  if (rendering_scheduled_on_main) {
+    UMA_HISTOGRAM_SCROLL_LATENCY_LONG(
+        "Event.Latency.FirstScrollUpdate.FirstTouchToHandled_Main",
+        original_component, rendering_scheduled_component);
+  } else {
+    UMA_HISTOGRAM_SCROLL_LATENCY_LONG(
+        "Event.Latency.FirstScrollUpdate.FirstTouchToHandled_Impl",
+        original_component, rendering_scheduled_component);
+  }
+
+  LatencyInfo::LatencyComponent renderer_swap_component;
+  if (!latency.FindLatency(ui::INPUT_EVENT_LATENCY_RENDERER_SWAP_COMPONENT, 0,
+                           &renderer_swap_component)) {
+    return;
+  }
+
+  if (rendering_scheduled_on_main) {
+    UMA_HISTOGRAM_SCROLL_LATENCY_LONG(
+        "Event.Latency.FirstScrollUpdate.HandledToRendererSwap_Main",
+        rendering_scheduled_component, renderer_swap_component);
+  } else {
+    UMA_HISTOGRAM_SCROLL_LATENCY_LONG(
+        "Event.Latency.FirstScrollUpdate.HandledToRendererSwap_Impl",
+        rendering_scheduled_component, renderer_swap_component);
+  }
+
+  LatencyInfo::LatencyComponent browser_received_swap_component;
+  if (!latency.FindLatency(
+          ui::INPUT_EVENT_BROWSER_RECEIVED_RENDERER_SWAP_COMPONENT, 0,
+          &browser_received_swap_component)) {
+    return;
+  }
+
+  UMA_HISTOGRAM_SCROLL_LATENCY_SHORT(
+      "Event.Latency.FirstScrollUpdate.RendererSwapToBrowserNotified",
+      renderer_swap_component, browser_received_swap_component);
+
+  UMA_HISTOGRAM_SCROLL_LATENCY_LONG(
+      "Event.Latency.FirstScrollUpdate.BrowserNotifiedToBeforeGpuSwap",
+      browser_received_swap_component, gpu_swap_begin_component);
+
+  UMA_HISTOGRAM_SCROLL_LATENCY_SHORT("Event.Latency.FirstScrollUpdate.GpuSwap",
+                                     gpu_swap_begin_component,
+                                     gpu_swap_end_component);
+}
+
 void ComputeScrollLatencyHistograms(
     const LatencyInfo::LatencyComponent& gpu_swap_begin_component,
     const LatencyInfo::LatencyComponent& gpu_swap_end_component,
@@ -108,25 +193,9 @@ void ComputeScrollLatencyHistograms(
   if (latency.FindLatency(
           ui::INPUT_EVENT_LATENCY_FIRST_SCROLL_UPDATE_ORIGINAL_COMPONENT,
           latency_component_id, &original_component)) {
-    // This UMA metric tracks the time between the final frame swap for the
-    // first scroll event in a sequence and the original timestamp of that
-    // scroll event's underlying touch event.
-    for (size_t i = 0; i < original_component.event_count; i++) {
-      UMA_HISTOGRAM_TOUCH_TO_SCROLL_LATENCY(
-          "Event.Latency.TouchToFirstScrollUpdateSwapBegin",
-          original_component, gpu_swap_begin_component);
-    }
-    // TODO(horo): IsRunningNavigationHintTask UMAs are only for
-    // SpeculativeLaunchServiceWorker experimentation. So remove this UMA when
-    // the experimentation finished (crbug.com/638827).
-    if (is_running_navigation_hint_task) {
-      for (size_t i = 0; i < original_component.event_count; i++) {
-        UMA_HISTOGRAM_TOUCH_TO_SCROLL_LATENCY(
-            "Event.Latency.TouchToFirstScrollUpdateSwapBegin_"
-            "IsRunningNavigationHintTask",
-            original_component, gpu_swap_begin_component);
-      }
-    }
+    ComputeFirstScrollLatencyHistograms(

[tdresser, 2016/08/24 12:48:57]

Could we start by checking if this is the first scroll or not, and then have a
function that records FirstScrollUpdate or ScrollUpdate metrics depending on a
parameter?

[tdresser, 2016/08/24 12:51:39]

We'll end up having to look up some components more than once if we take this
approach, but I think that's acceptable.

In the long term, we won't have a metrics that records both scroll start and
continuous scroll, only one or the other, which will allow us to stop
recomputing these twice.

[lanwei, 2016/08/25 04:17:11]

I tried several  ways, but none of them works, because the metric function is
defined as macro. Using a variable as the metric name will mess up later, and
make Checkname fail,
https://codesearch.chromium.org/chromium/src/base/metrics/histogram_base.cc?d....

Is there any way to make this work?

[tdresser, 2016/08/25 13:31:05]

Hmmm, we want something like:

https://cs.chromium.org/chromium/src/ui/events/event.cc?rcl=0&l=66

I'm not sure if we should move that method to somewhere more generic, or whether
there's a preferable approach. asvitkine would know.

+        original_component, gpu_swap_begin_component, gpu_swap_end_component,
+        latency, is_running_navigation_hint_task);
   } else if (!latency.FindLatency(
                  ui::INPUT_EVENT_LATENCY_SCROLL_UPDATE_ORIGINAL_COMPONENT,
                  latency_component_id, &original_component)) {
@@ -152,18 +221,16 @@ void ComputeScrollLatencyHistograms(
     }
   }
 
-  // TODO(miletus): Add validation for making sure the following components
-  // are present and their event times are legit.
   LatencyInfo::LatencyComponent rendering_scheduled_component;
   bool rendering_scheduled_on_main = latency.FindLatency(
       ui::INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_MAIN_COMPONENT,
       0, &rendering_scheduled_component);
-
   if (!rendering_scheduled_on_main) {
     if (!latency.FindLatency(
-            ui::INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_IMPL_COMPONENT,
-            0, &rendering_scheduled_component))
+            ui::INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_IMPL_COMPONENT, 0,
+            &rendering_scheduled_component)) {

[tdresser, 2016/08/24 12:48:57]

Are these changes needed?

[lanwei, 2016/08/25 04:17:11]

I remembered you told me that if the statement has multi-line, then use {}, am I
wrong?

[tdresser, 2016/08/25 13:31:05]

Er, nope, I prefer this, sorry.

       return;
+    }
   }
 
   if (rendering_scheduled_on_main) {
@@ -177,9 +244,10 @@ void ComputeScrollLatencyHistograms(
   }
 
   LatencyInfo::LatencyComponent renderer_swap_component;
-  if (!latency.FindLatency(ui::INPUT_EVENT_LATENCY_RENDERER_SWAP_COMPONENT,
-                           0, &renderer_swap_component))
+  if (!latency.FindLatency(ui::INPUT_EVENT_LATENCY_RENDERER_SWAP_COMPONENT, 0,
+                           &renderer_swap_component)) {
     return;
+  }
 
   if (rendering_scheduled_on_main) {
     UMA_HISTOGRAM_SCROLL_LATENCY_LONG(
@@ -193,9 +261,10 @@ void ComputeScrollLatencyHistograms(
 
   LatencyInfo::LatencyComponent browser_received_swap_component;
   if (!latency.FindLatency(
-          ui::INPUT_EVENT_BROWSER_RECEIVED_RENDERER_SWAP_COMPONENT,
-          0, &browser_received_swap_component))
+          ui::INPUT_EVENT_BROWSER_RECEIVED_RENDERER_SWAP_COMPONENT, 0,
+          &browser_received_swap_component)) {
     return;
+  }
 
   UMA_HISTOGRAM_SCROLL_LATENCY_SHORT(
       "Event.Latency.ScrollUpdate.RendererSwapToBrowserNotified",