Add PLMObserver::FlushMetricsOnAppEnterBackground.

Add PLMObserver::FlushMetricsOnAppEnterBackground.

PLMObserver::FlushMetricsOnAppEnterBackground gets invoked when the metrics
subsystem is flushing metrics as part of the app entering the background.

Observers that buffer metrics information and flush it in the OnComplete
callback may want to implement this method, in order to avoid losing data
due to the application being killed after being backgrounded.

BUG=608360
Committed: https://crrev.com/6e7584a091cbb8078ed4289ffa739281bf170cbb
Cr-Commit-Position: refs/heads/master@{#421206}

[Bryan McQuade, 2016-09-26 18:53:59]

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

[commit-bot: I haz the power, 2016-09-26 18:54:59]

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

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

[commit-bot: I haz the power, 2016-09-26 19:02:42]

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-09-26 19:07:02]

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

[commit-bot: I haz the power, 2016-09-26 19:07:39]

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-09-26 19:09:46]

Description was changed from

==========
Add PLMO::FlushMetricsOnAppEnterBackground and a default implementation.

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

to

==========
Add PLMO::FlushMetricsOnAppEnterBackground.

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

[Bryan McQuade, 2016-09-26 19:42:34]

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

[commit-bot: I haz the power, 2016-09-26 19:42: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-09-26 20:02:37]

Description was changed from

==========
Add PLMO::FlushMetricsOnAppEnterBackground.

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

to

==========
Add PLMObserver::FlushMetricsOnAppEnterBackground.

PLMObserver::FlushMetricsOnAppEnterBackground gets invoked when the metrics
subsystem is flushing metrics as part of the app entering the background.

Observers that buffer metrics information and flush it in the OnComplete
callback may want to implement this method, in order to avoid losing data
due to the application being killed after being backgrounded.

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

[Bryan McQuade, 2016-09-26 20:15:56]

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

[Bryan McQuade, 2016-09-26 20:16:05]

PTAL, thanks!

[commit-bot: I haz the power, 2016-09-26 20:40:09]

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

[commit-bot: I haz the power, 2016-09-26 20:40:10]

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

[Charlie Harrison, 2016-09-26 20:56:39]

Looks good. Do you think the chrome:// patch should augment OnStart and OnCommit
to return this enum? If so that would require changing all consumers :/

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

https://codereview.chromium.org/2371883002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:137: enum
ObservePolicy {
Cool. This is a nice generalization of what I had :)

[Bryan McQuade, 2016-09-27 12:36:33]

On 2016/09/26 at 20:56:39, csharrison wrote:
> Looks good. Do you think the chrome:// patch should augment OnStart and
OnCommit to return this enum? If so that would require changing all consumers :/
> 
>
https://codereview.chromium.org/2371883002/diff/60001/chrome/browser/page_loa...
> File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):
> 
>
https://codereview.chromium.org/2371883002/diff/60001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/page_load_metrics_observer.h:137: enum
ObservePolicy {
> Cool. This is a nice generalization of what I had :)

Yeah, I think it makes sense to make OnStart and OnCommit return ObservePolicy
as well...

I think it might be worth adding those return values as their own CL and then
following with the rest of your chrome:// change. Why don't we split the work -
if you do OnStart I can do OnCommit. WDYT?

[Charlie Harrison, 2016-09-27 12:50:09]

That's fine by me. This looks good but per my comment I'm interested in if this
could be merged with the typical OnHidden callback to align behavior w/ tab
discarding work?

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

https://codereview.chromium.org/2371883002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:224: //
application goes into the background, on platforms where metrics are
s/are/should be?

https://codereview.chromium.org/2371883002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:225: // persisted
as part of backgrounding (Android). Implementers should persist
Note: there have been experiments where background tabs have been killed on
desktop too... Do you know if that experiment failed? I'm mostly now wondering
if this callback is redundant if that work ends up shipping.

[Charlie Harrison, 2016-09-27 13:05:52]

lgtm % comment nit. As discussed in person, app backgrounding is special and can
result in the browser process being killed. Tab discarding is just for the
renderer process.

[Bryan McQuade, 2016-09-27 13:17:08]

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

[Bryan McQuade, 2016-09-27 13:17:20]

Thanks! I updated the comment to be clearer.

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

https://codereview.chromium.org/2371883002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:137: enum
ObservePolicy {
On 2016/09/26 at 20:56:39, Charlie Harrison wrote:
> Cool. This is a nice generalization of what I had :)

Thanks for putting your change together! Glad we can generalize to support
multiple cases.

https://codereview.chromium.org/2371883002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:224: //
application goes into the background, on platforms where metrics are
On 2016/09/27 at 12:50:09, Charlie Harrison wrote:
> s/are/should be?

Updated the comment to be a clearer, thanks!

https://codereview.chromium.org/2371883002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:225: // persisted
as part of backgrounding (Android). Implementers should persist
On 2016/09/27 at 12:50:09, Charlie Harrison wrote:
> Note: there have been experiments where background tabs have been killed on
desktop too... Do you know if that experiment failed? I'm mostly now wondering
if this callback is redundant if that work ends up shipping.

Ah, I didn't know about this. I think we'd want to invoke this callback any time
the browser process may be killed w/o further notification. It sounds like tab
killing on desktop would not kill browser process, so we wouldn't want to invoke
this callback there, is my impression. I agree we should watch for changes and
if there are other platforms where browser process kills can happen, we should
try to invoke this callback on those platforms as well.

[commit-bot: I haz the power, 2016-09-27 13:17:22]

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

[commit-bot: I haz the power, 2016-09-27 14:35:04]

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

[commit-bot: I haz the power, 2016-09-27 14:35:05]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-09-27 14:37:28]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2016-09-27 14:37:29]

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/2371883002/#ps80001
(title: "fix comment")

[commit-bot: I haz the power, 2016-09-27 14:38:02]

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-09-27 14:43:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-27 14:44:45]

Description was changed from

==========
Add PLMObserver::FlushMetricsOnAppEnterBackground.

PLMObserver::FlushMetricsOnAppEnterBackground gets invoked when the metrics
subsystem is flushing metrics as part of the app entering the background.

Observers that buffer metrics information and flush it in the OnComplete
callback may want to implement this method, in order to avoid losing data
due to the application being killed after being backgrounded.

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

to

==========
Add PLMObserver::FlushMetricsOnAppEnterBackground.

PLMObserver::FlushMetricsOnAppEnterBackground gets invoked when the metrics
subsystem is flushing metrics as part of the app entering the background.

Observers that buffer metrics information and flush it in the OnComplete
callback may want to implement this method, in order to avoid losing data
due to the application being killed after being backgrounded.

BUG=608360

Committed: https://crrev.com/6e7584a091cbb8078ed4289ffa739281bf170cbb
Cr-Commit-Position: refs/heads/master@{#421206}
==========

[commit-bot: I haz the power, 2016-09-27 14:44:46]

Patchset 5 (id:??) landed as
https://crrev.com/6e7584a091cbb8078ed4289ffa739281bf170cbb
Cr-Commit-Position: refs/heads/master@{#421206}