Switch from SignedSettings to DeviceSettingsService.

Switch from SignedSettings to DeviceSettingsService.

DevicePolicyCache and DeviceSettingsProvider now go through
DeviceSettingsService, which provides a unified and simpler API to
Chrome OS device settings.

BUG=chromium:139126
TEST=Unit tests, everything related to enterprise enrollment and device settings still works.
TBR=ben@chromium.org


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

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

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

This is the second part that switches existing device settings stuff over to the
new infrastructure. Note that policy and CrosSettings now go through the same
path and share more code.

[pastarmovj, 2012-07-30 13:55:02]

This is my initial set of comments on this one too.

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

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:30: // Public for testing.
This looks dangerous to me. It gets too easy to instantiate a new instance when
the singleton should have been used.

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

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:97:
base::WeakPtrFactory<CrosSettingsTest> weak_factory_;
== Just a random rant begins here. ==
I know that most of the code uses the name weak_factory but i find this name
brain-dead this is a factory for weak pointers so if we are willing to miss a
word for brevity,  rather the qualifier "weak" than the subject "pointer". Weak
factory sounds like a factory that is not strong enough to do its job rather
than a factory that can produce weak pointers. :-(
== Just a random rant ends here ==

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

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:88:
ownership_status_(device_settings_service_->GetOwnershipStatus(false)),
Have you checked if kiosk mode is fine with the possibility that this call will
return UNKNOWN?

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:221: {
Remove the {} too.

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_cache.cc (left):

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_cache.cc:26: void
OnStorePolicyCompleted(SignedSettings::ReturnCode code) {
2 tests come to my mind to verify that this here works as expected:
1. Check the opt-in for metrics on the eula page and verify it is still there
after owning the device.
2. Do the same through the migration code. Create the file /home/chronos/Consent
To Send Stats it can be done again around the owning time and verify this is
also going correctly in the policy blob once it is created.

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (left):

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:348: // configured reporting bits.
Are you sure this is not needed anymore? I remember we had to add this code here
because we were getting in trouble shortly around boot times or some such.

[Nikita (slow), 2012-07-31 05:57:53]

chromeos/login lgtm

[Mattias Nissler (ping if slow), 2012-08-02 12:01:52]

I addressed all comments, ready for another review cycle.

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

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:30: // Public for testing.
On 2012/07/30 13:55:02, pastarmovj wrote:
> This looks dangerous to me. It gets too easy to instantiate a new instance
when
> the singleton should have been used.

Done.

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

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_unittest.cc:97:
base::WeakPtrFactory<CrosSettingsTest> weak_factory_;
On 2012/07/30 13:55:02, pastarmovj wrote:
> == Just a random rant begins here. ==
> I know that most of the code uses the name weak_factory but i find this name
> brain-dead this is a factory for weak pointers so if we are willing to miss a
> word for brevity,  rather the qualifier "weak" than the subject "pointer".
Weak
> factory sounds like a factory that is not strong enough to do its job rather
> than a factory that can produce weak pointers. :-(
> == Just a random rant ends here ==

Well, you know where to direct this. Ignoring :)

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

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/dev...
chrome/browser/chromeos/device_settings_provider.cc:88:
ownership_status_(device_settings_service_->GetOwnershipStatus(false)),
On 2012/07/30 13:55:02, pastarmovj wrote:
> Have you checked if kiosk mode is fine with the possibility that this call
will
> return UNKNOWN?

KioskModeSettings has a proper PrepareTrustedValues guard, and I've checked in
the VM that it still works.

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:221: {
On 2012/07/30 13:55:02, pastarmovj wrote:
> Remove the {} too.

Done.

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_cache.cc (left):

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_cache.cc:26: void
OnStorePolicyCompleted(SignedSettings::ReturnCode code) {
On 2012/07/30 13:55:02, pastarmovj wrote:
> 2 tests come to my mind to verify that this here works as expected:
> 1. Check the opt-in for metrics on the eula page and verify it is still there
> after owning the device.
> 2. Do the same through the migration code. Create the file
/home/chronos/Consent
> To Send Stats it can be done again around the owning time and verify this is
> also going correctly in the policy blob once it is created.

I confirmed that both the cache and migration work fine.

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (left):

http://codereview.chromium.org/10832035/diff/1032/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:348: // configured reporting bits.
On 2012/07/30 13:55:02, pastarmovj wrote:
> Are you sure this is not needed anymore? I remember we had to add this code
here
> because we were getting in trouble shortly around boot times or some such.

The issue was that the policy stuff would start talking to the server before
CrosSettings was initialized. Now that everything goes through
DeviceSettingsService (also token loading), we get the proper device settings in
the first request as well (I tested that).

[pastarmovj, 2012-08-03 13:35:58]

LGTM. Please fix my nit before you commit and consider the suggestion about
preserving the history of a few files that will be lost if you commit like that.

http://codereview.chromium.org/10832035/diff/16001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/cros_settings.h (right):

http://codereview.chromium.org/10832035/diff/16001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.h:98: // Public for testing.
This comment is wrong now.

http://codereview.chromium.org/10832035/diff/16001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_cache.cc (right):

http://codereview.chromium.org/10832035/diff/16001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_cache.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Shouldn't this file be A+? Maybe it has diverged too much to be detected. Could
you preserve the history by making the rename change in a separate CL first? If
you think this history is worth preserving.

http://codereview.chromium.org/10832035/diff/16001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_migration_helper.cc
(right):

http://codereview.chromium.org/10832035/diff/16001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_migration_helper.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
Shouldn't this file be A+? Maybe it has diverged too much to be detected. Could
you preserve the history by making the rename change in a separate CL first? If
you think this history is worth preserving.

http://codereview.chromium.org/10832035/diff/16001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_migration_helper.h
(right):

http://codereview.chromium.org/10832035/diff/16001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_migration_helper.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
Shouldn't this file be A+? Maybe it has diverged too much to be detected. Could
you preserve the history by making the rename change in a separate CL first? If
you think this history is worth preserving.

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

Nikita, can you answer my inline question?

I'll send another review request once the prerequisite CL is good for commit.

http://codereview.chromium.org/10832035/diff/6014/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/10832035/diff/6014/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:379: if
(!login_performer_.get() || num_login_attempts_ <= 1) {
Nikita, are you OK with folding the common parts of CompleteLogin() and Login()
into PerformLogin() as I've done here? Also, they differed in the
num_login_attempts_ <= 1 check here, is that actually needed/intentional?

[Nikita (slow), 2012-08-08 09:46:36]

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

http://codereview.chromium.org/10832035/diff/6014/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:263: void
LoginPerformer::PerformLogin(const std::string& username,
Why it is being renamed?
The point of "CompleteLogin" was that it actually completing login on ChromeOS
side after GAIA login is done.

[Nikita (slow), 2012-08-08 10:28:39]

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

http://codereview.chromium.org/10832035/diff/6014/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:263: void
LoginPerformer::PerformLogin(const std::string& username,
On 2012/08/08 09:46:36, Nikita Kostylev wrote:
> Why it is being renamed?
> The point of "CompleteLogin" was that it actually completing login on ChromeOS
> side after GAIA login is done.

Ok, I think it makes sense to combine these.

[Nikita (slow), 2012-08-08 10:50:02]

lgtm

http://codereview.chromium.org/10832035/diff/6014/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/10832035/diff/6014/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:379: if
(!login_performer_.get() || num_login_attempts_ <= 1) {
On 2012/08/06 21:38:00, Mattias Nissler wrote:
> Nikita, are you OK with folding the common parts of CompleteLogin() and
Login()
> into PerformLogin() as I've done here? Also, they differed in the
> num_login_attempts_ <= 1 check here, is that actually needed/intentional?

num_login_attempts_ is reset to 0 when new user starts login attempt.
That will make sure that new LoginPerformer instance is created.

[Mattias Nissler (ping if slow), 2012-08-23 23:11:01]

Julian, I made some changes to DeviceSettingsProvider, and the current code now
passes all the manual tests I could think of. I think you should take another
look before I submit this.

[pastarmovj, 2012-08-24 15:31:25]

On 2012/08/23 23:11:01, Mattias Nissler wrote:
> Julian, I made some changes to DeviceSettingsProvider, and the current code
now
> passes all the manual tests I could think of. I think you should take another
> look before I submit this.

still lgtm

[Mattias Nissler (ping if slow), 2012-08-30 20:17:07]

I'm finally getting ready to submit this.

Julian, can you take a look at DeviceSettingsProvider again (I've simplified
migration).

Nikita: I sneaked in a crash fix, see the comment below. Can you confirm that's
OK?

http://codereview.chromium.org/10832035/diff/33001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/10832035/diff/33001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/existing_user_controller.cc:232: if (host_ !=
NULL) {
Nikita: I added this to avoid crashing if there's a notification when the
|host_| is already gone. Makes sense?

[Nikita (slow), 2012-08-31 12:45:16]

http://codereview.chromium.org/10832035/diff/33001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/10832035/diff/33001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/existing_user_controller.cc:232: if (host_ !=
NULL) {
On 2012/08/30 20:17:07, Mattias Nissler wrote:
> Nikita: I added this to avoid crashing if there's a notification when the
> |host_| is already gone. Makes sense?

ok

[Nikita (slow), 2012-08-31 12:45:21]

lgtm

[commit-bot: I haz the power, 2012-08-31 15:10:04]

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

[commit-bot: I haz the power, 2012-08-31 15:10:28]

Presubmit check for 10832035-37003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 3.6s to calculate.

[Mattias Nissler (ping if slow), 2012-08-31 15:22:47]

TBR Ben for the gyp and chrome/common changes.

[commit-bot: I haz the power, 2012-08-31 15:22:59]

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

[commit-bot: I haz the power, 2012-08-31 17:36:12]

Change committed as 154457

[commit-bot: I haz the power, 2012-09-01 18:20:30]

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

[commit-bot: I haz the power, 2012-09-01 20:47:22]

Change committed as 154625

[(unused - use chromium), 2017-04-27 21:08:40]

thakis@google.com changed reviewers:
+ thakis@google.com

[(unused - use chromium), 2017-04-27 21:08:42]

The dreaded 4-year-7-months-later code review!

https://codereview.chromium.org/10832035/diff/47002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/10832035/diff/47002/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:1193:
'browser/chromeos/settings/signed_settings_cache_unittest.cc',
This CL renamed browser/chromeos/settings/signed_settings_cache_unittest.cc to
browser/chromeos/settings/device_settings_cache_unittest.cc and removed the old
file from chrome_tests.gypi but didn't add the new name. The file has been
missing from build files ever since. Do you want to try to add it to
chrome/browser/chromeos/BUILD.gn and see what it takes to get it to compile and
paass?

[Mattias Nissler (ping if slow), 2017-04-28 08:48:29]

https://codereview.chromium.org/10832035/diff/47002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/10832035/diff/47002/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:1193:
'browser/chromeos/settings/signed_settings_cache_unittest.cc',
On 2017/04/27 21:08:42, (unused - use chromium) wrote:
> This CL renamed browser/chromeos/settings/signed_settings_cache_unittest.cc to
> browser/chromeos/settings/device_settings_cache_unittest.cc and removed the
old
> file from chrome_tests.gypi but didn't add the new name. The file has been
> missing from build files ever since. Do you want to try to add it to
> chrome/browser/chromeos/BUILD.gn and see what it takes to get it to compile
and
> paass?

Uh, good catch. I can take a quick look to see whether this situation can be
rescued. Fortunately the file is rather short! ;-)