FirstMeaningfulPaint UMA

FirstMeaningfulPaint UMA

This patch implements First Meaningful Paint (FMP) [1] as a UMA metric.
FMP is computed in FirstMeaningfulPaintDetector class, using the same
algorithm as existing trace-based FMP [2].

FirstMeaningfulPaintDetector observes layout operations during page load
until network stable (2 seconds of no network activity), and then
reports computed FMP to PaintTiming.

If the user navigated away from the page while observing layouts or
user interaction happened between FP and FMP, instead of reporting Time
to first meaningful paint, just log the event in separate
FirstMeaningfulPaintStatus histogram.

Design doc: http://goo.gl/TEiMi4

[1] https://goo.gl/vpaxv6
[2] https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metrics/system_health/loading_metric.html

BUG=632081
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/b190d8e50285016c58c89ae5717446388945fd8f
Cr-Commit-Position: refs/heads/master@{#411876}

[Kunihiko Sakamoto, 2016-06-07 06:47:38]

Description was changed from

==========
WIP
==========

to

==========
[WIP] FirstMeaningfulPaint UMA
==========

[Kunihiko Sakamoto, 2016-07-27 06:14:35]

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

[commit-bot: I haz the power, 2016-07-27 06:14:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2016-07-27 10:11:35]

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

[commit-bot: I haz the power, 2016-07-27 10:11:54]

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, 2016-07-27 11:46:29]

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

[commit-bot: I haz the power, 2016-07-27 11:46:29]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2016-07-28 01:33:12]

Description was changed from

==========
[WIP] FirstMeaningfulPaint UMA
==========

to

==========
FirstMeaningfulPaint UMA

This patch implements First Meaningful Paint (FMP) [1] as a UMA metric.
FMP is computed in FirstMeaningfulPaintDetector class, using the same
algorithm as existing trace-based FMP [2].

FirstMeaningfulPaintDetector observes layout operations during page load
until network stable (2 seconds of no network activity), and then
reports computed FMP to PaintTiming.

If the user navigated away from the page while observing layouts or
user interaction happened before FMP, instead of reporting Time to
first meaningful paint, just log the event in separate
FirstMeaningfulPaintStatus histogram.

Design doc: http://goo.gl/TEiMi4

[1] https://goo.gl/vpaxv6
[2]
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...

BUG=632081
==========

[Kunihiko Sakamoto, 2016-07-28 01:45:18]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:45:26]

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

[Kunihiko Sakamoto, 2016-07-28 01:45:32]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-07-28 01:45:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2016-07-28 01:46:52]

Patchset #1 (id:40001) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:47:05]

Patchset #1 (id:60001) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:47:18]

Patchset #1 (id:80001) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:47:32]

Patchset #1 (id:100001) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:47:46]

Patchset #1 (id:120001) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:47:58]

Patchset #1 (id:140001) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:48:09]

Patchset #1 (id:160001) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:48:20]

Patchset #1 (id:180001) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:48:30]

Patchset #1 (id:200001) has been deleted

[Kunihiko Sakamoto, 2016-07-28 01:48:44]

Patchset #1 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-07-28 01:56:30]

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

[commit-bot: I haz the power, 2016-07-28 01:56:31]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Kunihiko Sakamoto, 2016-07-28 02:02:02]

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

[Kunihiko Sakamoto, 2016-07-28 02:02:03]

PTAL

[kouhei (in TOK), 2016-07-28 03:45:04]

Would you split this patch when you land?

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:419:
FirstMeaningfulPaintDetector::from(*m_document).checkNetworkStable();
I think we can add a hook in FrameLoader::checkCompleted() for this purpose,
instead of doing active loader checks on each Resource load.

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp
(right):

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:78: if
(m_state == Reported || !document() ||
document()->fetcher()->hasPendingRequest())
s/hasPendingRequest/!document()->frame()->isLoading()?

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:404:
m_client->notifyPaint();
Should we refactor this so that we pass notifyPaint(textPainted/imagePainted)
every time, and client manages m_{image,text}Painted flag?

[paulirish, 2016-07-28 20:49:08]

We were discussing if it'll be easy to confirm the UMA numbers here match the
trace-based FMP calculation.
Can we add a trace_event right at the point where we have the calculated number?

[Bryan McQuade, 2016-07-29 15:02:26]

Thanks! Here are some initial comments for the page load metrics portion.

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:229:
observer->OnFirstImagePaint(new_timing, extra_info);
there's also some logic in this file that validates that the various events in a
PageLoadTiming are in order (see the calls to EventsInOrder). Can you add a
call, something like:

if (!EventsInOrder(timing.first_paint, timing.first_meaningful_paint)) {
  NOTREACHED() << ...;
  return false;
}

and there's some similar code in the test harness that makes sure timing data is
populated correctly which you'll need to update as well.

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

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:380:
base::Time stamp =
Ah, we need to get rid of the base::Time for navigation_start, as base::Time is
non-monotonic.

Instead, can you add a base::TimeTicks navigation_start_ member to this class,
and set it in OnCommit:
navigation_start_ = navigation_handle->GetNavigationStart();  (i think that's
the method name...)

Then you can create a timestamp using TimeTicks which is monotonic.

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:382:
if (!first_user_interaction_time_ || stamp < first_user_interaction_time_) {
I'm not sure I agree that FMP should only be recorded if it's before
interaction. If I just touch the screen while it's loading for instance, with no
intent of actually interacting, that doesn't seem to meaningfully affect whether
we should log FMP.

If you want to log a separate histogram restricted to only cases before any
interactions,
PageLoad.PaintTiming.NavigationToFirstMeaningfulPaint.BeforeInteraction, I could
see potentially doing that, but I'm still not sure it's useful. Not clear to me
that whether the user has touched the screen is indicative of a meaningful paint
happening or not.

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:498:
first_user_interaction_time_ = base::Time::Now();
same - I'm hoping we can remove this, but we should use TimeTicks, which is
monotonic, rather than Time.

https://codereview.chromium.org/2039363003/diff/240001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_load_timing.cc (right):

https://codereview.chromium.org/2039363003/diff/240001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_load_timing.cc:26: first_contentful_paint
== other.first_contentful_paint &&
oh wow we weren't testing this in the equality implementation? yikes, thanks for
catching it. we should get that fix landed asap. i'll land it in a separate
change and we can merge to M53.

[Bryan McQuade, 2016-07-29 17:59:27]

https://codereview.chromium.org/2039363003/diff/240001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_load_timing.cc (right):

https://codereview.chromium.org/2039363003/diff/240001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_load_timing.cc:26: first_contentful_paint
== other.first_contentful_paint &&
On 2016/07/29 at 15:02:26, Bryan McQuade wrote:
> oh wow we weren't testing this in the equality implementation? yikes, thanks
for catching it. we should get that fix landed asap. i'll land it in a separate
change and we can merge to M53.

I landed a fix for this here: https://codereview.chromium.org/2195743002

Note that you'll also need to update IsEmpty() below to include a test for
first_meaningful_paint.

[Kunihiko Sakamoto, 2016-08-01 08:49:54]

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

[commit-bot: I haz the power, 2016-08-01 08:50:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2016-08-01 08:52:07]

On 2016/07/28 03:45:04, kouhei wrote:
> Would you split this patch when you land?

Into Blink part chrome/ part?


On 2016/07/28 20:49:08, paulirish wrote:
> We were discussing if it'll be easy to confirm the UMA numbers here match the
> trace-based FMP calculation.
> Can we add a trace_event right at the point where we have the calculated
number?

Added a trace logging into PaintTiming::setFirstMeaningfulPaint().

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:229:
observer->OnFirstImagePaint(new_timing, extra_info);
On 2016/07/29 15:02:26, Bryan McQuade wrote:
> there's also some logic in this file that validates that the various events in
a
> PageLoadTiming are in order (see the calls to EventsInOrder). Can you add a
> call, something like:
> 
> if (!EventsInOrder(timing.first_paint, timing.first_meaningful_paint)) {
>   NOTREACHED() << ...;
>   return false;
> }
> 
> and there's some similar code in the test harness that makes sure timing data
is
> populated correctly which you'll need to update as well.

Done.
The only ordering we can guarantee is FP <= FMP.

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

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:380:
base::Time stamp =
On 2016/07/29 15:02:26, Bryan McQuade wrote:
> Ah, we need to get rid of the base::Time for navigation_start, as base::Time
is
> non-monotonic.
> 
> Instead, can you add a base::TimeTicks navigation_start_ member to this class,
> and set it in OnCommit:
> navigation_start_ = navigation_handle->GetNavigationStart();  (i think that's
> the method name...)
> 
> Then you can create a timestamp using TimeTicks which is monotonic.

Done.

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:382:
if (!first_user_interaction_time_ || stamp < first_user_interaction_time_) {
On 2016/07/29 15:02:26, Bryan McQuade wrote:
> I'm not sure I agree that FMP should only be recorded if it's before
> interaction. If I just touch the screen while it's loading for instance, with
no
> intent of actually interacting, that doesn't seem to meaningfully affect
whether
> we should log FMP.
> 
> If you want to log a separate histogram restricted to only cases before any
> interactions,
> PageLoad.PaintTiming.NavigationToFirstMeaningfulPaint.BeforeInteraction, I
could
> see potentially doing that, but I'm still not sure it's useful. Not clear to
me
> that whether the user has touched the screen is indicative of a meaningful
paint
> happening or not.

Since current implementation has fairly long layout observation period, I think
there is a risk of FMPDetector capturing a layout that triggered by user
interactions (for example, user opens a thread in GMail). I wanted to make
initial FMP implementation have as few noises as possible.

Ideally we'd filter user interactions that triggered some JavaScript work (not
sure how hard that is). But before trying to implement complicated heuristics, I
would like to see how often FMP comes after user interaction.

I'm not sure if it would be useful if we log another FMP histogram including
values after user interactions. I'm happy to add it if it has any use cases.

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:498:
first_user_interaction_time_ = base::Time::Now();
On 2016/07/29 15:02:26, Bryan McQuade wrote:
> same - I'm hoping we can remove this, but we should use TimeTicks, which is
> monotonic, rather than Time.

Done.

https://codereview.chromium.org/2039363003/diff/240001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_load_timing.cc (right):

https://codereview.chromium.org/2039363003/diff/240001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_load_timing.cc:26: first_contentful_paint
== other.first_contentful_paint &&
On 2016/07/29 17:59:27, Bryan McQuade wrote:
> Note that you'll also need to update IsEmpty() below to include a test for
> first_meaningful_paint.

Done.

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:419:
FirstMeaningfulPaintDetector::from(*m_document).checkNetworkStable();
On 2016/07/28 03:45:04, kouhei wrote:
> I think we can add a hook in FrameLoader::checkCompleted() for this purpose,
> instead of doing active loader checks on each Resource load.

I wanted to take into account all loading activities, including
non-load-event-blocking ones that don't trigger checkCompleted().

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp
(right):

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:78: if
(m_state == Reported || !document() ||
document()->fetcher()->hasPendingRequest())
On 2016/07/28 03:45:04, kouhei wrote:
> s/hasPendingRequest/!document()->frame()->isLoading()?

IIUC frame()->isLoading() never returns true after the load event. That is not
ideal, as FMP can happen after the load event.

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2039363003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:404:
m_client->notifyPaint();
On 2016/07/28 03:45:04, kouhei wrote:
> Should we refactor this so that we pass notifyPaint(textPainted/imagePainted)
> every time, and client manages m_{image,text}Painted flag?

Done.

[commit-bot: I haz the power, 2016-08-01 09:42:12]

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

[commit-bot: I haz the power, 2016-08-01 09:42:13]

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

[Kunihiko Sakamoto, 2016-08-02 01:56:07]

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

[commit-bot: I haz the power, 2016-08-02 01:56:23]

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, 2016-08-02 03:08:09]

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

[commit-bot: I haz the power, 2016-08-02 03:08:10]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2016-08-02 03:19:37]

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2039363003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:229:
observer->OnFirstImagePaint(new_timing, extra_info);
On 2016/08/01 08:52:06, Kunihiko Sakamoto wrote:
> On 2016/07/29 15:02:26, Bryan McQuade wrote:
> > there's also some logic in this file that validates that the various events
in
> a
> > PageLoadTiming are in order (see the calls to EventsInOrder). Can you add a
> > call, something like:
> > 
> > if (!EventsInOrder(timing.first_paint, timing.first_meaningful_paint)) {
> >   NOTREACHED() << ...;
> >   return false;
> > }
> > 
> > and there's some similar code in the test harness that makes sure timing
data
> is
> > populated correctly which you'll need to update as well.
> 
> Done.
> The only ordering we can guarantee is FP <= FMP.

Um, actually that wasn't true...
If the page has no foreground elements, FMP may be first (empty) paint where FP
is not recorded.

In patch set 4 I've made FMPDetector skip background-only paints.

[Bryan McQuade, 2016-08-02 16:16:57]

Thanks! Here are a few more pageloadmetrics questions/comments.

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

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:338:
if (!first_user_interaction_time_ || stamp < first_user_interaction_time_) {
i think i misunderstood this the first time through, but i continue to be
concerned about filtering this metric if a user interaction has occurred. if we
could know that the user interaction had a significant impact on the page
contents, then i agree that we'd want to filter that out, but if the user just
taps their finger on the screen while it is blank or even after a little content
has been painted, then that's not an interaction that affected the page, and we
shouldn't filter that out.

this is worrying because it means we'll bias metrics towards users that have
certain behaviors: users that tap the screen more during page load will have
more metrics filtered out. additionally, slow page loads may end up getting
filtered out because users may tap the screen to keep it from locking the screen
after the inactivity timeout has expired.

is there some way we can understand if the given user interaction actually had a
meaningful effect ont the page? can we know if the page itself consumed an evenf
ro the given user interaction, for instance? can we ignore any interactions
before first paint?

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:345:
base::TimeTicks::Now() - stamp);
i'd rather compute this delta once and store it in a variable above

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:352:
base::TimeTicks::Now() - stamp);
same

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h:106:
base::Optional<base::TimeTicks> first_user_interaction_time_;
if you use TimeTicks here, you can probably omit using Optional, since a zero
time ticks is unset. We only need Optional for TimeDelta, where it's possible to
have a zero length delta. (feel free to switch this to Optional<TimeDelta>
baselined from navigation_start_ if that makes using it elsewhere in the code
more straightforward)

[Kunihiko Sakamoto, 2016-08-03 08:15:27]

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

[commit-bot: I haz the power, 2016-08-03 08:15:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2016-08-03 08:35:16]

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

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:338:
if (!first_user_interaction_time_ || stamp < first_user_interaction_time_) {
On 2016/08/02 16:16:57, Bryan McQuade wrote:
> i think i misunderstood this the first time through, but i continue to be
> concerned about filtering this metric if a user interaction has occurred. if
we
> could know that the user interaction had a significant impact on the page
> contents, then i agree that we'd want to filter that out, but if the user just
> taps their finger on the screen while it is blank or even after a little
content
> has been painted, then that's not an interaction that affected the page, and
we
> shouldn't filter that out.
> 
> this is worrying because it means we'll bias metrics towards users that have
> certain behaviors: users that tap the screen more during page load will have
> more metrics filtered out. additionally, slow page loads may end up getting
> filtered out because users may tap the screen to keep it from locking the
screen
> after the inactivity timeout has expired.
> 
> is there some way we can understand if the given user interaction actually had
a
> meaningful effect ont the page? can we know if the page itself consumed an
evenf
> ro the given user interaction, for instance? can we ignore any interactions
> before first paint?

I think it's impractical to detect user interactions only those have impact on
the page contents. One possible approximation is to check if the touch event is
handled by JS or not. But some JS frameworks (jQuery, React, etc.) has
"catch-all" handler to implement their own event dispatching system, so this
approach would make another bias towards pages using such frameworks.

Can we proceed with current approach and check initial UMA data to see how often
user interaction happens before FMP? I think we cannot completely remove this
bias. If this happens too often in the wild, we may need to come up with more
sophisticated approach to reduce the bias.

Ignoring interactions before first paint would be safe. Thanks for the
suggestion, updated the code.

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:345:
base::TimeTicks::Now() - stamp);
On 2016/08/02 16:16:57, Bryan McQuade wrote:
> i'd rather compute this delta once and store it in a variable above

Done.

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:352:
base::TimeTicks::Now() - stamp);
On 2016/08/02 16:16:57, Bryan McQuade wrote:
> same

Done.

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2039363003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h:106:
base::Optional<base::TimeTicks> first_user_interaction_time_;
On 2016/08/02 16:16:57, Bryan McQuade wrote:
> if you use TimeTicks here, you can probably omit using Optional, since a zero
> time ticks is unset. We only need Optional for TimeDelta, where it's possible
to
> have a zero length delta. (feel free to switch this to Optional<TimeDelta>
> baselined from navigation_start_ if that makes using it elsewhere in the code
> more straightforward)

Omitted Optional<>.

[commit-bot: I haz the power, 2016-08-03 10:31:38]

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

[commit-bot: I haz the power, 2016-08-03 10:31:39]

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_...)

[Bryan McQuade, 2016-08-03 21:44:56]

bmcquade@chromium.org changed reviewers:
+ csharrison@chromium.org

[Bryan McQuade, 2016-08-03 21:44:58]

Thanks! The metrics side looks pretty good to me now. I'm going to be unable to
review for a bit, so I'm adding csharrison who can take over for the metrics
side of the review, and who can also give feedback on the blink side.

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

https://codereview.chromium.org/2039363003/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:110:
"PageLoad.PaintTiming.NavigationToFirstMeaningfulPaint";
given that these are currently experimental, I'm inclined to tag them as such
for now in the histogram name. Maybe
"PageLoad.Experimental.PaintTiming.NavigationToFirstMeaningfulPaint"
?

Essentially use an "Experimental" in the name for any metric that we don't yet
have total confidence in?

https://codereview.chromium.org/2039363003/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:112:
"PageLoad.PaintTiming.NavigationToFirstMeaningfulPaint.Background";
same experimental here?

https://codereview.chromium.org/2039363003/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:180:
"PageLoad.PaintTiming.FirstMeaningfulPaintStatus";
experimental

https://codereview.chromium.org/2039363003/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:496:
if (!timing.first_meaningful_paint) {
let's only record this if we actually got a first_paint as well. so:
if (timing.first_paint && !timing.first_meaningful_paint) {
...
}

https://codereview.chromium.org/2039363003/diff/320001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2039363003/diff/320001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_load_timing.h:40:
base::Optional<base::TimeDelta> dom_loading;
looks like you'll need to rebase - we recently removed dom_loading

[Kunihiko Sakamoto, 2016-08-04 01:45:41]

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

[commit-bot: I haz the power, 2016-08-04 01:46:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2016-08-04 01:47:36]

Thanks for your review, Bryan!

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

https://codereview.chromium.org/2039363003/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:110:
"PageLoad.PaintTiming.NavigationToFirstMeaningfulPaint";
On 2016/08/03 21:44:58, Bryan McQuade wrote:
> given that these are currently experimental, I'm inclined to tag them as such
> for now in the histogram name. Maybe
> "PageLoad.Experimental.PaintTiming.NavigationToFirstMeaningfulPaint"
> ?
> 
> Essentially use an "Experimental" in the name for any metric that we don't yet
> have total confidence in?

Yeah it's a nice idea, thanks.
Done.

https://codereview.chromium.org/2039363003/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:112:
"PageLoad.PaintTiming.NavigationToFirstMeaningfulPaint.Background";
On 2016/08/03 21:44:58, Bryan McQuade wrote:
> same experimental here?

Done.

https://codereview.chromium.org/2039363003/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:180:
"PageLoad.PaintTiming.FirstMeaningfulPaintStatus";
On 2016/08/03 21:44:58, Bryan McQuade wrote:
> experimental

Done.

https://codereview.chromium.org/2039363003/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:496:
if (!timing.first_meaningful_paint) {
On 2016/08/03 21:44:58, Bryan McQuade wrote:
> let's only record this if we actually got a first_paint as well. so:
> if (timing.first_paint && !timing.first_meaningful_paint) {
> ...
> }

Done.

https://codereview.chromium.org/2039363003/diff/320001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2039363003/diff/320001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_load_timing.h:40:
base::Optional<base::TimeDelta> dom_loading;
On 2016/08/03 21:44:58, Bryan McQuade wrote:
> looks like you'll need to rebase - we recently removed dom_loading

Rebased in PS7.

[Kunihiko Sakamoto, 2016-08-04 01:52:26]

Description was changed from

==========
FirstMeaningfulPaint UMA

This patch implements First Meaningful Paint (FMP) [1] as a UMA metric.
FMP is computed in FirstMeaningfulPaintDetector class, using the same
algorithm as existing trace-based FMP [2].

FirstMeaningfulPaintDetector observes layout operations during page load
until network stable (2 seconds of no network activity), and then
reports computed FMP to PaintTiming.

If the user navigated away from the page while observing layouts or
user interaction happened before FMP, instead of reporting Time to
first meaningful paint, just log the event in separate
FirstMeaningfulPaintStatus histogram.

Design doc: http://goo.gl/TEiMi4

[1] https://goo.gl/vpaxv6
[2]
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...

BUG=632081
==========

to

==========
FirstMeaningfulPaint UMA

This patch implements First Meaningful Paint (FMP) [1] as a UMA metric.
FMP is computed in FirstMeaningfulPaintDetector class, using the same
algorithm as existing trace-based FMP [2].

FirstMeaningfulPaintDetector observes layout operations during page load
until network stable (2 seconds of no network activity), and then
reports computed FMP to PaintTiming.

If the user navigated away from the page while observing layouts or
user interaction happened between FP and FMP, instead of reporting Time
to first meaningful paint, just log the event in separate
FirstMeaningfulPaintStatus histogram.

Design doc: http://goo.gl/TEiMi4

[1] https://goo.gl/vpaxv6
[2]
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...

BUG=632081
==========

[Charlie Harrison, 2016-08-04 02:18:30]

Did a first pass on page_load_metrics. Consider the comment to add a parse start
to first meaningful paint optional if this is too experimental.

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(left):

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:102:
const char kHistogramParseStartToFirstContentfulPaint[] =
Do you mind adding a ParseStartToFirstMeaningfulPaint variant? This is useful
for de-noising lots of non-navigation based improvements.

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

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:69:
FMP_ENUM_MAX
nit: page_load_metrics component uses LAST_VALUE instead of MAX.

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:111:
const char kBackgroundHistogramFirstMeaningfulPaint[] =
I am slightly opposed to adding background paint histograms at this point. No
one looks at them, and the existing ones are good enough imo. Do you have a good
reason to include these?

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h:94:

nit: Remove this empty line.

https://codereview.chromium.org/2039363003/diff/360001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2039363003/diff/360001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_load_timing.h:55: // Time when the page's
primary content is painted.
If this is experimental can you document that here?

https://codereview.chromium.org/2039363003/diff/360001/chrome/renderer/page_l...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right):

https://codereview.chromium.org/2039363003/diff/360001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:143: if
(perf.firstMeaningfulPaint() > 0.0)
nit: Can you wrap the if with braces? I'm not a fan of multi-line if bodies
without braces. (Can do it for FCP too).

[Kunihiko Sakamoto, 2016-08-04 02:56:15]

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(left):

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:102:
const char kHistogramParseStartToFirstContentfulPaint[] =
On 2016/08/04 02:18:29, csharrison wrote:
> Do you mind adding a ParseStartToFirstMeaningfulPaint variant? This is useful
> for de-noising lots of non-navigation based improvements.

Added.

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

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:69:
FMP_ENUM_MAX
On 2016/08/04 02:18:29, csharrison wrote:
> nit: page_load_metrics component uses LAST_VALUE instead of MAX.

Not LAST_VALUE but LAST_ENTRY right?
Done.

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:111:
const char kBackgroundHistogramFirstMeaningfulPaint[] =
On 2016/08/04 02:18:29, csharrison wrote:
> I am slightly opposed to adding background paint histograms at this point. No
> one looks at them, and the existing ones are good enough imo. Do you have a
good
> reason to include these?

If no one looks at existing ones, no reason to add them.
Changed to log only FMP status for backgrounded case.

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h:94:

On 2016/08/04 02:18:29, csharrison wrote:
> nit: Remove this empty line.

Done.

https://codereview.chromium.org/2039363003/diff/360001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2039363003/diff/360001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_load_timing.h:55: // Time when the page's
primary content is painted.
On 2016/08/04 02:18:29, csharrison wrote:
> If this is experimental can you document that here?

Done.

https://codereview.chromium.org/2039363003/diff/360001/chrome/renderer/page_l...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right):

https://codereview.chromium.org/2039363003/diff/360001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:143: if
(perf.firstMeaningfulPaint() > 0.0)
On 2016/08/04 02:18:29, csharrison wrote:
> nit: Can you wrap the if with braces? I'm not a fan of multi-line if bodies
> without braces. (Can do it for FCP too).

Done.

[Charlie Harrison, 2016-08-04 18:27:28]

Can you add one browsertest to page_load_metrics_browsertest to show that e.g.
navigating to /title1.html gives us a FMP?

Otherwise page_load_metrics looks good. I briefly looked at Blink side and
didn't find anything wrong but I haven't dug deep. That will probably want unit
tests though :)

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

https://codereview.chromium.org/2039363003/diff/360001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:69:
FMP_ENUM_MAX
On 2016/08/04 02:56:14, Kunihiko Sakamoto wrote:
> On 2016/08/04 02:18:29, csharrison wrote:
> > nit: page_load_metrics component uses LAST_VALUE instead of MAX.
> 
> Not LAST_VALUE but LAST_ENTRY right?
> Done.

Oops yes :P thanks.

https://codereview.chromium.org/2039363003/diff/380001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_load_timing.cc (right):

https://codereview.chromium.org/2039363003/diff/380001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_load_timing.cc:39:
!first_meaningful_paint&& !parse_start && !parse_stop &&
nit: add space before &&

[Charlie Harrison, 2016-08-04 18:28:42]

+loading-reviews

[Kunihiko Sakamoto, 2016-08-08 06:47:12]

Added tests in page_load_metrics_browsertest.cc,
core_page_load_metrics_observer_unittest.cc, and
FirstMeaningfulPaintDetectorTest.cpp.

https://codereview.chromium.org/2039363003/diff/380001/chrome/common/page_loa...
File chrome/common/page_load_metrics/page_load_timing.cc (right):

https://codereview.chromium.org/2039363003/diff/380001/chrome/common/page_loa...
chrome/common/page_load_metrics/page_load_timing.cc:39:
!first_meaningful_paint&& !parse_start && !parse_stop &&
On 2016/08/04 18:27:28, csharrison wrote:
> nit: add space before &&

Done.

[Kunihiko Sakamoto, 2016-08-08 07:18:57]

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

[commit-bot: I haz the power, 2016-08-08 07:19:16]

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, 2016-08-08 08:38:59]

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

[commit-bot: I haz the power, 2016-08-08 08:39:00]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-08-08 12:35:44]

page_load_metrics lgtm with one comment. Still haven't taken a hard look at
Blink side.

https://codereview.chromium.org/2039363003/diff/420001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2039363003/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:388: // Wait
until a FMP is reported, by polling the histogram counter.
Can this be avoided? I would slightly prefer using
content::ExecuteScriptAndExtractBool + setTimeout to use a one-shot delay that
we know will have triggered FMP.

Note: NavigateToURL should wait until the load event, which will probably (at
least in this case) be after the true FMP.

[Kunihiko Sakamoto, 2016-08-09 02:32:49]

https://codereview.chromium.org/2039363003/diff/420001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2039363003/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:388: // Wait
until a FMP is reported, by polling the histogram counter.
On 2016/08/08 12:35:44, csharrison wrote:
> Can this be avoided? I would slightly prefer using
> content::ExecuteScriptAndExtractBool + setTimeout to use a one-shot delay that
> we know will have triggered FMP.
> 
> Note: NavigateToURL should wait until the load event, which will probably (at
> least in this case) be after the true FMP.

I'm afraid that that will be a source of flakiness, unless we use a pretty long
time margin. The longer timeout we use, the slower the test will be...

btw, is it possible to delay ExecuteScriptAndExtractBool's return by using
setTimeout?

[Charlie Harrison, 2016-08-09 02:36:43]

https://codereview.chromium.org/2039363003/diff/420001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2039363003/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:388: // Wait
until a FMP is reported, by polling the histogram counter.
On 2016/08/09 02:32:49, Kunihiko Sakamoto wrote:
> On 2016/08/08 12:35:44, csharrison wrote:
> > Can this be avoided? I would slightly prefer using
> > content::ExecuteScriptAndExtractBool + setTimeout to use a one-shot delay
that
> > we know will have triggered FMP.
> > 
> > Note: NavigateToURL should wait until the load event, which will probably
(at
> > least in this case) be after the true FMP.
> 
> I'm afraid that that will be a source of flakiness, unless we use a pretty
long
> time margin. The longer timeout we use, the slower the test will be...
> 
> btw, is it possible to delay ExecuteScriptAndExtractBool's return by using
> setTimeout?

Yeah, that was my idea. The content API call will wait for JS to post a message
back to it (codesearch for usage examples). So you can e.g. do:

ExecuteScriptAndExtractBool("setTimeout(function() {
window.domAutomationController.send(true); }, kNetworkIdleTime);");

Hopefully that should be good enough to unflake without resorting to spinning
the message loop.

[Kunihiko Sakamoto, 2016-08-09 03:27:15]

Patchset #11 (id:440001) has been deleted

[Kunihiko Sakamoto, 2016-08-09 03:30:45]

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

[commit-bot: I haz the power, 2016-08-09 03:31:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2016-08-09 03:31:14]

https://codereview.chromium.org/2039363003/diff/420001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2039363003/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:388: // Wait
until a FMP is reported, by polling the histogram counter.
On 2016/08/09 02:36:43, csharrison wrote:
> On 2016/08/09 02:32:49, Kunihiko Sakamoto wrote:
> > On 2016/08/08 12:35:44, csharrison wrote:
> > > Can this be avoided? I would slightly prefer using
> > > content::ExecuteScriptAndExtractBool + setTimeout to use a one-shot delay
> that
> > > we know will have triggered FMP.
> > > 
> > > Note: NavigateToURL should wait until the load event, which will probably
> (at
> > > least in this case) be after the true FMP.
> > 
> > I'm afraid that that will be a source of flakiness, unless we use a pretty
> long
> > time margin. The longer timeout we use, the slower the test will be...
> > 
> > btw, is it possible to delay ExecuteScriptAndExtractBool's return by using
> > setTimeout?
> 
> Yeah, that was my idea. The content API call will wait for JS to post a
message
> back to it (codesearch for usage examples). So you can e.g. do:
> 
> ExecuteScriptAndExtractBool("setTimeout(function() {
> window.domAutomationController.send(true); }, kNetworkIdleTime);");
> 
> Hopefully that should be good enough to unflake without resorting to spinning
> the message loop.

Ah, now I got your idea.
Done, thanks!

[commit-bot: I haz the power, 2016-08-09 05:00:05]

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

[commit-bot: I haz the power, 2016-08-09 05:00:07]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-08-09 16:27:33]

Some provisional renderer side comments. Overall lg.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp
(right):

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:21: const
double kTimeWithoutNetworkActivity = 2.0;
Hm, this name isn't quite clear. The name as-is makes me think it is counting
the current time without network activity, rather than establishing a threshold.
Could we s/Time/Seconds/ and possibly add "Threshold" on the end?

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:41: void
FirstMeaningfulPaintDetector::computeLayoutSignificance(
It would help to document this method with justification, etc.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:51: double
ratioBefore = std::max(1.0, static_cast<double>(contentsHeightBeforeLayout) /
visibleHeight);
Can visibleHeight ever be 0? If not can you add a DCHECK (possibly with an
explanation)?

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.h
(right):

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.h:22: class
CORE_EXPORT FirstMeaningfulPaintDetector : public
GarbageCollectedFinalized<FirstMeaningfulPaintDetector> {
Just to make sure: does this have to be finalized due to the
m_networkStableTimer?

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.h:25: class
LayoutObjectCounter {
Can you document this class and who should be using it?

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

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp:78:
TEST_F(FirstMeaningfulPaintDetectorTest, TwoLayoutsSignificantFirst)
Seems like the second is significant here, not the first.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp:90:
TEST_F(FirstMeaningfulPaintDetectorTest, TwoLayoutsSignificantSecond)
Seems like the first is significant here, not the second.

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

https://codereview.chromium.org/2039363003/diff/460001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:78911: +      label="User interaction
happend before first meaningful paint."/>
s/happend/happened

[kouhei (in TOK), 2016-08-10 01:58:14]

Only reviewed tp/WebKit

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp
(right):

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:41: void
FirstMeaningfulPaintDetector::computeLayoutSignificance(
markNextPaintAsMeaningfulIfNeeded?

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:83: if
(m_state == Reported || !document() ||
document()->fetcher()->hasPendingRequest())
Move !document() -> DCHECK(document());

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:86: if
(m_networkStableTimer.isActive())
if (!m_networkStableTimer.isActive())
    m_networkStableTimer.startOneShot(kTimeWithoutNetworkActivity,
BLINK_FROM_HERE);

[Kunihiko Sakamoto, 2016-08-10 02:28:15]

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

[commit-bot: I haz the power, 2016-08-10 02:28:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2016-08-10 02:33:32]

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp
(right):

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:21: const
double kTimeWithoutNetworkActivity = 2.0;
On 2016/08/09 16:27:32, Charlie Harrison wrote:
> Hm, this name isn't quite clear. The name as-is makes me think it is counting
> the current time without network activity, rather than establishing a
threshold.
> Could we s/Time/Seconds/ and possibly add "Threshold" on the end?

Thanks for the suggestion. Done.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:41: void
FirstMeaningfulPaintDetector::computeLayoutSignificance(
On 2016/08/09 16:27:32, Charlie Harrison wrote:
> It would help to document this method with justification, etc.

Done.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:41: void
FirstMeaningfulPaintDetector::computeLayoutSignificance(
On 2016/08/10 01:58:13, kouhei wrote:
> markNextPaintAsMeaningfulIfNeeded?

Yeah, that's a better description. Done.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:51: double
ratioBefore = std::max(1.0, static_cast<double>(contentsHeightBeforeLayout) /
visibleHeight);
On 2016/08/09 16:27:32, Charlie Harrison wrote:
> Can visibleHeight ever be 0? If not can you add a DCHECK (possibly with an
> explanation)?

Good point, actually it can be 0.
Added an early return, since layout significance in such a case is considered to
be 0.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:83: if
(m_state == Reported || !document() ||
document()->fetcher()->hasPendingRequest())
On 2016/08/10 01:58:13, kouhei wrote:
> Move !document() -> DCHECK(document());

Done.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp:86: if
(m_networkStableTimer.isActive())
On 2016/08/10 01:58:13, kouhei wrote:
> if (!m_networkStableTimer.isActive())
>     m_networkStableTimer.startOneShot(kTimeWithoutNetworkActivity,
> BLINK_FROM_HERE);

We need to update the timer to fire after 2 secs after this point.

It seems startOneShot() cancels active one if exists, so I've just removed this
conditional.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.h
(right):

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.h:22: class
CORE_EXPORT FirstMeaningfulPaintDetector : public
GarbageCollectedFinalized<FirstMeaningfulPaintDetector> {
On 2016/08/09 16:27:32, Charlie Harrison wrote:
> Just to make sure: does this have to be finalized due to the
> m_networkStableTimer?

Yes.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.h:25: class
LayoutObjectCounter {
On 2016/08/09 16:27:33, Charlie Harrison wrote:
> Can you document this class and who should be using it?

Done.

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

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp:78:
TEST_F(FirstMeaningfulPaintDetectorTest, TwoLayoutsSignificantFirst)
On 2016/08/09 16:27:33, Charlie Harrison wrote:
> Seems like the second is significant here, not the first.

Ooops, fixed.

https://codereview.chromium.org/2039363003/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetectorTest.cpp:90:
TEST_F(FirstMeaningfulPaintDetectorTest, TwoLayoutsSignificantSecond)
On 2016/08/09 16:27:33, Charlie Harrison wrote:
> Seems like the first is significant here, not the second.

Done.

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

https://codereview.chromium.org/2039363003/diff/460001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:78911: +      label="User interaction
happend before first meaningful paint."/>
On 2016/08/09 16:27:33, Charlie Harrison wrote:
> s/happend/happened

Done.

https://codereview.chromium.org/2039363003/diff/480001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2039363003/diff/480001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:393:
kNetworkIdleTime + kMargin);
I noticed this test fails about once a ten times for some reasons (IPC race?) so
I had to add some margin.

[commit-bot: I haz the power, 2016-08-10 02:37:04]

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

[commit-bot: I haz the power, 2016-08-10 02:37:05]

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

[kouhei (in TOK), 2016-08-10 03:28:24]

core lgtm

[Kunihiko Sakamoto, 2016-08-10 03:45:16]

ksakamoto@chromium.org changed reviewers:
+ chrishtr@chromium.org, isherman@chromium.org, tkent@chromium.org

[Kunihiko Sakamoto, 2016-08-10 03:45:18]

+chrishtr for platform/graphics
+tkent for {Source,public}/web
+isherman for histograms.xml

[tkent, 2016-08-10 04:15:00]

> +tkent for {Source,public}/web

lgtm

[Ilya Sherman, 2016-08-10 05:42:37]

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

https://codereview.chromium.org/2039363003/diff/480001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:358:
base::TimeTicks fmp =
nit: I think "paint" would be a clearer name than "fmp", and still quite short.

https://codereview.chromium.org/2039363003/diff/480001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:521:
internal::FMP_LAST_ENTRY);
nit: I would recommend having a wrapper function for emitting to this histogram,
that you could call as something like

  RecordFirstMeaningfulPaintStatus(internal::FMP_DID_NOT_REACH_NETWORK_STABLE);

Also, we generally try to avoid acronyms like "FMP" in code.

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

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38039: +    Records whether the First
Meaningful Paint metric was reported for the page
It would be helpful to link to some document explaining what the "first
meaningful paint" is, exactly.

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38050: +    have happened, and when we
actually logged the metric.
"when we actually logged the metric" doesn't really give a clear explanation of
why that's relevant.  Why is this metric relevant?  Please clarify in the
description.

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38065: +   
name="PageLoad.Experimental.PaintTiming.ParseStartToFirstMeaningfulPaint"
Hmm, why is it useful to measure both the duration from parse start to FMP and
from navigation to FMP?  Is the duration between navigation and parse start not
already measured in other histograms?

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:78911: +      label="User interaction
happened before first meaningful paint."/>
These labels are quite long, and will be hard to read as the left-hand column in
a tabular view.  Could you shorten them somewhat, without reducing their clarity
in context?

[Kunihiko Sakamoto, 2016-08-10 07:25:13]

Description was changed from

==========
FirstMeaningfulPaint UMA

This patch implements First Meaningful Paint (FMP) [1] as a UMA metric.
FMP is computed in FirstMeaningfulPaintDetector class, using the same
algorithm as existing trace-based FMP [2].

FirstMeaningfulPaintDetector observes layout operations during page load
until network stable (2 seconds of no network activity), and then
reports computed FMP to PaintTiming.

If the user navigated away from the page while observing layouts or
user interaction happened between FP and FMP, instead of reporting Time
to first meaningful paint, just log the event in separate
FirstMeaningfulPaintStatus histogram.

Design doc: http://goo.gl/TEiMi4

[1] https://goo.gl/vpaxv6
[2]
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...

BUG=632081
==========

to

==========
FirstMeaningfulPaint UMA

This patch implements First Meaningful Paint (FMP) [1] as a UMA metric.
FMP is computed in FirstMeaningfulPaintDetector class, using the same
algorithm as existing trace-based FMP [2].

FirstMeaningfulPaintDetector observes layout operations during page load
until network stable (2 seconds of no network activity), and then
reports computed FMP to PaintTiming.

If the user navigated away from the page while observing layouts or
user interaction happened between FP and FMP, instead of reporting Time
to first meaningful paint, just log the event in separate
FirstMeaningfulPaintStatus histogram.

Design doc: http://goo.gl/TEiMi4

[1] https://goo.gl/vpaxv6
[2]
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...

BUG=632081
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Kunihiko Sakamoto, 2016-08-10 07:27:23]

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

[commit-bot: I haz the power, 2016-08-10 07:27:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kunihiko Sakamoto, 2016-08-10 07:43:09]

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

https://codereview.chromium.org/2039363003/diff/480001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:358:
base::TimeTicks fmp =
On 2016/08/10 05:42:36, Ilya Sherman wrote:
> nit: I think "paint" would be a clearer name than "fmp", and still quite
short.

Done.

https://codereview.chromium.org/2039363003/diff/480001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:521:
internal::FMP_LAST_ENTRY);
On 2016/08/10 05:42:36, Ilya Sherman wrote:
> nit: I would recommend having a wrapper function for emitting to this
histogram,
> that you could call as something like
> 
>  
RecordFirstMeaningfulPaintStatus(internal::FMP_DID_NOT_REACH_NETWORK_STABLE);
> 
> Also, we generally try to avoid acronyms like "FMP" in code.

Added a wrapper function, and expanded the acronym.

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

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38039: +    Records whether the First
Meaningful Paint metric was reported for the page
On 2016/08/10 05:42:36, Ilya Sherman wrote:
> It would be helpful to link to some document explaining what the "first
> meaningful paint" is, exactly.

Done.

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38050: +    have happened, and when we
actually logged the metric.
On 2016/08/10 05:42:36, Ilya Sherman wrote:
> "when we actually logged the metric" doesn't really give a clear explanation
of
> why that's relevant.  Why is this metric relevant?  Please clarify in the
> description.

Done.

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38065: +   
name="PageLoad.Experimental.PaintTiming.ParseStartToFirstMeaningfulPaint"
On 2016/08/10 05:42:36, Ilya Sherman wrote:
> Hmm, why is it useful to measure both the duration from parse start to FMP and
> from navigation to FMP?  Is the duration between navigation and parse start
not
> already measured in other histograms?

This can be less-noisy optimization target for non-navigation based
improvements. csharrison@ suggested this earlier in this review (#53), so he
might have some more context.

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:78911: +      label="User interaction
happened before first meaningful paint."/>
On 2016/08/10 05:42:36, Ilya Sherman wrote:
> These labels are quite long, and will be hard to read as the left-hand column
in
> a tabular view.  Could you shorten them somewhat, without reducing their
clarity
> in context?

Done.

[commit-bot: I haz the power, 2016-08-10 09:59:43]

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

[commit-bot: I haz the power, 2016-08-10 09:59:45]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-08-10 12:17:07]

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

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38065: +   
name="PageLoad.Experimental.PaintTiming.ParseStartToFirstMeaningfulPaint"
On 2016/08/10 07:43:09, Kunihiko Sakamoto wrote:
> On 2016/08/10 05:42:36, Ilya Sherman wrote:
> > Hmm, why is it useful to measure both the duration from parse start to FMP
and
> > from navigation to FMP?  Is the duration between navigation and parse start
> not
> > already measured in other histograms?
> 
> This can be less-noisy optimization target for non-navigation based
> improvements. csharrison@ suggested this earlier in this review (#53), so he
> might have some more context.

Yeah in general it is a challenge of the loading team to get stable metrics,
especially with data from some of the early channels. Cutting out a
(significant) time in the loading pipeline de-noises our experiments that only
take affect once the page starts parsing HTML (which is the majority of them).

[Ilya Sherman, 2016-08-10 18:48:09]

Thanks!  LG % discussion of whether we can record fewer metrics for this
purpose.

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

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38065: +   
name="PageLoad.Experimental.PaintTiming.ParseStartToFirstMeaningfulPaint"
On 2016/08/10 12:17:07, Charlie Harrison wrote:
> On 2016/08/10 07:43:09, Kunihiko Sakamoto wrote:
> > On 2016/08/10 05:42:36, Ilya Sherman wrote:
> > > Hmm, why is it useful to measure both the duration from parse start to FMP
> and
> > > from navigation to FMP?  Is the duration between navigation and parse
start
> > not
> > > already measured in other histograms?
> > 
> > This can be less-noisy optimization target for non-navigation based
> > improvements. csharrison@ suggested this earlier in this review (#53), so he
> > might have some more context.
> 
> Yeah in general it is a challenge of the loading team to get stable metrics,
> especially with data from some of the early channels. Cutting out a
> (significant) time in the loading pipeline de-noises our experiments that only
> take affect once the page starts parsing HTML (which is the majority of them).

So, would it make sense to *only* measure the duration from parse start time,
and not to measure the duration from navigation?

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

https://codereview.chromium.org/2039363003/diff/520001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38131: +    have happened, and when we
actually logged the metric.
nit: Please add the bitly link here, and for the other FMP metrics as well =)

[Charlie Harrison, 2016-08-10 19:12:20]

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

https://codereview.chromium.org/2039363003/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38065: +   
name="PageLoad.Experimental.PaintTiming.ParseStartToFirstMeaningfulPaint"
On 2016/08/10 at 18:48:09, Ilya Sherman wrote:
> On 2016/08/10 12:17:07, Charlie Harrison wrote:
> > On 2016/08/10 07:43:09, Kunihiko Sakamoto wrote:
> > > On 2016/08/10 05:42:36, Ilya Sherman wrote:
> > > > Hmm, why is it useful to measure both the duration from parse start to
FMP
> > and
> > > > from navigation to FMP?  Is the duration between navigation and parse
start
> > > not
> > > > already measured in other histograms?
> > > 
> > > This can be less-noisy optimization target for non-navigation based
> > > improvements. csharrison@ suggested this earlier in this review (#53), so
he
> > > might have some more context.
> > 
> > Yeah in general it is a challenge of the loading team to get stable metrics,
> > especially with data from some of the early channels. Cutting out a
> > (significant) time in the loading pipeline de-noises our experiments that
only
> > take affect once the page starts parsing HTML (which is the majority of
them).
> 
> So, would it make sense to *only* measure the duration from parse start time,
and not to measure the duration from navigation?

It would, except for the fact that some optimizations do take into account the
navigation (speculative prefetch, plznavigate, preconnect, http2 push, etc). If
that weren't the case then yeah cutting out navigation time wholesale would be
fine.

[Ilya Sherman, 2016-08-10 19:20:08]

Okay, sounds like there's value to both metrics, so LGTM % the one remaining
nit.

[chrishtr, 2016-08-11 00:43:35]

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:243: if (m_node)
On every LayoutObject creation? That's expensive.

[Charlie Harrison, 2016-08-11 00:48:57]

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:243: if (m_node)
On 2016/08/11 00:43:35, chrishtr wrote:
> On every LayoutObject creation? That's expensive.

It's just upping a counter, right? Or are you more concerned with the pointer
indirection.

[chrishtr, 2016-08-11 00:55:47]

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:243: if (m_node)
On 2016/08/11 at 00:48:57, Charlie Harrison wrote:
> On 2016/08/11 00:43:35, chrishtr wrote:
> > On every LayoutObject creation? That's expensive.
> 
> It's just upping a counter, right? Or are you more concerned with the pointer
indirection.

I guess it's ok...

Isn't it already counted by line 242's use of InstanceCounters?

[Charlie Harrison, 2016-08-11 01:01:38]

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:243: if (m_node)
On 2016/08/11 00:55:46, chrishtr wrote:
> On 2016/08/11 at 00:48:57, Charlie Harrison wrote:
> > On 2016/08/11 00:43:35, chrishtr wrote:
> > > On every LayoutObject creation? That's expensive.
> > 
> > It's just upping a counter, right? Or are you more concerned with the
pointer
> indirection.
> 
> I guess it's ok...
> 
> Isn't it already counted by line 242's use of InstanceCounters?

Good catch. It would be nice to not duplicate this work, but I think the
instance counters are per process instead of per frame. Not sure what the right
answer is here.

[Kunihiko Sakamoto, 2016-08-11 03:50:18]

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2039363003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:243: if (m_node)
On 2016/08/11 01:01:37, Charlie Harrison wrote:
> On 2016/08/11 00:55:46, chrishtr wrote:
> > On 2016/08/11 at 00:48:57, Charlie Harrison wrote:
> > > On 2016/08/11 00:43:35, chrishtr wrote:
> > > > On every LayoutObject creation? That's expensive.
> > > 
> > > It's just upping a counter, right? Or are you more concerned with the
> pointer
> > indirection.
> > 
> > I guess it's ok...
> > 
> > Isn't it already counted by line 242's use of InstanceCounters?
> 
> Good catch. It would be nice to not duplicate this work, but I think the
> instance counters are per process instead of per frame. Not sure what the
right
> answer is here.

Yeah the instance counters are process global. We do not want to count layout
objects created in other frames (or even worse, in other tabs).

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

https://codereview.chromium.org/2039363003/diff/520001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38131: +    have happened, and when we
actually logged the metric.
On 2016/08/10 18:48:09, Ilya Sherman wrote:
> nit: Please add the bitly link here, and for the other FMP metrics as well =)

Done.

[chrishtr, 2016-08-11 15:11:40]

lgtm

[Kunihiko Sakamoto, 2016-08-12 00:54:58]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2016-08-12 00:54:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, tkent@chromium.org, kouhei@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2039363003/#ps540001
(title: "histogram descriptions")

[commit-bot: I haz the power, 2016-08-12 00:55:32]

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, 2016-08-12 01:02:49]

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

[commit-bot: I haz the power, 2016-08-12 01:02:53]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Harrison, 2016-08-12 01:05:25]

Looks like IPCs need security reviews.

[Kunihiko Sakamoto, 2016-08-12 01:13:00]

ksakamoto@chromium.org changed reviewers:
+ tsepez@chromium.org

[Kunihiko Sakamoto, 2016-08-12 01:13:02]

+tsepez for IPC (page_load_metrics_messages.h)

[Tom Sepez, 2016-08-12 15:31:13]

messages LGTM.

[Charlie Harrison, 2016-08-12 16:08:21]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-08-12 16:08:51]

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, 2016-08-12 16:15:10]

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

[commit-bot: I haz the power, 2016-08-12 16:15:12]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Kunihiko Sakamoto, 2016-08-13 01:15:31]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2016-08-13 01:15:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, tkent@chromium.org, csharrison@chromium.org,
tsepez@chromium.org, isherman@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2039363003/#ps560001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-13 01:15:56]

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, 2016-08-13 03:10:34]

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

[commit-bot: I haz the power, 2016-08-13 03:10:37]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Kunihiko Sakamoto, 2016-08-13 04:13:01]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2016-08-13 04:13:12]

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, 2016-08-13 04:49:16]

Committed patchset #16 (id:560001)

[commit-bot: I haz the power, 2016-08-13 04:50:44]

Description was changed from

==========
FirstMeaningfulPaint UMA

This patch implements First Meaningful Paint (FMP) [1] as a UMA metric.
FMP is computed in FirstMeaningfulPaintDetector class, using the same
algorithm as existing trace-based FMP [2].

FirstMeaningfulPaintDetector observes layout operations during page load
until network stable (2 seconds of no network activity), and then
reports computed FMP to PaintTiming.

If the user navigated away from the page while observing layouts or
user interaction happened between FP and FMP, instead of reporting Time
to first meaningful paint, just log the event in separate
FirstMeaningfulPaintStatus histogram.

Design doc: http://goo.gl/TEiMi4

[1] https://goo.gl/vpaxv6
[2]
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...

BUG=632081
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
FirstMeaningfulPaint UMA

This patch implements First Meaningful Paint (FMP) [1] as a UMA metric.
FMP is computed in FirstMeaningfulPaintDetector class, using the same
algorithm as existing trace-based FMP [2].

FirstMeaningfulPaintDetector observes layout operations during page load
until network stable (2 seconds of no network activity), and then
reports computed FMP to PaintTiming.

If the user navigated away from the page while observing layouts or
user interaction happened between FP and FMP, instead of reporting Time
to first meaningful paint, just log the event in separate
FirstMeaningfulPaintStatus histogram.

Design doc: http://goo.gl/TEiMi4

[1] https://goo.gl/vpaxv6
[2]
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...

BUG=632081
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/b190d8e50285016c58c89ae5717446388945fd8f
Cr-Commit-Position: refs/heads/master@{#411876}
==========

[commit-bot: I haz the power, 2016-08-13 04:50:46]

Patchset 16 (id:??) landed as
https://crrev.com/b190d8e50285016c58c89ae5717446388945fd8f
Cr-Commit-Position: refs/heads/master@{#411876}