Refactor recording of the Startup.FirstWebContents.RenderProcessHostInit.ToNonEmptyPaint metric

Refactor recording of the Startup.FirstWebContents.RenderProcessHostInit.ToNonEmptyPaint metric

BUG=728324

[hans, 2017-06-01 22:29:16]

hans@chromium.org changed reviewers:
+ gab@chromium.org

[hans, 2017-06-01 22:29:18]

This doesn't pass presubmit:

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  content/browser/renderer_host/render_process_host_impl.cc
      Illegal include:
"components/startup_metric_utils/browser/startup_metric_utils.h"
    Because of "-components" from content's include_rules.


But I wanted to upload anyway to get some feedback.

gab: As you suggested on the other review, it would be much nicer if we could
just record the timestamps separately.

But it seems I can't include the startup_metric_utils.h in content/ ? Do you
have any ideas for how we could organize this cleanly?

[gab, 2017-06-02 18:31:33]

gab@chromium.org changed reviewers:
+ fdoray@chromium.org

[gab, 2017-06-02 18:31:34]

+fdoray who has experience with the whole content/components dance for startup
metrics. fdoray: do you agree it's better to call startup_metrics_utils method
individually per timing? If so how do we get this timing to the component
without violating DEPS?

https://codereview.chromium.org/2913423003/diff/1/components/startup_metric_u...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2913423003/diff/1/components/startup_metric_u...
components/startup_metric_utils/browser/startup_metric_utils.cc:752:
UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE_AND_SAME_VERSION_COUNT(
Let's not bother with "AND_SAME_VERSION_COUNT" (we should drop that suffix, no
one looks at it anymore IMO)

[hans, 2017-06-05 23:39:43]

On 2017/06/02 18:31:34, gab wrote:
> +fdoray who has experience with the whole content/components dance for startup
> metrics. fdoray: do you agree it's better to call startup_metrics_utils method
> individually per timing? If so how do we get this timing to the component
> without violating DEPS?

fdoray: ping?

[fdoray, 2017-06-06 17:38:40]

On 2017/06/05 23:39:43, hans wrote:
> On 2017/06/02 18:31:34, gab wrote:
> > +fdoray who has experience with the whole content/components dance for
startup
> > metrics. fdoray: do you agree it's better to call startup_metrics_utils
method
> > individually per timing? If so how do we get this timing to the component
> > without violating DEPS?
> 
> fdoray: ping?

The solution to avoid violating DEPS would be to add a method to
content::ContentBrowserClient. The ChromeContentBrowserClient implementation
would call the function in components/startup_metric_utils/ to record the
histogram. I'm not convinced that the complexity of this change is worth the
benefits. Making this change would change nothing to the recorded value.

[gab, 2017-06-06 17:44:37]

Description was changed from

==========
Refactor recording of the
Startup.FirstWebContents.RenderProcessHostInit.ToNonEmptyPaint metric

BUG=728324
==========

to

==========
Refactor recording of the
Startup.FirstWebContents.RenderProcessHostInit.ToNonEmptyPaint metric

BUG=728324
==========

[gab, 2017-06-06 17:44:37]

gab@chromium.org changed reviewers:
+ oysteine@chromium.org

[gab, 2017-06-06 17:44:52]

On 2017/06/06 17:38:40, fdoray wrote:
> On 2017/06/05 23:39:43, hans wrote:
> > On 2017/06/02 18:31:34, gab wrote:
> > > +fdoray who has experience with the whole content/components dance for
> startup
> > > metrics. fdoray: do you agree it's better to call startup_metrics_utils
> method
> > > individually per timing? If so how do we get this timing to the component
> > > without violating DEPS?
> > 
> > fdoray: ping?
> 
> The solution to avoid violating DEPS would be to add a method to
> content::ContentBrowserClient. The ChromeContentBrowserClient implementation
> would call the function in components/startup_metric_utils/ to record the
> histogram. I'm not convinced that the complexity of this change is worth the
> benefits. Making this change would change nothing to the recorded value.

True but passing a specific timing to RecordFirstWebContentsNonEmptyPaint() is
weird (i.e. if we do this a few more times this code will be a spaghetti mess).

Ideally each timing signal would be passed to the resource coordinator service
without violating DEPS and it would be the one recording desired timings. It
currently doesn't do that though... +oysteine: this was in the initial plan are
we still pursuing that (GRC as a metrics organizer)?