Reinitialize LocalState's CommandLinePrefStore after about_flags updates the command line.

Reinitialize LocalState's CommandLinePrefStore after about_flags updates the command line. Otherwise flags set in about:flags are ignored by the LocalState.

BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=118926

[Lei Zhang, 2012-01-19 00:23:26]

It looks like switches set in about:flags that manipulate pref values in the
Local State aren't being honored.

https://chromiumcodereview.appspot.com/9251026/diff/8/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/9251026/diff/8/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1250:
about_flags::ConvertFlagsToSwitches(local_state_,
We have a circular dependency here, since ConvertFlagsToSwitches() needs
|local_state_|, and |local_state_| creates its CommandLinePrefStore before
ConvertFlagsToSwitches() has a chance to update the command line.

UpdateCommandLinePrefStore() is my minimal hack to break this circular
dependency.

[Nico, 2012-01-19 00:39:18]

This looks much better than I thought it would.

Since you don't have a BUG=, I think you should include your motivation in the
CL description (basically what you told me over gtalk).

Maybe you should add the first paragraph of your rietveld comment above as
comment to the code, not sure.

Other than that, LGTM. Two nits below.

https://chromiumcodereview.appspot.com/9251026/diff/8/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/9251026/diff/8/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_unittest.cc:156: const CommandLine::CharType*
argv[] = { FILE_PATH_LITERAL("program") };
I think the usual pattern is

CommandLine command_line(CommandLine::NO_PROGRAM);
command_line.Append...

https://chromiumcodereview.appspot.com/9251026/diff/8/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_unittest.cc:174: 
Do you want to check if ConvertFlagsToSwiches() added something to cmd_line
here? (I think it would make the test a bit more readable)

[Lei Zhang, 2012-01-19 01:10:29]

https://chromiumcodereview.appspot.com/9251026/diff/8/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/9251026/diff/8/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_unittest.cc:156: const CommandLine::CharType*
argv[] = { FILE_PATH_LITERAL("program") };
On 2012/01/19 00:39:18, Nico wrote:
> I think the usual pattern is
> 
> CommandLine command_line(CommandLine::NO_PROGRAM);
> command_line.Append...

Done.

https://chromiumcodereview.appspot.com/9251026/diff/8/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_unittest.cc:174: 
On 2012/01/19 00:39:18, Nico wrote:
> Do you want to check if ConvertFlagsToSwiches() added something to cmd_line
> here? (I think it would make the test a bit more readable)

Actually, i don't need about_flags in this test, since I already manually
appended the switch.

[battre, 2012-01-19 09:52:20]

The PrefValueStore was designed to be immutable. I think the approach is
reasonable, but could you please add a couple of safe guards? I have indicated
the relevant places in the comments.

Thanks for the clean ups!

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
File chrome/browser/prefs/pref_service.cc (right):

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_service.cc:182: PrefStore* incognito_extension_prefs)
{
please add something like
pref_service_forked_ = true;

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_service.cc:206: PrefService*
PrefService::CreatePrefServiceWithPerTabPrefStore() {
please add something like
pref_service_forked_ = true;

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_service.cc:886: void
PrefService::UpdateCommandLinePrefStore(CommandLine* command_line) {
Could you please add a DCHECK to verify that no IncognitoPrefService has been
created yet? Otherwise, it would not be aware of the new command line pref
store.

--> DCHECK(!pref_service_forked_);

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
File chrome/browser/prefs/pref_service.h (right):

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_service.h:300: // Tell our PrefValueStore to update
itself using |command_line|.
Please add a comment that you must not call this after having derived an
incognito or pertab pref service.

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
File chrome/browser/prefs/pref_value_store.cc (right):

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_value_store.cc:29: pref_store_->RemoveObserver(this);
could you please add a DCHECK that verifies that no observers are left after
this? Otherwise, these observers would not notice that the pref_store_ is
replaced.

[Lei Zhang, 2012-01-19 20:33:59]

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
File chrome/browser/prefs/pref_service.cc (right):

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_service.cc:182: PrefStore* incognito_extension_prefs)
{
On 2012/01/19 09:52:21, battre wrote:
> please add something like
> pref_service_forked_ = true;

Done.

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_service.cc:206: PrefService*
PrefService::CreatePrefServiceWithPerTabPrefStore() {
On 2012/01/19 09:52:21, battre wrote:
> please add something like
> pref_service_forked_ = true;

Done.

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_service.cc:886: void
PrefService::UpdateCommandLinePrefStore(CommandLine* command_line) {
On 2012/01/19 09:52:21, battre wrote:
> Could you please add a DCHECK to verify that no IncognitoPrefService has been
> created yet? Otherwise, it would not be aware of the new command line pref
> store.
> 
> --> DCHECK(!pref_service_forked_);

Done.

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
File chrome/browser/prefs/pref_service.h (right):

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_service.h:300: // Tell our PrefValueStore to update
itself using |command_line|.
On 2012/01/19 09:52:21, battre wrote:
> Please add a comment that you must not call this after having derived an
> incognito or pertab pref service.

Done.

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
File chrome/browser/prefs/pref_value_store.cc (right):

https://chromiumcodereview.appspot.com/9251026/diff/5003/chrome/browser/prefs...
chrome/browser/prefs/pref_value_store.cc:29: pref_store_->RemoveObserver(this);
On 2012/01/19 09:52:21, battre wrote:
> could you please add a DCHECK that verifies that no observers are left after
> this? Otherwise, these observers would not notice that the pref_store_ is
> replaced.

Done.

[Lei Zhang, 2012-01-23 20:40:41]

ping

[battre, 2012-01-23 20:44:54]

Sorry, forgot about the CL.

LGTM

[commit-bot: I haz the power, 2012-01-23 20:56:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/9251026/8001

[commit-bot: I haz the power, 2012-01-23 20:56:37]

Presubmit check for 9251026-8001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/policy/configuration_policy_pref_store.h,chrome/browser/policy/configuration_policy_pref_store.cc

Presubmit checks took 2.2s to calculate.

[Lei Zhang, 2012-01-23 21:04:04]

+joaodasilva for chrome/browser/policy OWNERS

[Joao da Silva, 2012-01-24 10:39:45]

policy LGTM

[commit-bot: I haz the power, 2012-01-24 19:56:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/9251026/8001