Move the user interaction policy for FirstMeaningfulPaint UMA into renderer

Move the user interaction policy for FirstMeaningfulPaint UMA into renderer

FirstMeaningfulPaint UMA is logged only if there's no user interaction
between FirstPaint and FirstMeaningfulPaint. But the other consumer of
FirstMeaningfulPaint did not have this logic.

This patch moves the logic for this user interaction policy from browser
(page_load_metrics) to renderer (FirstMeaningfulPaintDetector), so that
other page_load_metrics observers will get consistent FMP value.

This also removes user-input related FirstMeaningfulPaint histograms,
as we no longer track them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2883273003
Cr-Commit-Position: refs/heads/master@{#482507}
Committed: https://chromium.googlesource.com/chromium/src/+/6b1f67dbfa7dc753e17264a38c7cb51f63fac525

[Kunihiko Sakamoto, 2017-05-17 04:14:46]

Description was changed from

==========
Remove user interaction policy for FirstMeaningfulPaint UMA

Before this patch, FirstMeaningfulPaint UMA was logged only if there's
no user interaction between FirstPaint and FirstMeaningfulPaint. But
the other consumer of FirstMeaningfulPaint do not have this logic.

UMA shows that only 2% of page loads had user interaction before FMP,
and most of them wouldn't affect FMP value. So this patch removes this
user-interaction condition, for consistency with other FMP UMAs.

This also removes the breakdown histograms of first-meaningful-paint by
the presence or absence of user input, as we decided not to use user
input as a signal for FMP.
==========

to

==========
Remove user interaction policy for FirstMeaningfulPaint UMA

Before this patch, FirstMeaningfulPaint UMA was logged only if there's
no user interaction between FirstPaint and FirstMeaningfulPaint. But
the other consumer of FirstMeaningfulPaint do not have this logic.

UMA shows that only 2% of page loads had user interaction before FMP,
and most of them wouldn't affect FMP value. So this patch removes this
user-interaction condition, for consistency with other FMP UMAs.

This also removes the breakdown histograms of FirstMeaningfulPaint by
the presence or absence of user input, as we decided not to use user
input as a signal for FMP.
==========

[Kunihiko Sakamoto, 2017-05-17 04:15:28]

The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 04:16:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2017-05-17 04:25:14]

ksakamoto@chromium.org changed reviewers:
+ bmcquade@chromium.org, isherman@chromium.org

[commit-bot: I haz the power, 2017-05-17 05:00:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 05:00:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kunihiko Sakamoto, 2017-05-17 05:08:13]

The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 05:09:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 05:27:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 05:27:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ilya Sherman, 2017-05-17 06:44:48]

Hmm, are there existing histograms whose descriptions should be updated to note
the new semantics?  And, if so, is it worth renaming the histograms, since their
semantics have changed?  (Will defer to Bryan on the latter question
especially.)

[Kunihiko Sakamoto, 2017-05-17 07:53:58]

On 2017/05/17 06:44:48, Ilya Sherman wrote:
> Hmm, are there existing histograms whose descriptions should be updated to
note
> the new semantics?  And, if so, is it worth renaming the histograms, since
their
> semantics have changed?  (Will defer to Bryan on the latter question
> especially.)

Description of
PageLoad.Experimental.PaintTiming.FirstMeaningfulPaintSignalStatus2 mentioned
about user interaction. This histogram is no longer useful, so I just removed
it.

The other histogram descriptions don't say anything about user interactions.

For renaming, we have tweaked the FMP algorithm several times without renaming
the histogram, as this UMA is considered experimental.
Bryan, what do you think?

[tdresser, 2017-05-17 12:02:27]

tdresser@chromium.org changed reviewers:
+ tdresser@chromium.org

[tdresser, 2017-05-17 12:02:28]

For what it's worth, I'm always in favor of renaming when we make changes to
histograms.

We also decided to move FMP out of experimental, didn't we?
Perhaps we should just rename it to be non-experimental as part of this change?

[Bryan McQuade, 2017-05-17 12:11:36]

On 2017/05/17 at 12:02:28, tdresser wrote:
> For what it's worth, I'm always in favor of renaming when we make changes to
histograms.
> 
> We also decided to move FMP out of experimental, didn't we?
> Perhaps we should just rename it to be non-experimental as part of this
change?

+1 to renaming to non-experimental.

I'm open to doing that rename in a follow up change if you prefer.

We also need to update all other page load metrics observers to migrate their
versions of FMP out of experimental, too.

https://cs.chromium.org/search/?q=file:page_load_metrics+FirstMeaningfulPaint...

Thanks!

[Bryan McQuade, 2017-05-17 12:15:16]

https://codereview.chromium.org/2883273003/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2883273003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:476:
PAGE_LOAD_HISTOGRAM(internal::kHistogramFirstMeaningfulPaintToNetworkStable,
is this an important histogram to keep recording? if it's important we should
consider recording this time delta in the render process. the callbacks that run
in the browser process at these point aren't the exact point when these events
happened - there's buffering in the render process and then delays from IPC
delivery etc. So I'd say let's either add a comment noting that this is
approximate, and a TODO to record this time delta in the render process, or
remove this histogram.

[Kunihiko Sakamoto, 2017-05-19 07:12:19]

Description was changed from

==========
Remove user interaction policy for FirstMeaningfulPaint UMA

Before this patch, FirstMeaningfulPaint UMA was logged only if there's
no user interaction between FirstPaint and FirstMeaningfulPaint. But
the other consumer of FirstMeaningfulPaint do not have this logic.

UMA shows that only 2% of page loads had user interaction before FMP,
and most of them wouldn't affect FMP value. So this patch removes this
user-interaction condition, for consistency with other FMP UMAs.

This also removes the breakdown histograms of FirstMeaningfulPaint by
the presence or absence of user input, as we decided not to use user
input as a signal for FMP.
==========

to

==========
Remove user interaction policy for FirstMeaningfulPaint UMA

Before this patch, FirstMeaningfulPaint UMA was logged only if there's
no user interaction between FirstPaint and FirstMeaningfulPaint. But
the other consumer of FirstMeaningfulPaint do not have this logic.

UMA shows that only 2% of page loads had user interaction before FMP,
and most of them wouldn't affect FMP value. So this patch removes this
user-interaction condition, for consistency with other FMP UMAs.

This also removes the breakdown histograms of FirstMeaningfulPaint by
the presence or absence of user input, as we decided not to use user
input as a signal for FMP.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Kunihiko Sakamoto, 2017-05-19 07:29:38]

Description was changed from

==========
Remove user interaction policy for FirstMeaningfulPaint UMA

Before this patch, FirstMeaningfulPaint UMA was logged only if there's
no user interaction between FirstPaint and FirstMeaningfulPaint. But
the other consumer of FirstMeaningfulPaint do not have this logic.

UMA shows that only 2% of page loads had user interaction before FMP,
and most of them wouldn't affect FMP value. So this patch removes this
user-interaction condition, for consistency with other FMP UMAs.

This also removes the breakdown histograms of FirstMeaningfulPaint by
the presence or absence of user input, as we decided not to use user
input as a signal for FMP.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Move the user interaction policy for FirstMeaningfulPaint UMA into renderer

FirstMeaningfulPaint UMA is logged only if there's no user interaction
between FirstPaint and FirstMeaningfulPaint. But the other consumer of
FirstMeaningfulPaint did not have this logic.

This patch moves the logic for this user interaction policy from browser
(page_load_metrics) to renderer (FirstMeaningfulPaintDetector), so that
other page_load_metrics observers will get consistent FMP value.

This also removes user-input related FirstMeaningfulPaint histograms,
as we no longer track them.
==========

[Kunihiko Sakamoto, 2017-05-19 08:14:04]

Description was changed from

==========
Move the user interaction policy for FirstMeaningfulPaint UMA into renderer

FirstMeaningfulPaint UMA is logged only if there's no user interaction
between FirstPaint and FirstMeaningfulPaint. But the other consumer of
FirstMeaningfulPaint did not have this logic.

This patch moves the logic for this user interaction policy from browser
(page_load_metrics) to renderer (FirstMeaningfulPaintDetector), so that
other page_load_metrics observers will get consistent FMP value.

This also removes user-input related FirstMeaningfulPaint histograms,
as we no longer track them.
==========

to

==========
Move the user interaction policy for FirstMeaningfulPaint UMA into renderer

FirstMeaningfulPaint UMA is logged only if there's no user interaction
between FirstPaint and FirstMeaningfulPaint. But the other consumer of
FirstMeaningfulPaint did not have this logic.

This patch moves the logic for this user interaction policy from browser
(page_load_metrics) to renderer (FirstMeaningfulPaintDetector), so that
other page_load_metrics observers will get consistent FMP value.

This also removes user-input related FirstMeaningfulPaint histograms,
as we no longer track them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Kunihiko Sakamoto, 2017-05-19 08:21:31]

PTAL

Based on the discussion at chrome-page-load-metrics-infra, I moved the filtering
logic to renderer.

https://codereview.chromium.org/2883273003/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2883273003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:476:
PAGE_LOAD_HISTOGRAM(internal::kHistogramFirstMeaningfulPaintToNetworkStable,
On 2017/05/17 12:15:16, Bryan McQuade wrote:
> is this an important histogram to keep recording? if it's important we should
> consider recording this time delta in the render process. the callbacks that
run
> in the browser process at these point aren't the exact point when these events
> happened - there's buffering in the render process and then delays from IPC
> delivery etc. So I'd say let's either add a comment noting that this is
> approximate, and a TODO to record this time delta in the render process, or
> remove this histogram.

This histogram was useful to tune FMPDetector's network-idle logic, but we don't
have plan to tweak it further. Let's remove it.

[Ilya Sherman, 2017-05-22 20:51:55]

On 2017/05/17 12:11:36, Bryan McQuade wrote:
> On 2017/05/17 at 12:02:28, tdresser wrote:
> > For what it's worth, I'm always in favor of renaming when we make changes to
> histograms.
> > 
> > We also decided to move FMP out of experimental, didn't we?
> > Perhaps we should just rename it to be non-experimental as part of this
> change?
> 
> +1 to renaming to non-experimental.
> 
> I'm open to doing that rename in a follow up change if you prefer.
> 
> We also need to update all other page load metrics observers to migrate their
> versions of FMP out of experimental, too.
> 
>
https://cs.chromium.org/search/?q=file:page_load_metrics+FirstMeaningfulPaint...
> 
> Thanks!

It's not clear to me what the status of this discussion is -- I don't see any
histograms being renamed in histograms.xml.  Marking the three histograms as
obsolete LG; I just want to make sure there aren't other changes that should be
being made to histograms.xml that we're missing?

[Kunihiko Sakamoto, 2017-05-23 09:45:53]

On 2017/05/22 20:51:55, Ilya Sherman wrote:
> On 2017/05/17 12:11:36, Bryan McQuade wrote:
> > On 2017/05/17 at 12:02:28, tdresser wrote:
> > > For what it's worth, I'm always in favor of renaming when we make changes
to
> > histograms.
> > > 
> > > We also decided to move FMP out of experimental, didn't we?
> > > Perhaps we should just rename it to be non-experimental as part of this
> > change?
> > 
> > +1 to renaming to non-experimental.
> > 
> > I'm open to doing that rename in a follow up change if you prefer.
> > 
> > We also need to update all other page load metrics observers to migrate
their
> > versions of FMP out of experimental, too.
> > 
> >
>
https://cs.chromium.org/search/?q=file:page_load_metrics+FirstMeaningfulPaint...
> > 
> > Thanks!
> 
> It's not clear to me what the status of this discussion is -- I don't see any
> histograms being renamed in histograms.xml.  Marking the three histograms as
> obsolete LG; I just want to make sure there aren't other changes that should
be
> being made to histograms.xml that we're missing?

Sorry for not being clear.

In terms of metrics, I would like to deprecate the three histograms since
browser side will no longer be aware of user-interaction status.
The user-interaction policy remains in renderer side, so it won't change current
semantics of FMP UMA.

Tim and Bryan, WDYT?

[tdresser, 2017-05-23 12:06:34]

On 2017/05/23 09:45:53, Kunihiko Sakamoto wrote:
> On 2017/05/22 20:51:55, Ilya Sherman wrote:
> > On 2017/05/17 12:11:36, Bryan McQuade wrote:
> > > On 2017/05/17 at 12:02:28, tdresser wrote:
> > > > For what it's worth, I'm always in favor of renaming when we make
changes
> to
> > > histograms.
> > > > 
> > > > We also decided to move FMP out of experimental, didn't we?
> > > > Perhaps we should just rename it to be non-experimental as part of this
> > > change?
> > > 
> > > +1 to renaming to non-experimental.
> > > 
> > > I'm open to doing that rename in a follow up change if you prefer.
> > > 
> > > We also need to update all other page load metrics observers to migrate
> their
> > > versions of FMP out of experimental, too.
> > > 
> > >
> >
>
https://cs.chromium.org/search/?q=file:page_load_metrics+FirstMeaningfulPaint...
> > > 
> > > Thanks!
> > 
> > It's not clear to me what the status of this discussion is -- I don't see
any
> > histograms being renamed in histograms.xml.  Marking the three histograms as
> > obsolete LG; I just want to make sure there aren't other changes that should
> be
> > being made to histograms.xml that we're missing?
> 
> Sorry for not being clear.
> 
> In terms of metrics, I would like to deprecate the three histograms since
> browser side will no longer be aware of user-interaction status.
> The user-interaction policy remains in renderer side, so it won't change
current
> semantics of FMP UMA.
> 
> Tim and Bryan, WDYT?

Why are we marking
PageLoad.Experimental.PaintTiming.FirstMeaningfulPaintToNetworkStable as
obsolete?

[Bryan McQuade, 2017-05-23 14:25:36]

Thanks! This looks fine to me. Only thought would be whether we should add some
UMA in the render process (in blink) to count the relative frequency of FMP
happening before vs after user input, after this change. We know it was 2% IIRC
in the old impl, but given the buffering issues for consuming events in the
browser process, it may be useful to confirm that this doesn't change
significantly in the new impl. If the percentage increases significantly that
seems really important to know.

I'd also be fine with adding a temporary field to the page load metrics code
that sends FMP when it happens after user input to the page load metrics
observers in the browser process, with a comment noting it's just for temporary
metrics collection and will be removed soon. We could then record both the FMP
observed and the frequency of FMP after input after this change.

Up to you how to approach this but I think having some basic data here to
understand the frequency will be helpful / important to convince ourselves this
is still reasonably rare.

[Kunihiko Sakamoto, 2017-05-30 02:25:47]

The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 02:26:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 03:16:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 03:16:26]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Kunihiko Sakamoto, 2017-05-30 03:50:06]

Sorry for the delay.

On 2017/05/23 12:06:34, tdresser wrote:
> Why are we marking
> PageLoad.Experimental.PaintTiming.FirstMeaningfulPaintToNetworkStable as
> obsolete?

See here:
https://codereview.chromium.org/2883273003/diff/40001/chrome/browser/page_loa...

Since we don't have plan to change network idle heuristic further, I thought
this is no longer needed.


On 2017/05/23 14:25:36, Bryan McQuade wrote:
> Thanks! This looks fine to me. Only thought would be whether we should add
some
> UMA in the render process (in blink) to count the relative frequency of FMP
> happening before vs after user input, after this change. We know it was 2%
IIRC
> in the old impl, but given the buffering issues for consuming events in the
> browser process, it may be useful to confirm that this doesn't change
> significantly in the new impl. If the percentage increases significantly that
> seems really important to know.
> 
> I'd also be fine with adding a temporary field to the page load metrics code
> that sends FMP when it happens after user input to the page load metrics
> observers in the browser process, with a comment noting it's just for
temporary
> metrics collection and will be removed soon. We could then record both the FMP
> observed and the frequency of FMP after input after this change.
> 
> Up to you how to approach this but I think having some basic data here to
> understand the frequency will be helpful / important to convince ourselves
this
> is still reasonably rare.

Makes sense. Added a UMA in PaintTiming that counts FMP before vs after unser
input.

[Ilya Sherman, 2017-05-30 20:02:30]

Seems like the details of the metrics are still somewhat in flux.  Please let me
know when those settle and you'd like me to take another look.  Thanks!

[Kunihiko Sakamoto, 2017-06-07 08:26:37]

Hi Bryan and Tim, would you take another look?

[tdresser, 2017-06-07 12:32:54]

How big a pain would it be to also record FMP after input as a separate
histogram?

I think if we ever want to do proper survival analysis accounting for censoring
(https://en.wikipedia.org/wiki/Survival_analysis#Censoring) we'll need to know
these times.

This isn't high priority, but if it's easy to keep this histogram, I'd prefer we
keep it.

https://codereview.chromium.org/2883273003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp
(right):

https://codereview.chromium.org/2883273003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp:261: }
Should we have a test showing that we ignore input before FP?

[Kunihiko Sakamoto, 2017-06-08 07:54:49]

On 2017/06/07 12:32:54, tdresser wrote:
> How big a pain would it be to also record FMP after input as a separate
> histogram?
> 
> I think if we ever want to do proper survival analysis accounting for
censoring
> (https://en.wikipedia.org/wiki/Survival_analysis#Censoring) we'll need to know
> these times.
> 
> This isn't high priority, but if it's easy to keep this histogram, I'd prefer
we
> keep it.

To add a field to the page load metrics, we'll need to touch 10+ files for
plumbing, mostly mechanical though.

I'm not familiar with survival analysis, but IIUC we'll need time-to-first-input
for FMP-after-input cases, as the censoring time?

https://codereview.chromium.org/2883273003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp
(right):

https://codereview.chromium.org/2883273003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp:261: }
On 2017/06/07 12:32:54, tdresser wrote:
> Should we have a test showing that we ignore input before FP?

Done.

[tdresser, 2017-06-08 12:16:12]

On 2017/06/08 07:54:49, Kunihiko Sakamoto wrote:
> On 2017/06/07 12:32:54, tdresser wrote:
> > How big a pain would it be to also record FMP after input as a separate
> > histogram?
> > 
> > I think if we ever want to do proper survival analysis accounting for
> censoring
> > (https://en.wikipedia.org/wiki/Survival_analysis#Censoring) we'll need to
know
> > these times.
> > 
> > This isn't high priority, but if it's easy to keep this histogram, I'd
prefer
> we
> > keep it.
> 
> To add a field to the page load metrics, we'll need to touch 10+ files for
> plumbing, mostly mechanical though.
> 
> I'm not familiar with survival analysis, but IIUC we'll need
time-to-first-input
> for FMP-after-input cases, as the censoring time?

That matches my understanding.

This LGTM.

One thing to note - because we're only paying attention to input which hits the
renderer, this is fundamentally different from the previous definition. Things
like composited scrolling won't count as input now. I think that's fine.

[Bryan McQuade, 2017-06-08 17:47:12]

Thanks! A couple small additions. RE: logging FMP after input as well (Tim's
earlier suggestion), I'm fine with adding a temporary field to plumb that info
over to browser process as long as we add comments noting that it is temporary
and will be removed / should not be depended on. Up to you and Tim though.

https://codereview.chromium.org/2883273003/diff/160001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2883273003/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:478:
internal::FIRST_MEANINGFUL_PAINT_BACKGROUNDED);
if the status now only has 2 values, should we get rid of it and instead load a
FirstMeaningfulPaint.Background histogram? That's more consistent with how we
log other metrics.

https://codereview.chromium.org/2883273003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right):

https://codereview.chromium.org/2883273003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.cpp:121:
DEFINE_STATIC_LOCAL(EnumerationHistogram, had_user_input_histogram,
i'd rather not use PageLoad as the prefix here, as those are only tracked for
loads that are filtered according to page load metrics filtering policy. I don't
want to get into the habit of having other renderer-based page load metrics
either.

A couple ideas:
1. Use PageLoad.Internal namespace, which is used for internal tracking /
debugging metrics for PLM maintainers (seems right), e.g.
PageLoad.Internal.PaintTiming.HadUserInputBeforeFirstMeaningfulPaint - no need
for 'Experimental' here since there is no plan to ever promote such a metrics to
non experimental - it's just intended to be internal

2. use a totally new metrics namespace (up to you, just not PageLoad.*)

[Kunihiko Sakamoto, 2017-06-09 05:39:45]

On 2017/06/08 12:16:12, tdresser wrote:
> On 2017/06/08 07:54:49, Kunihiko Sakamoto wrote:
> > On 2017/06/07 12:32:54, tdresser wrote:
> > > How big a pain would it be to also record FMP after input as a separate
> > > histogram?
> > > 
> > > I think if we ever want to do proper survival analysis accounting for
> > censoring
> > > (https://en.wikipedia.org/wiki/Survival_analysis#Censoring) we'll need to
> know
> > > these times.
> > > 
> > > This isn't high priority, but if it's easy to keep this histogram, I'd
> prefer
> > we
> > > keep it.
> > 
> > To add a field to the page load metrics, we'll need to touch 10+ files for
> > plumbing, mostly mechanical though.
> > 
> > I'm not familiar with survival analysis, but IIUC we'll need
> time-to-first-input
> > for FMP-after-input cases, as the censoring time?
> 
> That matches my understanding.

I just realized survival analysis might be even more trickier. Let's consider
the following sequence of events:

t1: layout (significance = 50)
t2: user input
t3: layout (significance = 80)

If the layout at t3 was caused by user input at t2, the page's "real" FMP is at
t1. Otherwise (layout at t3 was not related to user input), the page's "real"
FMP is t3.
So, it's hard to say anything about the page's real FMP from this sample. We
cannot use t2 as the censoring time, since t1 might be actual FMP.

> 
> This LGTM.
> 
> One thing to note - because we're only paying attention to input which hits
the
> renderer, this is fundamentally different from the previous definition. Things
> like composited scrolling won't count as input now. I think that's fine.

Yeah, I think this is closer to what we want.

[Kunihiko Sakamoto, 2017-06-09 05:48:16]

On 2017/06/08 17:47:12, Bryan McQuade wrote:
> Thanks! A couple small additions. RE: logging FMP after input as well (Tim's
> earlier suggestion), I'm fine with adding a temporary field to plumb that info
> over to browser process as long as we add comments noting that it is temporary
> and will be removed / should not be depended on. Up to you and Tim though.

I'm a little reluctant to add the temporary field, since it's not clear what we
can reason about from FMP after input (see the example in the above comment).

https://codereview.chromium.org/2883273003/diff/160001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2883273003/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:478:
internal::FIRST_MEANINGFUL_PAINT_BACKGROUNDED);
On 2017/06/08 17:47:12, Bryan McQuade wrote:
> if the status now only has 2 values, should we get rid of it and instead load
a
> FirstMeaningfulPaint.Background histogram? That's more consistent with how we
> log other metrics.

It has two more values, "User left the page before {first contentful paint,
network stable}", recorded from OnComplete().

https://codereview.chromium.org/2883273003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right):

https://codereview.chromium.org/2883273003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.cpp:121:
DEFINE_STATIC_LOCAL(EnumerationHistogram, had_user_input_histogram,
On 2017/06/08 17:47:12, Bryan McQuade wrote:
> i'd rather not use PageLoad as the prefix here, as those are only tracked for
> loads that are filtered according to page load metrics filtering policy. I
don't
> want to get into the habit of having other renderer-based page load metrics
> either.
> 
> A couple ideas:
> 1. Use PageLoad.Internal namespace, which is used for internal tracking /
> debugging metrics for PLM maintainers (seems right), e.g.
> PageLoad.Internal.PaintTiming.HadUserInputBeforeFirstMeaningfulPaint - no need
> for 'Experimental' here since there is no plan to ever promote such a metrics
to
> non experimental - it's just intended to be internal
> 
> 2. use a totally new metrics namespace (up to you, just not PageLoad.*)

Thanks for the suggestions, renamed to PageLoad.Internal.

[tdresser, 2017-06-09 15:45:49]

On 2017/06/09 05:48:16, Kunihiko Sakamoto wrote:
> On 2017/06/08 17:47:12, Bryan McQuade wrote:
> > Thanks! A couple small additions. RE: logging FMP after input as well (Tim's
> > earlier suggestion), I'm fine with adding a temporary field to plumb that
info
> > over to browser process as long as we add comments noting that it is
temporary
> > and will be removed / should not be depended on. Up to you and Tim though.
> 
> I'm a little reluctant to add the temporary field, since it's not clear what
we
> can reason about from FMP after input (see the example in the above comment).
> 
>
https://codereview.chromium.org/2883273003/diff/160001/chrome/browser/page_lo...
> File
> chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
> (right):
> 
>
https://codereview.chromium.org/2883273003/diff/160001/chrome/browser/page_lo...
>
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:478:
> internal::FIRST_MEANINGFUL_PAINT_BACKGROUNDED);
> On 2017/06/08 17:47:12, Bryan McQuade wrote:
> > if the status now only has 2 values, should we get rid of it and instead
load
> a
> > FirstMeaningfulPaint.Background histogram? That's more consistent with how
we
> > log other metrics.
> 
> It has two more values, "User left the page before {first contentful paint,
> network stable}", recorded from OnComplete().
> 
>
https://codereview.chromium.org/2883273003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintTiming.cpp (right):
> 
>
https://codereview.chromium.org/2883273003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintTiming.cpp:121:
> DEFINE_STATIC_LOCAL(EnumerationHistogram, had_user_input_histogram,
> On 2017/06/08 17:47:12, Bryan McQuade wrote:
> > i'd rather not use PageLoad as the prefix here, as those are only tracked
for
> > loads that are filtered according to page load metrics filtering policy. I
> don't
> > want to get into the habit of having other renderer-based page load metrics
> > either.
> > 
> > A couple ideas:
> > 1. Use PageLoad.Internal namespace, which is used for internal tracking /
> > debugging metrics for PLM maintainers (seems right), e.g.
> > PageLoad.Internal.PaintTiming.HadUserInputBeforeFirstMeaningfulPaint - no
need
> > for 'Experimental' here since there is no plan to ever promote such a
metrics
> to
> > non experimental - it's just intended to be internal
> > 
> > 2. use a totally new metrics namespace (up to you, just not PageLoad.*)
> 
> Thanks for the suggestions, renamed to PageLoad.Internal.

Hmmm, good point, survival analysis will be extremely tricky here.

I'm fine ignoring it for now.

[Kunihiko Sakamoto, 2017-06-14 02:30:17]

The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 02:30:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 03:27:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 03:27:48]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Kunihiko Sakamoto, 2017-06-14 05:27:33]

The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 05:27:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2017-06-14 06:58:54]

ksakamoto@chromium.org changed reviewers:
+ kinuko@chromium.org, oysteine@chromium.org

[Kunihiko Sakamoto, 2017-06-14 06:58:55]

Thanks Tim!

Asking for OWNERs review, PTAL:
 oysteine@ for base/trace_event/
 bmcquade@ for page_load_metrics
 kinuko@ for blink
 isherman@ for histograms

[kinuko, 2017-06-14 07:58:42]

blink lgtm

https://codereview.chromium.org/2883273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.h (right):

https://codereview.chromium.org/2883273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.h:53: void
SetFirstMeaningfulPaint(double stamp, bool was_after_user_input);
nit: can we use enum over bool?  (Maybe define it in FMPDetector and use it
throughout in blink code?)

[commit-bot: I haz the power, 2017-06-14 07:59:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 07:59:47]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-06-15 03:40:20]

Metrics LGTM, thanks.

https://codereview.chromium.org/2883273003/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2883273003/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49843: +    FirstMeaningfulPaint.
Recorded when the page load reached network idle.
nit: s/reached/reaches

[Kunihiko Sakamoto, 2017-06-15 03:53:57]

https://codereview.chromium.org/2883273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintTiming.h (right):

https://codereview.chromium.org/2883273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintTiming.h:53: void
SetFirstMeaningfulPaint(double stamp, bool was_after_user_input);
On 2017/06/14 07:58:41, kinuko wrote:
> nit: can we use enum over bool?  (Maybe define it in FMPDetector and use it
> throughout in blink code?) 

Done.

https://codereview.chromium.org/2883273003/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2883273003/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49843: +    FirstMeaningfulPaint.
Recorded when the page load reached network idle.
On 2017/06/15 03:40:20, Ilya Sherman wrote:
> nit: s/reached/reaches

Done.

[Bryan McQuade, 2017-06-19 12:38:58]

LGTM, thanks!

[Kunihiko Sakamoto, 2017-06-20 05:52:12]

Hi oysteine@, could you take a look for base/trace_event/?
Thanks!

[Kunihiko Sakamoto, 2017-06-26 05:56:58]

The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 05:57:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2017-06-26 06:39:14]

ksakamoto@chromium.org changed reviewers:
+ primiano@chromium.org
- oysteine@chromium.org

[Kunihiko Sakamoto, 2017-06-26 06:39:15]

On 2017/06/20 05:52:12, Kunihiko Sakamoto wrote:
> Hi oysteine@, could you take a look for base/trace_event/?
> Thanks!

Looks like oysteine@ is ooo, +primiano@ for base/trace_event/

[Primiano Tucci (use gerrit), 2017-06-26 09:52:27]

primiano@chromium.org changed reviewers:
+ fmeawad@chromium.org

[Primiano Tucci (use gerrit), 2017-06-26 09:52:29]

base/trace_event RS-LGTM with +fmeawad  ACK.
Let's double-check with hi as that file is shared with v8 and other non-chrome
embedders, and I never remember if there is anything else that needs to be
updated (don't think so, but let's quickly check with Fadi)

[commit-bot: I haz the power, 2017-06-26 10:19:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 10:19:59]

Dry run: This issue passed the CQ dry run.

[fmeawad, 2017-06-26 17:23:12]

As long as you are using internal existing macros, nothing to worry about.

base/trace_event LGTM as well

[Kunihiko Sakamoto, 2017-06-27 01:34:31]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2017-06-27 01:34:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, kinuko@chromium.org, isherman@chromium.org,
bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2883273003/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-27 01:34:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 01:39:38]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1498527271711140,
"parent_rev": "c1f83c1e9c3eb2dcf4a8d07a60c0753fd7dba5ea", "commit_rev":
"6b1f67dbfa7dc753e17264a38c7cb51f63fac525"}

[commit-bot: I haz the power, 2017-06-27 01:39:53]

Description was changed from

==========
Move the user interaction policy for FirstMeaningfulPaint UMA into renderer

FirstMeaningfulPaint UMA is logged only if there's no user interaction
between FirstPaint and FirstMeaningfulPaint. But the other consumer of
FirstMeaningfulPaint did not have this logic.

This patch moves the logic for this user interaction policy from browser
(page_load_metrics) to renderer (FirstMeaningfulPaintDetector), so that
other page_load_metrics observers will get consistent FMP value.

This also removes user-input related FirstMeaningfulPaint histograms,
as we no longer track them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Move the user interaction policy for FirstMeaningfulPaint UMA into renderer

FirstMeaningfulPaint UMA is logged only if there's no user interaction
between FirstPaint and FirstMeaningfulPaint. But the other consumer of
FirstMeaningfulPaint did not have this logic.

This patch moves the logic for this user interaction policy from browser
(page_load_metrics) to renderer (FirstMeaningfulPaintDetector), so that
other page_load_metrics observers will get consistent FMP value.

This also removes user-input related FirstMeaningfulPaint histograms,
as we no longer track them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2883273003
Cr-Commit-Position: refs/heads/master@{#482507}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b1f67dbfa7dc753e17264a38c7c...
==========

[commit-bot: I haz the power, 2017-06-27 01:39:55]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/6b1f67dbfa7dc753e17264a38c7c...