[cros] Add --stub-cros-settings option for testing.

[cros] Add --stub-cros-settings option for testing.

With this option, the current user is always treated as owner and all signed settings are stored in memory without signing.
All CrosSettingProviders now use a callback to notify CrosSettings observers.


BUG=None
TEST=StubCrosSettingsProviderTest.*; manual: see CL description

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

[Ivan Korotkov, 2011-12-09 17:32:47]

PTAL
pastarmovj: *CrosSettings
nkostylev: OwnershipService/UserManager

[Nikita (slow), 2011-12-12 11:04:57]

lgtm

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/cros_se...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/cros_se...
chrome/browser/chromeos/cros_settings.cc:270: switches::kStubCrosSettings))
nit: Multiline condition requires { } block.

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/login/o...
File chrome/browser/chromeos/login/ownership_service.h (right):

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/login/o...
chrome/browser/chromeos/login/ownership_service.h:132: // If true, current user
is regarded as owner (for testing only).
nit: insert empty line before comment.

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/stub_cr...
File chrome/browser/chromeos/stub_cros_settings_provider.cc (right):

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/stub_cr...
chrome/browser/chromeos/stub_cros_settings_provider.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Please add unittest for this class.

[Ivan Korotkov, 2011-12-13 13:15:09]

Unit-tests added, with necessary changes to CrosSettingsProviders to make
FireObservers mocking possible.

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/cros_se...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/cros_se...
chrome/browser/chromeos/cros_settings.cc:270: switches::kStubCrosSettings))
On 2011/12/12 11:04:57, Nikita Kostylev wrote:
> nit: Multiline condition requires { } block.

Done.

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/login/o...
File chrome/browser/chromeos/login/ownership_service.h (right):

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/login/o...
chrome/browser/chromeos/login/ownership_service.h:132: // If true, current user
is regarded as owner (for testing only).
On 2011/12/12 11:04:57, Nikita Kostylev wrote:
> nit: insert empty line before comment.

Done.

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/stub_cr...
File chrome/browser/chromeos/stub_cros_settings_provider.cc (right):

http://codereview.chromium.org/8899002/diff/1/chrome/browser/chromeos/stub_cr...
chrome/browser/chromeos/stub_cros_settings_provider.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2011/12/12 11:04:57, Nikita Kostylev wrote:
> Please add unittest for this class.

Done.

[pastarmovj, 2011-12-13 13:41:06]

LGTM the change to CrosSettingsProvider and children with two  nits.

I have a generic question though. This flag seems a bit dangerous to me because
it allows to circumvent policies on the device. Is it fine to have such a
mechanism compiled in release builds too?

http://codereview.chromium.org/8899002/diff/7001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8899002/diff/7001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:277: AddSettingsProvider(new
SystemSettingsProvider(notify_cb));
You might even want to put the real providers in an else clause to avoid having
to reload them which might not work well in test environment without proper
mocking.

http://codereview.chromium.org/8899002/diff/7001/chrome/browser/chromeos/stub...
File chrome/browser/chromeos/stub_cros_settings_provider_unittest.cc (right):

http://codereview.chromium.org/8899002/diff/7001/chrome/browser/chromeos/stub...
chrome/browser/chromeos/stub_cros_settings_provider_unittest.cc:82: 
Maybe one more test for Get would be good where you check Get both existing and
non existing keys.

[Ivan Korotkov, 2011-12-13 14:46:16]

I think this is OK since Chrome is not supposed to be started with arbitrary
flags (i.e. user cannot change them) on an official image, and on developer
images the user has any control he wants anyways.

http://codereview.chromium.org/8899002/diff/7001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8899002/diff/7001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:277: AddSettingsProvider(new
SystemSettingsProvider(notify_cb));
On 2011/12/13 13:41:07, pastarmovj wrote:
> You might even want to put the real providers in an else clause to avoid
having
> to reload them which might not work well in test environment without proper
> mocking.

Done for Device provider (System provider is not mocked for now anyway).

http://codereview.chromium.org/8899002/diff/7001/chrome/browser/chromeos/stub...
File chrome/browser/chromeos/stub_cros_settings_provider_unittest.cc (right):

http://codereview.chromium.org/8899002/diff/7001/chrome/browser/chromeos/stub...
chrome/browser/chromeos/stub_cros_settings_provider_unittest.cc:82: 
On 2011/12/13 13:41:07, pastarmovj wrote:
> Maybe one more test for Get would be good where you check Get both existing
and
> non existing keys.

Done.