Notify page load metrics when the app enters the background.

Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

On Android (and iOS), when an app enters the background, it may be
killed without further notifications. Thus, having this callback
will allow us to persist metrics before the app may be killed.
Our page load metrics subsystem isn't used on iOS, so we only target
Android in this change.

Unfortunately, we can't use the existing WebContents hidden callback,
as this gets invoked after the UMA subsystem has already flushed
metrics to disk. Thus, we need to add this explicit callback which
allows us to persist metrics as part of the UMA subsystem handling
the backgrounding callback.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
Committed: https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240
Cr-Commit-Position: refs/heads/master@{#408744}

[Bryan McQuade, 2016-07-27 02:31:48]

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

[commit-bot: I haz the power, 2016-07-27 02:32:01]

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

[commit-bot: I haz the power, 2016-07-27 03:22:05]

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

[commit-bot: I haz the power, 2016-07-27 03:22:05]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-27 12:46:48]

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

[commit-bot: I haz the power, 2016-07-27 12:46: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, 2016-07-27 12:54:58]

Description was changed from

==========
Initial implementation using OnAppEnterBackground callback.

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

to

==========
Initial implementation using OnAppEnterBackground callback.

BUG=608360
==========

[Bryan McQuade, 2016-07-27 12:59:46]

Description was changed from

==========
Initial implementation using OnAppEnterBackground callback.

BUG=608360
==========

to

==========
Notify page load metrics when the app enters the background.

BUG=608360
==========

[Bryan McQuade, 2016-07-27 13:02:38]

Description was changed from

==========
Notify page load metrics when the app enters the background.

BUG=608360
==========

to

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMOnbsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

[Bryan McQuade, 2016-07-27 13:03:05]

Description was changed from

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMOnbsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

to

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

[Bryan McQuade, 2016-07-27 13:40:22]

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

[commit-bot: I haz the power, 2016-07-27 13:40:29]

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

[commit-bot: I haz the power, 2016-07-27 14:35:53]

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

[commit-bot: I haz the power, 2016-07-27 14:35:54]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-27 18:28:00]

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

[commit-bot: I haz the power, 2016-07-27 18:28:14]

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

[commit-bot: I haz the power, 2016-07-27 18:30:31]

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

[commit-bot: I haz the power, 2016-07-27 18:30:32]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Bryan McQuade, 2016-07-27 19:34:09]

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

[commit-bot: I haz the power, 2016-07-27 19:34: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, 2016-07-27 19:37:30]

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

[Bryan McQuade, 2016-07-27 19:37:56]

PTAL. Once we're happy with this, I'll send to asvitkine for review of the new
MetricsProvider.

[Bryan McQuade, 2016-07-27 19:41:21]

Description was changed from

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

to

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

On Android (and iOS), when an app enters the background, it may be
killed without further notifications. Thus, having this callback
will allow us to persist metrics before the app may be killed.

Unfortunately, we can't use the existing WebContents hidden callback,
as this gets invoked after the UMA subsystem has already flushed
metrics to disk. Thus, we need to add this explicit callback which
allows us to persist metrics as part of being backgrounded.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

[Bryan McQuade, 2016-07-27 19:47:09]

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

[commit-bot: I haz the power, 2016-07-27 19:47: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, 2016-07-27 19:59:22]

Description was changed from

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

On Android (and iOS), when an app enters the background, it may be
killed without further notifications. Thus, having this callback
will allow us to persist metrics before the app may be killed.

Unfortunately, we can't use the existing WebContents hidden callback,
as this gets invoked after the UMA subsystem has already flushed
metrics to disk. Thus, we need to add this explicit callback which
allows us to persist metrics as part of being backgrounded.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

to

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

On Android (and iOS), when an app enters the background, it may be
killed without further notifications. Thus, having this callback
will allow us to persist metrics before the app may be killed.

Unfortunately, we can't use the existing WebContents hidden callback,
as this gets invoked after the UMA subsystem has already flushed
metrics to disk. Thus, we need to add this explicit callback which
allows us to persist metrics as part of the UMA subsystem handling
the backgrounding callback.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

[commit-bot: I haz the power, 2016-07-27 20:42:35]

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

[commit-bot: I haz the power, 2016-07-27 20:42:36]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-07-28 14:45:28]

Thanks, this one is pretty necessary.

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/page_load_metrics_provider.cc (right):

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/page_load_metrics_provider.cc:18: for (auto observer :
observers) {
I would prefer if iterating through all MWCO happens here, instead of on a
static method on MWCO.

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

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:271: if
(app_entered_background_) {
Hm. Can you log a 3-state histogram instead of a 2 state one? I'd like a count
of how many page load trackers destruct with app_entered_background_ = true.

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
(web_contents->GetRenderViewHost() != rvh)
How can this happen?

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:656:
DCHECK(std::find(result.begin(), result.end(), observer) == result.end());
If you want this feature let's use an std::set instead of vector, then you can
check this upon insertion.

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:313: void
FlushMetricsOnAppEnterBackground();
Do we need to include "flush metrics" in the name? Can observers just do what
they want with the knowledge that they app entered the background?

[Bryan McQuade, 2016-07-28 16:26:11]

Thanks! See what you think about moving GetAllWebContents() from WebContentsImpl
to WebContents.

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/page_load_metrics_provider.cc (right):

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/page_load_metrics_provider.cc:18: for (auto observer :
observers) {
On 2016/07/28 at 14:45:27, csharrison wrote:
> I would prefer if iterating through all MWCO happens here, instead of on a
static method on MWCO.

See later comment - I think I'm inclined to move the
WebContentsImpl::GetAllWebContents to a public header that can be used by code
in chrome/. If we decide not to do that, I'd like to keep this method in a
header so I can test it (as I've done in our browsertest). I could move it to
page_load_metrics_provider.h but that feels like a stranger place for that
method to live than the MWCO header.

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

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:271: if
(app_entered_background_) {
On 2016/07/28 at 14:45:27, csharrison wrote:
> Hm. Can you log a 3-state histogram instead of a 2 state one? I'd like a count
of how many page load trackers destruct with app_entered_background_ = true.

I think the third state you describe should just be the difference between the
false counts and the true counts. Does that sound right to you?

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
(web_contents->GetRenderViewHost() != rvh)
On 2016/07/28 at 14:45:27, csharrison wrote:
> How can this happen?

I actually lifted this logic from
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
which I would otherwise have just called directly, except it's in
WebContentsImpl instead of WebContents.

I don't see a reason though not to just move this to WebContents so code outside
of content/ can use it. WDYT? If you think it's reasonable I could ask a content
owner what they think.

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:656:
DCHECK(std::find(result.begin(), result.end(), observer) == result.end());
On 2016/07/28 at 14:45:27, csharrison wrote:
> If you want this feature let's use an std::set instead of vector, then you can
check this upon insertion.

The drawback to set<> is that iteration order will vary depending on pointer
order, which I've seen lead to subtle issues in some cases. Using vector<> means
they'll consistently be returned in RWHIterator order.

This DCHECK is essentially a code version of the comment in the WebContentsImpl:
    // Because a WebContents can only have one current RVH at a time, there will
    // be no duplicate WebContents here.
I could just include that comment instead if you prefer.

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:313: void
FlushMetricsOnAppEnterBackground();
On 2016/07/28 at 14:45:28, csharrison wrote:
> Do we need to include "flush metrics" in the name? Can observers just do what
they want with the knowledge that they app entered the background?

Yeah - I debated this. Technically, this method is coupled to the metrics
subsystem's backgrounding logic. This callback is guaranteed to be called before
the metrics subsystem writes metrics out to disk as part of its backgrounding
logic. A generic OnAppEnterBackground wouldn't necessarily make this guarantee -
it might get called during backgrounding, but after the metrics subsystem has
finished flushing metrics, which wouldn't be the right timing for flushing
metrics. Similarly, if there's some other chrome subsystem that observers need
to interact with as part of backgrounding in the future, we'll probably need a
separate callback that's guaranteed to get called as part of the backgrounding
flow for that component, so we can be sure it's called e.g. before that other
subsystem is done persisting its data. Given all this, I decided it was better
to explicitly include 'FlushMetrics' in the name.

I added a comment to better explain.

[Bryan McQuade, 2016-07-28 16:27:14]

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

[commit-bot: I haz the power, 2016-07-28 16:27: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, 2016-07-28 16:29:15]

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

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:271: if
(app_entered_background_) {
On 2016/07/28 at 16:26:11, Bryan McQuade wrote:
> On 2016/07/28 at 14:45:27, csharrison wrote:
> > Hm. Can you log a 3-state histogram instead of a 2 state one? I'd like a
count of how many page load trackers destruct with app_entered_background_ =
true.
> 
> I think the third state you describe should just be the difference between the
false counts and the true counts. Does that sound right to you?

Ah, sorry, re-reading your comment, I think this histogram logging is the thing
you're asking for "a count of how many page load trackers destruct with
app_entered_background_ = true". Did you mean something else?

[Charlie Harrison, 2016-07-28 16:38:04]

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

[Charlie Harrison, 2016-07-28 16:38:05]

Thanks. +creis for question about exposing WebContentsImpl method. Is this
acceptable if our alternative is to copy-paste this logic?

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

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:271: if
(app_entered_background_) {
On 2016/07/28 16:26:11, Bryan McQuade wrote:
> On 2016/07/28 at 14:45:27, csharrison wrote:
> > Hm. Can you log a 3-state histogram instead of a 2 state one? I'd like a
count
> of how many page load trackers destruct with app_entered_background_ = true.
> 
> I think the third state you describe should just be the difference between the
> false counts and the true counts. Does that sound right to you?

Ah you're right, my mistake.

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
(web_contents->GetRenderViewHost() != rvh)
On 2016/07/28 16:26:11, Bryan McQuade wrote:
> On 2016/07/28 at 14:45:27, csharrison wrote:
> > How can this happen?
> 
> I actually lifted this logic from
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> which I would otherwise have just called directly, except it's in
> WebContentsImpl instead of WebContents.
> 
> I don't see a reason though not to just move this to WebContents so code
outside
> of content/ can use it. WDYT? If you think it's reasonable I could ask a
content
> owner what they think. 

Yeah, let's see if we can expose this. No point duplicating the logic.

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:656:
DCHECK(std::find(result.begin(), result.end(), observer) == result.end());
On 2016/07/28 16:26:11, Bryan McQuade wrote:
> On 2016/07/28 at 14:45:27, csharrison wrote:
> > If you want this feature let's use an std::set instead of vector, then you
can
> check this upon insertion.
> 
> The drawback to set<> is that iteration order will vary depending on pointer
> order, which I've seen lead to subtle issues in some cases. Using vector<>
means
> they'll consistently be returned in RWHIterator order.
> 
> This DCHECK is essentially a code version of the comment in the
WebContentsImpl:
>     // Because a WebContents can only have one current RVH at a time, there
will
>     // be no duplicate WebContents here.
> I could just include that comment instead if you prefer.

No, your reasoning makes sense. Keeping this DCHECK SGTM.

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h (right):

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:313: void
FlushMetricsOnAppEnterBackground();
On 2016/07/28 16:26:11, Bryan McQuade wrote:
> On 2016/07/28 at 14:45:28, csharrison wrote:
> > Do we need to include "flush metrics" in the name? Can observers just do
what
> they want with the knowledge that they app entered the background?
> 
> Yeah - I debated this. Technically, this method is coupled to the metrics
> subsystem's backgrounding logic. This callback is guaranteed to be called
before
> the metrics subsystem writes metrics out to disk as part of its backgrounding
> logic. A generic OnAppEnterBackground wouldn't necessarily make this guarantee
-
> it might get called during backgrounding, but after the metrics subsystem has
> finished flushing metrics, which wouldn't be the right timing for flushing
> metrics. Similarly, if there's some other chrome subsystem that observers need
> to interact with as part of backgrounding in the future, we'll probably need a
> separate callback that's guaranteed to get called as part of the backgrounding
> flow for that component, so we can be sure it's called e.g. before that other
> subsystem is done persisting its data. Given all this, I decided it was better
> to explicitly include 'FlushMetrics' in the name.
> 
> I added a comment to better explain.

SGTM.

[commit-bot: I haz the power, 2016-07-28 17:27:20]

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

[commit-bot: I haz the power, 2016-07-28 17:27:21]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-07-28 17:36:48]

creis@chromium.org changed reviewers:
+ jam@chromium.org

[Charlie Reis, 2016-07-28 17:36:49]

John, can you weigh in on the question about iterating over WebContents below?

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

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
(web_contents->GetRenderViewHost() != rvh)
On 2016/07/28 16:38:04, csharrison wrote:
> On 2016/07/28 16:26:11, Bryan McQuade wrote:
> > On 2016/07/28 at 14:45:27, csharrison wrote:
> > > How can this happen?
> > 
> > I actually lifted this logic from
> >
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> > which I would otherwise have just called directly, except it's in
> > WebContentsImpl instead of WebContents.
> > 
> > I don't see a reason though not to just move this to WebContents so code
> outside
> > of content/ can use it. WDYT? If you think it's reasonable I could ask a
> content
> > owner what they think. 
> 
> Yeah, let's see if we can expose this. No point duplicating the logic.

Adding jam@, who has been against having a public way to iterate over all
WebContents.  I think the idea is that the embedder should have a way to know
about all the tabs it has created?  (Obviously, copy/pasting the code isn't
desirable either.)

[Charlie Harrison, 2016-07-28 17:42:17]

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

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
(web_contents->GetRenderViewHost() != rvh)
On 2016/07/28 17:36:49, Charlie Reis wrote:
> On 2016/07/28 16:38:04, csharrison wrote:
> > On 2016/07/28 16:26:11, Bryan McQuade wrote:
> > > On 2016/07/28 at 14:45:27, csharrison wrote:
> > > > How can this happen?
> > > 
> > > I actually lifted this logic from
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> > > which I would otherwise have just called directly, except it's in
> > > WebContentsImpl instead of WebContents.
> > > 
> > > I don't see a reason though not to just move this to WebContents so code
> > outside
> > > of content/ can use it. WDYT? If you think it's reasonable I could ask a
> > content
> > > owner what they think. 
> > 
> > Yeah, let's see if we can expose this. No point duplicating the logic.
> 
> Adding jam@, who has been against having a public way to iterate over all
> WebContents.  I think the idea is that the embedder should have a way to know
> about all the tabs it has created?  (Obviously, copy/pasting the code isn't
> desirable either.)

Ah that makes sense. Bryan, have you look at doing this from the tab strip?

[Bryan McQuade, 2016-07-28 17:50:32]

On 2016/07/28 at 17:42:17, csharrison wrote:
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):
> 
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
(web_contents->GetRenderViewHost() != rvh)
> On 2016/07/28 17:36:49, Charlie Reis wrote:
> > On 2016/07/28 16:38:04, csharrison wrote:
> > > On 2016/07/28 16:26:11, Bryan McQuade wrote:
> > > > On 2016/07/28 at 14:45:27, csharrison wrote:
> > > > > How can this happen?
> > > > 
> > > > I actually lifted this logic from
> > > >
> > >
> >
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> > > > which I would otherwise have just called directly, except it's in
> > > > WebContentsImpl instead of WebContents.
> > > > 
> > > > I don't see a reason though not to just move this to WebContents so code
> > > outside
> > > > of content/ can use it. WDYT? If you think it's reasonable I could ask a
> > > content
> > > > owner what they think. 
> > > 
> > > Yeah, let's see if we can expose this. No point duplicating the logic.
> > 
> > Adding jam@, who has been against having a public way to iterate over all
> > WebContents.  I think the idea is that the embedder should have a way to
know
> > about all the tabs it has created?  (Obviously, copy/pasting the code isn't
> > desirable either.)
> 
> Ah that makes sense. Bryan, have you look at doing this from the tab strip?

Ah, it looks like I could do:

for (int i = 0; i < browser->tab_strip_model()->count(); ++i) {
  content::WebContents* contents =
      browser->tab_strip_model()->GetWebContentsAt(i);
  ...
}

how does that sound?

Are there any significant behavior differences between iterating the tab strip
and the RWHIterator approach?

[Charlie Reis, 2016-07-28 17:56:44]

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

https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
(web_contents->GetRenderViewHost() != rvh)
On 2016/07/28 17:50:32, Bryan McQuade wrote:
> On 2016/07/28 at 17:42:17, csharrison wrote:
> >
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
> (web_contents->GetRenderViewHost() != rvh)
> > On 2016/07/28 17:36:49, Charlie Reis wrote:
> > > On 2016/07/28 16:38:04, csharrison wrote:
> > > > On 2016/07/28 16:26:11, Bryan McQuade wrote:
> > > > > On 2016/07/28 at 14:45:27, csharrison wrote:
> > > > > > How can this happen?
> > > > > 
> > > > > I actually lifted this logic from
> > > > >
> > > >
> > >
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> > > > > which I would otherwise have just called directly, except it's in
> > > > > WebContentsImpl instead of WebContents.
> > > > > 
> > > > > I don't see a reason though not to just move this to WebContents so
code
> > > > outside
> > > > > of content/ can use it. WDYT? If you think it's reasonable I could ask
a
> > > > content
> > > > > owner what they think. 
> > > > 
> > > > Yeah, let's see if we can expose this. No point duplicating the logic.
> > > 
> > > Adding jam@, who has been against having a public way to iterate over all
> > > WebContents.  I think the idea is that the embedder should have a way to
> know
> > > about all the tabs it has created?  (Obviously, copy/pasting the code
isn't
> > > desirable either.)
> > 
> > Ah that makes sense. Bryan, have you look at doing this from the tab strip?
> 
> Ah, it looks like I could do:
> 
> for (int i = 0; i < browser->tab_strip_model()->count(); ++i) {
>   content::WebContents* contents =
>       browser->tab_strip_model()->GetWebContentsAt(i);
>   ...
> }
> 
> how does that sound?

That would be great if it covered all the cases you need.  Are you just looking
for tabs in a single window, or would you loop through all |browser| objects as
well?

> 
> Are there any significant behavior differences between iterating the tab strip
> and the RWHIterator approach?

There are lots of WebContents which don't appear in a tab strip, like Devtools,
prerender, and other non-tab cases.  (Not sure if we still have notifications or
background WebContents anymore, but I think so.)  Your approach would be nice if
you only care about the ones in the tab strip.

[Bryan McQuade, 2016-07-28 18:02:07]

On 2016/07/28 at 17:56:44, creis wrote:
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):
> 
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
(web_contents->GetRenderViewHost() != rvh)
> On 2016/07/28 17:50:32, Bryan McQuade wrote:
> > On 2016/07/28 at 17:42:17, csharrison wrote:
> > >
> >
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > > File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > > chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
> > (web_contents->GetRenderViewHost() != rvh)
> > > On 2016/07/28 17:36:49, Charlie Reis wrote:
> > > > On 2016/07/28 16:38:04, csharrison wrote:
> > > > > On 2016/07/28 16:26:11, Bryan McQuade wrote:
> > > > > > On 2016/07/28 at 14:45:27, csharrison wrote:
> > > > > > > How can this happen?
> > > > > > 
> > > > > > I actually lifted this logic from
> > > > > >
> > > > >
> > > >
> >
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> > > > > > which I would otherwise have just called directly, except it's in
> > > > > > WebContentsImpl instead of WebContents.
> > > > > > 
> > > > > > I don't see a reason though not to just move this to WebContents so
code
> > > > > outside
> > > > > > of content/ can use it. WDYT? If you think it's reasonable I could
ask a
> > > > > content
> > > > > > owner what they think. 
> > > > > 
> > > > > Yeah, let's see if we can expose this. No point duplicating the logic.
> > > > 
> > > > Adding jam@, who has been against having a public way to iterate over
all
> > > > WebContents.  I think the idea is that the embedder should have a way to
> > know
> > > > about all the tabs it has created?  (Obviously, copy/pasting the code
isn't
> > > > desirable either.)
> > > 
> > > Ah that makes sense. Bryan, have you look at doing this from the tab
strip?
> > 
> > Ah, it looks like I could do:
> > 
> > for (int i = 0; i < browser->tab_strip_model()->count(); ++i) {
> >   content::WebContents* contents =
> >       browser->tab_strip_model()->GetWebContentsAt(i);
> >   ...
> > }
> > 
> > how does that sound?
> 
> That would be great if it covered all the cases you need.  Are you just
looking for tabs in a single window, or would you loop through all |browser|
objects as well?

I'd like to iterate across all tabs, across all windows. Is there an API to
iterate browser objects?

RE: devtools, prerender, and others not in the tab strip, I'm not interested in
those, so it sounds like the tab_strip_model is actually a better fit for me.
Thanks!

> 
> > 
> > Are there any significant behavior differences between iterating the tab
strip
> > and the RWHIterator approach?
> 
> There are lots of WebContents which don't appear in a tab strip, like
Devtools, prerender, and other non-tab cases.  (Not sure if we still have
notifications or background WebContents anymore, but I think so.)  Your approach
would be nice if you only care about the ones in the tab strip.

[Bryan McQuade, 2016-07-28 18:09:16]

On 2016/07/28 at 18:02:07, Bryan McQuade wrote:
> On 2016/07/28 at 17:56:44, creis wrote:
> >
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):
> > 
> >
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
(web_contents->GetRenderViewHost() != rvh)
> > On 2016/07/28 17:50:32, Bryan McQuade wrote:
> > > On 2016/07/28 at 17:42:17, csharrison wrote:
> > > >
> > >
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > > > File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
> > > (right):
> > > > 
> > > >
> > >
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > > > chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649:
if
> > > (web_contents->GetRenderViewHost() != rvh)
> > > > On 2016/07/28 17:36:49, Charlie Reis wrote:
> > > > > On 2016/07/28 16:38:04, csharrison wrote:
> > > > > > On 2016/07/28 16:26:11, Bryan McQuade wrote:
> > > > > > > On 2016/07/28 at 14:45:27, csharrison wrote:
> > > > > > > > How can this happen?
> > > > > > > 
> > > > > > > I actually lifted this logic from
> > > > > > >
> > > > > >
> > > > >
> > >
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> > > > > > > which I would otherwise have just called directly, except it's in
> > > > > > > WebContentsImpl instead of WebContents.
> > > > > > > 
> > > > > > > I don't see a reason though not to just move this to WebContents
so code
> > > > > > outside
> > > > > > > of content/ can use it. WDYT? If you think it's reasonable I could
ask a
> > > > > > content
> > > > > > > owner what they think. 
> > > > > > 
> > > > > > Yeah, let's see if we can expose this. No point duplicating the
logic.
> > > > > 
> > > > > Adding jam@, who has been against having a public way to iterate over
all
> > > > > WebContents.  I think the idea is that the embedder should have a way
to
> > > know
> > > > > about all the tabs it has created?  (Obviously, copy/pasting the code
isn't
> > > > > desirable either.)
> > > > 
> > > > Ah that makes sense. Bryan, have you look at doing this from the tab
strip?
> > > 
> > > Ah, it looks like I could do:
> > > 
> > > for (int i = 0; i < browser->tab_strip_model()->count(); ++i) {
> > >   content::WebContents* contents =
> > >       browser->tab_strip_model()->GetWebContentsAt(i);
> > >   ...
> > > }
> > > 
> > > how does that sound?
> > 
> > That would be great if it covered all the cases you need.  Are you just
looking for tabs in a single window, or would you loop through all |browser|
objects as well?
> 
> I'd like to iterate across all tabs, across all windows. Is there an API to
iterate browser objects?

I found BrowserList::GetInstance(). Is this the right way to iterate all browser
instances?

Alternatively, is TabContentsIterator a better approach here? Looks like it
iterates over all browsers and in turn all tabs. If that seems like an ok
solution, that's definitely much simpler and likely better than what I had been
doing previously.

> 
> RE: devtools, prerender, and others not in the tab strip, I'm not interested
in those, so it sounds like the tab_strip_model is actually a better fit for me.
Thanks!
> 
> > 
> > > 
> > > Are there any significant behavior differences between iterating the tab
strip
> > > and the RWHIterator approach?
> > 
> > There are lots of WebContents which don't appear in a tab strip, like
Devtools, prerender, and other non-tab cases.  (Not sure if we still have
notifications or background WebContents anymore, but I think so.)  Your approach
would be nice if you only care about the ones in the tab strip.

[jam, 2016-07-28 18:11:57]

On 2016/07/28 17:36:49, Charlie Reis wrote:
> John, can you weigh in on the question about iterating over WebContents below?
> 
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):
> 
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
> (web_contents->GetRenderViewHost() != rvh)
> On 2016/07/28 16:38:04, csharrison wrote:
> > On 2016/07/28 16:26:11, Bryan McQuade wrote:
> > > On 2016/07/28 at 14:45:27, csharrison wrote:
> > > > How can this happen?
> > > 
> > > I actually lifted this logic from
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> > > which I would otherwise have just called directly, except it's in
> > > WebContentsImpl instead of WebContents.
> > > 
> > > I don't see a reason though not to just move this to WebContents so code
> > outside
> > > of content/ can use it. WDYT? If you think it's reasonable I could ask a
> > content
> > > owner what they think. 
> > 
> > Yeah, let's see if we can expose this. No point duplicating the logic.
> 
> Adding jam@, who has been against having a public way to iterate over all
> WebContents.  I think the idea is that the embedder should have a way to know
> about all the tabs it has created?  (Obviously, copy/pasting the code isn't
> desirable either.)

Right. The main motivation is that we had layering violations. for example, code
in src/extensions would be interacting with tabs created by other features.

[Charlie Reis, 2016-07-28 18:18:33]

On 2016/07/28 18:09:16, Bryan McQuade wrote:
> On 2016/07/28 at 18:02:07, Bryan McQuade wrote:
> > On 2016/07/28 at 17:56:44, creis wrote:
> > >
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > > File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
> (right):
> > > 
> > >
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > > chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649: if
> (web_contents->GetRenderViewHost() != rvh)
> > > On 2016/07/28 17:50:32, Bryan McQuade wrote:
> > > > On 2016/07/28 at 17:42:17, csharrison wrote:
> > > > >
> > > >
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > > > > File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
> > > > (right):
> > > > > 
> > > > >
> > > >
>
https://codereview.chromium.org/2189543002/diff/100001/chrome/browser/page_lo...
> > > > > chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:649:
> if
> > > > (web_contents->GetRenderViewHost() != rvh)
> > > > > On 2016/07/28 17:36:49, Charlie Reis wrote:
> > > > > > On 2016/07/28 16:38:04, csharrison wrote:
> > > > > > > On 2016/07/28 16:26:11, Bryan McQuade wrote:
> > > > > > > > On 2016/07/28 at 14:45:27, csharrison wrote:
> > > > > > > > > How can this happen?
> > > > > > > > 
> > > > > > > > I actually lifted this logic from
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> > > > > > > > which I would otherwise have just called directly, except it's
in
> > > > > > > > WebContentsImpl instead of WebContents.
> > > > > > > > 
> > > > > > > > I don't see a reason though not to just move this to WebContents
> so code
> > > > > > > outside
> > > > > > > > of content/ can use it. WDYT? If you think it's reasonable I
could
> ask a
> > > > > > > content
> > > > > > > > owner what they think. 
> > > > > > > 
> > > > > > > Yeah, let's see if we can expose this. No point duplicating the
> logic.
> > > > > > 
> > > > > > Adding jam@, who has been against having a public way to iterate
over
> all
> > > > > > WebContents.  I think the idea is that the embedder should have a
way
> to
> > > > know
> > > > > > about all the tabs it has created?  (Obviously, copy/pasting the
code
> isn't
> > > > > > desirable either.)
> > > > > 
> > > > > Ah that makes sense. Bryan, have you look at doing this from the tab
> strip?
> > > > 
> > > > Ah, it looks like I could do:
> > > > 
> > > > for (int i = 0; i < browser->tab_strip_model()->count(); ++i) {
> > > >   content::WebContents* contents =
> > > >       browser->tab_strip_model()->GetWebContentsAt(i);
> > > >   ...
> > > > }
> > > > 
> > > > how does that sound?
> > > 
> > > That would be great if it covered all the cases you need.  Are you just
> looking for tabs in a single window, or would you loop through all |browser|
> objects as well?
> > 
> > I'd like to iterate across all tabs, across all windows. Is there an API to
> iterate browser objects?
> 
> I found BrowserList::GetInstance(). Is this the right way to iterate all
browser
> instances?
> 
> Alternatively, is TabContentsIterator a better approach here? Looks like it
> iterates over all browsers and in turn all tabs. If that seems like an ok
> solution, that's definitely much simpler and likely better than what I had
been
> doing previously.

Yes, I think TabContentsIterator is what you want.  Hope that helps!

> 
> > 
> > RE: devtools, prerender, and others not in the tab strip, I'm not interested
> in those, so it sounds like the tab_strip_model is actually a better fit for
me.
> Thanks!
> > 
> > > 
> > > > 
> > > > Are there any significant behavior differences between iterating the tab
> strip
> > > > and the RWHIterator approach?
> > > 
> > > There are lots of WebContents which don't appear in a tab strip, like
> Devtools, prerender, and other non-tab cases.  (Not sure if we still have
> notifications or background WebContents anymore, but I think so.)  Your
approach
> would be nice if you only care about the ones in the tab strip.

[Bryan McQuade, 2016-07-28 18:31:52]

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

[commit-bot: I haz the power, 2016-07-28 18:32:30]

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

[Bryan McQuade, 2016-07-28 18:35:05]

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

[commit-bot: I haz the power, 2016-07-28 18:35:15]

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

[Bryan McQuade, 2016-07-28 18:49:33]

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

[commit-bot: I haz the power, 2016-07-28 18:49:41]

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

[Bryan McQuade, 2016-07-28 19:06:16]

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

[commit-bot: I haz the power, 2016-07-28 19:06: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, 2016-07-28 19:23:22]

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

[commit-bot: I haz the power, 2016-07-28 19:23: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, 2016-07-28 19:46:54]

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

[commit-bot: I haz the power, 2016-07-28 19:46:55]

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, 2016-07-28 19:54:06]

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

[commit-bot: I haz the power, 2016-07-28 19:54:39]

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

[commit-bot: I haz the power, 2016-07-28 20:06:44]

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

[commit-bot: I haz the power, 2016-07-28 20:06:47]

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

[Bryan McQuade, 2016-07-28 21:09:23]

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

[commit-bot: I haz the power, 2016-07-28 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...

[Bryan McQuade, 2016-07-28 21:16:09]

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

[commit-bot: I haz the power, 2016-07-28 21:16: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, 2016-07-28 21:22:06]

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

[commit-bot: I haz the power, 2016-07-28 21:22:10]

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

[commit-bot: I haz the power, 2016-07-28 22:17:16]

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

[commit-bot: I haz the power, 2016-07-28 22:17:17]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-28 22:19:26]

Since this change targets Android, I ended up not being able to use
TabContentsIterator. I used a slightly different Android-specific tab iteration
approach instead. PTAL.

[Charlie Harrison, 2016-07-28 22:31:32]

LGTM but I'm not 100% familiar with the new iteration technique.

[Bryan McQuade, 2016-07-28 23:35:46]

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

[commit-bot: I haz the power, 2016-07-28 23:36:17]

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

[Bryan McQuade, 2016-07-28 23:44:08]

On 2016/07/28 at 22:31:32, csharrison wrote:
> LGTM but I'm not 100% familiar with the new iteration technique.

Thanks! The iteration logic is based on
TabModelList::GetTabModelForWebContents() which uses the same two-layer
iteration mechanism. I tested this out on an Android device and it works as I
expect it to. So I think we are good to go here. Will add asvitkine to the
review.

[Bryan McQuade, 2016-07-28 23:44:54]

bmcquade@chromium.org changed reviewers:
+ asvitkine@chromium.org
- creis@chromium.org, jam@chromium.org

[Bryan McQuade, 2016-07-28 23:45:15]

bmcquade@chromium.org changed reviewers:
+ creis@chromium.org, jam@chromium.org
- asvitkine@chromium.org

[Bryan McQuade, 2016-07-28 23:45:16]

asvitkine, PTAL, thanks!

[commit-bot: I haz the power, 2016-07-29 00:56:54]

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

[commit-bot: I haz the power, 2016-07-29 00:56:55]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-29 00:57:21]

bmcquade@chromium.org changed reviewers:
+ asvitkine@chromium.org
- creis@chromium.org, jam@chromium.org

[Bryan McQuade, 2016-07-29 00:57:59]

Oops, forgot to add asvitkine.

asvitkine, PTAL. Thanks!

[Bryan McQuade, 2016-07-29 13:48:42]

Description was changed from

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

On Android (and iOS), when an app enters the background, it may be
killed without further notifications. Thus, having this callback
will allow us to persist metrics before the app may be killed.

Unfortunately, we can't use the existing WebContents hidden callback,
as this gets invoked after the UMA subsystem has already flushed
metrics to disk. Thus, we need to add this explicit callback which
allows us to persist metrics as part of the UMA subsystem handling
the backgrounding callback.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

to

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

On Android (and iOS), when an app enters the background, it may be
killed without further notifications. Thus, having this callback
will allow us to persist metrics before the app may be killed.
Our page load metrics subsystem isn't used on iOS, so we only target
Android in this change.

Unfortunately, we can't use the existing WebContents hidden callback,
as this gets invoked after the UMA subsystem has already flushed
metrics to disk. Thus, we need to add this explicit callback which
allows us to persist metrics as part of the UMA subsystem handling
the backgrounding callback.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

[Alexei Svitkine (slow), 2016-07-29 17:31:27]

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

https://codereview.chromium.org/2189543002/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:270:
UMA_HISTOGRAM_BOOLEAN(internal::kPageLoadCompletedAfterAppBackground, true);
Each histogram macro resolves to a lot of machine code.

To avoid the extra bloat, please make a helper function for logging this
histogram and call that.

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

https://codereview.chromium.org/2189543002/diff/280001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:37831: +    enum="Boolean">
Please use a more specific enum, which you can define.

[Bryan McQuade, 2016-07-29 17:45:51]

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

[commit-bot: I haz the power, 2016-07-29 17:46: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, 2016-07-29 17:48:07]

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

[Bryan McQuade, 2016-07-29 17:48:10]

Thanks! Addressed comments. PTAL.

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

https://codereview.chromium.org/2189543002/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:270:
UMA_HISTOGRAM_BOOLEAN(internal::kPageLoadCompletedAfterAppBackground, true);
On 2016/07/29 at 17:31:27, Alexei Svitkine (slow) wrote:
> Each histogram macro resolves to a lot of machine code.
> 
> To avoid the extra bloat, please make a helper function for logging this
histogram and call that.

Ah, right, thanks for reminding me about this. Done, thanks!

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

https://codereview.chromium.org/2189543002/diff/280001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:37831: +    enum="Boolean">
On 2016/07/29 at 17:31:27, Alexei Svitkine (slow) wrote:
> Please use a more specific enum, which you can define.

Sure, done.

[commit-bot: I haz the power, 2016-07-29 17:48: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, 2016-07-29 17:52:14]

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

[commit-bot: I haz the power, 2016-07-29 17:52:37]

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

[Alexei Svitkine (slow), 2016-07-29 17:57:11]

lgtm

[Bryan McQuade, 2016-07-29 19:32:13]

The CQ bit was unchecked by bmcquade@chromium.org

[Bryan McQuade, 2016-07-29 19:32:21]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2016-07-29 19:32:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2189543002/#ps340001
(title: "fix histogram description")

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

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

[commit-bot: I haz the power, 2016-07-29 21:07:58]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-07-29 21:09:17]

Description was changed from

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

On Android (and iOS), when an app enters the background, it may be
killed without further notifications. Thus, having this callback
will allow us to persist metrics before the app may be killed.
Our page load metrics subsystem isn't used on iOS, so we only target
Android in this change.

Unfortunately, we can't use the existing WebContents hidden callback,
as this gets invoked after the UMA subsystem has already flushed
metrics to disk. Thus, we need to add this explicit callback which
allows us to persist metrics as part of the UMA subsystem handling
the backgrounding callback.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360
==========

to

==========
Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

On Android (and iOS), when an app enters the background, it may be
killed without further notifications. Thus, having this callback
will allow us to persist metrics before the app may be killed.
Our page load metrics subsystem isn't used on iOS, so we only target
Android in this change.

Unfortunately, we can't use the existing WebContents hidden callback,
as this gets invoked after the UMA subsystem has already flushed
metrics to disk. Thus, we need to add this explicit callback which
allows us to persist metrics as part of the UMA subsystem handling
the backgrounding callback.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG=608360

Committed: https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240
Cr-Commit-Position: refs/heads/master@{#408744}
==========

[commit-bot: I haz the power, 2016-07-29 21:09:19]

Patchset 18 (id:??) landed as
https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240
Cr-Commit-Position: refs/heads/master@{#408744}