Allow MetricsProviders to request an initial stability log.

Allow MetricsProviders to request an initial stability log.

BUG=412384
Committed: https://crrev.com/c179dd0632b2a6cc26f0016bdddfeea68e9d2a3d
Cr-Commit-Position: refs/heads/master@{#294182}

[Sigurður Ásgeirsson, 2014-09-09 15:14:19]

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

[Sigurður Ásgeirsson, 2014-09-09 15:14:19]

PTAL - first step towards adding a Windows browser watch.

[Alexei Svitkine (slow), 2014-09-09 15:34:28]

https://codereview.chromium.org/558653002/diff/1/components/metrics/metrics_s...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/558653002/diff/1/components/metrics/metrics_s...
components/metrics/metrics_service.cc:570: if
(!local_state_->GetBoolean(metrics::prefs::kStabilityExitedCleanly)) {
It's kind of confusing to have the check for this to be both in
ShouldPrepareStabilityLog() and here, since it relies on
ShouldPrepareStabilityLog() to check this pref which is not obvious by just
looking at the code here.

How about using the following code structure instead:

(in this function)

boolean exited_cleanly = true;
if (!local_state_->GetBoolean(metrics::prefs::kStabilityExitedCleanly)) {
  IncrementPrefValue(metrics::prefs::kStabilityCrashCount);
  local_state_->SetBoolean(metrics::prefs::kStabilityExitedCleanly, true);
  exited_cleanly = false;
}

if (!exited_cleanly || ProvidersHaveStabilityMetrics()) {
   ...
}

https://codereview.chromium.org/558653002/diff/1/components/metrics/metrics_s...
components/metrics/metrics_service.cc:585: // If the previous session didn't
exit cleanly, then prepare an initial
Update comment.

[Sigurður Ásgeirsson, 2014-09-09 17:13:40]

Thanks - please take another look.

https://codereview.chromium.org/558653002/diff/1/components/metrics/metrics_s...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/558653002/diff/1/components/metrics/metrics_s...
components/metrics/metrics_service.cc:570: if
(!local_state_->GetBoolean(metrics::prefs::kStabilityExitedCleanly)) {
On 2014/09/09 15:34:28, Alexei Svitkine wrote:
> It's kind of confusing to have the check for this to be both in
> ShouldPrepareStabilityLog() and here, since it relies on
> ShouldPrepareStabilityLog() to check this pref which is not obvious by just
> looking at the code here.
> 
> How about using the following code structure instead:
> 
> (in this function)
> 
> boolean exited_cleanly = true;
> if (!local_state_->GetBoolean(metrics::prefs::kStabilityExitedCleanly)) {
>   IncrementPrefValue(metrics::prefs::kStabilityCrashCount);
>   local_state_->SetBoolean(metrics::prefs::kStabilityExitedCleanly, true);
>   exited_cleanly = false;
> }
> 
> if (!exited_cleanly || ProvidersHaveStabilityMetrics()) {
>    ...
> }

Yups, that's much clearer.

https://codereview.chromium.org/558653002/diff/1/components/metrics/metrics_s...
components/metrics/metrics_service.cc:585: // If the previous session didn't
exit cleanly, then prepare an initial
On 2014/09/09 15:34:28, Alexei Svitkine wrote:
> Update comment.

Done.

[Alexei Svitkine (slow), 2014-09-09 17:24:43]

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
File components/metrics/metrics_service.cc (left):

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service.cc:572: // monitoring.
Nit: Preserve the comment.

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
File components/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:37: TestMetricsProvider(bool
has_stability_metrics) :
Nit: explicit

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:42: bool HasStabilityMetrics()
OVERRIDE { return has_stability_metrics_; }
nit: virtual, same below

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:52: public:
private:

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:55: };
Add DISALLOW_COPY_AND_ASSIGN().

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:209: MetricsLog::GetBuildTime());
Nit: Align. Same below.

[Sigurður Ásgeirsson, 2014-09-09 19:27:00]

Thanks - one moar time?

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
File components/metrics/metrics_service.cc (left):

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service.cc:572: // monitoring.
On 2014/09/09 17:24:43, Alexei Svitkine wrote:
> Nit: Preserve the comment.

Oops, my bad. Done.

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
File components/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:37: TestMetricsProvider(bool
has_stability_metrics) :
On 2014/09/09 17:24:43, Alexei Svitkine wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:42: bool HasStabilityMetrics()
OVERRIDE { return has_stability_metrics_; }
On 2014/09/09 17:24:43, Alexei Svitkine wrote:
> nit: virtual, same below

Done.

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:52: public:
On 2014/09/09 17:24:43, Alexei Svitkine wrote:
> private:

Done.

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:55: };
On 2014/09/09 17:24:43, Alexei Svitkine wrote:
> Add DISALLOW_COPY_AND_ASSIGN().

Done.

https://codereview.chromium.org/558653002/diff/20001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:209: MetricsLog::GetBuildTime());
On 2014/09/09 17:24:43, Alexei Svitkine wrote:
> Nit: Align. Same below.

Done.

[Alexei Svitkine (slow), 2014-09-09 19:28:26]

lgtm

https://codereview.chromium.org/558653002/diff/40001/components/metrics/metri...
File components/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/558653002/diff/40001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:233: // The test provider should
have been called upon to provide stabilit
Nit: stability

[Sigurður Ásgeirsson, 2014-09-09 21:13:14]

Hey Alexei,

I got some style violation failures due to a non-empty virtual member body, so
had to add a .cc. Would you like me to move the other virtual methods to the .cc
while I'm at it?

Siggi

https://codereview.chromium.org/558653002/diff/40001/components/metrics/metri...
File components/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/558653002/diff/40001/components/metrics/metri...
components/metrics/metrics_service_unittest.cc:233: // The test provider should
have been called upon to provide stabilit
On 2014/09/09 19:28:26, Alexei Svitkine wrote:
> Nit: stability

Done.

[Alexei Svitkine (slow), 2014-09-09 21:22:22]

Sure, if we have to add a .cc file, might as well move all the method bodies
there.

Thanks!

[Sigurður Ásgeirsson, 2014-09-10 13:00:47]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-09-10 13:01:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/siggi@chromium.org/558653002/100001

[Sigurður Ásgeirsson, 2014-09-10 13:01:36]

On 2014/09/09 21:22:22, Alexei Svitkine wrote:
> Sure, if we have to add a .cc file, might as well move all the method bodies
> there.
> 
> Thanks!

Done - committing.

[commit-bot: I haz the power, 2014-09-10 15:02:05]

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

[commit-bot: I haz the power, 2014-09-10 15:02:05]

Exceeded time limit waiting for builds to trigger.

[Sigurður Ásgeirsson, 2014-09-10 15:03:54]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-09-10 15:04:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/siggi@chromium.org/558653002/100001

[commit-bot: I haz the power, 2014-09-10 16:58:55]

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

[commit-bot: I haz the power, 2014-09-10 16:58:56]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (None)

[Sigurður Ásgeirsson, 2014-09-10 17:00:25]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-09-10 17:01:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/siggi@chromium.org/558653002/100001

[commit-bot: I haz the power, 2014-09-10 17:05:13]

Committed patchset #6 (id:100001) as 12f308f24b53fb45bf6dc347581b12a11a02add1

[commit-bot: I haz the power, 2014-09-10 17:08:38]

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