Buffer cross frame paint timing updates.

Buffer cross frame paint timing updates.

With the recent change to merge cross-frame paint timings, we sometimes encounter
cases where these timings arrive out of order.

For example, we may receive a first paint notification for frame A first,
then a first paint notification for frame B, but frame B's first paint
time may have happened before frame A's first paint time.

In the current implementation, we dispatch a first paint notification to observers
as soon as we receive a first paint notification from any frame in the page.

This means we don't end up recording the true page-level first paint time on
all pages.

This change adds a 1s buffering period before we dispatch first paint notifications
to observers. During this buffering period, we accumulate all first paint updates
across all frames, and retain the minimum first paint time. We then dispatch the min
observed first paint time across this window.

This change should significantly reduce the frequency with which we log out of order
paint timings. We have added new UMA to keep track of how often this continues to
happen, even with the 1s buffering.

BUG=726387
Review-Url: https://codereview.chromium.org/2901383002
Cr-Commit-Position: refs/heads/master@{#475878}
Committed: https://chromium.googlesource.com/chromium/src/+/457de7596c43cbce74ea3cbae7d9ab1e93fe9d5a

[Bryan McQuade, 2017-05-25 02:16:30]

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

[commit-bot: I haz the power, 2017-05-25 02:17:52]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-25 02:19:40]

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

[commit-bot: I haz the power, 2017-05-25 02:20:22]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-25 02:59:42]

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

[commit-bot: I haz the power, 2017-05-25 03:00:50]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-25 03:15:56]

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

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

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-25 03:30:37]

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

[commit-bot: I haz the power, 2017-05-25 03:31:02]

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-25 05:09:00]

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

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

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-25 15:55:00]

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

[commit-bot: I haz the power, 2017-05-25 15:55:22]

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-25 17:34:41]

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

[commit-bot: I haz the power, 2017-05-25 17:34:42]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-25 17:39:56]

Description was changed from

==========
Buffer cross frame paint timing updates.

BUG=
==========

to

==========
Buffer cross frame paint timing updates.

BUG=726387
==========

[Bryan McQuade, 2017-05-25 18:11:57]

Description was changed from

==========
Buffer cross frame paint timing updates.

BUG=726387
==========

to

==========
Buffer cross frame paint timing updates.

With the recent change to merge cross-frame paint timings, we sometimes
encounter
cases where these timings arrive out of order.

For example, we may receive a first paint notification for frame A first,
then a first paint notification for frame B, but frame B's first paint
time may have happened before frame A's first paint time.

In the current implementation, we dispatch a first paint notification to
observers
as soon as we receive a first paint notification from any frame in the page.

This means we don't end up recording the true page-level first paint time on
all pages.

This change adds a 1s buffering period before we dispatch first paint
notifications
to observers. During this buffering period, we accumulate all first paint
updates
across all frames, and retain the minimum first paint time. We then dispatch the
min
observed first paint time across this window.

This change should significantly reduce the frequency with which we log out of
order
paint timings. We have added new UMA to keep track of how often this continues
to
happen, even with the 1s buffering.

BUG=726387
==========

[Bryan McQuade, 2017-05-25 18:44:07]

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

[commit-bot: I haz the power, 2017-05-25 18:44:52]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-25 18:52:48]

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

[commit-bot: I haz the power, 2017-05-25 18:53:28]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-25 18:56:37]

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

[Bryan McQuade, 2017-05-25 18:56:49]

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

[commit-bot: I haz the power, 2017-05-25 18:57:12]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-25 19:00:11]

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

[commit-bot: I haz the power, 2017-05-25 19:00:32]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-25 19:00:45]

PTAL, thanks! This change builds on https://codereview.chromium.org/2903693004.

[commit-bot: I haz the power, 2017-05-25 20:35:36]

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

[commit-bot: I haz the power, 2017-05-25 20:35:37]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-25 21:01:25]

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

[commit-bot: I haz the power, 2017-05-25 21:02:17]

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-25 21:40:24]

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

[commit-bot: I haz the power, 2017-05-25 21:40:25]

Dry run: 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...)

[Bryan McQuade, 2017-05-26 00:24:52]

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

[commit-bot: I haz the power, 2017-05-26 00:26:07]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-26 00:27:00]

On 2017/05/25 at 19:00:45, Bryan McQuade wrote:
> PTAL, thanks! This change builds on
https://codereview.chromium.org/2903693004.

I implemented the MockTimer getter approach (using WeakPtr) that we discussed,
and I think it improves things nicely. The ForTesting() getters are gone, and we
are able to retain driving the timer instance directly. See what you think.
Thanks!

[Bryan McQuade, 2017-05-26 00:29:49]

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

[commit-bot: I haz the power, 2017-05-26 00:31:02]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-26 00:31:20]

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

[commit-bot: I haz the power, 2017-05-26 00:32:02]

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-26 03:25:44]

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

[commit-bot: I haz the power, 2017-05-26 03:25:45]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-26 13:44:50]

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

[commit-bot: I haz the power, 2017-05-26 13:44:57]

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-26 14:58:42]

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

[commit-bot: I haz the power, 2017-05-26 14:58:44]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-05-26 19:31:34]

It looks pretty good so far

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

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:104: //
PageLoadMetricsObservers can cause code to execute that wants to be able to
which code? It would be nice to be specific here if possible.

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h
(right):

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h:10:
#include "base/timer/timer.h"
I think it can just be a fwd declaration

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

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:222:
class PaintTimingMerger {
This feels a bit weird as a class. It's only allocated on the stack and used
very shortly. If you think the ergonomics of the class are much better than
straight functions I may suggest bringing it to a top-level class and allowing
it to be exposed + unit testable?

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:326:
timer_->Stop();
It feels wrong that we need to explicitly stop the timer here (due to
DispatchTimingUpdates implementation). I've suggested a change in that method.

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:327:
DispatchTimingUpdates();
Maybe user Timer::user_task to avoid duplicating the method?

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:475: if
(timer_->IsRunning())
I would prefer if we could DCHECK(!timer_->IsRunning) here

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:12:
#include "base/timer/timer.h"
Can just fwd declare I think

https://codereview.chromium.org/2901383002/diff/260001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (left):

https://codereview.chromium.org/2901383002/diff/260001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:113: 
keep the whitespace

https://codereview.chromium.org/2901383002/diff/260001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2901383002/diff/260001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:894:
// The first paint value should be the min of all received first paints, wich
which

[Bryan McQuade, 2017-05-26 20:03:26]

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

[commit-bot: I haz the power, 2017-05-26 20:04:06]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-26 20:07:33]

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

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:104: //
PageLoadMetricsObservers can cause code to execute that wants to be able to
On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> which code? It would be nice to be specific here if possible.

any access to web_contents() in the context of a WCO will blow up.

the specific case i saw was in some prerender code, but it could happen anywhere
as long as the call stack ends up at code that tries to access the
web_contents() for a WCO, for example.

I'm disinclined to mention the specific case of prerender in this comment,
especially since prerender may be going away in the future, but I'm happy to add
more detail about the general issue if you think that would help.

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h
(right):

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h:10:
#include "base/timer/timer.h"
On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> I think it can just be a fwd declaration

Done

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

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:222:
class PaintTimingMerger {
On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> This feels a bit weird as a class. It's only allocated on the stack and used
very shortly. If you think the ergonomics of the class are much better than
straight functions I may suggest bringing it to a top-level class and allowing
it to be exposed + unit testable?

Yeah, I wasn't planning to make this a helper class initially, but it made
exporting the did_merge_new_timing_value() bit ugly to instead return that as a
bool. In particular, I had to do things like:

  bool did_merge_new_timing_value = MaybeUpdateTimeDelta(&target_->first_paint,
navigation_start_offset,
                       new_paint_timing.first_paint);
  did_merge_new_timing_value = MaybeUpdateTimeDelta(&target_->first_text_paint,
navigation_start_offset,
                       new_paint_timing.first_text_paint) ||
did_merge_new_timing_value;
  did_merge_new_timing_value = MaybeUpdateTimeDelta(&target_->first_image_paint,
navigation_start_offset,
                       new_paint_timing.first_image_paint) ||
did_merge_new_timing_value;
  did_merge_new_timing_value =
MaybeUpdateTimeDelta(&target_->first_contentful_paint,
                       navigation_start_offset,
                       new_paint_timing.first_contentful_paint) ||
did_merge_new_timing_value;

which is both ugly and fragile, as if we forget one of these conditionals it
breaks.

By moving to a class, we can instead set the member did_merge_new_timing_value_
inside MaybeUpdateTimeDelta and it becomes much cleaner.

I think doing this in a helper class is pretty clearly preferable for this
reason. Let me know if you prefer to use the did_merge_new_timing_value = ... ||
did_merge_new_timing_value; approach above.

I'm skeptical that exposing this in the header and testing it improves test
coverage over our existing tests, and it pollutes the header with this
implementation detail. I can do it if you think it's important though.

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:326:
timer_->Stop();
On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> It feels wrong that we need to explicitly stop the timer here (due to
DispatchTimingUpdates implementation). I've suggested a change in that method.

Looking at your comment there, you suggested retaining a DCHECK, which I think
makes sense, but then that means we still need to stop the timer here. 

The motivation for calling Stop() here wasn't to interact nicely with
DispatchTimingUpdates() - rather, it's more explicit that we're cleaning up
resources & IMO more readable than assuming the timer is stopped when it goes
out of scope.

Note that we do the same thing in the PageTimingMetricsSender destructor:
PageTimingMetricsSender::~PageTimingMetricsSender() {
  if (timer_->IsRunning()) {
    timer_->Stop();
    SendNow();
  }
}

And we see this in other places in the codebase as well, e.g.
https://cs.chromium.org/chromium/src/ui/message_center/popup_timer.cc?l=21

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:327:
DispatchTimingUpdates();
On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> Maybe user Timer::user_task to avoid duplicating the method?

Need to circle back on this one - will look and see shortly.

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:475: if
(timer_->IsRunning())
On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> I would prefer if we could DCHECK(!timer_->IsRunning) here

Done

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:12:
#include "base/timer/timer.h"
On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> Can just fwd declare I think

Done

https://codereview.chromium.org/2901383002/diff/260001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (left):

https://codereview.chromium.org/2901383002/diff/260001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:113: 
On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> keep the whitespace

Done

https://codereview.chromium.org/2901383002/diff/260001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2901383002/diff/260001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:894:
// The first paint value should be the min of all received first paints, wich
On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> which

Done

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

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

[commit-bot: I haz the power, 2017-05-26 21:30:04]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-26 21:39:05]

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

[commit-bot: I haz the power, 2017-05-26 21:39:50]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-26 21:48:48]

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

[commit-bot: I haz the power, 2017-05-26 21:49:28]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-26 21:51:33]

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

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:327:
DispatchTimingUpdates();
On 2017/05/26 at 20:07:32, Bryan McQuade wrote:
> On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> > Maybe user Timer::user_task to avoid duplicating the method?
> 
> Need to circle back on this one - will look and see shortly.

I look a look - it appears to not be possible to keep hold of the user_task. I
tried:

base::Closure user_task = timer_->user_task();
timer_->Stop();
user_task.Run();

but this blows up at runtime. the user_task() method returns a const ref so I'm
not even sure how the assignment is compiling, but it's clearly not working.

I could do:

  if (timer_->IsRunning()) {
    timer_->user_task().Run();
    timer_->Stop();
  }

but this feels wrong (reaching into timer internal state to force invoke the
timer) and it crashes, presumably for a similar reason (I suspect invoking Run()
on the returned const ref may cause an implicit copy, which compiles but doesn't
actually work at runtime).

So I left this as is.

[commit-bot: I haz the power, 2017-05-26 23:19:55]

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

[commit-bot: I haz the power, 2017-05-26 23:19:56]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-30 13:07:28]

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

[commit-bot: I haz the power, 2017-05-30 13:07:35]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-30 13:08:28]

On 2017/05/26 at 21:51:33, Bryan McQuade wrote:
>
https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
> File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
(right):
> 
>
https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:327:
DispatchTimingUpdates();
> On 2017/05/26 at 20:07:32, Bryan McQuade wrote:
> > On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> > > Maybe user Timer::user_task to avoid duplicating the method?
> > 
> > Need to circle back on this one - will look and see shortly.
> 
> I look a look - it appears to not be possible to keep hold of the user_task. I
tried:
> 
> base::Closure user_task = timer_->user_task();
> timer_->Stop();
> user_task.Run();
> 
> but this blows up at runtime. the user_task() method returns a const ref so
I'm not even sure how the assignment is compiling, but it's clearly not working.
> 
> I could do:
> 
>   if (timer_->IsRunning()) {
>     timer_->user_task().Run();
>     timer_->Stop();
>   }
> 
> but this feels wrong (reaching into timer internal state to force invoke the
timer) and it crashes, presumably for a similar reason (I suspect invoking Run()
on the returned const ref may cause an implicit copy, which compiles but doesn't
actually work at runtime).
> 
> So I left this as is.

Just FYI, I finished up the change to factor WeakMockTimer into its own class -
we now use it in 3 test classes.

[commit-bot: I haz the power, 2017-05-30 13:28:13]

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

[commit-bot: I haz the power, 2017-05-30 13:28:14]

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

[Charlie Harrison, 2017-05-30 14:49:08]

LGTM % comments

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

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:222:
class PaintTimingMerger {
On 2017/05/26 20:07:32, Bryan McQuade wrote:
> On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> > This feels a bit weird as a class. It's only allocated on the stack and used
> very shortly. If you think the ergonomics of the class are much better than
> straight functions I may suggest bringing it to a top-level class and allowing
> it to be exposed + unit testable?
> 
> Yeah, I wasn't planning to make this a helper class initially, but it made
> exporting the did_merge_new_timing_value() bit ugly to instead return that as
a
> bool. In particular, I had to do things like:
> 
>   bool did_merge_new_timing_value =
MaybeUpdateTimeDelta(&target_->first_paint,
> navigation_start_offset,
>                        new_paint_timing.first_paint);
>   did_merge_new_timing_value =
MaybeUpdateTimeDelta(&target_->first_text_paint,
> navigation_start_offset,
>                        new_paint_timing.first_text_paint) ||
> did_merge_new_timing_value;
>   did_merge_new_timing_value =
MaybeUpdateTimeDelta(&target_->first_image_paint,
> navigation_start_offset,
>                        new_paint_timing.first_image_paint) ||
> did_merge_new_timing_value;
>   did_merge_new_timing_value =
> MaybeUpdateTimeDelta(&target_->first_contentful_paint,
>                        navigation_start_offset,
>                        new_paint_timing.first_contentful_paint) ||
> did_merge_new_timing_value;
> 
> which is both ugly and fragile, as if we forget one of these conditionals it
> breaks.
> 
> By moving to a class, we can instead set the member
did_merge_new_timing_value_
> inside MaybeUpdateTimeDelta and it becomes much cleaner.
> 
> I think doing this in a helper class is pretty clearly preferable for this
> reason. Let me know if you prefer to use the did_merge_new_timing_value = ...
||
> did_merge_new_timing_value; approach above.
> 
> I'm skeptical that exposing this in the header and testing it improves test
> coverage over our existing tests, and it pollutes the header with this
> implementation detail. I can do it if you think it's important though.

Those reasons make sense to me.

I was thinking of moving it to a separate file wholesale. I don't feel strongly
about it though.

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:327:
DispatchTimingUpdates();
On 2017/05/26 21:51:32, Bryan McQuade wrote:
> On 2017/05/26 at 20:07:32, Bryan McQuade wrote:
> > On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> > > Maybe user Timer::user_task to avoid duplicating the method?
> > 
> > Need to circle back on this one - will look and see shortly.
> 
> I look a look - it appears to not be possible to keep hold of the user_task. I
> tried:
> 
> base::Closure user_task = timer_->user_task();
> timer_->Stop();
> user_task.Run();
> 
> but this blows up at runtime. the user_task() method returns a const ref so
I'm
> not even sure how the assignment is compiling, but it's clearly not working.
> 
> I could do:
> 
>   if (timer_->IsRunning()) {
>     timer_->user_task().Run();
>     timer_->Stop();
>   }
> 
> but this feels wrong (reaching into timer internal state to force invoke the
> timer) and it crashes, presumably for a similar reason (I suspect invoking
Run()
> on the returned const ref may cause an implicit copy, which compiles but
doesn't
> actually work at runtime).
> 
> So I left this as is.

Yeah I feel like this comment was piggybacking on the above one so the code
would look like:
if (timer_->IsRunning())
  timer_->user_task().Run()

Without needing to call Stop() on the timer since it's going out of scope. If
that crashes I guess it's not workable but it's surprising to me since we call
Run() on user_task() elsewhere.

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:42:
std::unique_ptr<WeakMockTimer> timer = base::MakeUnique<WeakMockTimer>();
nit: usually base::MakeUnique is paired with auto:
auto timer = base::MakeUnique<WeakMockTimer>() to avoid type stuttering.

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
(right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h:11:
#include "base/timer/mock_timer.h"
clean up includes, I think you only need weak_mock_timer

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:146:
std::unique_ptr<base::Timer> PageLoadMetricsEmbedder::CreateTimer() {
#include base/timer

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
(right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:304: //
first meaningful paint is only tracked in the main frame.
First

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:328:
void PageLoadMetricsUpdateDispatcher::ShutDown() {
As mentioned earlier, if we just used
unique_ptr<PageLoadMetricsUpdateDispatcher> we wouldn't need this extra
Shutdown() logic and could just move to the destructor.

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:139:
std::unique_ptr<base::Timer> timer_;
#include <memory>

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:201:
metrics_update_dispatcher_.ShutDown();
optional: It seems a bit strange to have a Shutdown() method called in the
destructor. What about having the dispatcher just be a unique_ptr and call reset
here?

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
File chrome/common/page_load_metrics/test/weak_mock_timer.cc (right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
chrome/common/page_load_metrics/test/weak_mock_timer.cc:1: // Copyright (c) 2017
The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
File chrome/common/page_load_metrics/test/weak_mock_timer.h (right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
chrome/common/page_load_metrics/test/weak_mock_timer.h:11: namespace
page_load_metrics {
We didn't talk about this before but it is also not uncommon to have an
additional "testing" namespace. Up to you if you want it, I don't care either
way.

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
chrome/common/page_load_metrics/test/weak_mock_timer.h:18: };
Do we want to DISALLOW_COPY_AND_ASSIGN to these timers/ timer providers? I don't
think we need to copy them anywhere.

[Bryan McQuade, 2017-05-30 14:59:38]

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

[commit-bot: I haz the power, 2017-05-30 14:59:58]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-30 15:02:36]

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

[commit-bot: I haz the power, 2017-05-30 15:02:56]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-30 15:21:40]

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

[commit-bot: I haz the power, 2017-05-30 15:21:50]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-30 15:30:23]

Thanks! Addressed comments. I also factored some common test code in
PLMOTestHarness into a common test util. PLMOTestHarness was actually being
fully compiled into both browser tests and unit tests - it should really only be
compiled into unit tests but was in the browser tests because prerender wanted
to use a single utility method there. Now we move that method into
page_load_metrics_test_util.h in common, which is compiled into the common test
support lib used by both browser and unit tests.

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

https://codereview.chromium.org/2901383002/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:327:
DispatchTimingUpdates();
On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> On 2017/05/26 21:51:32, Bryan McQuade wrote:
> > On 2017/05/26 at 20:07:32, Bryan McQuade wrote:
> > > On 2017/05/26 at 19:31:34, Charlie Harrison wrote:
> > > > Maybe user Timer::user_task to avoid duplicating the method?
> > > 
> > > Need to circle back on this one - will look and see shortly.
> > 
> > I look a look - it appears to not be possible to keep hold of the user_task.
I
> > tried:
> > 
> > base::Closure user_task = timer_->user_task();
> > timer_->Stop();
> > user_task.Run();
> > 
> > but this blows up at runtime. the user_task() method returns a const ref so
I'm
> > not even sure how the assignment is compiling, but it's clearly not working.
> > 
> > I could do:
> > 
> >   if (timer_->IsRunning()) {
> >     timer_->user_task().Run();
> >     timer_->Stop();
> >   }
> > 
> > but this feels wrong (reaching into timer internal state to force invoke the
> > timer) and it crashes, presumably for a similar reason (I suspect invoking
Run()
> > on the returned const ref may cause an implicit copy, which compiles but
doesn't
> > actually work at runtime).
> > 
> > So I left this as is.
> 
> Yeah I feel like this comment was piggybacking on the above one so the code
would look like:
> if (timer_->IsRunning())
>   timer_->user_task().Run()
> 
> Without needing to call Stop() on the timer since it's going out of scope. If
that crashes I guess it's not workable but it's surprising to me since we call
Run() on user_task() elsewhere.

My understanding is that it's safe to call Run() on the user_task() as long as
retain_user_task is true in the constructor, which is the case for a repeating
timer, but not a OneShotTimer which we're using. LMK if you find out otherwise -
happy to make changes.

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:42:
std::unique_ptr<WeakMockTimer> timer = base::MakeUnique<WeakMockTimer>();
On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> nit: usually base::MakeUnique is paired with auto:
> auto timer = base::MakeUnique<WeakMockTimer>() to avoid type stuttering.

done

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
(right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h:11:
#include "base/timer/mock_timer.h"
On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> clean up includes, I think you only need weak_mock_timer

done

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:146:
std::unique_ptr<base::Timer> PageLoadMetricsEmbedder::CreateTimer() {
On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> #include base/timer

done

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
(right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:304: //
first meaningful paint is only tracked in the main frame.
On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> First

done

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:328:
void PageLoadMetricsUpdateDispatcher::ShutDown() {
On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> As mentioned earlier, if we just used
unique_ptr<PageLoadMetricsUpdateDispatcher> we wouldn't need this extra
Shutdown() logic and could just move to the destructor.

My sense is these are two equivalent ways to accomplish the same thing, and in
some ways, having a ShutDown is preferable since it's more explicit. If shut
down code is important and needs to run at a certain time outside of the normal
destruction flow, my preference is to have an explicit shutdown method, which
makes it clearer than calling reset() (which is more commonly associated with
freeing up resources than with running important shutdown code).

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
(right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h:139:
std::unique_ptr<base::Timer> timer_;
On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> #include <memory>

done

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:201:
metrics_update_dispatcher_.ShutDown();
On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> optional: It seems a bit strange to have a Shutdown() method called in the
destructor. What about having the dispatcher just be a unique_ptr and call reset
here?

It's not that unusual / strange - see
https://cs.chromium.org/search/?q=::~+Shutdown

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
File chrome/common/page_load_metrics/test/weak_mock_timer.cc (right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
chrome/common/page_load_metrics/test/weak_mock_timer.cc:1: // Copyright (c) 2017
The Chromium Authors. All rights reserved.
On 2017/05/30 at 14:49:08, Charlie Harrison wrote:
> no (c)

done

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
File chrome/common/page_load_metrics/test/weak_mock_timer.h (right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
chrome/common/page_load_metrics/test/weak_mock_timer.h:11: namespace
page_load_metrics {
On 2017/05/30 at 14:49:08, Charlie Harrison wrote:
> We didn't talk about this before but it is also not uncommon to have an
additional "testing" namespace. Up to you if you want it, I don't care either
way.

Ah, I grepped around until chrome/common and didn't see this much, so decided
not to, but you're right that this appears to be more common in chrome/browser.
namespace test seems the most common so will use that.

https://codereview.chromium.org/2901383002/diff/340001/chrome/common/page_loa...
chrome/common/page_load_metrics/test/weak_mock_timer.h:18: };
On 2017/05/30 at 14:49:08, Charlie Harrison wrote:
> Do we want to DISALLOW_COPY_AND_ASSIGN to these timers/ timer providers? I
don't think we need to copy them anywhere.

done

[Bryan McQuade, 2017-05-30 15:30:51]

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

[Bryan McQuade, 2017-05-30 15:31:14]

isherman, PTAL for histograms.xml, thanks!
mattcary, PTAL for prerender_browsertest.cc, thanks!

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

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

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

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

[Bryan McQuade, 2017-05-30 16:23:22]

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

[commit-bot: I haz the power, 2017-05-30 16:23:41]

Dry run: CQ is trying da patch.

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

[Charlie Harrison, 2017-05-30 16:33:15]

Clarifying my argument below, still lgtm though

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
(right):

https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:328:
void PageLoadMetricsUpdateDispatcher::ShutDown() {
On 2017/05/30 15:30:23, Bryan McQuade wrote:
> On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> > As mentioned earlier, if we just used
> unique_ptr<PageLoadMetricsUpdateDispatcher> we wouldn't need this extra
> Shutdown() logic and could just move to the destructor.
> 
> My sense is these are two equivalent ways to accomplish the same thing, and in
> some ways, having a ShutDown is preferable since it's more explicit. If shut
> down code is important and needs to run at a certain time outside of the
normal
> destruction flow, my preference is to have an explicit shutdown method, which
> makes it clearer than calling reset() (which is more commonly associated with
> freeing up resources than with running important shutdown code).

I hear you, and that argument makes sense. My preference is to use the
unique_ptr way so we never have a state where the object is accessible but in an
invalid state. Up to you.

[Bryan McQuade, 2017-05-30 16:37:43]

On 2017/05/30 at 16:33:15, csharrison wrote:
> Clarifying my argument below, still lgtm though
> 
>
https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
> File chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
(right):
> 
>
https://codereview.chromium.org/2901383002/diff/340001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc:328:
void PageLoadMetricsUpdateDispatcher::ShutDown() {
> On 2017/05/30 15:30:23, Bryan McQuade wrote:
> > On 2017/05/30 at 14:49:07, Charlie Harrison wrote:
> > > As mentioned earlier, if we just used
> > unique_ptr<PageLoadMetricsUpdateDispatcher> we wouldn't need this extra
> > Shutdown() logic and could just move to the destructor.
> > 
> > My sense is these are two equivalent ways to accomplish the same thing, and
in
> > some ways, having a ShutDown is preferable since it's more explicit. If shut
> > down code is important and needs to run at a certain time outside of the
normal
> > destruction flow, my preference is to have an explicit shutdown method,
which
> > makes it clearer than calling reset() (which is more commonly associated
with
> > freeing up resources than with running important shutdown code).
> 
> I hear you, and that argument makes sense. My preference is to use the
unique_ptr way so we never have a state where the object is accessible but in an
invalid state. Up to you.

That's a good point - it's arguably weird to have the object live on past when
it is active. If we can avoid that I agree that it reduces complexity and makes
the code easier to understand.

Unfortunately we do actually need to keep the object around for the remainder of
the destructor, to look up the last observed timing data.

We could potentially break this class into two: a dispatcher, responsible only
for accumulating and dispatching updates, and a data collector / container,
responsible for holding on to the most recently updated timing and metadata.
Each would have a different lifetime (we only need the data collector /
container to live through the end of the destructor).

I'll defer this for now though - but might be a nice cleanup for the future.

[commit-bot: I haz the power, 2017-05-30 17:55:56]

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

[commit-bot: I haz the power, 2017-05-30 17:55:57]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-05-30 19:31:38]

Metrics LGTM % a question:

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

https://codereview.chromium.org/2901383002/diff/420001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:90778: +      label="Recorded after
timing update buffering in the browser process
What is "timing update buffering"?  Am I just failing to English today?  I'm
finding it hard to understand.

[Bryan McQuade, 2017-05-31 01:04:52]

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

[commit-bot: I haz the power, 2017-05-31 01:05:29]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-31 01:05:30]

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

https://codereview.chromium.org/2901383002/diff/420001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:90778: +      label="Recorded after
timing update buffering in the browser process
On 2017/05/30 at 19:31:38, Ilya Sherman wrote:
> What is "timing update buffering"?  Am I just failing to English today?  I'm
finding it hard to understand.

Thanks! You're right, this isn't very clear. I updated the text to make it
clearer.

[commit-bot: I haz the power, 2017-05-31 01:53:49]

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

[commit-bot: I haz the power, 2017-05-31 01:53:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mattcary, 2017-05-31 07:38:05]

lgtm

for prerender_browsertest

[Bryan McQuade, 2017-05-31 11:36:11]

The CQ bit was checked by bmcquade@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2901383002/#ps440001
(title: "address comment")

[commit-bot: I haz the power, 2017-05-31 11:36:23]

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-31 11:41:09]

CQ is committing da patch.

Bot data: {"patchset_id": 440001, "attempt_start_ts": 1496230571698850,
"parent_rev": "960ae7f16addc69a2369cda1d8462f0b1850a77e", "commit_rev":
"457de7596c43cbce74ea3cbae7d9ab1e93fe9d5a"}

[commit-bot: I haz the power, 2017-05-31 11:41:21]

Description was changed from

==========
Buffer cross frame paint timing updates.

With the recent change to merge cross-frame paint timings, we sometimes
encounter
cases where these timings arrive out of order.

For example, we may receive a first paint notification for frame A first,
then a first paint notification for frame B, but frame B's first paint
time may have happened before frame A's first paint time.

In the current implementation, we dispatch a first paint notification to
observers
as soon as we receive a first paint notification from any frame in the page.

This means we don't end up recording the true page-level first paint time on
all pages.

This change adds a 1s buffering period before we dispatch first paint
notifications
to observers. During this buffering period, we accumulate all first paint
updates
across all frames, and retain the minimum first paint time. We then dispatch the
min
observed first paint time across this window.

This change should significantly reduce the frequency with which we log out of
order
paint timings. We have added new UMA to keep track of how often this continues
to
happen, even with the 1s buffering.

BUG=726387
==========

to

==========
Buffer cross frame paint timing updates.

With the recent change to merge cross-frame paint timings, we sometimes
encounter
cases where these timings arrive out of order.

For example, we may receive a first paint notification for frame A first,
then a first paint notification for frame B, but frame B's first paint
time may have happened before frame A's first paint time.

In the current implementation, we dispatch a first paint notification to
observers
as soon as we receive a first paint notification from any frame in the page.

This means we don't end up recording the true page-level first paint time on
all pages.

This change adds a 1s buffering period before we dispatch first paint
notifications
to observers. During this buffering period, we accumulate all first paint
updates
across all frames, and retain the minimum first paint time. We then dispatch the
min
observed first paint time across this window.

This change should significantly reduce the frequency with which we log out of
order
paint timings. We have added new UMA to keep track of how often this continues
to
happen, even with the 1s buffering.

BUG=726387

Review-Url: https://codereview.chromium.org/2901383002
Cr-Commit-Position: refs/heads/master@{#475878}
Committed:
https://chromium.googlesource.com/chromium/src/+/457de7596c43cbce74ea3cbae7d9...
==========

[commit-bot: I haz the power, 2017-05-31 11:41:23]

Committed patchset #24 (id:440001) as
https://chromium.googlesource.com/chromium/src/+/457de7596c43cbce74ea3cbae7d9...

[Mathieu, 2017-05-31 14:03:26]

On 2017/05/31 11:41:23, commit-bot: I haz the power wrote:
> Committed patchset #24 (id:440001) as
>
https://chromium.googlesource.com/chromium/src/+/457de7596c43cbce74ea3cbae7d9...

A revert has been created (manually, ugh):

https://chromium-review.googlesource.com/c/519363/