Prevent GetDeviceId from invoking its callback multiple times in failure cases.

Prevent GetDeviceId from invoking its callback multiple times in failure cases.

Fix a bug in PrefMetricsService resulting from this issue occuring when RLZ is disabled on Windows.

BUG=322173
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236889

[gab, 2013-11-21 21:41:56]

Bernhard PTAL at c/b/prefs and Jeffrey PTAL at c/b/extensions

Thanks!
Gab

[Jeffrey Yasskin, 2013-11-21 22:23:22]

https://codereview.chromium.org/81683002/diff/1/chrome/browser/prefs/pref_met...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/81683002/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:263: static bool called = false;
This is dangerous because it's process-wide. How sure are you that the user will
never create a PrefMetricsService for more than one profile in the same process?

https://codereview.chromium.org/81683002/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:268: // Always expect a non-empty
device ID, except on Windows if RLZ is disabled.
This has too many negatives. Can you make it easier to understand?

https://codereview.chromium.org/81683002/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:276: called = true;
Move this just after the DCHECK(!called) to reduce the number of #ifs.

[gab, 2013-11-22 01:52:13]

Thanks, PTAL.

Gab

https://codereview.chromium.org/81683002/diff/1/chrome/browser/prefs/pref_met...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/81683002/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:263: static bool called = false;
On 2013/11/21 22:23:22, Jeffrey Yasskin wrote:
> This is dangerous because it's process-wide. How sure are you that the user
will
> never create a PrefMetricsService for more than one profile in the same
process?

Oops, indeed, this is currently only called for the first profile, but we need
to change this soon, so I'll do it correctly, thanks!

https://codereview.chromium.org/81683002/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:268: // Always expect a non-empty
device ID, except on Windows if RLZ is disabled.
On 2013/11/21 22:23:22, Jeffrey Yasskin wrote:
> This has too many negatives. Can you make it easier to understand?

Done.

https://codereview.chromium.org/81683002/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:276: called = true;
On 2013/11/21 22:23:22, Jeffrey Yasskin wrote:
> Move this just after the DCHECK(!called) to reduce the number of #ifs.

Done.

[Bernhard Bauer, 2013-11-22 09:55:02]

https://codereview.chromium.org/81683002/diff/160001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/81683002/diff/160001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.h:125: #if defined(ENABLE_DCHECK)
Does this work? It seems ENABLE_DCHECK is #undef'd again in logging.h
(https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...).
I'm not sure there is currently a #define that is defined iff DCHECKs are
compiled in :-(

[gab, 2013-11-22 21:05:13]

https://codereview.chromium.org/81683002/diff/160001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/81683002/diff/160001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.h:125: #if defined(ENABLE_DCHECK)
On 2013/11/22 09:55:03, Bernhard Bauer wrote:
> Does this work? It seems ENABLE_DCHECK is #undef'd again in logging.h
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...).
> I'm not sure there is currently a #define that is defined iff DCHECKs are
> compiled in :-(

Ah, good point... I think
#if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)
would do it (see patch set 3), but it feels like it relies on implementation
details too much...

I removed them for now (sadly keeping a class member around in official builds
which is only used for a DCHECK...).

Please let me know what you think.

Cheers,
Gab

[Jeffrey Yasskin, 2013-11-22 21:41:45]

lgtm for extensions

[Bernhard Bauer, 2013-11-22 21:53:55]

LGTM with the TODO as described below.

https://codereview.chromium.org/81683002/diff/160001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/81683002/diff/160001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.h:125: #if defined(ENABLE_DCHECK)
On 2013/11/22 21:05:13, gab wrote:
> On 2013/11/22 09:55:03, Bernhard Bauer wrote:
> > Does this work? It seems ENABLE_DCHECK is #undef'd again in logging.h
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...).
> > I'm not sure there is currently a #define that is defined iff DCHECKs are
> > compiled in :-(
> 
> Ah, good point... I think
> #if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)
> would do it (see patch set 3), but it feels like it relies on implementation
> details too much...
> 
> I removed them for now (sadly keeping a class member around in official builds
> which is only used for a DCHECK...).
> 
> Please let me know what you think.
> 
> Cheers,
> Gab

I think what we should do is add a #define for this.

For now, can you add a TODO to #ifdef the field out once we have a define? (And
possibly reference a bug)

[commit-bot: I haz the power, 2013-11-22 22:34:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/81683002/590015

[commit-bot: I haz the power, 2013-11-23 01:00:13]

Change committed as 236889