Add support for tracking loading behavior of child frames.

Add support for tracking loading behavior of child frames.

This is required for the existing subresource filtering and css scanning
page load metrics, as well as for the upcoming metrics for pages where
media is played.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames
* send at most one update per second per frame over IPC to the browser process
  (this is the existing page load metrics IPC policy)

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849
Review-Url: https://codereview.chromium.org/2737563007
Cr-Commit-Position: refs/heads/master@{#456166}
Committed: https://chromium.googlesource.com/chromium/src/+/2a2a964c75a71e56aa52790d2ce0bcd5a9af434a

[Bryan McQuade, 2017-03-08 19:50:47]

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

[commit-bot: I haz the power, 2017-03-08 19:51:31]

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-03-08 20:00:31]

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

[commit-bot: I haz the power, 2017-03-08 20: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-03-08 20:13:38]

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

[commit-bot: I haz the power, 2017-03-08 20:14: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-03-08 20:18:21]

Description was changed from

==========
Add support for tracking loading behavior of child frames.

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

to

==========
Add support for tracking loading behavior of child frames.

* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

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

[Bryan McQuade, 2017-03-08 20:20:04]

Description was changed from

==========
Add support for tracking loading behavior of child frames.

* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

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

to

==========
Add support for tracking loading behavior of child frames.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

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

[Bryan McQuade, 2017-03-08 20:34:16]

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

[commit-bot: I haz the power, 2017-03-08 20:34:33]

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-03-08 21:38:40]

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

[commit-bot: I haz the power, 2017-03-08 21:38:41]

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

[Charlie Harrison, 2017-03-08 21:47:16]

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

[Charlie Harrison, 2017-03-08 21:47:17]

Yes this looks reasonable. I left some initial comments.

https://codereview.chromium.org/2737563007/diff/10012/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2737563007/diff/10012/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:513:
content::RenderFrameHost* root_render_frame_host = render_frame_host;
I would probably prefer GetRenderViewHost()->GetMainFrame() so you don't have to
walk the frame tree.

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

https://codereview.chromium.org/2737563007/diff/10012/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:194: const int
child_loading_behavior_flags;
I would prefer either
1. Make this a full blown PageLoadMetadata
2. Put this int in the existing metadata.

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right):

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:176: bool
MetricsRenderFrameObserver::IsMainFrame() const {
RenderFrame has an IsMainFrame method already. I'm not sure if what you have
works with OOPIF.

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.h (left):

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.h:24: // sends
page load timing information to the browser process over IPC.
Can you please update the class comments for this class to describe the new
behavior?

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right):

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:22:
PageTimingMetricsSender::PageTimingMetricsSender(
Class comment in the header about how this class behaves with subframes would be
good.

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:32: if
(!last_timing_.IsEmpty()) {
nit: I think using the initial_timing makes the code more readable here.

[Bryan McQuade, 2017-03-08 22:14:40]

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

[commit-bot: I haz the power, 2017-03-08 22:14:57]

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

[RyanSturm, 2017-03-08 23:54:41]

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

[RyanSturm, 2017-03-08 23:54:42]

Seems reasonable.

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

https://codereview.chromium.org/2737563007/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:191: const
PageLoadMetadata metadata;
Can you change this to main_frame_metadata and update the comment?

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

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

[commit-bot: I haz the power, 2017-03-09 00:03:51]

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

[Bryan McQuade, 2017-03-09 00:20:26]

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

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

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-03-09 00:24:02]

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

[commit-bot: I haz the power, 2017-03-09 00:24: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-03-09 00:28:33]

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

[commit-bot: I haz the power, 2017-03-09 00:29: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-03-09 02:30:58]

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

[commit-bot: I haz the power, 2017-03-09 02:30:59]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-03-09 02:43:23]

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

[commit-bot: I haz the power, 2017-03-09 02:43:44]

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

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

[commit-bot: I haz the power, 2017-03-09 03:00:19]

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-03-09 03:06:12]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/2737563007/diff/10012/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2737563007/diff/10012/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:513:
content::RenderFrameHost* root_render_frame_host = render_frame_host;
On 2017/03/08 at 21:47:17, Charlie Harrison wrote:
> I would probably prefer GetRenderViewHost()->GetMainFrame() so you don't have
to walk the frame tree.

ah, absolutely, thanks! didn't know about that.

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

https://codereview.chromium.org/2737563007/diff/10012/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:194: const int
child_loading_behavior_flags;
On 2017/03/08 at 21:47:17, Charlie Harrison wrote:
> I would prefer either
> 1. Make this a full blown PageLoadMetadata
> 2. Put this int in the existing metadata.

sure, switched to a separate PageLoadMetadata

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc (right):

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:176: bool
MetricsRenderFrameObserver::IsMainFrame() const {
On 2017/03/08 at 21:47:17, Charlie Harrison wrote:
> RenderFrame has an IsMainFrame method already. I'm not sure if what you have
works with OOPIF.

excellent, thanks! i kept this virtual method for overriding in tests, but have
it call render_frame()->IsMainFrame() directly.

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
File chrome/renderer/page_load_metrics/metrics_render_frame_observer.h (left):

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.h:24: // sends
page load timing information to the browser process over IPC.
On 2017/03/08 at 21:47:17, Charlie Harrison wrote:
> Can you please update the class comments for this class to describe the new
behavior?

Good idea, done.

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
File chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc (right):

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:22:
PageTimingMetricsSender::PageTimingMetricsSender(
On 2017/03/08 at 21:47:17, Charlie Harrison wrote:
> Class comment in the header about how this class behaves with subframes would
be good.

This class actually doesn't have any subframe-specific policy. All of the
subframe policy is in MetricsRFO. I added a comment there. This class doesn't
send an empty timing update in the constructor, but that's standard behavior to
suppress unnecessary IPCs, not specific to subframes.

https://codereview.chromium.org/2737563007/diff/10012/chrome/renderer/page_lo...
chrome/renderer/page_load_metrics/page_timing_metrics_sender.cc:32: if
(!last_timing_.IsEmpty()) {
On 2017/03/08 at 21:47:17, Charlie Harrison wrote:
> nit: I think using the initial_timing makes the code more readable here.

done

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

https://codereview.chromium.org/2737563007/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:191: const
PageLoadMetadata metadata;
On 2017/03/08 at 23:54:41, Ryan Sturm wrote:
> Can you change this to main_frame_metadata and update the comment?

good idea, done, thanks!

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

Description was changed from

==========
Add support for tracking loading behavior of child frames.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

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

to

==========
Add support for tracking loading behavior of child frames.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849
==========

[Bryan McQuade, 2017-03-09 03:27:31]

Description was changed from

==========
Add support for tracking loading behavior of child frames.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849
==========

to

==========
Add support for tracking loading behavior of child frames.

This is required for the existing subresource filtering page load metrics,
as well as for the upcoming metrics for pages where media is played.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849
==========

[commit-bot: I haz the power, 2017-03-09 04:00:55]

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

[commit-bot: I haz the power, 2017-03-09 04:00:56]

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

[Bryan McQuade, 2017-03-09 04:07:42]

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

[Bryan McQuade, 2017-03-09 04:08:01]

engedy, PTAL for subresource filter browser test update, thanks!

[Bryan McQuade, 2017-03-09 15:53:07]

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

[commit-bot: I haz the power, 2017-03-09 15:53:24]

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-03-09 16:21:29]

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

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:515: if
(!render_frame_host->GetRenderViewHost() ||
Is it possible to have a null RVH here?

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/css_scanning_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/css_scanning_page_load_metrics_observer.cc:17:
if (info.main_frame_metadata.behavior_flags &
This class probably also wants to observe all cases where this happens.

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:192:
const int all_frame_loading_behavior_flags =
Maybe we should have a helper so classes don't need to OR these together
manually.

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

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.h:166: void
UpdateChildMetadata(const PageLoadMetadata& child_metadata);
I think it deserves a comment for why this is separate from UpdateTiming.

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

https://codereview.chromium.org/2737563007/diff/190001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:41: // Only
track timing metrics for main frames.
Can we ensure this in unit tests?

[commit-bot: I haz the power, 2017-03-09 17:28:43]

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

[commit-bot: I haz the power, 2017-03-09 17:28:44]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-03-09 17:33:08]

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

[commit-bot: I haz the power, 2017-03-09 17:33:36]

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-03-09 17:33:50]

Description was changed from

==========
Add support for tracking loading behavior of child frames.

This is required for the existing subresource filtering page load metrics,
as well as for the upcoming metrics for pages where media is played.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849
==========

to

==========
Add support for tracking loading behavior of child frames.

This is required for the existing subresource filtering and css scanning
page load metrics, as well as for the upcoming metrics for pages where
media is played.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849
==========

[Bryan McQuade, 2017-03-09 17:36:32]

Thanks! Addressed comments. PTAL.

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

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:515: if
(!render_frame_host->GetRenderViewHost() ||
On 2017/03/09 at 16:21:28, Charlie Harrison wrote:
> Is it possible to have a null RVH here?

it's a good question. i don't know for sure. i can omit this if you want -
trying to be conservative/careful given i plan to merge this to m58.

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/css_scanning_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/css_scanning_page_load_metrics_observer.cc:17:
if (info.main_frame_metadata.behavior_flags &
On 2017/03/09 at 16:21:28, Charlie Harrison wrote:
> This class probably also wants to observe all cases where this happens.

ah, excellent, i wasn't sure, thanks! will update it.

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:192:
const int all_frame_loading_behavior_flags =
On 2017/03/09 at 16:21:29, Charlie Harrison wrote:
> Maybe we should have a helper so classes don't need to OR these together
manually.

great idea, done.

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

https://codereview.chromium.org/2737563007/diff/190001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.h:166: void
UpdateChildMetadata(const PageLoadMetadata& child_metadata);
On 2017/03/09 at 16:21:29, Charlie Harrison wrote:
> I think it deserves a comment for why this is separate from UpdateTiming.

good idea, done

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

https://codereview.chromium.org/2737563007/diff/190001/chrome/renderer/page_l...
chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc:41: // Only
track timing metrics for main frames.
On 2017/03/09 at 16:21:29, Charlie Harrison wrote:
> Can we ensure this in unit tests?

good idea, done

[commit-bot: I haz the power, 2017-03-09 19:12:43]

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

[commit-bot: I haz the power, 2017-03-09 19:12:44]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-03-09 19:15:09]

lgtm. It might be worth adding a unittest for an observer for checking passing
child frame behaviors. 

If it's not worth it in this CL because you are merging it back, I will add a
little more to the test harness for the media CL.

[Charlie Harrison, 2017-03-09 19:17:11]

LGTM

[Bryan McQuade, 2017-03-09 19:35:51]

Description was changed from

==========
Add support for tracking loading behavior of child frames.

This is required for the existing subresource filtering and css scanning
page load metrics, as well as for the upcoming metrics for pages where
media is played.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849
==========

to

==========
Add support for tracking loading behavior of child frames.

This is required for the existing subresource filtering and css scanning
page load metrics, as well as for the upcoming metrics for pages where
media is played.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames
* send at most one update per second per frame over IPC to the browser process
  (this is the existing page load metrics IPC policy)

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849
==========

[Bryan McQuade, 2017-03-09 19:36:26]

bmcquade@chromium.org changed reviewers:
+ holte@chromium.org, jochen@chromium.org

[Bryan McQuade, 2017-03-09 19:37:03]

jochen, PTAL for chrome_content_renderer_client.cc change, thanks!
holte, PTAL for histograms.xml change, thanks!

[jochen (gone - plz use gerrit), 2017-03-10 10:43:14]

lgtm

[engedy, 2017-03-10 12:07:42]

*subresource_filter* LGTM.

[Bryan McQuade, 2017-03-10 18:29:45]

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

[commit-bot: I haz the power, 2017-03-10 18:30:21]

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-03-10 20:10:14]

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

[commit-bot: I haz the power, 2017-03-10 20:10:16]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-03-10 20:11:18]

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

[Bryan McQuade, 2017-03-10 20:11:46]

Looks like holte may not be available for review, so adding isherman.

isherman, PTAL for one line change to histograms.xml, thanks!

[Ilya Sherman, 2017-03-10 20:42:42]

histograms.xml

[Ilya Sherman, 2017-03-10 20:42:45]

histograms.xml lgtm

[Bryan McQuade, 2017-03-10 21:22:20]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-03-10 21:22:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, jochen@chromium.org, engedy@chromium.org,
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2737563007/#ps230001
(title: "stop observing if started in background")

[commit-bot: I haz the power, 2017-03-10 21:22:54]

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-03-10 21:29:43]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1489180940918360,
"parent_rev": "0e08dc7abfc81c398f35ad2eb6db7c492a9d06df", "commit_rev":
"2a2a964c75a71e56aa52790d2ce0bcd5a9af434a"}

[commit-bot: I haz the power, 2017-03-10 21:30:29]

Description was changed from

==========
Add support for tracking loading behavior of child frames.

This is required for the existing subresource filtering and css scanning
page load metrics, as well as for the upcoming metrics for pages where
media is played.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames
* send at most one update per second per frame over IPC to the browser process
  (this is the existing page load metrics IPC policy)

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849
==========

to

==========
Add support for tracking loading behavior of child frames.

This is required for the existing subresource filtering and css scanning
page load metrics, as well as for the upcoming metrics for pages where
media is played.

In the render process:
* move policy decision about whether to process timing or metadata updates
  from blink to chrome/renderer/page_load_metrics code
* instantiate MetricsRenderFrameObservers for child frames
* track metadata updates for child frames
* send at most one update per second per frame over IPC to the browser process
  (this is the existing page load metrics IPC policy)

In the browser process:
* track child frame loading behavior flags
* notify observers whenever the loading behavior flags for a child frame
  are updated

In the SubresourceFilterMetricsObserver:
* consider the page to have filtered subresources if subresource filter loading
  behavior is observed in either the main frame or child frames

Note that we still do not track timing updates for child frames. This change
gets us closer to being able to do so, but given that there is no current
need, we don't do this for the time being.

BUG=699849

Review-Url: https://codereview.chromium.org/2737563007
Cr-Commit-Position: refs/heads/master@{#456166}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a2a964c75a71e56aa52790d2ce0...
==========

[commit-bot: I haz the power, 2017-03-10 21:30:31]

Committed patchset #13 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/2a2a964c75a71e56aa52790d2ce0...