Add DeviceSettingsService.

Add DeviceSettingsService.

DeviceSettingsService consolidates Chrome OS device settings access,
keeping an authoratitive copy of device settings in memory and providing
a simpler API to read and write settings. The intention is to make both
DeviceSettingsProvider and DevicePolicyCache go through the new
interface to remove duplication.

BUG=chromium:139126
TEST=Unit tests, new code is not used yet.


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

[Mattias Nissler (ping if slow), 2012-07-26 15:21:16]

I meant to clean up the low-level device settings for a long time, and now I'm
finally doing it. This is the first change in a series and is basically a
rewrite of what used to be *signed_settings*.

[pastarmovj, 2012-07-30 12:19:19]

First batch of comments :)

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_service.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.cc:210:
pending_operations_.front()->RestartLoad(force_key_load);
What if the operation is a store op?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.cc:237: is_owner =
new_key->private_key();
I think this will be clearer if you wrote down the full boolean expression
private_key != NULL.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.cc:243: if
(owner_key_.get() != new_key.get()) {
If I understand this right this code will now trigger those notifications every
time key rotation happens as well. I don't think this is an issue but have you
considered that scenario at least for a manual verification that nothing breaks?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_service.h (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:76: // events so
interested parties and reload state as appropriate.
Hmm something is missing in this sentence.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:114:
DeviceSettingsService();
Isn't it better to create another static method like CreateInstanceForTests or
the like to make sure this constructor is not going to get called by
coincidence.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:123: void Shutdown();
Please document with one line. Now it feels more like this is going to be called
on init too.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:126: // settings have
not yet been retrieved from session_manager yet.
yet...yet. Please remove one of them.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:167: // the UI thread.
FYI: I have an idea how we can get rid of the call to this one from the
DeviceSettingsProvider once you land this I will try to fix it. Maybe we'll be
able to get rid of it completely then.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_service_unittest.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service_unittest.cc:153:
device_settings_test_helper_.Flush();
Please add the expectation as in the next test.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service_unittest.cc:317:
EXPECT_FALSE(device_settings_service_.GetOwnerKey()->private_key());
You should verify if the "is current user owner" check returns false
respectively true below here as well.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_test_helper.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_test_helper.cc:57: for (int i = 0;
i < 2; ++i) {
Any chance this can be replaced with a while(has_pending_ops()) instead?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_test_helper.h (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_test_helper.h:21: // call one of
the Flush() variants that will resume processing.
Working around the "true mocks are bad for your health" here? :) Sigh...

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_test_helper.h:28: // Runs all
pending store callbacks. Returns false if nothing is pending.
Returns false from a void fnc?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_test_helper.h:31: // Runs all
pending retrieve callbacks. Returns false if nothing is pending.
Returns false from a void fnc?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/owner_key_util.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_util.cc:130: return
file_util::PathExists(FilePath(OwnerKeyUtilImpl::kOwnerKeyFile));
Given that an empty file will fail import I think this should be taken into
consideration here too and return false if the file is there but is empty as
well.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/owner_key_util.h (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_util.h:36: // Verfiy that |signature| is
a SHA1-with-RSA signature over |data| with
fix Verify

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/owner_key_util_unittest.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_util_unittest.cc:49:
crypto::RSAPrivateKey::CreateSensitive(kKeySizeInBits));
Just a thought here: generating a new key every time will add some time to the
unit tests isn't it wiser to generate one and hard code it as a blob in the
source code of this test to save a few milliseconds on the unit tests?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_util_unittest.cc:67: }
==size + memcmp?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session_manager_operation.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session_manager_operation.cc:72: if
(force_key_load_ ||
Don't those fit on a single line?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session_manager_operation.cc:291:
ReportResult(DeviceSettingsService::STORE_KEY_UNAVAILABLE);
This error code seems not always to be the right one. The assembler above will
return an empty string in a number of cases not all of them necessarily being a
missing key.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session_manager_operation.h (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session_manager_operation.h:30:
DeviceSettingsService::Status)>
Maybe put Callback on this line?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session_manager_operation.h:163: class
SignAndStoreSettingsOperation : public SessionManagerOperation {
Could this one inherit the StoreSettingsOperation and reduce even further the
need for code duplication?

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_validator_unittest.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_validator_unittest.cc:40:
policy_.set_new_signing_key(PolicyBuilder::CreateTestNewSigningKey());
Again I think that tests that don't need special keys e.g. key rotation etc.
will profit from using a hard coded key pair. On my machine ssh-keygen -b 2048
takes around 100ms so this doesn't seem like a premature optimization to me.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_validator_unittest.cc:55:
validator->ValidateTimestamp(timestamp_, timestamp_, false);
Given that we have some more degrees of freedom now it seems that maybe there is
more to be tested here now, namely the cross product of the two new flags.

[Nikita (slow), 2012-07-31 05:44:11]

It feels like this should be placed somewhere like

src/chromeos/settings

[Nikita (slow), 2012-07-31 05:58:29]

On 2012/07/31 05:44:11, Nikita Kostylev wrote:
> It feels like this should be placed somewhere like
> 
> src/chromeos/settings

With the proper OWNERS file in place :)

[Mattias Nissler (ping if slow), 2012-07-31 13:02:15]

Here's a new version that addresses Julian's comments. Nikita's suggestion to
move all device settings into a subdirectory is actually a good idea. I'll sneak
in a pre-factoring patch that moves all the signed settings and CrosSettings
stuff to chrome/browser/chromeos/settings and rebase this one on top of that.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_service.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.cc:210:
pending_operations_.front()->RestartLoad(force_key_load);
On 2012/07/30 12:19:19, pastarmovj wrote:
> What if the operation is a store op?

If the Store op is still storing data, it'll not be interrupted. If the Store op
is already in the reload phase, it'll restart the reload.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.cc:237: is_owner =
new_key->private_key();
On 2012/07/30 12:19:19, pastarmovj wrote:
> I think this will be clearer if you wrote down the full boolean expression
> private_key != NULL.

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.cc:243: if
(owner_key_.get() != new_key.get()) {
On 2012/07/30 12:19:19, pastarmovj wrote:
> If I understand this right this code will now trigger those notifications
every
> time key rotation happens as well. I don't think this is an issue but have you
> considered that scenario at least for a manual verification that nothing
breaks?

Yes, I verified key rotation thoroughly, and it still works :)

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_service.h (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:76: // events so
interested parties and reload state as appropriate.
On 2012/07/30 12:19:19, pastarmovj wrote:
> Hmm something is missing in this sentence.

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:114:
DeviceSettingsService();
On 2012/07/30 12:19:19, pastarmovj wrote:
> Isn't it better to create another static method like CreateInstanceForTests or
> the like to make sure this constructor is not going to get called by
> coincidence.

How would you call a constructor by coincidence? CreateInstanceForTests would
require to use a pointer, so more noise in tests.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:123: void Shutdown();
On 2012/07/30 12:19:19, pastarmovj wrote:
> Please document with one line. Now it feels more like this is going to be
called
> on init too.

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:126: // settings have
not yet been retrieved from session_manager yet.
On 2012/07/30 12:19:19, pastarmovj wrote:
> yet...yet. Please remove one of them.

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:167: // the UI thread.
On 2012/07/30 12:19:19, pastarmovj wrote:
> FYI: I have an idea how we can get rid of the call to this one from the
> DeviceSettingsProvider once you land this I will try to fix it. Maybe we'll be
> able to get rid of it completely then.

I've fixed DeviceSettingsProvider in the second patch set already :)

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_service_unittest.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service_unittest.cc:153:
device_settings_test_helper_.Flush();
On 2012/07/30 12:19:19, pastarmovj wrote:
> Please add the expectation as in the next test.

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service_unittest.cc:317:
EXPECT_FALSE(device_settings_service_.GetOwnerKey()->private_key());
On 2012/07/30 12:19:19, pastarmovj wrote:
> You should verify if the "is current user owner" check returns false
> respectively true below here as well.

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_test_helper.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_test_helper.cc:57: for (int i = 0;
i < 2; ++i) {
On 2012/07/30 12:19:19, pastarmovj wrote:
> Any chance this can be replaced with a while(has_pending_ops()) instead?

No, there's no good way to find out for the blocking pool.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_test_helper.h (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_test_helper.h:21: // call one of
the Flush() variants that will resume processing.
On 2012/07/30 12:19:19, pastarmovj wrote:
> Working around the "true mocks are bad for your health" here? :) Sigh...

In this case they are, I actually had the gmock version before and decided
against it.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_test_helper.h:28: // Runs all
pending store callbacks. Returns false if nothing is pending.
On 2012/07/30 12:19:19, pastarmovj wrote:
> Returns false from a void fnc?

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_test_helper.h:31: // Runs all
pending retrieve callbacks. Returns false if nothing is pending.
On 2012/07/30 12:19:19, pastarmovj wrote:
> Returns false from a void fnc?

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/owner_key_util.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_util.cc:130: return
file_util::PathExists(FilePath(OwnerKeyUtilImpl::kOwnerKeyFile));
On 2012/07/30 12:19:19, pastarmovj wrote:
> Given that an empty file will fail import I think this should be taken into
> consideration here too and return false if the file is there but is empty as
> well.

This is intentional. If the file is present, there is a good chance the device
is owned (or has been owned). We don't want to treat a device that used to have
a proper owner as un-owned.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/owner_key_util.h (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_util.h:36: // Verfiy that |signature| is
a SHA1-with-RSA signature over |data| with
On 2012/07/30 12:19:19, pastarmovj wrote:
> fix Verify

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/owner_key_util_unittest.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_util_unittest.cc:49:
crypto::RSAPrivateKey::CreateSensitive(kKeySizeInBits));
On 2012/07/30 12:19:19, pastarmovj wrote:
> Just a thought here: generating a new key every time will add some time to the
> unit tests isn't it wiser to generate one and hard code it as a blob in the
> source code of this test to save a few milliseconds on the unit tests?

This is copied from OwnerKeyUtilsTest and has been just fine thus far :) I
reworked the OwnerKeyUtil stuff a bit more and this uses static data now.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_util_unittest.cc:67: }
On 2012/07/30 12:19:19, pastarmovj wrote:
> ==size + memcmp?

Actually, using EXPECT_EQ on the vectors is even better.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session_manager_operation.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session_manager_operation.cc:72: if
(force_key_load_ ||
On 2012/07/30 12:19:19, pastarmovj wrote:
> Don't those fit on a single line?

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session_manager_operation.cc:291:
ReportResult(DeviceSettingsService::STORE_KEY_UNAVAILABLE);
On 2012/07/30 12:19:19, pastarmovj wrote:
> This error code seems not always to be the right one. The assembler above will
> return an empty string in a number of cases not all of them necessarily being
a
> missing key.

This should have been STORE_POLICY_ERROR. Fixed.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session_manager_operation.h (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session_manager_operation.h:30:
DeviceSettingsService::Status)>
On 2012/07/30 12:19:19, pastarmovj wrote:
> Maybe put Callback on this line?

Done.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session_manager_operation.h:163: class
SignAndStoreSettingsOperation : public SessionManagerOperation {
On 2012/07/30 12:19:19, pastarmovj wrote:
> Could this one inherit the StoreSettingsOperation and reduce even further the
> need for code duplication?

Some aspects are identical, others are different (this class doesn't need
policy_blob_, for example). I could make an intermediate base class, but I don't
think the savings justify the additional complexity.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_validator_unittest.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_validator_unittest.cc:40:
policy_.set_new_signing_key(PolicyBuilder::CreateTestNewSigningKey());
On 2012/07/30 12:19:19, pastarmovj wrote:
> Again I think that tests that don't need special keys e.g. key rotation etc.
> will profit from using a hard coded key pair. On my machine ssh-keygen -b 2048
> takes around 100ms so this doesn't seem like a premature optimization to me.

Have you checked PolicyBuilder? This function actually returns a key object
created from static key data :)

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_validator_unittest.cc:55:
validator->ValidateTimestamp(timestamp_, timestamp_, false);
On 2012/07/30 12:19:19, pastarmovj wrote:
> Given that we have some more degrees of freedom now it seems that maybe there
is
> more to be tested here now, namely the cross product of the two new flags.

Done.

[Mattias Nissler (ping if slow), 2012-07-31 17:28:00]

Ok, this is no rebased on top of http://codereview.chromium.org/10824112/, which
moves the device settings stuff to chrome/browser/chromeos/settings. Ready for
another round of review.

[pastarmovj, 2012-08-01 08:46:51]

LGTM

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/device_settings_service.h (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/device_settings_service.h:114:
DeviceSettingsService();
On 2012/07/31 13:02:15, Mattias Nissler wrote:
> On 2012/07/30 12:19:19, pastarmovj wrote:
> > Isn't it better to create another static method like CreateInstanceForTests
or
> > the like to make sure this constructor is not going to get called by
> > coincidence.
> 
> How would you call a constructor by coincidence? CreateInstanceForTests would
> require to use a pointer, so more noise in tests.

What I meant by "coincidence" is somebody doing this :

DeviceSettingsService myservice;
myservice.callSomething();

instead of using the singleton.

I am fine with keeping it like that basically my concern was more for the user
facing classes like CrosSettings which you also changed this way in your second
CL.

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/owner_key_util_unittest.cc (right):

http://codereview.chromium.org/10828032/diff/2002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_util_unittest.cc:67: }
On 2012/07/31 13:02:15, Mattias Nissler wrote:
> On 2012/07/30 12:19:19, pastarmovj wrote:
> > ==size + memcmp?
> 
> Actually, using EXPECT_EQ on the vectors is even better.

Agreed :)

[Chris Masone, 2012-08-03 16:28:25]

http://codereview.chromium.org/10828032/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/owner_key_util.cc (right):

http://codereview.chromium.org/10828032/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/owner_key_util.cc:83: if (to_export.size() >
static_cast<uint>(INT_MAX)) {
numeric_limits<int>::max(), right?

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:27: static
base::LazyInstance<DeviceSettingsService> g_device_settings_service =
There was recently a thread about making all these 'Leaky', wasn't there?  Do we
need to do anything special to make that happen, or has it been made the default
now?  It seems safe for this LazyInstance to leak at shutdown, as it doesn't
need to do any cleanup.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:89:
scoped_ptr<em::ChromeDeviceSettingsProto> settings,
IF this class is holding this settings proto, why is one passed in here?

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:115:
ownership_status_lock_.Acquire();
I always liked AutoLock() for this stuff.

{
  AutoLock scoped_lock(ownership_status_lock_);
  status = ownership_status_;
}

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:134: void
DeviceSettingsService::GetOwnershipStatusAsync(
it seems like the semantics of this and the above differ, beyond the obvious
sync/async.  The above code says OWNERSHIP_TAKEN if the public key file is
present on disk, and this one says OWNERSHIP_TAKEN if the public key has been
loaded into owner_key_.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:142:
owner_key_->private_key()));
Hm.  The level of abstraction between this point of call and the actual
definition of an OwnershipStatusCallback makes it hard to remember that this
isn't actually passing the private key around here.  I understand that it gets
coerced to a bool, but it might be nice to put a '!= NULL' just to make it
obvious

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:145: // wait for an
existing operation to finish.
Well...you don't wait for an existing operation to finish, do you?  If there are
any existing operations, you just don't EnqueueLoad().  In that case, it's up to
the caller to try again later, yes?  What if the pending operation is not a
load?

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:159: // The private
key may have become available, so force a key reload.
How does this follow?

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:170: // the UI
thread.
File an issue describing what work this will entail and reference it here?

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:175: void
GetOwnershipStatusAsync(const OwnershipStatusCallback& callback);
Async is guaranteed never to return OWNERSHIP_UNKNOWN?  That's surprising, given
that the comment above indicates that GetOwnershipStatus(false) -- a
non-blocking GetOwnershipStatus() call -- might return OWNERSHIP_UNKNOWN

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:187: // Removes and
observer.
s/and/an/

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/owner_key_util.cc (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/owner_key_util.cc:13: #include
"chrome/browser/chromeos/cros/cros_library.h"
I probably just missed it, but what's this used for here?

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/session_manager_operation.cc:29:
SessionManagerOperation::SessionManagerOperation(const Callback& callback)
No unittests for this class?  Did you forget to add the file to the CL?

[Mattias Nissler (ping if slow), 2012-08-06 21:28:14]

Addressed/commented all of Chris' input, ready for another round.

http://codereview.chromium.org/10828032/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/owner_key_util.cc (right):

http://codereview.chromium.org/10828032/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/owner_key_util.cc:83: if (to_export.size() >
static_cast<uint>(INT_MAX)) {
On 2012/08/03 16:28:25, Chris Masone wrote:
> numeric_limits<int>::max(), right?

Correct, but obsolete :)

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:27: static
base::LazyInstance<DeviceSettingsService> g_device_settings_service =
On 2012/08/03 16:28:25, Chris Masone wrote:
> There was recently a thread about making all these 'Leaky', wasn't there?  Do
we
> need to do anything special to make that happen, or has it been made the
default
> now?  It seems safe for this LazyInstance to leak at shutdown, as it doesn't
> need to do any cleanup.

I think you're referring to this thread:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/JsPbr3UngS4/dis...

Since I don't specify the singleton traits explicitly, this should be perfectly
good when the traits switch to leaky by default.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:89:
scoped_ptr<em::ChromeDeviceSettingsProto> settings,
On 2012/08/03 16:28:25, Chris Masone wrote:
> IF this class is holding this settings proto, why is one passed in here?

It holds the current settings proto. The one passed here is the update that's
supposed to be signed and sent to session manager, then reloaded, verified and
eventually to become the new settings proto. I changed the name to new_settings
to make it clearer.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:115:
ownership_status_lock_.Acquire();
On 2012/08/03 16:28:25, Chris Masone wrote:
> I always liked AutoLock() for this stuff.
> 
> {
>   AutoLock scoped_lock(ownership_status_lock_);
>   status = ownership_status_;
> }

Done. This function is actually copied legacy code, and I'm hoping to get rid of
it soonish.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:134: void
DeviceSettingsService::GetOwnershipStatusAsync(
On 2012/08/03 16:28:25, Chris Masone wrote:
> it seems like the semantics of this and the above differ, beyond the obvious
> sync/async.  The above code says OWNERSHIP_TAKEN if the public key file is
> present on disk, and this one says OWNERSHIP_TAKEN if the public key has been
> loaded into owner_key_.

True. As commented above, the blocking and locking was meant to go away in any
case, so I've just done that now.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:142:
owner_key_->private_key()));
On 2012/08/03 16:28:25, Chris Masone wrote:
> Hm.  The level of abstraction between this point of call and the actual
> definition of an OwnershipStatusCallback makes it hard to remember that this
> isn't actually passing the private key around here.  I understand that it gets
> coerced to a bool, but it might be nice to put a '!= NULL' just to make it
> obvious

Totally agree. Done.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:145: // wait for an
existing operation to finish.
On 2012/08/03 16:28:25, Chris Masone wrote:
> Well...you don't wait for an existing operation to finish, do you?  If there
are
> any existing operations, you just don't EnqueueLoad().  In that case, it's up
to
> the caller to try again later, yes?  What if the pending operation is not a
> load?

Every operation returns the current key status. So if there already is one
pending operation, it will invoke all pending callbacks on completion. I
clarified the comment.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:159: // The private
key may have become available, so force a key reload.
On 2012/08/03 16:28:25, Chris Masone wrote:
> How does this follow?

We throw away the old key, so the next operation that runs will have to re-check
the disk. I've now made this explicit here instead of piggybacking on
OwnerKeySet().

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:170: // the UI
thread.
On 2012/08/03 16:28:25, Chris Masone wrote:
> File an issue describing what work this will entail and reference it here?

I checked again, and there's only one dependency, so I've converted that
dependency and dropped the blocking flag entirely.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:175: void
GetOwnershipStatusAsync(const OwnershipStatusCallback& callback);
On 2012/08/03 16:28:25, Chris Masone wrote:
> Async is guaranteed never to return OWNERSHIP_UNKNOWN?  That's surprising,
given
> that the comment above indicates that GetOwnershipStatus(false) -- a
> non-blocking GetOwnershipStatus() call -- might return OWNERSHIP_UNKNOWN

Why is that surprising? GetOwnershipStatusAsync() goes off and does I/O and
comes back with a result, whereas GetOwnershipStatus(false) cannot do I/O, so it
might say UNKNOWN.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:187: // Removes and
observer.
On 2012/08/03 16:28:25, Chris Masone wrote:
> s/and/an/

Done.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/owner_key_util.cc (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/owner_key_util.cc:13: #include
"chrome/browser/chromeos/cros/cros_library.h"
On 2012/08/03 16:28:25, Chris Masone wrote:
> I probably just missed it, but what's this used for here?

It's not. A few others as well, I removed them all.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/session_manager_operation.cc:29:
SessionManagerOperation::SessionManagerOperation(const Callback& callback)
On 2012/08/03 16:28:25, Chris Masone wrote:
> No unittests for this class?  Did you forget to add the file to the CL?

I consider the code here a mere implementation detail of DeviceSettingsService.
DeviceSettingsServiceTest has tests that make sure the correct session_manager
interactions happen, which tests that the code here does the right thing.

I'm not sure adding more tests for the code here will be a gain, they'd mostly
be of the brittle do-this-then-do-that variant. If you have a good idea on
useful tests, let me know and I'll add them.

[Chris Masone, 2012-08-07 20:20:03]

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/session_manager_operation.cc:29:
SessionManagerOperation::SessionManagerOperation(const Callback& callback)
On 2012/08/06 21:28:14, Mattias Nissler wrote:
> On 2012/08/03 16:28:25, Chris Masone wrote:
> > No unittests for this class?  Did you forget to add the file to the CL?
> 
> I consider the code here a mere implementation detail of
DeviceSettingsService.
> DeviceSettingsServiceTest has tests that make sure the correct session_manager
> interactions happen, which tests that the code here does the right thing.
> 
> I'm not sure adding more tests for the code here will be a gain, they'd mostly
> be of the brittle do-this-then-do-that variant. If you have a good idea on
> useful tests, let me know and I'll add them.

Well, wouldn't it be useful to test that LoadOwnerKey() correctly uses
current_key() if it is populated?  And that it loads a key if it's not?  Things
like that.  Or that StartSigning() results in a correctly signed policy blob
getting passed to StoreDevicePolicy()?

[Mattias Nissler (ping if slow), 2012-08-17 13:22:42]

Finally got around to add unit tests for SessionManagerOperation. Chris, can you
take another look please? Thanks.

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

http://codereview.chromium.org/10828032/diff/15003/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/session_manager_operation.cc:29:
SessionManagerOperation::SessionManagerOperation(const Callback& callback)
On 2012/08/07 20:20:03, Chris Masone wrote:
> On 2012/08/06 21:28:14, Mattias Nissler wrote:
> > On 2012/08/03 16:28:25, Chris Masone wrote:
> > > No unittests for this class?  Did you forget to add the file to the CL?
> > 
> > I consider the code here a mere implementation detail of
> DeviceSettingsService.
> > DeviceSettingsServiceTest has tests that make sure the correct
session_manager
> > interactions happen, which tests that the code here does the right thing.
> > 
> > I'm not sure adding more tests for the code here will be a gain, they'd
mostly
> > be of the brittle do-this-then-do-that variant. If you have a good idea on
> > useful tests, let me know and I'll add them.
> 
> Well, wouldn't it be useful to test that LoadOwnerKey() correctly uses
> current_key() if it is populated?  And that it loads a key if it's not? 
Things
> like that.  Or that StartSigning() results in a correctly signed policy blob
> getting passed to StoreDevicePolicy()? 

OK, I added unit tests that should cover SessionManagerOperation and friends
quite well.

[Chris Masone, 2012-08-17 18:21:34]

lgtm

[Mattias Nissler (ping if slow), 2012-08-23 23:02:15]

I'm finally getting ready to submit this.

Nico, can you do an OWNERS review for the chrome/common and chrome/*.gypi?

[Nico, 2012-08-23 23:18:18]

I'm about to head out until monday, and this looks like something that can't be
blindly stamped. So maybe look for someone else if it's urgent. (Or wait :-) )

[Mattias Nissler (ping if slow), 2012-08-23 23:21:18]

Fair, adding Ben :)

[Mattias Nissler (ping if slow), 2012-08-23 23:21:52]

On 2012/08/23 23:21:18, Mattias Nissler wrote:
> Fair, adding Ben :)

Oh, context: I need an OWNERS review for:

chrome/*.gypi
chrome/common/chrome_notification_types.h

[Ben Goodger (Google), 2012-08-27 15:44:09]

gyp lgtm

http://codereview.chromium.org/10828032/diff/25001/chrome/common/chrome_notif...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/10828032/diff/25001/chrome/common/chrome_notif...
chrome/common/chrome_notification_types.h:1077:
NOTIFICATION_OWNERSHIP_STATUS_CHANGED,
General comment for whoever works on this stuff - does it need to use
notifications?

Notifications in general should be a last resort.

[Mattias Nissler (ping if slow), 2012-08-27 16:23:46]

http://codereview.chromium.org/10828032/diff/25001/chrome/common/chrome_notif...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/10828032/diff/25001/chrome/common/chrome_notif...
chrome/common/chrome_notification_types.h:1077:
NOTIFICATION_OWNERSHIP_STATUS_CHANGED,
On 2012/08/27 15:44:09, Ben Goodger (Google) wrote:
> General comment for whoever works on this stuff - does it need to use
> notifications?
> 
> Notifications in general should be a last resort.

I agree wholeheartedly :)

I've tried to capture the point to avoid notifications in the comment I put on
this particular one. Note that the three that come before this one I'm removing
in a follow-up CL here: http://codereview.chromium.org/10823040/

IMHO, a general issue with Chrome OS code in the browser is that relies too
heavily on singletons. That means there is no well-defined
initialization/shutdown order, thus making observer deregistration impossible
when one of the singletons needs to listen for a signal from another singleton.
Even worse, there are cycles in the listens-for-signal-from graph... don't get
me started :)

I'm trying to clean up the code I find while I'm at it, but cutting down on the
number of singletons is certainly a larger effort.

[Mattias Nissler (ping if slow), 2012-08-28 12:34:47]

Ben, Nico, OK to land? Ben mentioned the magic 4 letters but I'm not sure
whether he expected a satisfactory answer to his question.

[Nico, 2012-08-28 15:50:52]

lgtm

[Nico, 2012-08-28 15:51:40]

(is there a tracking bug for getting rid of singleton soup in cros land?)

[Ben Goodger (Google), 2012-08-28 16:27:57]

you can tbr gyp changes like this to me

lgtm

[Mattias Nissler (ping if slow), 2012-08-29 16:17:37]

Thanks Ben and Nico.

FWIW, I filed a bug about the singleton soup:
https://code.google.com/p/chromium/issues/detail?id=145494

[commit-bot: I haz the power, 2012-08-29 16:17:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/10828032/41001

[commit-bot: I haz the power, 2012-08-29 20:34:44]

Try job failure for 10828032-41001 (retry) (retry) on win_rel for step
"browser_tests".
It's a second try, previously, steps "interactive_ui_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-29 22:07:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/10828032/41001

[commit-bot: I haz the power, 2012-08-30 01:01:06]

Try job failure for 10828032-41001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-30 07:13:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/10828032/41001

[commit-bot: I haz the power, 2012-08-30 09:03:49]

Change committed as 154100