Gather stability prefs into managing objects.

Gather stability prefs into managing objects.

This collects code reading and writing stability prefs from
MetricsLog and MetricsService into StabilityMetricsProvider
and EnvironmentRecorder objects.

Also removes the obsolete stability prefs which are now unneeded.

BUG=693676
Review-Url: https://codereview.chromium.org/2687393004
Cr-Commit-Position: refs/heads/master@{#452301}
Committed: https://chromium.googlesource.com/chromium/src/+/1bf273c8b89a8809183f6a6c93ebc1a1b935a8ee

[Steven Holte, 2017-02-11 01:10:56]

Description was changed from

==========
Gather stability prefs into managing objects.

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

to

==========
Gather stability prefs into managing objects.

This collects code reading and writing stability prefs from
MetricsLog and MetricsService into StabilityMetricsProvider
and EnvironmentRecorder objects.

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

[Steven Holte, 2017-02-11 01:10:57]

holte@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Steven Holte, 2017-02-11 01:11:59]

Description was changed from

==========
Gather stability prefs into managing objects.

This collects code reading and writing stability prefs from
MetricsLog and MetricsService into StabilityMetricsProvider
and EnvironmentRecorder objects.

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

to

==========
Gather stability prefs into managing objects.

This collects code reading and writing stability prefs from
MetricsLog and MetricsService into StabilityMetricsProvider
and EnvironmentRecorder objects.

Also removes the obsolete stability prefs which are now unneeded.

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

[rkaplow, 2017-02-14 20:11:19]

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/clea...
File components/metrics/clean_exit_beacon.h (right):

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/clea...
components/metrics/clean_exit_beacon.h:12: class PrefRegistrySimple;
nit alpha

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
File components/metrics/environment_recorder.h (right):

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:19: // Stores and loads system
information to prefs for stability logs.
to and from prefs for ... ?

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:25: std::string
RecordEnvironmentToPrefs(
prefer some comments - not obvious what string this is returning. Looking at
implementation, maybe should also change the name to mention it's returning the
serialized env?

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:25: std::string
RecordEnvironmentToPrefs(
cna you also mention returns empty string if it was unable to be serialized

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:27: bool
LoadEnvironmentFromPrefs(SystemProfileProto* system_profile);
similar, mention what is this returning (and it's recording into the input
|system_profile|)

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:34: static void
RegisterPrefs(PrefRegistrySimple* registry);
I havn't done a ton of pref logic, so maybe this is how people refer to it, but
would InitializePrefs be a better name?

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/metr...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/metr...
components/metrics/metrics_service.cc:292:
RegisterMetricsProvider(std::unique_ptr<metrics::MetricsProvider>(
nit, add a blank line, looks like it's related to the install date comment

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/metr...
components/metrics/metrics_service.cc:623: // Stability bookkeeping
can you change this to a more helpful comment

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/stab...
File components/metrics/stability_metrics_provider.cc (right):

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/stab...
components/metrics/stability_metrics_provider.cc:56: // The server refuses data
that doesn't have certain values.  crashcount and
? Is this obsolete? Not sure if this is tring to refer to it being a required
field in the stability subproto... ?

[Steven Holte, 2017-02-15 22:42:10]

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/clea...
File components/metrics/clean_exit_beacon.h (right):

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/clea...
components/metrics/clean_exit_beacon.h:12: class PrefRegistrySimple;
On 2017/02/14 20:11:19, rkaplow wrote:
> nit alpha

Done.

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
File components/metrics/environment_recorder.h (right):

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:19: // Stores and loads system
information to prefs for stability logs.
On 2017/02/14 20:11:19, rkaplow wrote:
> to and from prefs for ... ?

Expanded comment.

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:25: std::string
RecordEnvironmentToPrefs(
On 2017/02/14 20:11:19, rkaplow wrote:
> cna you also mention returns empty string if it was unable to be serialized

Done.

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:25: std::string
RecordEnvironmentToPrefs(
On 2017/02/14 20:11:19, rkaplow wrote:
> prefer some comments - not obvious what string this is returning. Looking at
> implementation, maybe should also change the name to mention it's returning
the
> serialized env?

Done.

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:27: bool
LoadEnvironmentFromPrefs(SystemProfileProto* system_profile);
On 2017/02/14 20:11:19, rkaplow wrote:
> similar, mention what is this returning (and it's recording into the input
> |system_profile|)

Done.

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/envi...
components/metrics/environment_recorder.h:34: static void
RegisterPrefs(PrefRegistrySimple* registry);
On 2017/02/14 20:11:19, rkaplow wrote:
> I havn't done a ton of pref logic, so maybe this is how people refer to it,
but
> would InitializePrefs be a better name?

No, this is a fairly common name.  It is describing the prefs to a registry,
rather than creating the data in the file.

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/metr...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/metr...
components/metrics/metrics_service.cc:292:
RegisterMetricsProvider(std::unique_ptr<metrics::MetricsProvider>(
On 2017/02/14 20:11:19, rkaplow wrote:
> nit, add a blank line, looks like it's related to the install date comment

Done.

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/metr...
components/metrics/metrics_service.cc:623: // Stability bookkeeping
On 2017/02/14 20:11:19, rkaplow wrote:
> can you change this to a more helpful comment

Done.

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/stab...
File components/metrics/stability_metrics_provider.cc (right):

https://codereview.chromium.org/2687393004/diff/60001/components/metrics/stab...
components/metrics/stability_metrics_provider.cc:56: // The server refuses data
that doesn't have certain values.  crashcount and
On 2017/02/14 20:11:19, rkaplow wrote:
> ? Is this obsolete? Not sure if this is tring to refer to it being a required
> field in the stability subproto... ?

After checking with Ilya, this comment dates back to when UMA data was sent as
XML, so it's very obsolete.  Updated the code to omit zero values.

[rkaplow, 2017-02-16 21:14:16]

lgtm

[Steven Holte, 2017-02-16 21:47:40]

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

[commit-bot: I haz the power, 2017-02-16 21:49:31]

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-02-16 21:55:52]

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

[commit-bot: I haz the power, 2017-02-16 21:55:53]

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...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Steven Holte, 2017-02-16 23:44:01]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2017-02-16 23:44:01]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2687393004/#ps100001
(title: "Rebase and fix tests")

[commit-bot: I haz the power, 2017-02-16 23:45:41]

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-02-16 23:56:13]

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

[commit-bot: I haz the power, 2017-02-16 23:56:14]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Steven Holte, 2017-02-17 01:19:56]

holte@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Steven Holte, 2017-02-17 01:19:57]

+asvitkine for chrome/browser/android/metrics/ OWNERS

[Steven Holte, 2017-02-17 01:20:03]

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

[commit-bot: I haz the power, 2017-02-17 01:21:43]

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

[commit-bot: I haz the power, 2017-02-17 01:35:22]

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

[commit-bot: I haz the power, 2017-02-17 01:35:23]

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

[Steven Holte, 2017-02-17 01:51:24]

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

[commit-bot: I haz the power, 2017-02-17 01:52:59]

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-02-17 02:05:47]

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

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

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

[Alexei Svitkine (slow), 2017-02-17 16:02:23]

Every non-trivial change should have a corresponding BUG= specified. Please do
this for this change as well as for future CLs, so that I don't need to point it
out again. :P

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/env...
File components/metrics/environment_recorder.cc (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/env...
components/metrics/environment_recorder.cc:61: 
Nit: Remove blank line.

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/env...
File components/metrics/environment_recorder.h (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/env...
components/metrics/environment_recorder.h:40: // Stores a buildtime and version
in prefs.
Please expand comment to mention what buildtime is.

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
File components/metrics/metrics_pref_names.cc (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
components/metrics/metrics_pref_names.cc:6: #include
"components/metrics/stability_pref_names.h"
Why is this needed?

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
components/metrics/metrics_service.cc:551: StabilityMetricsProvider
provider(local_state_);
Move this closer to where it's used.

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
components/metrics/metrics_service.cc:609: if (version_changed) {
Nit: No {}

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/sta...
File components/metrics/stability_metrics_provider.h (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/sta...
components/metrics/stability_metrics_provider.h:38: // Increment an Integer pref
value specified by |path|.
Increments.

[Steven Holte, 2017-02-17 19:18:12]

Description was changed from

==========
Gather stability prefs into managing objects.

This collects code reading and writing stability prefs from
MetricsLog and MetricsService into StabilityMetricsProvider
and EnvironmentRecorder objects.

Also removes the obsolete stability prefs which are now unneeded.

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

to

==========
Gather stability prefs into managing objects.

This collects code reading and writing stability prefs from
MetricsLog and MetricsService into StabilityMetricsProvider
and EnvironmentRecorder objects.

Also removes the obsolete stability prefs which are now unneeded.

BUG=693676
==========

[Steven Holte, 2017-02-17 20:21:33]

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/env...
File components/metrics/environment_recorder.cc (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/env...
components/metrics/environment_recorder.cc:61: 
On 2017/02/17 16:02:22, Alexei Svitkine (slow) wrote:
> Nit: Remove blank line.

Done.

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/env...
File components/metrics/environment_recorder.h (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/env...
components/metrics/environment_recorder.h:40: // Stores a buildtime and version
in prefs.
On 2017/02/17 16:02:22, Alexei Svitkine (slow) wrote:
> Please expand comment to mention what buildtime is.

Done.

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
File components/metrics/metrics_pref_names.cc (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
components/metrics/metrics_pref_names.cc:6: #include
"components/metrics/stability_pref_names.h"
On 2017/02/17 16:02:23, Alexei Svitkine (slow) wrote:
> Why is this needed?

Mostly I just felt it kept prefs organized.  I'll revert it for now, since it is
causing the CL to spill out of the component.

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
components/metrics/metrics_service.cc:551: StabilityMetricsProvider
provider(local_state_);
On 2017/02/17 16:02:23, Alexei Svitkine (slow) wrote:
> Move this closer to where it's used.

Done.

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
components/metrics/metrics_service.cc:609: if (version_changed) {
On 2017/02/17 16:02:23, Alexei Svitkine (slow) wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/sta...
File components/metrics/stability_metrics_provider.h (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/sta...
components/metrics/stability_metrics_provider.h:38: // Increment an Integer pref
value specified by |path|.
On 2017/02/17 16:02:23, Alexei Svitkine (slow) wrote:
> Increments.

Done.

[Steven Holte, 2017-02-17 20:21:39]

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

[commit-bot: I haz the power, 2017-02-17 20:23:20]

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), 2017-02-17 20:33:44]

lgtm

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
File components/metrics/metrics_pref_names.cc (right):

https://codereview.chromium.org/2687393004/diff/140001/components/metrics/met...
components/metrics/metrics_pref_names.cc:6: #include
"components/metrics/stability_pref_names.h"
On 2017/02/17 20:21:32, Steven Holte wrote:
> On 2017/02/17 16:02:23, Alexei Svitkine (slow) wrote:
> > Why is this needed?
> 
> Mostly I just felt it kept prefs organized.  I'll revert it for now, since it
is
> causing the CL to spill out of the component.

Sorry, I meant the include shouldn't be needed since I think this .cc file
doesn't reference the prefs in that header.

I'm not against splitting them. But I'm also OK if you want to revert it for now
like you did in the latest patch set.

[commit-bot: I haz the power, 2017-02-17 20:39:59]

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

[commit-bot: I haz the power, 2017-02-17 20:40:00]

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

[Steven Holte, 2017-02-21 20:22:03]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2017-02-21 20:22:04]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2687393004/#ps160001
(title: "Incorporate Feedback")

[commit-bot: I haz the power, 2017-02-21 20:23:11]

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-02-21 22:27:00]

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

[commit-bot: I haz the power, 2017-02-21 22:27:01]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Steven Holte, 2017-02-22 00:29:14]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2017-02-22 00:30:16]

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

[commit-bot: I haz the power, 2017-02-22 02:37:28]

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

[commit-bot: I haz the power, 2017-02-22 02:37:29]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Steven Holte, 2017-02-22 22:22:33]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2017-02-22 22:23:53]

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-02-23 00:22:33]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487802153718170,
"parent_rev": "6b9217880b3814ec933c74beb183375ec840cfaa", "commit_rev":
"1bf273c8b89a8809183f6a6c93ebc1a1b935a8ee"}

[commit-bot: I haz the power, 2017-02-23 00:23:18]

Description was changed from

==========
Gather stability prefs into managing objects.

This collects code reading and writing stability prefs from
MetricsLog and MetricsService into StabilityMetricsProvider
and EnvironmentRecorder objects.

Also removes the obsolete stability prefs which are now unneeded.

BUG=693676
==========

to

==========
Gather stability prefs into managing objects.

This collects code reading and writing stability prefs from
MetricsLog and MetricsService into StabilityMetricsProvider
and EnvironmentRecorder objects.

Also removes the obsolete stability prefs which are now unneeded.

BUG=693676

Review-Url: https://codereview.chromium.org/2687393004
Cr-Commit-Position: refs/heads/master@{#452301}
Committed:
https://chromium.googlesource.com/chromium/src/+/1bf273c8b89a8809183f6a6c93eb...
==========

[commit-bot: I haz the power, 2017-02-23 00:23:20]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/1bf273c8b89a8809183f6a6c93eb...