Let chrome_prefs handle Preferences initialization from master_preferences.

Let chrome_prefs handle Preferences initialization from master_preferences.

chrome_pref_service_factory owns implementation details of how prefs+metadata should be stored, let it handle initialization of its prefs.

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

[gab, 2014-02-03 18:59:17]

Erik/Bernhard PTAL, as discussed offline.

Cheers!
Gab

[Bernhard Bauer, 2014-02-04 09:08:01]

https://codereview.chromium.org/145053004/diff/70001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/145053004/diff/70001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:287: virtual
~DictionaryPrefStore() {}
Nit: Make the destructor private? Protected implies to me that you're intending
to subclass it.

[gab, 2014-02-04 14:01:03]

Done, PTAL, thanks!

https://codereview.chromium.org/145053004/diff/70001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/145053004/diff/70001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:287: virtual
~DictionaryPrefStore() {}
On 2014/02/04 09:08:02, Bernhard Bauer wrote:
> Nit: Make the destructor private? Protected implies to me that you're
intending
> to subclass it.

Done.

[Bernhard Bauer, 2014-02-04 14:27:49]

lgtm

[gab, 2014-02-04 19:49:56]

Erik PTAL, added a first run test to cover this (passes now; fails without this
CL).

@noms for profiles/ OWNER.

Thanks!
Gab

[gab, 2014-02-04 19:52:55]

On 2014/02/04 19:49:56, gab wrote:
> Erik PTAL, added a first run test to cover this (passes now; fails without
this
> CL).
> 
> @noms for profiles/ OWNER.
> 

+Alexei for histograms.

> Thanks!
> Gab

[noms (inactive), 2014-02-04 20:36:35]

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:437: bool
ProfileImpl::InitializePrefsFromMasterPrefs(
As discussed offline, these prefs related methods would probably live a better
life if they got moved out of profiles and into somewhere else like
chrome_prefs. The only Profile thing (other than the profile path) this method
needs is GetPrefHashStore, and that doesn't seem to actually need any Profile
specific things, (other than that path)

[erikwright (departed), 2014-02-04 20:55:27]

Regarding noms@ comment, I am open minded but I think that knowledge of the path
to preferences files should be in one place only, so if we expose that to
chrome_prefs we should completely remove it from profile_impl.

Withholding stamp until I see how that is implemented.

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
File chrome/browser/first_run/first_run.h (right):

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run.h:165: // location, processes it, and hands
it to ProfileImpl so it becomes the default
I find "hands it to ProfileImpl" to be an implementation detail.

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:237: "  \"homepage\":
\"test.com\",\n"
use example.com

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:240: // A test fixture which
will run with in a first run scenario with
with in -> in

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:240: // A test fixture which
will run with in a first run scenario with
which -> that

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:241: // master_preferences set
to kWithTrackedPrefs. Parametrizable on the
parameterizable

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:251:
command_line->AppendSwitchASCII(switches::kForceFieldTrials,
I'd just break after (, indent by 4 spaces, and inline
force_settings_enforcement_trial

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:254: };
blank line after };

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:258: std::string tmp;
tmp is unused

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:260:
EXPECT_FALSE(user_prefs->GetBoolean(prefs::kHomePageIsNewTabPage));
Can you add a test that the default is true, or something?

[gab, 2014-02-04 22:32:10]

Addressed comments in patch set 6.

Rebased on top of https://codereview.chromium.org/154593002/ in patch set 5 to
address noms's comment.

noms to CC since this CL no longer touches c/b/profiles/ :)

Cheers!
Gab

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
File chrome/browser/first_run/first_run.h (right):

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run.h:165: // location, processes it, and hands
it to ProfileImpl so it becomes the default
On 2014/02/04 20:55:28, erikwright wrote:
> I find "hands it to ProfileImpl" to be an implementation detail.

Done.

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:237: "  \"homepage\":
\"test.com\",\n"
On 2014/02/04 20:55:28, erikwright wrote:
> use http://example.com

Done.

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:240: // A test fixture which
will run with in a first run scenario with
On 2014/02/04 20:55:28, erikwright wrote:
> with in -> in

Done.

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:240: // A test fixture which
will run with in a first run scenario with
On 2014/02/04 20:55:28, erikwright wrote:
> which -> that

Done.

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:241: // master_preferences set
to kWithTrackedPrefs. Parametrizable on the
On 2014/02/04 20:55:28, erikwright wrote:
> parameterizable

Done.

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:251:
command_line->AppendSwitchASCII(switches::kForceFieldTrials,
On 2014/02/04 20:55:28, erikwright wrote:
> I'd just break after (, indent by 4 spaces, and inline
> force_settings_enforcement_trial

Done.

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:254: };
On 2014/02/04 20:55:28, erikwright wrote:
> blank line after };

Done.

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:258: std::string tmp;
On 2014/02/04 20:55:28, erikwright wrote:
> tmp is unused

Oops, got too used to the DictionaryValue::Get interface :) -- turns out
PrefService doesn't need an out-param for its getters!

https://codereview.chromium.org/145053004/diff/350001/chrome/browser/first_ru...
chrome/browser/first_run/first_run_browsertest.cc:260:
EXPECT_FALSE(user_prefs->GetBoolean(prefs::kHomePageIsNewTabPage));
On 2014/02/04 20:55:28, erikwright wrote:
> Can you add a test that the default is true, or something?

Done.

[Alexei Svitkine (slow), 2014-02-05 15:36:14]

histograms lgtm

[gab, 2014-02-05 22:44:32]

@Erik: PTAL.

Thanks!
Gab

[erikwright (departed), 2014-02-07 16:26:20]

LGTM.

[gab, 2014-02-07 16:28:51]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-02-07 16:29:42]

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

[commit-bot: I haz the power, 2014-02-07 18:38:11]

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

[commit-bot: I haz the power, 2014-02-07 18:38:11]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[gab, 2014-02-07 18:40:21]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-02-07 18:41:03]

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

[commit-bot: I haz the power, 2014-02-07 20:32:45]

Change committed as 249759