Report page load timing information for child frames.

Report page load timing information for child frames.

This change clarifies that our PLMO::On*Paint callbacks are for the whole page,
and adds support for aggregating timing data across frames in PageLoadTracker.

The current behavior tracks paints across all frames, however this is due to a bug
(crbug.com/705315) and will be fixed soon.

PLMO implementers actually want to track paints across all frames in the page, so
this is desired behavior.

This change adds support for tracking paints across all frames so the current
behavior doesn't regress when 705315 is fixed.

BUG=661702
Review-Url: https://codereview.chromium.org/2859393002
Cr-Commit-Position: refs/heads/master@{#471725}
Committed: https://chromium.googlesource.com/chromium/src/+/9f5f6b9548bc4cad0c64d3aa2b5b58ae8dad595b

[Bryan McQuade, 2017-05-05 01:01:12]

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

[commit-bot: I haz the power, 2017-05-05 01:02: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-05 01:04:12]

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

[commit-bot: I haz the power, 2017-05-05 01:04:34]

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-05 01:13:44]

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

[commit-bot: I haz the power, 2017-05-05 01:14:04]

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-05 01:16:25]

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

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

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-05 01:57:04]

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

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

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, 2017-05-05 15:22:52]

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

[commit-bot: I haz the power, 2017-05-05 15:23:12]

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-05 16:10:19]

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

[commit-bot: I haz the power, 2017-05-05 16:10:19]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-05 17:49:07]

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

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

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-05 18:42:08]

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

[commit-bot: I haz the power, 2017-05-05 18:42:09]

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

[jkarlin, 2017-05-05 19:19:24]

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

[jkarlin, 2017-05-05 19:19:25]

https://codereview.chromium.org/2859393002/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2859393002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:351:
base::TimeDelta navigation_start,
perhaps rename to main_frame_navigation_start?

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

https://codereview.chromium.org/2859393002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:519: if
(navigation_handle->HasCommitted() &&
Note that error pages count as commits.

https://codereview.chromium.org/2859393002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:587: // parent. we
probably want this to pass through to observers, but it's
This can also happen if a navigation aborts and falls back to the previous
navigation, which continues to do stuff.

[Bryan McQuade, 2017-05-06 19:41:47]

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

[commit-bot: I haz the power, 2017-05-06 19:41:55]

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-06 20:01:18]

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

[commit-bot: I haz the power, 2017-05-06 20:01:33]

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-06 21:09:31]

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

[commit-bot: I haz the power, 2017-05-06 21:09:44]

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-06 22:17:48]

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

[commit-bot: I haz the power, 2017-05-06 22:17:49]

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

[Bryan McQuade, 2017-05-07 19:07:58]

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

[commit-bot: I haz the power, 2017-05-07 19:08:03]

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-07 19:12:00]

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

[commit-bot: I haz the power, 2017-05-07 19:12:04]

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-07 19:27:28]

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

[commit-bot: I haz the power, 2017-05-07 19:27:33]

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-07 19:32:11]

Thanks! Addressed comments and a bunch of other things. I need to figure out how
to consolidate main/child frame FP/FCP updates and provide that info to
observers, but I think everything else is in place now. PTAL.

https://codereview.chromium.org/2859393002/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2859393002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:351:
base::TimeDelta navigation_start,
On 2017/05/05 at 19:19:25, jkarlin wrote:
> perhaps rename to main_frame_navigation_start?

I thought about this for a bit and ended up calling it 'navigation_start_offset'
since it's an offset relative to the main frame's nav start. I can use
'main_frame_navigation_start_offset' if you prefer - just let me know.

https://codereview.chromium.org/2859393002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:351:
base::TimeDelta navigation_start,
On 2017/05/05 at 19:19:25, jkarlin wrote:
> perhaps rename to main_frame_navigation_start?

I thought about this for a bit and ended up calling it 'navigation_start_offset'
since it's an offset relative to the main frame's nav start. I can use
'main_frame_navigation_start_offset' if you prefer - just let me know.

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

https://codereview.chromium.org/2859393002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:519: if
(navigation_handle->HasCommitted() &&
On 2017/05/05 at 19:19:25, jkarlin wrote:
> Note that error pages count as commits.

good catch, thanks!. I decided that for tracking timing info we should use the
same policy as we have in the main frame.

https://codereview.chromium.org/2859393002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:587: // parent. we
probably want this to pass through to observers, but it's
On 2017/05/05 at 19:19:25, jkarlin wrote:
> This can also happen if a navigation aborts and falls back to the previous
navigation, which continues to do stuff.

Yes - I fixed our tracking of loads in DidFinishSubFrameNavigation to continue
to support this case. Thanks!

[commit-bot: I haz the power, 2017-05-07 20:28:18]

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

[commit-bot: I haz the power, 2017-05-07 20:28:19]

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

[jkarlin, 2017-05-08 13:00:41]

General design works with me.

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

https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:316: if
(GetMainFrame(navigation_handle->GetRenderFrameHost()) !=
I *think* that GetRFH will dcheck if the navigation failed to commit.

https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:273: typedef int
FrameTreeNodeId;
using FrameTreeNodeId = int;

https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:342: //
PageLoadMetadata is available for a child frame of the current page
Need ||'s around the argument names.

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

https://codereview.chromium.org/2859393002/diff/220001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:74: this,
routing_id(), CreateTimer(), GetTiming()));
page_timing_metrics_sender_ = base::MakeUnique<PageTimingMetricsSender>(this,
routing_id(), CreateTimer(), GetTiming());

[Bryan McQuade, 2017-05-08 15:24:51]

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

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

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-08 15:26:32]

Thanks! Addressed comments.

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

https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:316: if
(GetMainFrame(navigation_handle->GetRenderFrameHost()) !=
On 2017/05/08 at 13:00:41, jkarlin wrote:
> I *think* that GetRFH will dcheck if the navigation failed to commit.

ah, good catch, thanks! I moved this check into the DidFinishSubFrameNavigation
impl, after we verify that there was a commit.

https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:273: typedef int
FrameTreeNodeId;
On 2017/05/08 at 13:00:41, jkarlin wrote:
> using FrameTreeNodeId = int;

Done

https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:342: //
PageLoadMetadata is available for a child frame of the current page
On 2017/05/08 at 13:00:41, jkarlin wrote:
> Need ||'s around the argument names.

Done

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

https://codereview.chromium.org/2859393002/diff/220001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:74: this,
routing_id(), CreateTimer(), GetTiming()));
On 2017/05/08 at 13:00:41, jkarlin wrote:
> page_timing_metrics_sender_ = base::MakeUnique<PageTimingMetricsSender>(this,
routing_id(), CreateTimer(), GetTiming());

Done

[commit-bot: I haz the power, 2017-05-08 15:39:12]

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

[commit-bot: I haz the power, 2017-05-08 15:39:13]

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

[Bryan McQuade, 2017-05-08 17:32:18]

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

[commit-bot: I haz the power, 2017-05-08 17:33:13]

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-08 19:30:15]

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

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

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-08 19:40:29]

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

[commit-bot: I haz the power, 2017-05-08 19:41:38]

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-08 19:52:15]

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

[commit-bot: I haz the power, 2017-05-08 19:53:03]

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-08 19:53:59]

On 2017/05/08 at 15:26:32, Bryan McQuade wrote:
> Thanks! Addressed comments.
> 
>
https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
> File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):
> 
>
https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:316: if
(GetMainFrame(navigation_handle->GetRenderFrameHost()) !=
> On 2017/05/08 at 13:00:41, jkarlin wrote:
> > I *think* that GetRFH will dcheck if the navigation failed to commit.
> 
> ah, good catch, thanks! I moved this check into the
DidFinishSubFrameNavigation impl, after we verify that there was a commit.
> 
>
https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
> File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):
> 
>
https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/page_load_metrics_observer.h:273: typedef int
FrameTreeNodeId;
> On 2017/05/08 at 13:00:41, jkarlin wrote:
> > using FrameTreeNodeId = int;
> 
> Done
> 
>
https://codereview.chromium.org/2859393002/diff/220001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/page_load_metrics_observer.h:342: //
PageLoadMetadata is available for a child frame of the current page
> On 2017/05/08 at 13:00:41, jkarlin wrote:
> > Need ||'s around the argument names.
> 
> Done
> 
>
https://codereview.chromium.org/2859393002/diff/220001/chrome/renderer/page_l...
> File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
(right):
> 
>
https://codereview.chromium.org/2859393002/diff/220001/chrome/renderer/page_l...
> chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:74: this,
routing_id(), CreateTimer(), GetTiming()));
> On 2017/05/08 at 13:00:41, jkarlin wrote:
> > page_timing_metrics_sender_ =
base::MakeUnique<PageTimingMetricsSender>(this, routing_id(), CreateTimer(),
GetTiming());
> 
> Done

I think this change is now finalized and ready for review. I used ryansturm's
naming suggestion and changed the paint callbacks to have an InPage() suffix,
e.g. OnFirstPaintInPage(). I am happy to do that refactor as a separate change
if you prefer, to reduce the size of this change. Just let me know. Thanks!

[Bryan McQuade, 2017-05-08 19:57:02]

Description was changed from

==========
Report page load timing information for child frames.

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

to

==========
Report page load timing information for child frames.

This change clarifies that our PLMO::On*Paint callbacks are for the whole page,
and adds support for aggregating timing data across frames in PageLoadTracker.

The current behavior tracks paints across all frames, however this is due to a
bug
(crbug.com/705315) and will be fixed soon.

PLMO implementers actually want to track paints across all frames in the page,
so
this is desired behavior.

This change adds support for tracking paints across all frames so the current
behavior doesn't regress when 705315 is fixed.

BUG=661702
==========

[commit-bot: I haz the power, 2017-05-08 20:29:37]

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

[commit-bot: I haz the power, 2017-05-08 20:29:38]

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-08 20:58:52]

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

[commit-bot: I haz the power, 2017-05-08 20:59:47]

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-08 21:30:07]

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

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

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

[Bryan McQuade, 2017-05-08 21:33:20]

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

[commit-bot: I haz the power, 2017-05-08 21:35:26]

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-09 01:21:26]

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

[commit-bot: I haz the power, 2017-05-09 01:21:27]

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-09 13:05:15]

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

[commit-bot: I haz the power, 2017-05-09 13:05:43]

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-09 14:26:08]

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

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

Dry run: This issue passed the CQ dry run.

[jkarlin, 2017-05-10 16:55:28]

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

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:51: enum
ExpectedTimingFields {
enum class

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:116:
main_frame_expected_fields_ &= ~matched_bits;
Maybe add a comment that we're incrementally removing the matched bits.

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:133: bool
expectations_satisfied_ = false;
expectations_satisfied_ can be replaced with main_frame_expected_fields ==
child_frame_expected_fields_ == 0

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:275:
IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PaintInChildFrame) {
I'd like to see a set of tests w/ mocked navigations and timings so we can see
that the page-level paint timing is correct based on which frame paints first. 

I don't see much utility in verifying that the frames got their timing bits.

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:349: virtual void
OnSubFrameTimingUpdate(FrameTreeNodeId child_frame_id,
I'd rather we not create this until a client that needs it (other than tests)
comes along.

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

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:664:
DispatchObserverTimingCallbacks(observer.get(), last_timing,
It's potentially expensive to call all the observers. Do we want to call them if
nothing has changed in merged_page_timing_?

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.h (right):

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.h:243: void
UpdateSubFrameMetadata(const PageLoadMetadata& child_metadata);
Why the rename? Should the comments and argument say "subframe" instead of
"child"?

[Bryan McQuade, 2017-05-10 18:06:15]

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

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:51: enum
ExpectedTimingFields {
On 2017/05/10 at 16:55:28, jkarlin wrote:
> enum class

thanks! i moved this whole change to https://codereview.chromium.org/2868213003
since this one got quite large, and addressed this comment there. once that
lands i'll merge it into this patch.

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:116:
main_frame_expected_fields_ &= ~matched_bits;
On 2017/05/10 at 16:55:28, jkarlin wrote:
> Maybe add a comment that we're incrementally removing the matched bits.

i think the refactor i did in the other change makes this cleaerer - i wrapped
the bitset operations in a helper class. take a look at that change and see what
you think. thanks!

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:133: bool
expectations_satisfied_ = false;
On 2017/05/10 at 16:55:28, jkarlin wrote:
> expectations_satisfied_ can be replaced with main_frame_expected_fields ==
child_frame_expected_fields_ == 0

done in other change

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:275:
IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PaintInChildFrame) {
On 2017/05/10 at 16:55:28, jkarlin wrote:
> I'd like to see a set of tests w/ mocked navigations and timings so we can see
that the page-level paint timing is correct based on which frame paints first. 
> 
> I don't see much utility in verifying that the frames got their timing bits.

i'd like to do this in a unit test too, and we once pitched adding a
MockNavigationHandle to clamy, but guidance was that people often misuse mocked
versions of these classes and don't test the right things. it's been a
longstanding issue that it's very hard for us to mock out navigation_start from
the nav handle. i started down the path of adding a helper class that could be
used to fake timing data out but concluded it was not worthwhile given the
complexity (basically we had to ask the helper what the nav handle's nav start
should be, and then have different impls of the helper in tests vs prod, but it
got really messy).

i think the main thing we benefit from here in the browsertest is verifying
proper end to end routing of timing data between render and browser processes.
that seems important & valuable to me.

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:349: virtual void
OnSubFrameTimingUpdate(FrameTreeNodeId child_frame_id,
On 2017/05/10 at 16:55:28, jkarlin wrote:
> I'd rather we not create this until a client that needs it (other than tests)
comes along.

sure - i think i can switch over to using the TestingObserver::OnTimingUpdated
callback once we land the other change.

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

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:664:
DispatchObserverTimingCallbacks(observer.get(), last_timing,
On 2017/05/10 at 16:55:28, jkarlin wrote:
> It's potentially expensive to call all the observers. Do we want to call them
if nothing has changed in merged_page_timing_?

done

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.h (right):

https://codereview.chromium.org/2859393002/diff/380001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.h:243: void
UpdateSubFrameMetadata(const PageLoadMetadata& child_metadata);
On 2017/05/10 at 16:55:28, jkarlin wrote:
> Why the rename? Should the comments and argument say "subframe" instead of
"child"?

I wanted to mirror your naming for the subframe commit. i can update child->sub
everywhere, sure

[Bryan McQuade, 2017-05-10 18:35:43]

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

[commit-bot: I haz the power, 2017-05-10 18:36:40]

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-10 19:12:53]

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

[commit-bot: I haz the power, 2017-05-10 19:12:55]

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

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

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

[commit-bot: I haz the power, 2017-05-11 19:21:48]

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-11 19:30:39]

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

[commit-bot: I haz the power, 2017-05-11 19:31:37]

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-11 19:38:49]

I've merged this change with the recent browsertest update, so this is ready for
a next round of review. Thanks!

[Bryan McQuade, 2017-05-11 19:39:01]

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

[Bryan McQuade, 2017-05-11 19:39:11]

bmcquade@chromium.org changed reviewers:
- isherman@chromium.org

[Bryan McQuade, 2017-05-11 19:39:12]

isherman, PTAL for histograms.xml, thanks!

[commit-bot: I haz the power, 2017-05-11 20:49:13]

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

[commit-bot: I haz the power, 2017-05-11 20:49:14]

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

[Bryan McQuade, 2017-05-12 12:34:41]

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

[Bryan McQuade, 2017-05-12 12:35:03]

adding isherman for real this time. :)

isherman, PTAL for histograms.xml, thanks!

[jkarlin, 2017-05-12 13:45:01]

lgtm with comment

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

https://codereview.chromium.org/2859393002/diff/440001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:285: void
MaybeUpdateTimeDelta(
This should be an anonymous namespace function.

[Bryan McQuade, 2017-05-12 14:08:37]

Thanks!

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

https://codereview.chromium.org/2859393002/diff/440001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:285: void
MaybeUpdateTimeDelta(
On 2017/05/12 at 13:45:01, jkarlin wrote:
> This should be an anonymous namespace function.

I think it's already there - anonymous namespace starts on line 110 and ends on
364. LMK if I am getting this wrong though.

[jkarlin, 2017-05-12 14:13:01]

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

https://codereview.chromium.org/2859393002/diff/440001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:285: void
MaybeUpdateTimeDelta(
On 2017/05/12 14:08:36, Bryan McQuade wrote:
> On 2017/05/12 at 13:45:01, jkarlin wrote:
> > This should be an anonymous namespace function.
> 
> I think it's already there - anonymous namespace starts on line 110 and ends
on
> 364. LMK if I am getting this wrong though.

yep, my bad. looks good.

[Bryan McQuade, 2017-05-12 14:19:32]

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

[commit-bot: I haz the power, 2017-05-12 14:20: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-12 15:36:05]

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

[commit-bot: I haz the power, 2017-05-12 15:36:07]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-05-12 21:03:55]

histograms.xml lgtm

[Bryan McQuade, 2017-05-12 23:51:15]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-05-12 23:51:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2859393002/#ps460001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-12 23:51:59]

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-13 00:03:42]

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

[commit-bot: I haz the power, 2017-05-13 00:03:44]

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

[Bryan McQuade, 2017-05-13 00:08:49]

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

[Bryan McQuade, 2017-05-13 00:09:03]

mattcary, PTAL for prerender_browsertest changes, thanks!

[Bryan McQuade, 2017-05-14 14:11:39]

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

[commit-bot: I haz the power, 2017-05-14 14:11:49]

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-14 15:37:33]

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

[commit-bot: I haz the power, 2017-05-14 15:37:34]

Dry run: This issue passed the CQ dry run.

[mattcary, 2017-05-15 07:46:00]

lgtm

For prerender_browsertest.cc

[Bryan McQuade, 2017-05-15 11:40:00]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-05-15 11:40:01]

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

[commit-bot: I haz the power, 2017-05-15 11:40:16]

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-15 11:46:11]

CQ is committing da patch.

Bot data: {"patchset_id": 480001, "attempt_start_ts": 1494848400695910,
"parent_rev": "62809a64d70ec8b67847b3cefc03e4ba4bda0d3f", "commit_rev":
"9f5f6b9548bc4cad0c64d3aa2b5b58ae8dad595b"}

[commit-bot: I haz the power, 2017-05-15 11:46:24]

Description was changed from

==========
Report page load timing information for child frames.

This change clarifies that our PLMO::On*Paint callbacks are for the whole page,
and adds support for aggregating timing data across frames in PageLoadTracker.

The current behavior tracks paints across all frames, however this is due to a
bug
(crbug.com/705315) and will be fixed soon.

PLMO implementers actually want to track paints across all frames in the page,
so
this is desired behavior.

This change adds support for tracking paints across all frames so the current
behavior doesn't regress when 705315 is fixed.

BUG=661702
==========

to

==========
Report page load timing information for child frames.

This change clarifies that our PLMO::On*Paint callbacks are for the whole page,
and adds support for aggregating timing data across frames in PageLoadTracker.

The current behavior tracks paints across all frames, however this is due to a
bug
(crbug.com/705315) and will be fixed soon.

PLMO implementers actually want to track paints across all frames in the page,
so
this is desired behavior.

This change adds support for tracking paints across all frames so the current
behavior doesn't regress when 705315 is fixed.

BUG=661702

Review-Url: https://codereview.chromium.org/2859393002
Cr-Commit-Position: refs/heads/master@{#471725}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f5f6b9548bc4cad0c64d3aa2b5b...
==========

[commit-bot: I haz the power, 2017-05-15 11:46:26]

Committed patchset #25 (id:480001) as
https://chromium.googlesource.com/chromium/src/+/9f5f6b9548bc4cad0c64d3aa2b5b...