PART1: Initiated the SignedSettings refactoring.

Initiated the SignedSettings refactoring.

This CL introdcues the front-end change which will enforce all clients of
signed settings to go through the CrosSettings class which mimics a normal
PrefService and will hide the gory details of the underlying layer.
The only difference being the extra GetTrusted function which will ensure
that fresh data from the policy blob has been fetched before returning the
cached value.

BUG=chromium-os:14054
TEST=All existing signed settings tests should still be green.


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

[pastarmovj, 2011-09-20 12:34:08]

This is the first part of the refactoring. It gets rid of all the cached_xxxx
and GetTrustedXXX methods and streamlines the whole access to device settings
through the CrosSettings class.

Next on the menu will be to refactor all the UserWhitelist calls to use the
normal policy path for setting and not some fancy roundabouts.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/timezone_settings.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/timezone_settings.cc:15: #include
"content/browser/browser_thread.h"
Ignore this file when reviewing it is part of this CL (
http://codereview.chromium.org/7904012/ ) and is here only to prevent Chrome
from crashing.

[Nikita (slow), 2011-09-20 13:11:19]

Adding Denis Lagno as an additional reviewer.

[Denis Lagno, 2011-09-20 14:05:22]

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:27: #include
"chrome/browser/chromeos/network_login_observer.h"
nit: ASCII order

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.cc (left):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:8: #include "base/stl_util.h"
you seem to use STLDeleteElements yet you remove stl_util header?

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

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:163: if (provider) {
nit: we usually do not put curly braces for one-liners

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.h:188:
ScopedRunnableMethodFactory<ExistingUserController> method_factory_;
you seem to replace ScopedFactory with base::Bind magic.
I am not sure if it safe, but if it is then factory should be removed.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:18: #include
"chrome/browser/chromeos/cros_settings.h"
nit: / precedes _ in ASCII order.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:20: #include
"chrome/browser/chromeos/cros_settings.h"
nit (here and in other headers): / precedes _ in ASCII order.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:202:
base::Bind(&RemoveAttempt::RemoveUser, base::Unretained(this)));
is it safe to replace ScopedMethodFactory by this?
AFAIK the former traces lifetime of an object?

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.h (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.h:44: static void
WhitelistUser(const std::string& email);
Is it good making them static?

Also, you forgot to mark implementation of those methods with //static comment.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:83:
reinterpret_cast<chromeos::SystemSettingsProvider*>(
btw, why not static_cast here?

[pastarmovj, 2011-09-20 17:11:52]

Thanks for reviewing this Denis!

I addressed the comments and as a bonus changed all NewRunnable* to base::Binds
in the affected files too. Had to refactor the Authenticator file on the way too
(I will pull out this small change as a separate CL to keep this one more
focused tomorrow).

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:27: #include
"chrome/browser/chromeos/network_login_observer.h"
On 2011/09/20 14:05:22, Denis Lagno wrote:
> nit: ASCII order

Done.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.cc (left):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:8: #include "base/stl_util.h"
On 2011/09/20 14:05:22, Denis Lagno wrote:
> you seem to use STLDeleteElements yet you remove stl_util header?

True. I must have removed this before I added the destructor code. Sorry.

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

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:152: if
(providers_[i]->HandlesSetting(path)) {
Removed the curlies here as well.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:163: if (provider) {
On 2011/09/20 14:05:22, Denis Lagno wrote:
> nit: we usually do not put curly braces for one-liners

Done.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:166: return false;
And this should be NULL. Baaad copy/paste.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.h:188:
ScopedRunnableMethodFactory<ExistingUserController> method_factory_;
You are right base::Bind by itself does no magic. What I had to do and did all
over the place is replace the ScopedRunnableMethodFactory by a WeakPtrFactory. I
ill do this for the other similar situations too.

On 2011/09/20 14:05:22, Denis Lagno wrote:
> you seem to replace ScopedFactory with base::Bind magic.
> I am not sure if it safe, but if it is then factory should be removed.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:18: #include
"chrome/browser/chromeos/cros_settings.h"
On 2011/09/20 14:05:22, Denis Lagno wrote:
> nit: / precedes _ in ASCII order.

Done.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:267:
base::Bind(&LoginPerformer::CompleteLogin, base::Unretained(this),
Fixed those too.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:20: #include
"chrome/browser/chromeos/cros_settings.h"
On 2011/09/20 14:05:22, Denis Lagno wrote:
> nit (here and in other headers): / precedes _ in ASCII order.

Done.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:202:
base::Bind(&RemoveAttempt::RemoveUser, base::Unretained(this)));
On 2011/09/20 14:05:22, Denis Lagno wrote:
> is it safe to replace ScopedMethodFactory by this?
> AFAIK the former traces lifetime of an object?

Done.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.h (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.h:44: static void
WhitelistUser(const std::string& email);
I intend on getting rid of the special handling of Whitelists in favor of native
ListValue operations as these are now not a special citizens of the signed
settings world.  So I see this only as a transitional solution until I replace
those calls with CrosSettings functions to set ListValues correctly.

On 2011/09/20 14:05:22, Denis Lagno wrote:
> Is it good making them static?
> 
> Also, you forgot to mark implementation of those methods with //static
comment.

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.cc (right):

http://codereview.chromium.org/7867044/diff/8001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:83:
reinterpret_cast<chromeos::SystemSettingsProvider*>(
On 2011/09/20 14:05:22, Denis Lagno wrote:
> btw, why not static_cast here?

Done.

[Mattias Nissler (ping if slow), 2011-09-21 11:12:59]

some comments :)

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:72:
chromeos::ProxyCrosSettingsProvider settings_provider;
why aren't we going through CrosSettings here?

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

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:183: *out_value = CreateSettingsValue(
It seems like wrapping stuff in a DictionaryValue is not a good idea, given that
IsEnterpriseManaged() and current_user_is_owner() are constant over all values.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:185:
g_browser_process->browser_policy_connector()->IsEnterpriseManaged(),
I dislike the global here, it complicates testing.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:207: const Value* value =
GetPref(path);
value is leaked (also below)

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:209: return
value->GetAsBoolean(bool_value);
indentation (also below)

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:34: // Helper function to test if given
path is a value cros settings name.
why is this public?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:42: void FireObservers(const char*
path);
why is this public?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:44: // Gets settings value of given
|path| to |out_value|.
So there's no way of distinguishing "not set" from "not available"?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:55: const
CrosSettingsProvider::Callback& callback) const;
Clients shouldn't need to depend on CrosSettingsProvider. Can we declare the
callback type in CrosSettings?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings_provider.h (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_provider.h:30: // Note that |out_value| is
owned by the caller, not this class.
Is there a technical reason for this? The interface would be easier to use if we
didn't have to worry about deleting the returned value.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:99:
&show_users_on_signin);
We have a long-standing bug about this value being read to early and the UI not
reflecting the actual device setting once we retrieved it. I had hoped the
refactoring will also address this or at least lay some groundwork for solving
this. One option could be to built a setting change observer mechanism into
CrosSettings that we could use here to register for changes (or the event of
data being loaded from session_manager in this case). Not necessarily something
for this CL, but worth considering in the design.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:274: username, password));
Doing the same thing twice to be extra-sure?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:283:
cros_settings->GetBoolean(kAccountsPrefAllowNewUser, &allow_new_user);
no guarantee that you have trusted values here if
ScreenLocker::default_screen_locker() is NULL. Is that intentional?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:323: username, password));
Same two identical things again. Also, this is pretty inconvenient to use if you
want to check several different prefs. Have you considered decoupling the
GetTrusted() check from the actual pref path? I.e. introduce a way to be
notified whenever we learn that all settings are trusted?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:332:
cros_settings->GetBoolean(kAccountsPrefAllowNewUser, &allow_new_user);
same comment regarding trustedness as above.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:134:
CrosSettings::Get()->GetBoolean(kAccountsPrefAllowGuest, &allow_guest);
same comment as before regarding the problem of reading this setting to early
and not being able to change our minds once we have retrieved the trusted value.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_controller.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.cc:138:
is_owner_(VerifyOwnership(user.email())),
same comment as before regarding the inaccuracy of the value saved here.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:315: // definitive
source for them.
This is wrong, since ProxyConfigServiceImpl does asynchronous initialization.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_cros_settings_provider.h (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_provider.h:18:
ProxyCrosSettingsProvider();
new line before comment

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:576: PrefService* prefs =
g_browser_process->local_state();
Have you considered moving the cache into a single dictionary pref into local
state? Also, have you considered removing the persistent cache entirely? The
upside of this would be that people would suddenly be forced to write code that
properly reacts to changing device settings. I guess we'd run into a problem
with the crash reporting pref as set in the eula screen possibly getting lost
then though...

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:612: void
UserCrosSettingsProvider::WhitelistUser(const std::string& email) {
These implementations still bypass CrosSettings.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/cras...
File chrome/browser/ui/webui/crashes_ui.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/cras...
chrome/browser/ui/webui/crashes_ui.cc:184: NOTREACHED() << "Error occurred while
accessing the pref value!";
This seems awkward. Is there any code actually doing useful when the return
value is false? If not, can we just move the DCHECK() into
CrosSettings::GetPref()? This would also more closely resemble PrefService,
which DCHECKs if you ask for the value of a pref that hasn't been registered.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/about_page_handler.h (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/about_page_handler.h:12: #include
"chrome/browser/ui/webui/options/options_ui.h"
including the same thing twice just to be sure?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:85:
chromeos::kSystemTimezone))->GetTimezoneList());
shouldn't this also go through CrosSettings?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc
(right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:216: //
TODO(pastarmovj): This will leak now if the callee doesn't delete it.
Isn't passing ownership to the caller the contract of this?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:217: // We
should anyhow cache this in the local_state.
If we're caching for all providers, should we move that logic into CrosSettings
so it doesn't need to be reimplemented over and over again?

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_settings_provider.h
(right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.h:33: 
extra newline

[Denis Lagno, 2011-09-22 13:54:05]

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:245:
pointer_factory_.GetWeakPtr()));
Are you really sure that this one still works as expected?
AFAIU WeakPtr turns to NULL when object is destructed.
I am not sure that code that calls callback checks that weak pointer to object
is not NULL.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:392: bool
is_owner = email == owner;
nit: seems better to put additional parenthesis for readability.

[pastarmovj, 2011-09-23 15:19:32]

Addressed comments.
I will have one more issue to solve - users screen in settings does not show the
list of users and I am pretty sure some tests will need tuning but I will be
glad to have a comment on my responses and fixes to your comments meanwhile.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:72:
chromeos::ProxyCrosSettingsProvider settings_provider;
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> why aren't we going through CrosSettings here?

Done.

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

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:183: *out_value = CreateSettingsValue(
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> It seems like wrapping stuff in a DictionaryValue is not a good idea, given
that
> IsEnterpriseManaged() and current_user_is_owner() are constant over all
values.

True but this function is being used by the UI and needs this extra stuff and
actually is the one that takes ownership of the data (sorry the comment was on
the wrong function).

GetPref is the function intended to be used by non-ui code and the Get<Type>
versions which does not create objects on the heap and does not decorate with
all this stuff.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:207: const Value* value =
GetPref(path);
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> value is leaked (also below)

It would has been leaking if we used the Get function but the GetPref function
only returns the const Value* to the original value in the local_store.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:209: return
value->GetAsBoolean(bool_value);
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> indentation (also below)

Done.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:34: // Helper function to test if given
path is a value cros settings name.
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> why is this public?

Because the CoreChromeOSOptionsHandler uses it to decide if it should handle a
pref from the CrosSettings or from the normal PrefStore.

See: core_chromeos_options_handler.cc

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:42: void FireObservers(const char*
path);
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> why is this public?

Right now this is used by the SignedSettingsOps to signal changes to the
observers of the CrosSettings. I will consider refactoring this once I refactor
the Ops.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:44: // Gets settings value of given
|path| to |out_value|.
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> So there's no way of distinguishing "not set" from "not available"?

What would the difference be for signed settings. We don't have default values
so the not present is not set at the same time. If you mean distinguishing
between bogus name and a  legitimate cros policy name you can use the
IsCrosSettings method above to check that or check the return value of
GetProvider against NULL for that too.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:55: const
CrosSettingsProvider::Callback& callback) const;
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> Clients shouldn't need to depend on CrosSettingsProvider. Can we declare the
> callback type in CrosSettings?

Replaced with base::Closure which it ended up being.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings_provider.h (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_provider.h:30: // Note that |out_value| is
owned by the caller, not this class.
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> Is there a technical reason for this? The interface would be easier to use if
we
> didn't have to worry about deleting the returned value.

Obsolete comment it doesn't really. Removed.

This is also why we don't leak in GetPref, GetBoolean...GetList in CrosSettings.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:99:
&show_users_on_signin);
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> We have a long-standing bug about this value being read to early and the UI
not
> reflecting the actual device setting once we retrieved it. I had hoped the
> refactoring will also address this or at least lay some groundwork for solving
> this. One option could be to built a setting change observer mechanism into
> CrosSettings that we could use here to register for changes (or the event of
> data being loaded from session_manager in this case). Not necessarily
something
> for this CL, but worth considering in the design.

Actually CrosSettings supports observer interface so I put a todo here to use
that to make this policy change aware once I make sure this interface actually
does what it should do.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:245:
pointer_factory_.GetWeakPtr()));
On 2011/09/22 13:54:05, Denis Lagno wrote:
> Are you really sure that this one still works as expected?
> AFAIU WeakPtr turns to NULL when object is destructed.
> I am not sure that code that calls callback checks that weak pointer to object
> is not NULL.

The Closure will take care of not calling run on a WeakPtr that has been
null-ed. So the tasks will cancel once the weak pointers held by the
WeakPtrFactory get invalided which happens in the destructor of the factory.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:274: username, password));
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> Doing the same thing twice to be extra-sure?

Done.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:283:
cros_settings->GetBoolean(kAccountsPrefAllowNewUser, &allow_new_user);
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> no guarantee that you have trusted values here if
> ScreenLocker::default_screen_locker() is NULL. Is that intentional?

I must admit I didn't really try to verify the logic that was already existing
here only updated it to use the right calls.
But looking in the code it seems right to me. If the screen locker is null the
code above will trigger trust check and if it is not null the if will be true
regardless of the allow_new_user value.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:323: username, password));
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> Same two identical things again. Also, this is pretty inconvenient to use if
you
> want to check several different prefs. Have you considered decoupling the
> GetTrusted() check from the actual pref path? I.e. introduce a way to be
> notified whenever we learn that all settings are trusted?

Done.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_performer.cc:332:
cros_settings->GetBoolean(kAccountsPrefAllowNewUser, &allow_new_user);
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> same comment regarding trustedness as above.

Same answer as above too :)

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/new_user_view.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/new_user_view.cc:134:
CrosSettings::Get()->GetBoolean(kAccountsPrefAllowGuest, &allow_guest);
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> same comment as before regarding the problem of reading this setting to early
> and not being able to change our minds once we have retrieved the trusted
value.

I put this todo here as well to make this class observer of CrosSettings.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_controller.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_controller.cc:138:
is_owner_(VerifyOwnership(user.email())),
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> same comment as before regarding the inaccuracy of the value saved here.

I already have a todo to consider switching this to TrustedGet above in the
VerifyOwnership fnc which might essentially boil down to observer story as well.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:315: // definitive
source for them.
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> This is wrong, since ProxyConfigServiceImpl does asynchronous initialization.

You are right however I couldn't find a function to do that so I'll need to
extend the ProxyConfigService interface to supply this or else have to do dirty
tricks here like the one I just implemented to work around the missing function
for now. 
I'd rather add it in a separate CL which I'll even try to land before this one
so that it will be neat in the end when it lands on trunk.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_cros_settings_provider.h (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_provider.h:18:
ProxyCrosSettingsProvider();
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> new line before comment

Done.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:576: PrefService* prefs =
g_browser_process->local_state();
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> Have you considered moving the cache into a single dictionary pref into local
> state? Also, have you considered removing the persistent cache entirely? The
> upside of this would be that people would suddenly be forced to write code
that
> properly reacts to changing device settings. I guess we'd run into a problem
> with the crash reporting pref as set in the eula screen possibly getting lost
> then though...

Doing the first might allow us to unit the live cache with the
TempSignedSettingsStore but I will put it in the list of further refactorings
because it will blow up this CL.

The second as you noticed will break some issues where we still have no owner
but need to save settings. I will still keep this in mind though.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:612: void
UserCrosSettingsProvider::WhitelistUser(const std::string& email) {
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> These implementations still bypass CrosSettings.

True, this is planned to come in with the WhiteList wipeout that is planned as
next step.

I just made them static now so that I can call them without having to
instantiate an instance of this class when no class fields are really needed
here.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:392: bool
is_owner = email == owner;
On 2011/09/22 13:54:05, Denis Lagno wrote:
> nit: seems better to put additional parenthesis for readability.

Done.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/cras...
File chrome/browser/ui/webui/crashes_ui.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/cras...
chrome/browser/ui/webui/crashes_ui.cc:184: NOTREACHED() << "Error occurred while
accessing the pref value!";
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> This seems awkward. Is there any code actually doing useful when the return
> value is false? If not, can we just move the DCHECK() into
> CrosSettings::GetPref()? This would also more closely resemble PrefService,
> which DCHECKs if you ask for the value of a pref that hasn't been registered.

Done.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/about_page_handler.h (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/about_page_handler.h:12: #include
"chrome/browser/ui/webui/options/options_ui.h"
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> including the same thing twice just to be sure?

Done.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.cc (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:85:
chromeos::kSystemTimezone))->GetTimezoneList());
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> shouldn't this also go through CrosSettings?

I put a todo not to forget it but I think it will be better to do this in the CL
that will introduce some more list support for CrosSettings (the same needed to
wipe out Whitelist functions from UserCrosSettingsProvider).

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc
(right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:216: //
TODO(pastarmovj): This will leak now if the callee doesn't delete it.
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> Isn't passing ownership to the caller the contract of this?

This misunderstanding comes again because of the wrong doucmentation of
CrosSettingsProvider::Get. No this is not the contract anymore. The idea is to
cache this one in local_store so that we can use this one instance and return
const refs to it instead of creating new ones every time here. Just like
UserCrosSettings does.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:217: // We
should anyhow cache this in the local_state.
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> If we're caching for all providers, should we move that logic into
CrosSettings
> so it doesn't need to be reimplemented over and over again?

Sounds like a nice idea but it might make ownership a bit more tricky. Will
think of it. Wrote it in my offline ideas list.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/system_settings_provider.h
(right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.h:33: 
On 2011/09/21 11:12:59, Mattias Nissler wrote:
> extra newline

Done.

[Mattias Nissler (ping if slow), 2011-09-26 17:26:37]

another round of comments.

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

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:183: *out_value = CreateSettingsValue(
On 2011/09/23 15:19:32, pastarmovj wrote:
> On 2011/09/21 11:12:59, Mattias Nissler wrote:
> > It seems like wrapping stuff in a DictionaryValue is not a good idea, given
> that
> > IsEnterpriseManaged() and current_user_is_owner() are constant over all
> values.
> 
> True but this function is being used by the UI and needs this extra stuff and
> actually is the one that takes ownership of the data (sorry the comment was on
> the wrong function).
> 
> GetPref is the function intended to be used by non-ui code and the Get<Type>
> versions which does not create objects on the heap and does not decorate with
> all this stuff.

If the UI needs this then why can't the UI implement this in
core_chromeos_options_handler.cc? I'm not convinced this is the right place for
this code.

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/7867044/diff/6041/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:44: // Gets settings value of given
|path| to |out_value|.
On 2011/09/23 15:19:32, pastarmovj wrote:
> On 2011/09/21 11:12:59, Mattias Nissler wrote:
> > So there's no way of distinguishing "not set" from "not available"?
> 
> What would the difference be for signed settings. We don't have default values
> so the not present is not set at the same time. If you mean distinguishing
> between bogus name and a  legitimate cros policy name you can use the
> IsCrosSettings method above to check that or check the return value of
> GetProvider against NULL for that too.

I'm not sure I understand your reply. What I meant that a NULL return value
could either indicate that the setting is actually not present in the device
settings, or that device settings haven't been loaded yet and thus the value is
not available yet. I'm aware there's GetTrusted() to handle the latter, but some
callers might just be interested whether the value is the actual value from
device settings?

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (left):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider_chromeos.cc:82: if
(settings_provider.Get(setting_path, &setting)) {
What's the plan for the auto-removal code here? Did you move it some place else?

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:31: bool disabled) {
As commented in reply to a previous comment, can't we just move this to the UI
code?

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:23: class ListValue;
alphabetize

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:42: // TODO(pastarmovj): Consider to
remove this function from the public i-face.
spell it out, please.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:45: // Gets settings value of given
|path| to |out_value|.
comment outdated.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:46: const base::Value* HasPref(const
std::string& path) const;
shouldn't this rather return bool? Then again, if it's the same as GetPref() why
do we have both?

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:53: bool Get(const std::string& path,
base::Value** out_value) const;
Why is there both Get() and GetPref()?

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_controller.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_controller.cc:58: // Helper function to
initialize the is_owner_ const field of UserController.
s/is_owner_/|is_owner_|/

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:320: return
CrosSettings::Get()->GetTrusted(kAccountsPrefAllowGuest, callback);
This is not correct either, since we might also be waiting from flimflam. Can
you just put a NOTREACHED() for now and fix the TODO soonish?

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.h (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.h:41: virtual bool
GetTrusted(const std::string& path,
As discussed, we should just drop the path argument.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:224: void
DoInstallDefaultAppsOnUIThread(const FilePath& profile_path,
Unrelated?

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (left):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:174: void InstallDefaultApps();
Unrelated change?

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:266: 
put a TODO here as well saying that we need to observe changes to the prefs.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/cra...
File chrome/browser/ui/webui/crashes_ui.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/cra...
chrome/browser/ui/webui/crashes_ui.cc:182: return reporting_enabled;
If you guarantee that reporting_enabled won't be clobbered on GetBoolean()
returning false, you could initialize reporting_enabled to false, get rid of the
conditional and just return reporting_enabled.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:83: //
handling functionality to come.
haven't you included that list handling already now?

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc
(right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:217: // We
should anyhow cache this in the local_state.
I don't like code that introduces leaks. Can you keep a local owned copy in
SystemSettingsProvider for now?

[pastarmovj, 2011-09-29 15:15:03]

PTAL. Thanks!

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (left):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider_chromeos.cc:82: if
(settings_provider.Get(setting_path, &setting)) {
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> What's the plan for the auto-removal code here? Did you move it some place
else?

We are returning a const pointer to the original data from GetPref so nothing to
destroy we don't return copies anymore.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:31: bool disabled) {
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> As commented in reply to a previous comment, can't we just move this to the UI
> code?

Done.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:23: class ListValue;
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> alphabetize

Done. Sorry about that I went for the logical order instead of the alphabetical
one.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:42: // TODO(pastarmovj): Consider to
remove this function from the public i-face.
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> spell it out, please.

Done. You are aware that you made me line break right? ;)

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:45: // Gets settings value of given
|path| to |out_value|.
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> comment outdated.

Not only the comment is outdated the whole function is not there anymore :)

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:46: const base::Value* HasPref(const
std::string& path) const;
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> shouldn't this rather return bool? Then again, if it's the same as GetPref()
why
> do we have both?

Done.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:53: bool Get(const std::string& path,
base::Value** out_value) const;
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> Why is there both Get() and GetPref()?

Get is what UI used to get Pref Dictionaries whapped around a copy of the Value
whereas GetPref only returns a const pointer to the original data. Now Get is
gone and moved to where it belongs the UI level.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_controller.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_controller.cc:58: // Helper function to
initialize the is_owner_ const field of UserController.
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> s/is_owner_/|is_owner_|/

Done.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:320: return
CrosSettings::Get()->GetTrusted(kAccountsPrefAllowGuest, callback);
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> This is not correct either, since we might also be waiting from flimflam. Can
> you just put a NOTREACHED() for now and fix the TODO soonish?

Done.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.h (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.h:41: virtual bool
GetTrusted(const std::string& path,
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> As discussed, we should just drop the path argument.

I'd rather do that once I am on the next step of refactoring this and I change
the calls to StartRetrievePropertyOp by the single StartRetrievePolicyOp (or
whatever the name will be).
Right now if I change the logic to use only one pref to store the trusted flag
(instead of the per pref ones now) I will introduce race conditions where a
single pref readout might set or reset the flag.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:224: void
DoInstallDefaultAppsOnUIThread(const FilePath& profile_path,
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> Unrelated?

Yes this is part of a CL that is landed on trunk but was crashing the browser
while I was working so on the next merge with trunk this changes will disappear.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (left):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:174: void InstallDefaultApps();
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> Unrelated change?

Yes this is part of a CL that is landed on trunk but was crashing the browser
while I was working so on the next merge with trunk this changes will disappear.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:266: 
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> put a TODO here as well saying that we need to observe changes to the prefs.

Done.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/cra...
File chrome/browser/ui/webui/crashes_ui.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/cra...
chrome/browser/ui/webui/crashes_ui.cc:182: return reporting_enabled;
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> If you guarantee that reporting_enabled won't be clobbered on GetBoolean()
> returning false, you could initialize reporting_enabled to false, get rid of
the
> conditional and just return reporting_enabled.

I give 2-year vendor guarantee that it won't change it so I implemented your
suggestion. :)

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/system_options_handler.cc (right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_options_handler.cc:83: //
handling functionality to come.
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> haven't you included that list handling already now?

I'd rather fix this in a separate CL.

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc
(right):

http://codereview.chromium.org/7867044/diff/15001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:217: // We
should anyhow cache this in the local_state.
On 2011/09/26 17:26:37, Mattias Nissler wrote:
> I don't like code that introduces leaks. Can you keep a local owned copy in
> SystemSettingsProvider for now?

Done.

[pastarmovj, 2011-10-04 16:44:55]

Adding Chris to the reviewers list as discussed offline.

[pastarmovj, 2011-10-05 09:47:16]

Cleaned up rebase artefacts in values.cc and profile_impl.cc.

http://codereview.chromium.org/7867044/diff/29001/base/values.cc
File base/values.cc (right):

http://codereview.chromium.org/7867044/diff/29001/base/values.cc#newcode890
base/values.cc:890: int ListValue::Find(const Value& value) const {
This is not part of the CL but a rebase artefact.

http://codereview.chromium.org/7867044/diff/29001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7867044/diff/29001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:223: void
DoInstallDefaultAppsOnUIThread(const FilePath& profile_path,
Sorry this has crept in from the rebase not part of this commit at all.

[Chris Masone, 2011-10-05 18:43:12]

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:51: // not be retrieved.
I don't think this contract is true.  Digging all the way down to
UserCrosSettingsTrust::RequestTrustedEntity (which is what you implement
UserCrosSettingsProvider::GetTrusted with), it only calls |callback| if it
cannot get |path|.  If it _can_ get |path|, it deletes the callback and returns
true.

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:283: const base::ListValue
*user_list;
Can we keep the comment here, explaining that this is the whitelist check?

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:285: if
(cros_settings->GetList(kAccountsPrefUsers, &user_list) &&
Where is the whitelist check now?

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:312:
base::Bind(&LoginPerformer::CompleteLogin,
This used to call Login again, not CompleteLogin

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.cc:613:
cached_whitelist_update->Append(Value::CreateStringValue(email));
hm.  I know you didn't add this code, but am I correct that this is appending
the email address to the cached whitelist _before_ the signed settings code has
successfully written it?  If so, this is a vulnerability.

[pastarmovj, 2011-10-13 11:23:02]

Addressed comments and polished up this commit. 

Please take into account the fact this this CL is part of the row and some
suggestions I have implemented for later steps to make reviewing simpler.
However every single commit in the row compiles and passes the tests on its own
too.

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:51: // not be retrieved.
On 2011/10/05 18:43:12, Chris Masone wrote:
> I don't think this contract is true.  Digging all the way down to
> UserCrosSettingsTrust::RequestTrustedEntity (which is what you implement
> UserCrosSettingsProvider::GetTrusted with), it only calls |callback| if it
> cannot get |path|.  If it _can_ get |path|, it deletes the callback and
returns
> true.

Done.

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:283: const base::ListValue
*user_list;
On 2011/10/05 18:43:12, Chris Masone wrote:
> Can we keep the comment here, explaining that this is the whitelist check?

Done.

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:285: if
(cros_settings->GetList(kAccountsPrefUsers, &user_list) &&
On 2011/10/05 18:43:12, Chris Masone wrote:
> Where is the whitelist check now?

Line 285 fetches the whitelist from the UserCrosSettingsProvider and on line 286
we search for |username_value| in it. It is equivalent to what was done deep in
the SignedSettings code before done on the Value representation now. Given that
the whitelist comes from the same policy blob as the rest of the prefs the check
for trustedness on line 264 will make sure that the whitelist is trusted too
when we get to this line.

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:312:
base::Bind(&LoginPerformer::CompleteLogin,
On 2011/10/05 18:43:12, Chris Masone wrote:
> This used to call Login again, not CompleteLogin

True sorry this was copy paste error :(

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.cc:613:
cached_whitelist_update->Append(Value::CreateStringValue(email));
On 2011/10/05 18:43:12, Chris Masone wrote:
> hm.  I know you didn't add this code, but am I correct that this is appending
> the email address to the cached whitelist _before_ the signed settings code
has
> successfully written it?  If so, this is a vulnerability.

Yes but the PolicyStoreOp will force reload of the signed settings if it fails
and thus the cache will be updated to the proper value after that again.
P.S. this function disappears in the newer version anyway. :) the new
implementation moves the caching to the CrosSettings and completely eliminates
cache updates before writes (the cache is updated on successful policy retrieve
notifications only). I know it is sort of stupid to forward reference changes
here but I wanted to break the work in smaller and easier to review CLs than a
single huge monster.

[Mattias Nissler (ping if slow), 2011-10-13 13:41:06]

LGTM with nits!

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:48: 
nit: remove extra blank line.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:53: // true and win't call the
|callback|.
nit: s/win't/won't/

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.h:183: CrosSettings*
user_settings_;
nit: Comment and variable name are outdated.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_controller_unittest.cc (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_controller_unittest.cc:28:
TEST(UserControllerTest, DISABLED_GetNameTooltipExistingUser) {
I feel bad disabling a test here. Can we maybe just fix it?

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:440: 
remove extra blank line.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:20:
virtual base::Value* FetchPref(const std::string& pref_name);
nit: OVERRIDE

[Denis Lagno, 2011-10-13 13:45:25]

LGTM

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:171: provider = GetProvider(path);
join lines 170+171 or even 170+171+172 together?

[Chris Masone, 2011-10-13 23:42:37]

lgtm

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/7867044/diff/32001/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.cc:613:
cached_whitelist_update->Append(Value::CreateStringValue(email));
On 2011/10/13 11:23:02, pastarmovj wrote:
> On 2011/10/05 18:43:12, Chris Masone wrote:
> > hm.  I know you didn't add this code, but am I correct that this is
appending
> > the email address to the cached whitelist _before_ the signed settings code
> has
> > successfully written it?  If so, this is a vulnerability.
> 
> Yes but the PolicyStoreOp will force reload of the signed settings if it fails
> and thus the cache will be updated to the proper value after that again.
> P.S. this function disappears in the newer version anyway. :) the new
> implementation moves the caching to the CrosSettings and completely eliminates
> cache updates before writes (the cache is updated on successful policy
retrieve
> notifications only). I know it is sort of stupid to forward reference changes
> here but I wanted to break the work in smaller and easier to review CLs than a
> single huge monster.

ok, sounds good.

[pastarmovj, 2011-10-26 15:43:19]

Addressed all comments. Any other changes are collateral damage from rebasing to
ToT.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:171: provider = GetProvider(path);
On 2011/10/13 13:45:25, Denis Lagno wrote:
> join lines 170+171 or even 170+171+172 together?

Done here for 170+171 as well as above for 158+159.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:48: 
On 2011/10/13 13:41:06, Mattias Nissler wrote:
> nit: remove extra blank line.

Done.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:53: // true and win't call the
|callback|.
On 2011/10/13 13:41:06, Mattias Nissler wrote:
> nit: s/win't/won't/

Done.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.h:183: CrosSettings*
user_settings_;
On 2011/10/13 13:41:06, Mattias Nissler wrote:
> nit: Comment and variable name are outdated.

Done.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_controller_unittest.cc (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_controller_unittest.cc:28:
TEST(UserControllerTest, DISABLED_GetNameTooltipExistingUser) {
On 2011/10/13 13:41:06, Mattias Nissler wrote:
> I feel bad disabling a test here. Can we maybe just fix it?

I reenable this test in PART6 where all the needed mockability is implemented.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:440: 
On 2011/10/13 13:41:06, Mattias Nissler wrote:
> remove extra blank line.

Done.

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

http://codereview.chromium.org/7867044/diff/65001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:20:
virtual base::Value* FetchPref(const std::string& pref_name);
On 2011/10/13 13:41:06, Mattias Nissler wrote:
> nit: OVERRIDE

Done throughout the whole file.

[Mattias Nissler (ping if slow), 2011-11-18 14:12:00]

another round of nits. LGTM once you fix them.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider_chromeos.cc:16: #include
"chrome/browser/chromeos/cros_settings.h"
needed?

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:132: #include
"chrome/browser/chromeos/cros_settings.h"
cros_settings.h and cros_settings_names.h should be next to each other.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:164: bool CrosSettings::GetTrusted(
no need for line break AFAICS.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_performer.h (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.h:12: #include
"base/memory/weak_ptr.h"
duplicate, please remove.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:474: //
TODO(pastarmovj): Observe for changes of this variables to update the UI
"Watch for" or "Observe". I have never heard of "observer for" in this context
though... ;)

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:476: bool
allow_new_user;
Properly initialize these variables (also below) if you choose to intentionally
ignore the GetBoolean return value.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/system_settings_provider.h
(right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.h:66: //
TODO(pastarmovj): This will be cached into the local_store soon.
What's the local_store? you mean local state PrefService?

[pastarmovj, 2011-11-18 15:01:38]

Addressed comments. Proceeding to landing verified the feel of try jobs no
issues found.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider_chromeos.cc:16: #include
"chrome/browser/chromeos/cros_settings.h"
On 2011/11/18 14:12:00, Mattias Nissler wrote:
> needed?

Nope rebasing artifact the real code change is obsolete but i didn't remove this
line in the end.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:132: #include
"chrome/browser/chromeos/cros_settings.h"
On 2011/11/18 14:12:00, Mattias Nissler wrote:
> cros_settings.h and cros_settings_names.h should be next to each other.

Done.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:164: bool CrosSettings::GetTrusted(
On 2011/11/18 14:12:00, Mattias Nissler wrote:
> no need for line break AFAICS.

Done.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_performer.h (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.h:12: #include
"base/memory/weak_ptr.h"
On 2011/11/18 14:12:00, Mattias Nissler wrote:
> duplicate, please remove.

Done.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:474: //
TODO(pastarmovj): Observe for changes of this variables to update the UI
On 2011/11/18 14:12:00, Mattias Nissler wrote:
> "Watch for" or "Observe". I have never heard of "observer for" in this context
> though... ;)

Done.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:476: bool
allow_new_user;
On 2011/11/18 14:12:00, Mattias Nissler wrote:
> Properly initialize these variables (also below) if you choose to
intentionally
> ignore the GetBoolean return value.

done.

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/system_settings_provider.h
(right):

http://codereview.chromium.org/7867044/diff/90001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.h:66: //
TODO(pastarmovj): This will be cached into the local_store soon.
On 2011/11/18 14:12:00, Mattias Nissler wrote:
> What's the local_store? you mean local state PrefService?

True. Changed.

[pastarmovj, 2011-11-18 15:15:21]

@davemoore: Can you please give owner approval for
chrome/browser/chromeos/cros/network_library.cc. It is only a minimal change due
to the changed interface to SignedSettings no internal logical or structure
changes.

[DaveMoore, 2011-11-18 23:25:14]

LGTM for network_settings

[commit-bot: I haz the power, 2011-11-22 10:32:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/7867044/98001

[commit-bot: I haz the power, 2011-11-22 11:42:36]

Change committed as 111133