Explicitly check reporting state when returning the entropy source.

Explicitly check reporting state when returning the entropy source.

We used to CHECK if the client ID was set but we were not reporting metrics, but this is not technically guaranteed since the user can enable or disable UMA without changing the locally stored client ID.

BUG=none
TEST=Ensure that enabling and disabling UMA in an official build does not crash Chrome.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138999

[SteveT, 2012-05-25 01:48:24]

This should do it. Let me know if you don't like ternary statements :)


Steve

[jar (doing other things), 2012-05-25 02:10:26]

drive by.....

https://chromiumcodereview.appspot.com/10452027/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/10452027/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:476: low_entropy_source;
nit: re: ternary (which you mentioned):  IMO, by the time you can't even fit the
statement on one line, you probably knew it was on the edge... or beyond ;-)

There is no style guide formatting for such wrapping, and I'd guess it is
because it is rarely done.  Looking at the codebase, if you had to do this (or
had a good reason),  I think the preferred format is to align the "?" with the 
":" on the next line.

...but a simple:
if (foo)
  return a;
return b;

would be much faster to read (a lot less scanning back and forth).

bottom line: Suggest changing to if() statement as shown.

[Ilya Sherman, 2012-05-25 02:49:38]

LGTM with Jim's nit addressed.

[Ilya Sherman, 2012-05-25 02:50:09]

Actually, one more nit:

https://chromiumcodereview.appspot.com/10452027/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/10452027/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:468: // Note that client_id_ is empty
if metrics reporting is not enabled. For
nit: This comment should be updated, too.

[SteveT, 2012-05-25 04:37:02]

This was a lesson I should have learned from a past Jim-review :)

Okay! Will commit shortly.

http://codereview.chromium.org/10452027/diff/1/chrome/browser/metrics/metrics...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/10452027/diff/1/chrome/browser/metrics/metrics...
chrome/browser/metrics/metrics_service.cc:468: // Note that client_id_ is empty
if metrics reporting is not enabled. For
On 2012/05/25 02:50:09, Ilya Sherman wrote:
> nit: This comment should be updated, too.

Done.

http://codereview.chromium.org/10452027/diff/1/chrome/browser/metrics/metrics...
chrome/browser/metrics/metrics_service.cc:476: low_entropy_source;
On 2012/05/25 02:10:26, jar wrote:
> nit: re: ternary (which you mentioned):  IMO, by the time you can't even fit
the
> statement on one line, you probably knew it was on the edge... or beyond ;-)
> 
> There is no style guide formatting for such wrapping, and I'd guess it is
> because it is rarely done.  Looking at the codebase, if you had to do this (or
> had a good reason),  I think the preferred format is to align the "?" with the

> ":" on the next line.
> 
> ...but a simple:
> if (foo)
>   return a;
> return b;
> 
> would be much faster to read (a lot less scanning back and forth).
> 
> bottom line: Suggest changing to if() statement as shown.

Done.

[commit-bot: I haz the power, 2012-05-25 04:37:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10452027/6002

[commit-bot: I haz the power, 2012-05-25 05:45:44]

Change committed as 138999