Change PrefStore::ReadResult to a boolean.

Change PrefStore::ReadResult to a boolean.

The third value in the enum (READ_USE_DEFAULT) isn't used anymore.

TBR=phajdan.jr@chromium.org,abodenha@chromium.org,tim@chromium.org
BUG=none

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

[Bernhard Bauer, 2012-11-07 21:34:41]

Please review.

[Mattias Nissler (ping if slow), 2012-11-08 10:29:58]

LGTM if you restore the CHECKs or convince me they should be DCHECKs.

http://codereview.chromium.org/11365112/diff/4001/base/prefs/default_pref_sto...
File base/prefs/default_pref_store.cc (right):

http://codereview.chromium.org/11365112/diff/4001/base/prefs/default_pref_sto...
base/prefs/default_pref_store.cc:12: DCHECK(!GetValue(key, NULL));
Why did you switch to a DCHECK here?

http://codereview.chromium.org/11365112/diff/4001/base/prefs/default_pref_sto...
base/prefs/default_pref_store.cc:17: DCHECK(GetValue(key, NULL));
ditto

[Bernhard Bauer, 2012-11-08 12:18:28]

http://codereview.chromium.org/11365112/diff/4001/base/prefs/default_pref_sto...
File base/prefs/default_pref_store.cc (right):

http://codereview.chromium.org/11365112/diff/4001/base/prefs/default_pref_sto...
base/prefs/default_pref_store.cc:12: DCHECK(!GetValue(key, NULL));
On 2012/11/08 10:29:58, Mattias Nissler wrote:
> Why did you switch to a DCHECK here?

It seemed like the sort of internal invariant that we usually verify with
DCHECKs. Nothing horrible happens if this is compiled out in a Release build.

[Mattias Nissler (ping if slow), 2012-11-08 12:35:17]

http://codereview.chromium.org/11365112/diff/4001/base/prefs/default_pref_sto...
File base/prefs/default_pref_store.cc (right):

http://codereview.chromium.org/11365112/diff/4001/base/prefs/default_pref_sto...
base/prefs/default_pref_store.cc:12: DCHECK(!GetValue(key, NULL));
On 2012/11/08 12:18:28, Bernhard Bauer wrote:
> On 2012/11/08 10:29:58, Mattias Nissler wrote:
> > Why did you switch to a DCHECK here?
> 
> It seemed like the sort of internal invariant that we usually verify with
> DCHECKs. Nothing horrible happens if this is compiled out in a Release build.

Well, not here, but this would imply registering a preference twice, which would
indicate something gets initialized twice, which is something I'd prefer to
learn the dramatic way ;)

I agree though that we should only have DCHECKs here, can we upgrade the DCHECKS
in PrefService::RegisterPreference() and PrefService::UnregisterPreference()
CHECKs instead? WDYT?

[Bernhard Bauer, 2012-11-08 14:03:24]

http://codereview.chromium.org/11365112/diff/4001/base/prefs/default_pref_sto...
File base/prefs/default_pref_store.cc (right):

http://codereview.chromium.org/11365112/diff/4001/base/prefs/default_pref_sto...
base/prefs/default_pref_store.cc:12: DCHECK(!GetValue(key, NULL));
On 2012/11/08 12:35:17, Mattias Nissler wrote:
> On 2012/11/08 12:18:28, Bernhard Bauer wrote:
> > On 2012/11/08 10:29:58, Mattias Nissler wrote:
> > > Why did you switch to a DCHECK here?
> > 
> > It seemed like the sort of internal invariant that we usually verify with
> > DCHECKs. Nothing horrible happens if this is compiled out in a Release
build.
> 
> Well, not here, but this would imply registering a preference twice, which
would
> indicate something gets initialized twice, which is something I'd prefer to
> learn the dramatic way ;)
> 
> I agree though that we should only have DCHECKs here, can we upgrade the
DCHECKS
> in PrefService::RegisterPreference() and PrefService::UnregisterPreference()
> CHECKs instead? WDYT?

Done.

[Mattias Nissler (ping if slow), 2012-11-08 14:12:31]

Thanks, LGTM.

[commit-bot: I haz the power, 2012-11-08 14:13:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11365112/9003

[commit-bot: I haz the power, 2012-11-08 14:14:01]

Presubmit check for 11365112-9003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/service
    chrome/test
    chrome/browser/sync

Presubmit checks took 1.8s to calculate.

[commit-bot: I haz the power, 2012-11-08 14:55:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11365112/9003

[commit-bot: I haz the power, 2012-11-08 18:24:38]

Change committed as 166706