Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms.

components/startup_metric_utils/browser/startup_metric_utils.cc

 // Copyright 2013 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/startup_metric_utils/browser/startup_metric_utils.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <string>
@@ skipping 751 matching lines @@
     return;
 
   base::StackSamplingProfiler::SetProcessMilestone(
       metrics::CallStackProfileMetricsProvider::MAIN_NAVIGATION_FINISHED);
   UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE_AND_SAME_VERSION_COUNT(
       UMA_HISTOGRAM_LONG_TIMES_100,
       "Startup.FirstWebContents.MainNavigationFinished",
       g_process_creation_ticks.Get(), ticks);
 }
 
+void RecordBrowserViewFirstPaint(const base::TimeTicks& ticks) {
+  static bool is_first_call = true;
+  if (!is_first_call || ticks.is_null())
+    return;
+  is_first_call = false;

[Peter Kasting, 2017/03/24 22:39:18]

It seems like the only reason we need these guards is because the histogram
helper is per-BrowserView rather than a singleton for the browser process.  That
also means that if multiple windows are opening, you'll really histogram the
first paint times for the first window to paint, without capturing details of
multiple windows.  I don't know if this is a problem or not.  What do you want
to record when there are multiple windows?

There are various solutions in this space, but it would be good to find
something that lets us remove these statics and thus test this stuff from unit
tests instead of browser tests.  One such solution that preserves the existing
behavior is to have the histogram helper object be a process singleton.

[gab, 2017/03/27 15:21:43]

Not sure it makes sense to test this outside an integration (browser) test FWIW.
It's based on process launch (which even if faked is a global variable).

I don't think we test the other startup metrics FWIW -- i.e. they're tested by
virtue of being collected in the wild and tracked via go/finch-chirp

BrowserViewHistogramHelperTest.* as a unit test would merely be call X(),
confirm X() was called... As a browser test at least it's an integration test
that startup called X(). But overall I'd argue to not bother and remove
BrowserViewHistogramHelperTest.* as we will see it in the histogram if it
doesn't work (unless we're worried about the recording being flaky?).

All of the other calls here have statics to only record the first call, some
might be overkill and we probably could ensure each call is made exactly once
but until someone tackles all of them, for consistency, it's better to have the
statics in these calls as well?

[themblsha, 2017/03/31 14:32:31]

We generally try test Yandex-specific histograms as it provides an early-warning
system, as noticing that the histograms stopped being reported only after the
release is sent to users could result in losing useful data.

[gab, 2017/04/03 16:18:25]

Okay then I would argue for a single browser test that ensures every startup
metric monitored by startup_metric_utils has been logged (instead of one test
per metric since the setup is the exact same and launching the browser once per
metric is overkill).

+
+  UMA_HISTOGRAM_WITH_TEMPERATURE(UMA_HISTOGRAM_LONG_TIMES_100,
+                                 "Startup.BrowserView.FirstPaint",
+                                 ticks - g_process_creation_ticks.Get());
+}
+
+void RecordBrowserViewFirstPaintCompositingEnded(const base::TimeTicks& ticks) {
+  static bool is_first_call = true;
+  if (!is_first_call || ticks.is_null())
+    return;
+  is_first_call = false;
+
+  UMA_HISTOGRAM_WITH_TEMPERATURE(

[gab, 2017/03/27 15:21:43]

Use UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE.

Then could you grab a local trace by launching chrome via:

chrome.exe --trace-startup --trace-startup-duration=10
--trace-stratup-file="mytrace.json"

open the trace, zoom in on the registered startup events, and upload a
screenshot to the bug to show how this metric's timing relates to the other
existing startup metrics.

[themblsha, 2017/03/31 14:32:32]

Done: https://bugs.chromium.org/p/chromium/issues/detail?id=704945#c2

+      UMA_HISTOGRAM_LONG_TIMES_100,
+      "Startup.BrowserView.FirstPaint.CompositingEnded",
+      ticks - g_process_creation_ticks.Get());
+}
+
 base::TimeTicks MainEntryPointTicks() {
   return g_browser_main_entry_point_ticks.Get();
 }
 
 }  // namespace startup_metric_utils