Signed settings refactoring: Proper caching and more tests.

PART 5 Of the signed settings refactoring. List of all changes follow beneath:

Read from the bottom to the top:
* Add new unit tests to cover what was removed before.
* Rename SignedSettingsTempStorage to SignedSettingsCache.
* Revitalize existing tests for SignedSettings[Helper].
* Add the needed infrastucture to support enrollment as well.
* Remove the second cache in OwnershipService it is obsolete.
* Remove the prop ops completely.
* Remove direct prop op from the proxy stuff.
* Serialize policy changes correctly and map side effects of policies.
    Mainly make sure we never serialize dirty policy.
    Don't reload if policy is serialized fine.
    Clear local state registration.
* Clean up redundand SS ops and make proper callbacks for the helper
    Move the temp storage finalization to where it belongs.
* Make the temp storage be the cache and use policy ops.
* Make DeviceSettingsProvider work with the protobuf blob directly.
* Merged DeviceSettingsProvider and UserCrosSettingsTrust.
* Rename UserCrosSettingsProvider to DeviceSettingsProvider.
* Extract the SignedSettingsMigrationHelper in its own file.

BUG=chromium-os:14054
TEST=unit_tests:SignedSettings*,*CrosSettings*,suite_Smoke:login_OwnershipApi

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

[pastarmovj, 2011-11-29 17:03:55]

Hey Mattias this should be it. This seems scary big but it actually isn't
because the core is in 4-5 files only. 

You might want to start at CrosSettings.cc and friends then go to the
DeviceSettingProvider (renamed from UserCros...) and then the rest should be
more or less transparent.

Once we got it mostly straight here I will pull in all the owners too.

P.S. Don't compare it to the old Part 5 it is a new one written from scratch.

[Mattias Nissler (ping if slow), 2011-11-30 11:22:44]

First round of comments, I'll do the rest after lunch. Also, your try jobs are
quite red.

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

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:46: base::Value* GetPref(const
std::string& path) const;
Why is this now non-const? From the rest of the code it looks like this actually
passes out ownership now. Are you sure this is a good thing to do?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:95: void ReloadProviders() const;
Needs a comment. What does it do? Recreate the providers? Tell all providers to
refetch their settings from the backing store?

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

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_provider.h:26: virtual base::Value*
Get(const std::string& path) const = 0;
Why is this no longer const? I hope it doesn't return a copy?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_unittest.cc:28: public:
all this can be protected

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_unittest.cc:40: virtual void SetUp() {
You can leave this out if you don't need it.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_unittest.cc:219: 
remove extra blank line.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:40: 
remove extra blank line.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:44: // For all our boolean
settings following is applicable:
missing "the"

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:46: // Exception:
kSignedDataRoamingEnabled which has default value of false.
This comment is interesting, but doesn't seem to be helpful at this point. Is
there a better place to put it?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:65: // upper bound for
number of retries to fetch a signed setting.
Upper-case upper :)

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:72: kBooleanSettings +
arraysize(kBooleanSettings);
how about const char* end = kBooleanSettings + arraysize(kBooleanSettings), and
then use it in the find? If that fits the column limit better, I guess the
result would be more readable.

(also below)

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:100:
base::ThreadRestrictions::ScopedAllowIO allow_io;
Any chance we can clean this up? Read the file asynchronously on Reload()?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:108: //
###########################################################################
what's this?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:116: // new proxy config.
Proxy config? Observe() doesn't appear to do proxy-config-related work.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:129:
ownership_service_->GetStatus(true);
All the GetStatus calls you have here are potentially blocking. Not good. Any
chance to improve this?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:166: // we have to load the
initially generated blob in the cache.
s/in/into/. Also, the comment is backwards. Maybe it would be clearer if you say
"Reload the initial policy blob, apply settings from temp storage, and write
back the blob" or somesuch.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:192: // No owner yet use the
temp storage.
Make a proper sentence? "If there is no owner yet, query temp storage". Also, I
don't quite understand the comment, since it pulls the policy object out of
SignedSettingsCache unconditionally.

Ah, now I understand: This is only called if there is no owner yet. So please
make the comment clearer :)

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:196: LOG(WARNING) << "Can't
retrieve temp store possibly not created yet";
Missing period, full sentence? Also, if this is a condition that can happen in
regular operation, we shouldn't log a warning.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:197: // prepare empty data
for the case we don't have temp cache yet.
s/prepare/Prepare/

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:209: // Just store it in the
memory cache no trusted checks no persisting.
Full sentence? Also, I don't quite understand why we need to have this branch at
all. Doesn't that mean people can call Set() to clobber our understanding of who
the owner is?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:220:
SignedSettingsHelper::Get()->StartRetrievePolicyOp(
Isn't this redundant? I mean, if RequestTrustedEntity() returns false don't we
have a retrieval op already running?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:311: policy_envelope,
base::Bind(
I think an additional line break after , would help here.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:329: 
remove extra blank line.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:435: << (new_value ? "on." :
"off.");
We always print this when you initially claim a device, correct? If so, this
should probably not be a WARNING.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:529:
MessageLoop::current()->PostTask(FROM_HERE, callbacks_[i]);
any reason to go through the message loop? Can't we just do callbacks_[i].Run()?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:540: NOTREACHED() << "No
policies present yet will use the temp storage.";
comma after yet

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.h:17: // settings store: Storing
and Retrieving the policy blob.
Now that this is much simpler, is it still useful to have both SignedSettings
and SignedSettingsHelper? Should we rewrite these two as well? Certainly not in
this CL though?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_cache.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_cache.cc:42: if
(!base::Base64Encode(policy_string, &encoded)) {
No need to encode stuff, JSON can handle binary strings.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_cache.h (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_cache.h:23: // (that are normally
go into SignedSettings storage)
s/are//

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_cache.h:26: class
SignedSettingsCache {
Only statics? Convert to namespace.

[Mattias Nissler (ping if slow), 2011-11-30 12:25:50]

moar comments.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_helper_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:43: mock_(new
MockKeyUtils),
Is this needed?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:44:
injector_(mock_) /* injector_ takes ownership of mock_ */,
Is this really needed?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:191: // This op
will be deleted and never will be executed (expect only one call
you don't close the parentheses opened on this line.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:193: // be
still called therefore we expect three pending ops.
comma after called

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wizard_controller.cc:391:
g_browser_process->local_state());*/
commented out? why?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.cc:796: // this function ends.
?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_policy_cache.h (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_cache.h:11: #include
"chrome/browser/chromeos/login/signed_settings_helper.h"
Drop these includes and forward-declare SignedSettingsHelper instead?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_cache.h:38: const em::PolicyFetchResponse&
policy) OVERRIDE;
this is no longer an override.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/prefs/browser...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/prefs/browser...
chrome/browser/prefs/browser_prefs.cc:92: #include
"chrome/browser/chromeos/device_settings_provider.h"
can be removed?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/about_page_handler.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/about_page_handler.cc:24: #include
"chrome/browser/chromeos/device_settings_provider.h"
required?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc
(right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc:18:
#include "chrome/browser/chromeos/device_settings_provider.h"
required?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_handler.h
(right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.h:14: class
DeviceSettingsProvider;
required?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:35:
#include "chrome/browser/chromeos/device_settings_provider.h"
required?

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/stats_options_handler.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/stats_options_handler.cc:11: #include
"chrome/browser/chromeos/device_settings_provider.h"
required?

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

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:237: // it
faster. This is not really so critical so for now we can leave it as is.
Just remove the second sentence.

[pastarmovj, 2011-11-30 17:21:16]

Thanks for the review! :)
Addressed all your comments and fixed the failing tests.

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

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:46: base::Value* GetPref(const
std::string& path) const;
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Why is this now non-const? From the rest of the code it looks like this
actually
> passes out ownership now. Are you sure this is a good thing to do?

Changed as discussed offline.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:95: void ReloadProviders() const;
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Needs a comment. What does it do? Recreate the providers? Tell all providers
to
> refetch their settings from the backing store?

Sorry my bad I forgot to comment that one. Done.

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

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_provider.h:26: virtual base::Value*
Get(const std::string& path) const = 0;
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Why is this no longer const? I hope it doesn't return a copy?

Const again.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_unittest.cc:28: public:
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> all this can be protected

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_unittest.cc:40: virtual void SetUp() {
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> You can leave this out if you don't need it.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_unittest.cc:219: 
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> remove extra blank line.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:40: 
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> remove extra blank line.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:44: // For all our boolean
settings following is applicable:
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> missing "the"

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:46: // Exception:
kSignedDataRoamingEnabled which has default value of false.
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> This comment is interesting, but doesn't seem to be helpful at this point. Is
> there a better place to put it?

Moved to UpdateValuesCache. The place we apply defaults actually.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:65: // upper bound for
number of retries to fetch a signed setting.
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Upper-case upper :)

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:72: kBooleanSettings +
arraysize(kBooleanSettings);
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> how about const char* end = kBooleanSettings + arraysize(kBooleanSettings),
and
> then use it in the find? If that fits the column limit better, I guess the
> result would be more readable.
> 
> (also below)

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:100:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Any chance we can clean this up? Read the file asynchronously on Reload()?

I changed a bit DeviceSettingsProvider::ApplyMetricsSetting to only go to this
line iof we are really migrating and not every time (which was stupid anyhow) so
I think it is not such a big deal because it should happen only once and only
for people that are upgrading from pre 16 version to new one.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:108: //
###########################################################################
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> what's this?

A bar made of hashes ... i think :S ... removed :)

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:116: // new proxy config.
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Proxy config? Observe() doesn't appear to do proxy-config-related work.

Nope the comment was wrong. Fixed now.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:129:
ownership_service_->GetStatus(true);
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> All the GetStatus calls you have here are potentially blocking. Not good. Any
> chance to improve this?

All are potentially blocking but only one at most will ever block - so I think
the overhead of this is acceptable.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:166: // we have to load the
initially generated blob in the cache.
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> s/in/into/. Also, the comment is backwards. Maybe it would be clearer if you
say
> "Reload the initial policy blob, apply settings from temp storage, and write
> back the blob" or somesuch.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:192: // No owner yet use the
temp storage.
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Make a proper sentence? "If there is no owner yet, query temp storage". Also,
I
> don't quite understand the comment, since it pulls the policy object out of
> SignedSettingsCache unconditionally.
> 
> Ah, now I understand: This is only called if there is no owner yet. So please
> make the comment clearer :)

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:196: LOG(WARNING) << "Can't
retrieve temp store possibly not created yet";
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Missing period, full sentence? Also, if this is a condition that can happen in
> regular operation, we shouldn't log a warning.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:197: // prepare empty data
for the case we don't have temp cache yet.
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> s/prepare/Prepare/

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:209: // Just store it in the
memory cache no trusted checks no persisting.
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Full sentence? Also, I don't quite understand why we need to have this branch
at
> all. Doesn't that mean people can call Set() to clobber our understanding of
who
> the owner is?

I made this branch set the trusted flag to false but I have to keep it because
we want to have the new owner name in the time while the key is generated
available already so we store it in the cache and can serve it from there else
we will have some time while the new owner will not be able to use his owner
powers after first boot.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:220:
SignedSettingsHelper::Get()->StartRetrievePolicyOp(
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Isn't this redundant? I mean, if RequestTrustedEntity() returns false don't we
> have a retrieval op already running?

Not necessarily. It might be a failed read so retrying here might be a good
idea. And the most important reason is that this ensures proper queuing of
writes so that we can call SetInPolicy while another SetInPolicy is still in
flow and this will ensure the second will wait for the async parts of the first
one to finish.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:311: policy_envelope,
base::Bind(
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> I think an additional line break after , would help here.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:329: 
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> remove extra blank line.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:435: << (new_value ? "on." :
"off.");
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> We always print this when you initially claim a device, correct? If so, this
> should probably not be a WARNING.

Made INFO out of it.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:529:
MessageLoop::current()->PostTask(FROM_HERE, callbacks_[i]);
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> any reason to go through the message loop? Can't we just do
callbacks_[i].Run()?

I guess not. This was legacy code using tasks.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:540: NOTREACHED() << "No
policies present yet will use the temp storage.";
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> comma after yet

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.h:17: // settings store: Storing
and Retrieving the policy blob.
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Now that this is much simpler, is it still useful to have both SignedSettings
> and SignedSettingsHelper? Should we rewrite these two as well? Certainly not
in
> this CL though?

Maybe if we rename the SignedSettingsHelper to SignedSettingsQueue it will
represent better what it does. It really only helps us queue operations and
ensure consistency this way.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_cache.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_cache.cc:42: if
(!base::Base64Encode(policy_string, &encoded)) {
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> No need to encode stuff, JSON can handle binary strings.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_cache.h (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_cache.h:23: // (that are normally
go into SignedSettings storage)
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> s/are//

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_cache.h:26: class
SignedSettingsCache {
On 2011/11/30 11:22:44, Mattias Nissler wrote:
> Only statics? Convert to namespace.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_helper_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:43: mock_(new
MockKeyUtils),
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> Is this needed?

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:44:
injector_(mock_) /* injector_ takes ownership of mock_ */,
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> Is this really needed?

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:191: // This op
will be deleted and never will be executed (expect only one call
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> you don't close the parentheses opened on this line.

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:193: // be
still called therefore we expect three pending ops.
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> comma after called

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wizard_controller.cc:391:
g_browser_process->local_state());*/
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> commented out? why?

Fixed this whole bunch of code I kind of forgot to pimp it while doing the rest.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.cc:796: // this function ends.
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> ?

Dead comment :)

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_policy_cache.h (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_cache.h:11: #include
"chrome/browser/chromeos/login/signed_settings_helper.h"
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> Drop these includes and forward-declare SignedSettingsHelper instead?

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_cache.h:38: const em::PolicyFetchResponse&
policy) OVERRIDE;
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> this is no longer an override.

Yeah clang already told me so :)

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/prefs/browser...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/prefs/browser...
chrome/browser/prefs/browser_prefs.cc:92: #include
"chrome/browser/chromeos/device_settings_provider.h"
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> can be removed?

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/about_page_handler.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/about_page_handler.cc:24: #include
"chrome/browser/chromeos/device_settings_provider.h"
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> required?

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc
(right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.cc:18:
#include "chrome/browser/chromeos/device_settings_provider.h"
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> required?

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_handler.h
(right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_handler.h:14: class
DeviceSettingsProvider;
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> required?

Nope.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:35:
#include "chrome/browser/chromeos/device_settings_provider.h"
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> required?

Done.

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/stats_options_handler.cc (right):

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/stats_options_handler.cc:11: #include
"chrome/browser/chromeos/device_settings_provider.h"
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> required?

Done.

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

http://codereview.chromium.org/8727037/diff/2001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:237: // it
faster. This is not really so critical so for now we can leave it as is.
On 2011/11/30 12:25:50, Mattias Nissler wrote:
> Just remove the second sentence.

Done.

[pastarmovj, 2011-11-30 17:30:54]

Pulling in dilmah as a second reviewer as he reviewed the previous parts as
well. 
If you are fine with Mattias only LG*M-ing this CL you can just say so and skip
reviewing it.

[Denis Lagno, 2011-12-01 00:38:12]

Feel free to commit with just Mattias' LG*M.

I just picked couple of stylistic nits.

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

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:199: for (size_t i = 0; i <
providers_.size(); ++i) {
nit: no curly braces for one-liner loops

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

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:97: void ReloadProviders() const;
just wondering why is should be const?

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:556: }
nit: shouldn't be there default: NOTREACHED(); ?

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_cache.h (right):

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_cache.h:28: void
RegisterPrefs(PrefService* local_state);
do we indent inside namespace?

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.cc:795: std::string
policy_value = "";
nit: just std::string policy_value; without = "" does the same.

[pastarmovj, 2011-12-01 10:07:17]

Addressed the nits and put some more comments around too.

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

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:199: for (size_t i = 0; i <
providers_.size(); ++i) {
On 2011/12/01 00:38:12, Denis Lagno wrote:
> nit: no curly braces for one-liner loops

Done.

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

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:97: void ReloadProviders() const;
On 2011/12/01 00:38:12, Denis Lagno wrote:
> just wondering why is should be const?

No special reason but it doesn't change the internal state of this class so I
decided to mark it as such.

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:556: }
On 2011/12/01 00:38:12, Denis Lagno wrote:
> nit: shouldn't be there default: NOTREACHED(); ?

I think not putting the default here will ensure that if new error cases are
added to the enum the one who added them will be forced to also handle them
properly in the first place to compile Chrome than reacting to a NOTREACHED
while testing.

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_cache.h (right):

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_cache.h:28: void
RegisterPrefs(PrefService* local_state);
On 2011/12/01 00:38:12, Denis Lagno wrote:
> do we indent inside namespace?

You are right we don't I just forgot to fix it when I transformed from a class.
Fixed.

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8727037/diff/3004/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.cc:795: std::string
policy_value = "";
On 2011/12/01 00:38:12, Denis Lagno wrote:
> nit: just std::string policy_value; without = "" does the same.

Done.

[Mattias Nissler (ping if slow), 2011-12-01 11:17:49]

Some comments. I'd like to do another full review pass once you've fixed the
things we've discussed.

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

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:97: void ReloadProviders() const;
please make non-const even if there's no technical reason.

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings_names.cc (right):

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_names.cc:12: const char
kAccountsPrefAllowNewUser[] = "cros.accounts.allowGuest";
please revert.

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/dev...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:345:
pol.allow_new_users().allow_new_users()) {
As discussed, we might replace these if-else patterns with simple calls to
SetBoolean with the proper condition.

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache_unittest.cc:77:
file_thread_(content::BrowserThread::FILE, &message_loop_) {}
I don't see why this test would need to setup threads. SignedSettingsHelper is
mocked out in the test, so it shouldn't actually ever get called.

[pastarmovj, 2011-12-01 15:34:49]

Addressed the comments and updated the CL title as discussed offline.

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

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:97: void ReloadProviders() const;
On 2011/12/01 11:17:49, Mattias Nissler wrote:
> please make non-const even if there's no technical reason.

Done.

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings_names.cc (right):

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_names.cc:12: const char
kAccountsPrefAllowNewUser[] = "cros.accounts.allowGuest";
On 2011/12/01 11:17:49, Mattias Nissler wrote:
> please revert.

Done.

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/dev...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:345:
pol.allow_new_users().allow_new_users()) {
On 2011/12/01 11:17:49, Mattias Nissler wrote:
> As discussed, we might replace these if-else patterns with simple calls to
> SetBoolean with the proper condition.

Done.

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache_unittest.cc:77:
file_thread_(content::BrowserThread::FILE, &message_loop_) {}
On 2011/12/01 11:17:49, Mattias Nissler wrote:
> I don't see why this test would need to setup threads. SignedSettingsHelper is
> mocked out in the test, so it shouldn't actually ever get called.

OwnershipCheck needs to have FILE thread.

[Mattias Nissler (ping if slow), 2011-12-02 12:13:36]

LGTM with lots of nits :)

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:41: // Reset the cache between
tests.
You might also want to do that on SetUp(), since otherwise tests that leave
behind state might kill you.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:52:
pointer_factory_.GetWeakPtr(), pref));
Do we need the task posting here? I guess not, so please remove.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:91: expected_props_[pref_name]
= value;
base::Value*& entry = expected_props_[pref_name];
delete entry;
entry = value;

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:135:
ASSERT_TRUE(expected_props_.empty());
Since you have the last two lines in every test, you may just want them to
TearDown().

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:155: scoped_ptr<base::Value>
hacky_user(base::Value::CreateStringValue("h@xxor"));
No need to wrap in a scoped_ptr (here and all over the place).

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:165: kAccountsPrefUsers,
hacky_user.get()));
task posting not needed here I guess.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:185:
message_loop_.RunAllPending();
I think at this point we should also verify that all expectations are met. You
probably want to write a VerifyAndClearExpections() helper function.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:22: #include
"chrome/browser/chromeos/login/ownership_status_checker.h"
not needed.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:181: // Just store it in the
memory cache no trusted checks no persisting.
no comma no sentence no grammar? :)

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
File chrome/browser/chromeos/device_settings_provider.h (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:57: const em::PolicyData
get_policy() const;
if this is a true accessor, it should be called policy().

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:59: void
RetrieveCachedData();
missing a comment on what it does.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:61: void SetInPolicy(const
std::string& prop, const base::Value& value);
same here: where does the data actually end up?

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:63: // Finalizes stores to
the policy file if the cache is dirt
y.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_cache.h (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_cache.h:22: // There is need
(proxy settings at OOBE stage) to store settings
update the comment.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_helper_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:60:
.WillRepeatedly(Store(true));
Can this be inlined with something along the lines of WithArgs<1,
2>(Invoke(base::Closure::Run))?

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:48:
static_cast<base::ListValue*>(pref_value));
No need to pass a copy into CreateUsersWhitelist?

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

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:221: if
(path == kSystemTimezone) {
no need for curlies.

[Mattias Nissler (ping if slow), 2011-12-02 12:13:36]

LGTM with lots of nits :)

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:41: // Reset the cache between
tests.
You might also want to do that on SetUp(), since otherwise tests that leave
behind state might kill you.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:52:
pointer_factory_.GetWeakPtr(), pref));
Do we need the task posting here? I guess not, so please remove.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:91: expected_props_[pref_name]
= value;
base::Value*& entry = expected_props_[pref_name];
delete entry;
entry = value;

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:135:
ASSERT_TRUE(expected_props_.empty());
Since you have the last two lines in every test, you may just want them to
TearDown().

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:155: scoped_ptr<base::Value>
hacky_user(base::Value::CreateStringValue("h@xxor"));
No need to wrap in a scoped_ptr (here and all over the place).

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:165: kAccountsPrefUsers,
hacky_user.get()));
task posting not needed here I guess.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:185:
message_loop_.RunAllPending();
I think at this point we should also verify that all expectations are met. You
probably want to write a VerifyAndClearExpections() helper function.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:22: #include
"chrome/browser/chromeos/login/ownership_status_checker.h"
not needed.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:181: // Just store it in the
memory cache no trusted checks no persisting.
no comma no sentence no grammar? :)

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
File chrome/browser/chromeos/device_settings_provider.h (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:57: const em::PolicyData
get_policy() const;
if this is a true accessor, it should be called policy().

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:59: void
RetrieveCachedData();
missing a comment on what it does.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:61: void SetInPolicy(const
std::string& prop, const base::Value& value);
same here: where does the data actually end up?

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:63: // Finalizes stores to
the policy file if the cache is dirt
y.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_cache.h (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_cache.h:22: // There is need
(proxy settings at OOBE stage) to store settings
update the comment.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_helper_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:60:
.WillRepeatedly(Store(true));
Can this be inlined with something along the lines of WithArgs<1,
2>(Invoke(base::Closure::Run))?

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:48:
static_cast<base::ListValue*>(pref_value));
No need to pass a copy into CreateUsersWhitelist?

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

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:221: if
(path == kSystemTimezone) {
no need for curlies.

[pastarmovj, 2011-12-02 14:43:38]

Addressed last round of comments. Running some try jobs and if all is good I
will commit this patch.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:41: // Reset the cache between
tests.
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> You might also want to do that on SetUp(), since otherwise tests that leave
> behind state might kill you.

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:52:
pointer_factory_.GetWeakPtr(), pref));
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> Do we need the task posting here? I guess not, so please remove.

Nope removed.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:91: expected_props_[pref_name]
= value;
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> base::Value*& entry = expected_props_[pref_name];
> delete entry;
> entry = value;

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:135:
ASSERT_TRUE(expected_props_.empty());
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> Since you have the last two lines in every test, you may just want them to
> TearDown().

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:155: scoped_ptr<base::Value>
hacky_user(base::Value::CreateStringValue("h@xxor"));
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> No need to wrap in a scoped_ptr (here and all over the place).

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:165: kAccountsPrefUsers,
hacky_user.get()));
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> task posting not needed here I guess.

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:185:
message_loop_.RunAllPending();
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> I think at this point we should also verify that all expectations are met. You
> probably want to write a VerifyAndClearExpections() helper function.

Put the proper assert here.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:22: #include
"chrome/browser/chromeos/login/ownership_status_checker.h"
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> not needed.

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:181: // Just store it in the
memory cache no trusted checks no persisting.
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> no comma no sentence no grammar? :)

Done,

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
File chrome/browser/chromeos/device_settings_provider.h (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:57: const em::PolicyData
get_policy() const;
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> if this is a true accessor, it should be called policy().

It is - renamed.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:59: void
RetrieveCachedData();
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> missing a comment on what it does.

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:61: void SetInPolicy(const
std::string& prop, const base::Value& value);
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> same here: where does the data actually end up?

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.h:63: // Finalizes stores to
the policy file if the cache is dirt
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> y.

e.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_cache.h (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_cache.h:22: // There is need
(proxy settings at OOBE stage) to store settings
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> update the comment.

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_helper_unittest.cc (right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_helper_unittest.cc:60:
.WillRepeatedly(Store(true));
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> Can this be inlined with something along the lines of WithArgs<1,
> 2>(Invoke(base::Closure::Run))?

Couldn't get this running with some gmock voodoo but at least I commented what
the idea of this code is.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:48:
static_cast<base::ListValue*>(pref_value));
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> No need to pass a copy into CreateUsersWhitelist?

Done.

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:207:
const base::Value* value = CrosSettings::Get()->GetPref(*setting_name);
This call was wrong and is now changed to FetchPref to make sure we don't pass
the naked values to the UI.

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

http://codereview.chromium.org/8727037/diff/16007/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/system_settings_provider.cc:221: if
(path == kSystemTimezone) {
On 2011/12/02 12:13:37, Mattias Nissler wrote:
> no need for curlies.

Done.