Switch to direct use of OwnerSettingsServiceChromeOS::Set() in tests.

Get rid of excessive CrosSettings::Get()->Set*() usage, mostly in tests.

The following tests were affected:

unit_tests:
AttestationPolicyObserverTest.*
CryptohomeAuthenticatorTest.*
ExtensionCacheTest.*
EPKPChallengeMachineKeyTest.DevicePolicyDisabled
EPKPChallengeUserKeyTest.DevicePolicyDisabled
HeartbeatSchedulerTest.*
NetworkConfigurationUpdaterTest.*
PlatformVerificationFlowTest.*
StatusUploaderTest.*
ShutdownPolicyHandlerTest.*
UserManagerTest.*

browser_tests:
DeviceStatusCollectorTest.*
KioskAppManagerTest.*
KioskEnterpriseTest.*
KioskTest.*
KioskUpdateTest.*

BUG=433840
Committed: https://crrev.com/3a599661a49fa5ebfe63cb4016bb8efcd2d02712
Cr-Commit-Position: refs/heads/master@{#326338}

[Ivan Podogov, 2015-03-26 08:30:31]

ginkage@chromium.org changed reviewers:
+ bartfab@chromium.org, oshima@chromium.org

[Ivan Podogov, 2015-03-26 12:25:17]

ginkage@chromium.org changed reviewers:
+ miket@chromium.org

[Mattias Nissler (ping if slow), 2015-03-27 12:42:04]

mnissler@chromium.org changed reviewers:
+ mnissler@chromium.org

[Mattias Nissler (ping if slow), 2015-03-27 12:42:06]

OK, I didn't look at tests but put some high-level comments. In general, I think
this is a step in the right direction.

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:188:
OwnerSettingsServiceChromeOS* service) {
It may be simpler to just move the code here to KioskAppsHandler, which runs in
context of a Profile anyways, so has access to the corresponding
OwnerSettingsServiceChromeOS instance. KioskAppManager would then become a
read-only interface as well, which seems cleaner given that it isn't tied to a
specific profile.

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:642: } else
if (g_test_owner_settings && path == kExtensionCacheSize) {
yuck - why is this needed?

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_provider.cc:722: if
(trusted_for_test_)
Uh, why is this needed?

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1019: Profile* profile =
ProfileHelper::Get()->GetProfileByUser(user);
Might be simpler to use ProfileManager::GetActiveUserProfile()?

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1126: Profile* profile =
ProfileHelper::Get()->GetProfileByUser(user);
ditto

[Ivan Podogov, 2015-03-27 12:51:14]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:642: } else
if (g_test_owner_settings && path == kExtensionCacheSize) {
On 2015/03/27 12:42:06, Mattias Nissler wrote:
> yuck - why is this needed?

You may see below a list of settings that are not really meant to be set by
user, hence those are read-only. But we need a way to specify them in tests.
Until now, it was by using CrosSettings::Set(), which is not the way to go.
Another approach would be using the (stub) DeviceSettingsProvider::Set(), which
might also become deprecated pretty quickly... This seemed like a fair
compromise.

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_provider.cc:722: if
(trusted_for_test_)
On 2015/03/27 12:42:06, Mattias Nissler wrote:
> Uh, why is this needed?

Because the first time the owner settings observers are called, they get
untrusted values, and those may then go on waiting for the settings to become
trusted some time in the future. And so the tests become broken, as right now
stub settings provider that's used in tests just always returns TRUSTED.

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1019: Profile* profile =
ProfileHelper::Get()->GetProfileByUser(user);
On 2015/03/27 12:42:06, Mattias Nissler wrote:
> Might be simpler to use ProfileManager::GetActiveUserProfile()?

Hm... Yes, it would probably be better. Maybe I should change the code at line
995 to use it as well.

[Mattias Nissler (ping if slow), 2015-03-27 12:58:53]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:642: } else
if (g_test_owner_settings && path == kExtensionCacheSize) {
On 2015/03/27 12:51:14, Ivan Podogov wrote:
> On 2015/03/27 12:42:06, Mattias Nissler wrote:
> > yuck - why is this needed?
> 
> You may see below a list of settings that are not really meant to be set by
> user, hence those are read-only. But we need a way to specify them in tests.
> Until now, it was by using CrosSettings::Set(), which is not the way to go.
> Another approach would be using the (stub) DeviceSettingsProvider::Set(),
which
> might also become deprecated pretty quickly... This seemed like a fair
> compromise.

Which are these tests? Can't we inject the necessary settings by manipulating
the underlying device settings (assuming these tests use a production
CrosSettings instance now)?

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_provider.cc:722: if
(trusted_for_test_)
On 2015/03/27 12:51:14, Ivan Podogov wrote:
> On 2015/03/27 12:42:06, Mattias Nissler wrote:
> > Uh, why is this needed?
> 
> Because the first time the owner settings observers are called, they get
> untrusted values, and those may then go on waiting for the settings to become
> trusted some time in the future. And so the tests become broken, as right now
> stub settings provider that's used in tests just always returns TRUSTED.

There are a number of tests that prepare device settings in their Setup() to get
them into trusted state. Can you point me at test tests that are broken?

[Ivan Podogov, 2015-03-27 13:10:50]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:642: } else
if (g_test_owner_settings && path == kExtensionCacheSize) {
On 2015/03/27 12:58:52, Mattias Nissler wrote:
> On 2015/03/27 12:51:14, Ivan Podogov wrote:
> > On 2015/03/27 12:42:06, Mattias Nissler wrote:
> > > yuck - why is this needed?
> > 
> > You may see below a list of settings that are not really meant to be set by
> > user, hence those are read-only. But we need a way to specify them in tests.
> > Until now, it was by using CrosSettings::Set(), which is not the way to go.
> > Another approach would be using the (stub) DeviceSettingsProvider::Set(),
> which
> > might also become deprecated pretty quickly... This seemed like a fair
> > compromise.
> 
> Which are these tests? Can't we inject the necessary settings by manipulating
> the underlying device settings (assuming these tests use a production
> CrosSettings instance now)?

In order of the settings listed here (their names basically speak for
themselves):
chrome/browser/chromeos/policy/extension_cache_unittest.cc (this one was
actually added by me, so as far as I understand we may remove
"g_test_owner_settings &&" from this one safely)
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc
chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc
chrome/browser/chromeos/policy/status_uploader_unittest.cc

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_provider.cc:722: if
(trusted_for_test_)
On 2015/03/27 12:58:52, Mattias Nissler wrote:
> On 2015/03/27 12:51:14, Ivan Podogov wrote:
> > On 2015/03/27 12:42:06, Mattias Nissler wrote:
> > > Uh, why is this needed?
> > 
> > Because the first time the owner settings observers are called, they get
> > untrusted values, and those may then go on waiting for the settings to
become
> > trusted some time in the future. And so the tests become broken, as right
now
> > stub settings provider that's used in tests just always returns TRUSTED.
> 
> There are a number of tests that prepare device settings in their Setup() to
get
> them into trusted state. Can you point me at test tests that are broken?

Off the top of my head those would be HeartbeatSchedulerTest and
ShutdownPolicyHandlerTest, maybe some others.

[Mattias Nissler (ping if slow), 2015-03-27 13:45:58]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:642: } else
if (g_test_owner_settings && path == kExtensionCacheSize) {
On 2015/03/27 13:10:50, Ivan Podogov wrote:
> On 2015/03/27 12:58:52, Mattias Nissler wrote:
> > On 2015/03/27 12:51:14, Ivan Podogov wrote:
> > > On 2015/03/27 12:42:06, Mattias Nissler wrote:
> > > > yuck - why is this needed?
> > > 
> > > You may see below a list of settings that are not really meant to be set
by
> > > user, hence those are read-only. But we need a way to specify them in
tests.
> > > Until now, it was by using CrosSettings::Set(), which is not the way to
go.
> > > Another approach would be using the (stub) DeviceSettingsProvider::Set(),
> > which
> > > might also become deprecated pretty quickly... This seemed like a fair
> > > compromise.
> > 
> > Which are these tests? Can't we inject the necessary settings by
manipulating
> > the underlying device settings (assuming these tests use a production
> > CrosSettings instance now)?
> 
> In order of the settings listed here (their names basically speak for
> themselves):
> chrome/browser/chromeos/policy/extension_cache_unittest.cc (this one was
> actually added by me, so as far as I understand we may remove
> "g_test_owner_settings &&" from this one safely)
> chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
>
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
> chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc
> chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc
> chrome/browser/chromeos/policy/status_uploader_unittest.cc

Looked at this - I think the correct approach is to either rely on
StubCrosSettingsProvider and use a Set method (which is fine) or if the test
uses DeviceSettingsTestHelper, update the device settings under the hood via the
test helper and then call ReloadDeviceSettings().

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:55: class
HeartbeatSchedulerTest : public chromeos::OwnerSettingsTestBase {
Instead of pulling in the heavy-weight full device settings test dependency
here, why not leave StubCrosSettingsProvider in place as before and make it have
its own Set() function that isn't related to CrosSettingsProvider::DoSet()? I
think it's fine to have Set() in test code in order to create the environment
the test needs.

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_provider.cc:722: if
(trusted_for_test_)
On 2015/03/27 13:10:50, Ivan Podogov wrote:
> On 2015/03/27 12:58:52, Mattias Nissler wrote:
> > On 2015/03/27 12:51:14, Ivan Podogov wrote:
> > > On 2015/03/27 12:42:06, Mattias Nissler wrote:
> > > > Uh, why is this needed?
> > > 
> > > Because the first time the owner settings observers are called, they get
> > > untrusted values, and those may then go on waiting for the settings to
> become
> > > trusted some time in the future. And so the tests become broken, as right
> now
> > > stub settings provider that's used in tests just always returns TRUSTED.
> > 
> > There are a number of tests that prepare device settings in their Setup() to
> get
> > them into trusted state. Can you point me at test tests that are broken?
> 
> Off the top of my head those would be HeartbeatSchedulerTest and
> ShutdownPolicyHandlerTest, maybe some others.

I think it's fine to actually retain StubSettingsProvider, see other comment in
heartbeat_scheduler_unittest.cc

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_test_helper.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_test_helper.cc:280:
settings_provider_->set_trusted_for_test(true);
This shouldn't be needed if you replace FlushDeviceSettings() with
ReloadDeviceSettings() below. The latter should put the system into TRUSTED
state as long as the owner key is available.

However, please see my other comment regarding the question whether we actually
need to migrate test code away from StubCrosSettingsProvider.

[Ivan Podogov, 2015-03-27 14:40:59]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:55: class
HeartbeatSchedulerTest : public chromeos::OwnerSettingsTestBase {
On 2015/03/27 13:45:58, Mattias Nissler wrote:
> Instead of pulling in the heavy-weight full device settings test dependency
> here, why not leave StubCrosSettingsProvider in place as before and make it
have
> its own Set() function that isn't related to CrosSettingsProvider::DoSet()? I
> think it's fine to have Set() in test code in order to create the environment
> the test needs.

True, sometimes it's the only option left... Although I really like how the
tests had their size, copy-paste amount and the number of stuff included in them
reduced. See the ExtensionCache test, for example: it basically has every single
irrelevant thing removed from it. And the tests themselves are more end-to end
in this case, they even check correctness of the policy protobuf decoding (which
isn't really checked anywhere else).

[Mattias Nissler (ping if slow), 2015-03-27 16:20:23]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:55: class
HeartbeatSchedulerTest : public chromeos::OwnerSettingsTestBase {
On 2015/03/27 14:40:59, Ivan Podogov wrote:
> On 2015/03/27 13:45:58, Mattias Nissler wrote:
> > Instead of pulling in the heavy-weight full device settings test dependency
> > here, why not leave StubCrosSettingsProvider in place as before and make it
> have
> > its own Set() function that isn't related to CrosSettingsProvider::DoSet()?
I
> > think it's fine to have Set() in test code in order to create the
environment
> > the test needs.
> 
> True, sometimes it's the only option left... Although I really like how the
> tests had their size, copy-paste amount and the number of stuff included in
them
> reduced. See the ExtensionCache test, for example: it basically has every
single
> irrelevant thing removed from it. And the tests themselves are more end-to end
> in this case, they even check correctness of the policy protobuf decoding
(which
> isn't really checked anywhere else).

The boilerplate reduction is certainly great. However, you can achieve the same
without pulling in DeviceSettingsTestHelper by just factoring the common
StubCrosSettingsProvider stuff that's currently copied-and-pasted into a common
base class.

Another argument in favor of not pulling in DeviceSettingsTestHelper is that all
the affected tests (at least the ones I've looked at) are unit tests, so it
doesn't make sense to have most of the production machinery for device settings
that does the entire round trip to SessionManagerClient running within the test
(and being implicitly tested as well). Instead, you want to mock that away as
good as you can, and StubCrosSettingsProvider is great at that.

[Ivan Podogov, 2015-03-30 08:54:28]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:55: class
HeartbeatSchedulerTest : public chromeos::OwnerSettingsTestBase {
On 2015/03/27 16:20:23, Mattias Nissler wrote:
> On 2015/03/27 14:40:59, Ivan Podogov wrote:
> > On 2015/03/27 13:45:58, Mattias Nissler wrote:
> > > Instead of pulling in the heavy-weight full device settings test
dependency
> > > here, why not leave StubCrosSettingsProvider in place as before and make
it
> > have
> > > its own Set() function that isn't related to
CrosSettingsProvider::DoSet()?
> I
> > > think it's fine to have Set() in test code in order to create the
> environment
> > > the test needs.
> > 
> > True, sometimes it's the only option left... Although I really like how the
> > tests had their size, copy-paste amount and the number of stuff included in
> them
> > reduced. See the ExtensionCache test, for example: it basically has every
> single
> > irrelevant thing removed from it. And the tests themselves are more end-to
end
> > in this case, they even check correctness of the policy protobuf decoding
> (which
> > isn't really checked anywhere else).
> 
> The boilerplate reduction is certainly great. However, you can achieve the
same
> without pulling in DeviceSettingsTestHelper by just factoring the common
> StubCrosSettingsProvider stuff that's currently copied-and-pasted into a
common
> base class.
> 
> Another argument in favor of not pulling in DeviceSettingsTestHelper is that
all
> the affected tests (at least the ones I've looked at) are unit tests, so it
> doesn't make sense to have most of the production machinery for device
settings
> that does the entire round trip to SessionManagerClient running within the
test
> (and being implicitly tested as well). Instead, you want to mock that away as
> good as you can, and StubCrosSettingsProvider is great at that.

OK, I guess I'll make such a class.
Do you think we should add SetInteger/SetBoolean/SetString methods to the
CrosSettingsProvider (or at least to the StubCrosSettingsProvider) for
convenience?

[Mattias Nissler (ping if slow), 2015-03-30 09:03:51]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:55: class
HeartbeatSchedulerTest : public chromeos::OwnerSettingsTestBase {
On 2015/03/30 08:54:28, Ivan Podogov wrote:
> On 2015/03/27 16:20:23, Mattias Nissler wrote:
> > On 2015/03/27 14:40:59, Ivan Podogov wrote:
> > > On 2015/03/27 13:45:58, Mattias Nissler wrote:
> > > > Instead of pulling in the heavy-weight full device settings test
> dependency
> > > > here, why not leave StubCrosSettingsProvider in place as before and make
> it
> > > have
> > > > its own Set() function that isn't related to
> CrosSettingsProvider::DoSet()?
> > I
> > > > think it's fine to have Set() in test code in order to create the
> > environment
> > > > the test needs.
> > > 
> > > True, sometimes it's the only option left... Although I really like how
the
> > > tests had their size, copy-paste amount and the number of stuff included
in
> > them
> > > reduced. See the ExtensionCache test, for example: it basically has every
> > single
> > > irrelevant thing removed from it. And the tests themselves are more end-to
> end
> > > in this case, they even check correctness of the policy protobuf decoding
> > (which
> > > isn't really checked anywhere else).
> > 
> > The boilerplate reduction is certainly great. However, you can achieve the
> same
> > without pulling in DeviceSettingsTestHelper by just factoring the common
> > StubCrosSettingsProvider stuff that's currently copied-and-pasted into a
> common
> > base class.
> > 
> > Another argument in favor of not pulling in DeviceSettingsTestHelper is that
> all
> > the affected tests (at least the ones I've looked at) are unit tests, so it
> > doesn't make sense to have most of the production machinery for device
> settings
> > that does the entire round trip to SessionManagerClient running within the
> test
> > (and being implicitly tested as well). Instead, you want to mock that away
as
> > good as you can, and StubCrosSettingsProvider is great at that.
> 
> OK, I guess I'll make such a class.
> Do you think we should add SetInteger/SetBoolean/SetString methods to the
> CrosSettingsProvider (or at least to the StubCrosSettingsProvider) for
> convenience?

Judgement call, depends on how convenient it ends up being ;) If the boilerplate
it avoids isn't too significant and there are only few callers, then don't
bother. If you find yourself adding the same block of code in various places,
add helpers.

[Ivan Podogov, 2015-03-30 09:10:23]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:55: class
HeartbeatSchedulerTest : public chromeos::OwnerSettingsTestBase {
On 2015/03/30 09:03:51, Mattias Nissler wrote:
> On 2015/03/30 08:54:28, Ivan Podogov wrote:
> > On 2015/03/27 16:20:23, Mattias Nissler wrote:
> > > On 2015/03/27 14:40:59, Ivan Podogov wrote:
> > > > On 2015/03/27 13:45:58, Mattias Nissler wrote:
> > > > > Instead of pulling in the heavy-weight full device settings test
> > dependency
> > > > > here, why not leave StubCrosSettingsProvider in place as before and
make
> > it
> > > > have
> > > > > its own Set() function that isn't related to
> > CrosSettingsProvider::DoSet()?
> > > I
> > > > > think it's fine to have Set() in test code in order to create the
> > > environment
> > > > > the test needs.
> > > > 
> > > > True, sometimes it's the only option left... Although I really like how
> the
> > > > tests had their size, copy-paste amount and the number of stuff included
> in
> > > them
> > > > reduced. See the ExtensionCache test, for example: it basically has
every
> > > single
> > > > irrelevant thing removed from it. And the tests themselves are more
end-to
> > end
> > > > in this case, they even check correctness of the policy protobuf
decoding
> > > (which
> > > > isn't really checked anywhere else).
> > > 
> > > The boilerplate reduction is certainly great. However, you can achieve the
> > same
> > > without pulling in DeviceSettingsTestHelper by just factoring the common
> > > StubCrosSettingsProvider stuff that's currently copied-and-pasted into a
> > common
> > > base class.
> > > 
> > > Another argument in favor of not pulling in DeviceSettingsTestHelper is
that
> > all
> > > the affected tests (at least the ones I've looked at) are unit tests, so
it
> > > doesn't make sense to have most of the production machinery for device
> > settings
> > > that does the entire round trip to SessionManagerClient running within the
> > test
> > > (and being implicitly tested as well). Instead, you want to mock that away
> as
> > > good as you can, and StubCrosSettingsProvider is great at that.
> > 
> > OK, I guess I'll make such a class.
> > Do you think we should add SetInteger/SetBoolean/SetString methods to the
> > CrosSettingsProvider (or at least to the StubCrosSettingsProvider) for
> > convenience?
> 
> Judgement call, depends on how convenient it ends up being ;) If the
boilerplate
> it avoids isn't too significant and there are only few callers, then don't
> bother. If you find yourself adding the same block of code in various places,
> add helpers.

I'm going to go over more than a dozen tests, and add "Set(...,
base::FundamentalValue(...))" to all of them. All those helpers will be trivial
one-liners that will hide FundamentalValue (and the corresponding include) and
somewhat increase readability. So, for me personally, it would be much nicer to
have them. :)

[Mattias Nissler (ping if slow), 2015-03-30 09:11:08]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:55: class
HeartbeatSchedulerTest : public chromeos::OwnerSettingsTestBase {
On 2015/03/30 09:10:23, Ivan Podogov wrote:
> On 2015/03/30 09:03:51, Mattias Nissler wrote:
> > On 2015/03/30 08:54:28, Ivan Podogov wrote:
> > > On 2015/03/27 16:20:23, Mattias Nissler wrote:
> > > > On 2015/03/27 14:40:59, Ivan Podogov wrote:
> > > > > On 2015/03/27 13:45:58, Mattias Nissler wrote:
> > > > > > Instead of pulling in the heavy-weight full device settings test
> > > dependency
> > > > > > here, why not leave StubCrosSettingsProvider in place as before and
> make
> > > it
> > > > > have
> > > > > > its own Set() function that isn't related to
> > > CrosSettingsProvider::DoSet()?
> > > > I
> > > > > > think it's fine to have Set() in test code in order to create the
> > > > environment
> > > > > > the test needs.
> > > > > 
> > > > > True, sometimes it's the only option left... Although I really like
how
> > the
> > > > > tests had their size, copy-paste amount and the number of stuff
included
> > in
> > > > them
> > > > > reduced. See the ExtensionCache test, for example: it basically has
> every
> > > > single
> > > > > irrelevant thing removed from it. And the tests themselves are more
> end-to
> > > end
> > > > > in this case, they even check correctness of the policy protobuf
> decoding
> > > > (which
> > > > > isn't really checked anywhere else).
> > > > 
> > > > The boilerplate reduction is certainly great. However, you can achieve
the
> > > same
> > > > without pulling in DeviceSettingsTestHelper by just factoring the common
> > > > StubCrosSettingsProvider stuff that's currently copied-and-pasted into a
> > > common
> > > > base class.
> > > > 
> > > > Another argument in favor of not pulling in DeviceSettingsTestHelper is
> that
> > > all
> > > > the affected tests (at least the ones I've looked at) are unit tests, so
> it
> > > > doesn't make sense to have most of the production machinery for device
> > > settings
> > > > that does the entire round trip to SessionManagerClient running within
the
> > > test
> > > > (and being implicitly tested as well). Instead, you want to mock that
away
> > as
> > > > good as you can, and StubCrosSettingsProvider is great at that.
> > > 
> > > OK, I guess I'll make such a class.
> > > Do you think we should add SetInteger/SetBoolean/SetString methods to the
> > > CrosSettingsProvider (or at least to the StubCrosSettingsProvider) for
> > > convenience?
> > 
> > Judgement call, depends on how convenient it ends up being ;) If the
> boilerplate
> > it avoids isn't too significant and there are only few callers, then don't
> > bother. If you find yourself adding the same block of code in various
places,
> > add helpers.
> 
> I'm going to go over more than a dozen tests, and add "Set(...,
> base::FundamentalValue(...))" to all of them. All those helpers will be
trivial
> one-liners that will hide FundamentalValue (and the corresponding include) and
> somewhat increase readability. So, for me personally, it would be much nicer
to
> have them. :)

I'd say go a head and add them then :)

[Ivan Podogov, 2015-03-30 14:22:57]

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:55: class
HeartbeatSchedulerTest : public chromeos::OwnerSettingsTestBase {
On 2015/03/30 09:11:08, Mattias Nissler wrote:
> On 2015/03/30 09:10:23, Ivan Podogov wrote:
> > On 2015/03/30 09:03:51, Mattias Nissler wrote:
> > > On 2015/03/30 08:54:28, Ivan Podogov wrote:
> > > > On 2015/03/27 16:20:23, Mattias Nissler wrote:
> > > > > On 2015/03/27 14:40:59, Ivan Podogov wrote:
> > > > > > On 2015/03/27 13:45:58, Mattias Nissler wrote:
> > > > > > > Instead of pulling in the heavy-weight full device settings test
> > > > dependency
> > > > > > > here, why not leave StubCrosSettingsProvider in place as before
and
> > make
> > > > it
> > > > > > have
> > > > > > > its own Set() function that isn't related to
> > > > CrosSettingsProvider::DoSet()?
> > > > > I
> > > > > > > think it's fine to have Set() in test code in order to create the
> > > > > environment
> > > > > > > the test needs.
> > > > > > 
> > > > > > True, sometimes it's the only option left... Although I really like
> how
> > > the
> > > > > > tests had their size, copy-paste amount and the number of stuff
> included
> > > in
> > > > > them
> > > > > > reduced. See the ExtensionCache test, for example: it basically has
> > every
> > > > > single
> > > > > > irrelevant thing removed from it. And the tests themselves are more
> > end-to
> > > > end
> > > > > > in this case, they even check correctness of the policy protobuf
> > decoding
> > > > > (which
> > > > > > isn't really checked anywhere else).
> > > > > 
> > > > > The boilerplate reduction is certainly great. However, you can achieve
> the
> > > > same
> > > > > without pulling in DeviceSettingsTestHelper by just factoring the
common
> > > > > StubCrosSettingsProvider stuff that's currently copied-and-pasted into
a
> > > > common
> > > > > base class.
> > > > > 
> > > > > Another argument in favor of not pulling in DeviceSettingsTestHelper
is
> > that
> > > > all
> > > > > the affected tests (at least the ones I've looked at) are unit tests,
so
> > it
> > > > > doesn't make sense to have most of the production machinery for device
> > > > settings
> > > > > that does the entire round trip to SessionManagerClient running within
> the
> > > > test
> > > > > (and being implicitly tested as well). Instead, you want to mock that
> away
> > > as
> > > > > good as you can, and StubCrosSettingsProvider is great at that.
> > > > 
> > > > OK, I guess I'll make such a class.
> > > > Do you think we should add SetInteger/SetBoolean/SetString methods to
the
> > > > CrosSettingsProvider (or at least to the StubCrosSettingsProvider) for
> > > > convenience?
> > > 
> > > Judgement call, depends on how convenient it ends up being ;) If the
> > boilerplate
> > > it avoids isn't too significant and there are only few callers, then don't
> > > bother. If you find yourself adding the same block of code in various
> places,
> > > add helpers.
> > 
> > I'm going to go over more than a dozen tests, and add "Set(...,
> > base::FundamentalValue(...))" to all of them. All those helpers will be
> trivial
> > one-liners that will hide FundamentalValue (and the corresponding include)
and
> > somewhat increase readability. So, for me personally, it would be much nicer
> to
> > have them. :)
> 
> I'd say go a head and add them then :)

OK, so I suppose the new patchset should look more like what you had in mind.

[Mattias Nissler (ping if slow), 2015-03-30 14:45:46]

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.h (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:126:
OwnerSettingsServiceChromeOS* service);
Any thoughts regarding my earlier comment to move the write path out of
KioskAppManager?

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:79:
public chromeos::SettingsProviderTestBase {
The coding style generally bans multiple inheritance:

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Multiple_Inh...

There's an exception for pure interfaces, but I don't think this applies here.
You might achieve your goal as well by instantiating your helper as a class
member.

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:88:
virtual ~AttestationPolicyObserverTest() { RestoreProvider(); }
Is there a reason RestoreProvider() can't just be called from
~SettingsProviderTestBase()?

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
(right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:439:
ReplaceProvider(kPolicyMissingMitigationMode);
If you made your helper into a ScopedCrosSettingsTestHelper (as suggested in the
other comment that proposed making it a class member), then you could just
declare one here and it could tear itself down automatically when the function
exits.

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:39:
provider->Set(setting, value);
This is not going to mix well with our goal to remove
CrosSettingsProvider::Set()

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.h (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.h:11: #include
"chrome/browser/chromeos/settings/cros_settings.h"
not needed?

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/settings_provider_test_base.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.cc:52: new
chromeos::FakeOwnerSettingsService(profile));
Does this also correctly associate the created service with
FakeOwnerSettingsServiceFactory?

FWIW, the proper way to replace Profile-KeyedService implementations for testing
is via TestingProfile::Builder::AddTestingFactory.

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/settings_provider_test_base.h (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.h:24:
SettingsProviderTestBase();
nit: blank line before comment.

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.h:47:
DISALLOW_COPY_AND_ASSIGN(SettingsProviderTestBase);
#include "base/macros.h"

https://codereview.chromium.org/1019283004/diff/160001/chromeos/settings/cros...
File chromeos/settings/cros_settings_provider.h (right):

https://codereview.chromium.org/1019283004/diff/160001/chromeos/settings/cros...
chromeos/settings/cros_settings_provider.h:47: // Convenience forms of Set(). 
These methods will replace any existing
These should be defined in StubSettingsProvider - remember our goal is  to get
rid of CrosSettingsProvider::Set().

[Ivan Podogov, 2015-03-31 08:40:00]

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.h (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:126:
OwnerSettingsServiceChromeOS* service);
On 2015/03/30 14:45:45, Mattias Nissler wrote:
> Any thoughts regarding my earlier comment to move the write path out of
> KioskAppManager?

Yes, it is possible, although both of the browsertests will need either to be
rewritten from scratch, or moved to
chrome/browser/ui/webui/extensions/chromeos...

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:79:
public chromeos::SettingsProviderTestBase {
On 2015/03/30 14:45:46, Mattias Nissler wrote:
> The coding style generally bans multiple inheritance:
> 
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Multiple_Inh...
> 
> There's an exception for pure interfaces, but I don't think this applies here.
> You might achieve your goal as well by instantiating your helper as a class
> member.

Okay.

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:88:
virtual ~AttestationPolicyObserverTest() { RestoreProvider(); }
On 2015/03/30 14:45:45, Mattias Nissler wrote:
> Is there a reason RestoreProvider() can't just be called from
> ~SettingsProviderTestBase()?

In some cases it cannot: if there is already a ScopedTestCrosSettings in the
test (like in CryptohomeAuthenticatorTest), this leads to a crash... I suppose
this shouldn't be an issue anymore if I change this to a
ScopedCrosSettingsTestHelper.

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:39:
provider->Set(setting, value);
On 2015/03/30 14:45:46, Mattias Nissler wrote:
> This is not going to mix well with our goal to remove
> CrosSettingsProvider::Set()

As doesn't the direct use of CrosSettingsProvider::Set() in all the tests... I'm
not sure what better option there is.

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/settings_provider_test_base.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.cc:52: new
chromeos::FakeOwnerSettingsService(profile));
On 2015/03/30 14:45:46, Mattias Nissler wrote:
> Does this also correctly associate the created service with
> FakeOwnerSettingsServiceFactory?
> 
> FWIW, the proper way to replace Profile-KeyedService implementations for
testing
> is via TestingProfile::Builder::AddTestingFactory.

It doesn't. I'll add the builder where necessary.

[Mattias Nissler (ping if slow), 2015-03-31 08:44:48]

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:39:
provider->Set(setting, value);
On 2015/03/31 08:40:00, Ivan Podogov wrote:
> On 2015/03/30 14:45:46, Mattias Nissler wrote:
> > This is not going to mix well with our goal to remove
> > CrosSettingsProvider::Set()
> 
> As doesn't the direct use of CrosSettingsProvider::Set() in all the tests...
I'm
> not sure what better option there is.

Ah, so we had our wires crossed here - I think all the tests have a
StubCrosSettingsProvider instance. We can have a Set() on that that we can call
from tests, but still remove CrosSettignsProvider::Set(). Or am I missing
something?

[Ivan Podogov, 2015-03-31 08:51:12]

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:39:
provider->Set(setting, value);
On 2015/03/31 08:44:48, Mattias Nissler wrote:
> On 2015/03/31 08:40:00, Ivan Podogov wrote:
> > On 2015/03/30 14:45:46, Mattias Nissler wrote:
> > > This is not going to mix well with our goal to remove
> > > CrosSettingsProvider::Set()
> > 
> > As doesn't the direct use of CrosSettingsProvider::Set() in all the tests...
> I'm
> > not sure what better option there is.
> 
> Ah, so we had our wires crossed here - I think all the tests have a
> StubCrosSettingsProvider instance. We can have a Set() on that that we can
call
> from tests, but still remove CrosSettignsProvider::Set(). Or am I missing
> something?

Hm, this should be very well possible, although it would require
FakeOwnerSettingsService to have the StubCrosSettingsProvider instance access
here. We could either add some SetProvider() to FakeOwnerSettingsService class
(or require to pass a provider in the constructor), or just expect
CrosSettings::GetProvider() to always return a StubCrosSettingsProvider here.
What do you think works better?

[Mattias Nissler (ping if slow), 2015-03-31 08:56:03]

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:39:
provider->Set(setting, value);
On 2015/03/31 08:51:12, Ivan Podogov wrote:
> On 2015/03/31 08:44:48, Mattias Nissler wrote:
> > On 2015/03/31 08:40:00, Ivan Podogov wrote:
> > > On 2015/03/30 14:45:46, Mattias Nissler wrote:
> > > > This is not going to mix well with our goal to remove
> > > > CrosSettingsProvider::Set()
> > > 
> > > As doesn't the direct use of CrosSettingsProvider::Set() in all the
tests...
> > I'm
> > > not sure what better option there is.
> > 
> > Ah, so we had our wires crossed here - I think all the tests have a
> > StubCrosSettingsProvider instance. We can have a Set() on that that we can
> call
> > from tests, but still remove CrosSettignsProvider::Set(). Or am I missing
> > something?
> 
> Hm, this should be very well possible, although it would require
> FakeOwnerSettingsService to have the StubCrosSettingsProvider instance access
> here. We could either add some SetProvider() to FakeOwnerSettingsService class
> (or require to pass a provider in the constructor), or just expect
> CrosSettings::GetProvider() to always return a StubCrosSettingsProvider here.
> What do you think works better?

Why would FakeOwnerSettingsService need to write settings at all?

[Ivan Podogov, 2015-03-31 09:01:21]

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:39:
provider->Set(setting, value);
On 2015/03/31 08:56:03, Mattias Nissler wrote:
> On 2015/03/31 08:51:12, Ivan Podogov wrote:
> > On 2015/03/31 08:44:48, Mattias Nissler wrote:
> > > On 2015/03/31 08:40:00, Ivan Podogov wrote:
> > > > On 2015/03/30 14:45:46, Mattias Nissler wrote:
> > > > > This is not going to mix well with our goal to remove
> > > > > CrosSettingsProvider::Set()
> > > > 
> > > > As doesn't the direct use of CrosSettingsProvider::Set() in all the
> tests...
> > > I'm
> > > > not sure what better option there is.
> > > 
> > > Ah, so we had our wires crossed here - I think all the tests have a
> > > StubCrosSettingsProvider instance. We can have a Set() on that that we can
> > call
> > > from tests, but still remove CrosSettignsProvider::Set(). Or am I missing
> > > something?
> > 
> > Hm, this should be very well possible, although it would require
> > FakeOwnerSettingsService to have the StubCrosSettingsProvider instance
access
> > here. We could either add some SetProvider() to FakeOwnerSettingsService
class
> > (or require to pass a provider in the constructor), or just expect
> > CrosSettings::GetProvider() to always return a StubCrosSettingsProvider
here.
> > What do you think works better?
> 
> Why would FakeOwnerSettingsService need to write settings at all?

For example, because KioskAppsManager (or KioskAppsHelper, which doesn't really
matter) writes settings in AddApp/RemoveApp through the OwnerSettingsService.
There may also be some other cases where OwnerSettingsService is used.
Basically, for the same reason the OwnerSettingsService itself has the write
path: shouldn't it become the default way to write settings?

[Mattias Nissler (ping if slow), 2015-03-31 09:16:09]

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:39:
provider->Set(setting, value);
On 2015/03/31 09:01:21, Ivan Podogov wrote:
> On 2015/03/31 08:56:03, Mattias Nissler wrote:
> > On 2015/03/31 08:51:12, Ivan Podogov wrote:
> > > On 2015/03/31 08:44:48, Mattias Nissler wrote:
> > > > On 2015/03/31 08:40:00, Ivan Podogov wrote:
> > > > > On 2015/03/30 14:45:46, Mattias Nissler wrote:
> > > > > > This is not going to mix well with our goal to remove
> > > > > > CrosSettingsProvider::Set()
> > > > > 
> > > > > As doesn't the direct use of CrosSettingsProvider::Set() in all the
> > tests...
> > > > I'm
> > > > > not sure what better option there is.
> > > > 
> > > > Ah, so we had our wires crossed here - I think all the tests have a
> > > > StubCrosSettingsProvider instance. We can have a Set() on that that we
can
> > > call
> > > > from tests, but still remove CrosSettignsProvider::Set(). Or am I
missing
> > > > something?
> > > 
> > > Hm, this should be very well possible, although it would require
> > > FakeOwnerSettingsService to have the StubCrosSettingsProvider instance
> access
> > > here. We could either add some SetProvider() to FakeOwnerSettingsService
> class
> > > (or require to pass a provider in the constructor), or just expect
> > > CrosSettings::GetProvider() to always return a StubCrosSettingsProvider
> here.
> > > What do you think works better?
> > 
> > Why would FakeOwnerSettingsService need to write settings at all?
> 
> For example, because KioskAppsManager (or KioskAppsHelper, which doesn't
really
> matter) writes settings in AddApp/RemoveApp through the OwnerSettingsService.
> There may also be some other cases where OwnerSettingsService is used.
> Basically, for the same reason the OwnerSettingsService itself has the write
> path: shouldn't it become the default way to write settings?

OwnerSettingsServiceChromeOS writes via DeviceSettingsService::Store().
CrosSettings will update its view of the world once the write has propagated
through the system. OwnerSettingsServiceChromeOS has no reason to push values
directly to CrosSettings.

For short-circuiting this in testing code, we might be able to find a way to
instantiate CrosSettings with a StubSettingsProvider and
FakeOwnerSettingsService s.t. we can pass a pointer to the former to the
latter's constructor? Anything is acceptable here IMHO, as long as we can get
rid of the write path in production code.

[Ivan Podogov, 2015-03-31 09:30:08]

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:39:
provider->Set(setting, value);
On 2015/03/31 09:16:09, Mattias Nissler wrote:
> On 2015/03/31 09:01:21, Ivan Podogov wrote:
> > On 2015/03/31 08:56:03, Mattias Nissler wrote:
> > > On 2015/03/31 08:51:12, Ivan Podogov wrote:
> > > > On 2015/03/31 08:44:48, Mattias Nissler wrote:
> > > > > On 2015/03/31 08:40:00, Ivan Podogov wrote:
> > > > > > On 2015/03/30 14:45:46, Mattias Nissler wrote:
> > > > > > > This is not going to mix well with our goal to remove
> > > > > > > CrosSettingsProvider::Set()
> > > > > > 
> > > > > > As doesn't the direct use of CrosSettingsProvider::Set() in all the
> > > tests...
> > > > > I'm
> > > > > > not sure what better option there is.
> > > > > 
> > > > > Ah, so we had our wires crossed here - I think all the tests have a
> > > > > StubCrosSettingsProvider instance. We can have a Set() on that that we
> can
> > > > call
> > > > > from tests, but still remove CrosSettignsProvider::Set(). Or am I
> missing
> > > > > something?
> > > > 
> > > > Hm, this should be very well possible, although it would require
> > > > FakeOwnerSettingsService to have the StubCrosSettingsProvider instance
> > access
> > > > here. We could either add some SetProvider() to FakeOwnerSettingsService
> > class
> > > > (or require to pass a provider in the constructor), or just expect
> > > > CrosSettings::GetProvider() to always return a StubCrosSettingsProvider
> > here.
> > > > What do you think works better?
> > > 
> > > Why would FakeOwnerSettingsService need to write settings at all?
> > 
> > For example, because KioskAppsManager (or KioskAppsHelper, which doesn't
> really
> > matter) writes settings in AddApp/RemoveApp through the
OwnerSettingsService.
> > There may also be some other cases where OwnerSettingsService is used.
> > Basically, for the same reason the OwnerSettingsService itself has the write
> > path: shouldn't it become the default way to write settings?
> 
> OwnerSettingsServiceChromeOS writes via DeviceSettingsService::Store().
> CrosSettings will update its view of the world once the write has propagated
> through the system. OwnerSettingsServiceChromeOS has no reason to push values
> directly to CrosSettings.
> 
> For short-circuiting this in testing code, we might be able to find a way to
> instantiate CrosSettings with a StubSettingsProvider and
> FakeOwnerSettingsService s.t. we can pass a pointer to the former to the
> latter's constructor? Anything is acceptable here IMHO, as long as we can get
> rid of the write path in production code.

Well, that's what I was asking in the first place: what is the better way to
pass StubSettingsProvider to FakeOwnerSettingsService for tests, since we don't
want to use the whole heavyweight device settings implementation like in PS#8.
If I'm going to use a TestingProfile::Builder to create all
FakeOwnerSettingsService instances anyway, then I guess I'll try passing the
provider in its constructor, that should be the easiest way.

[Mattias Nissler (ping if slow), 2015-03-31 09:36:02]

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:39:
provider->Set(setting, value);
On 2015/03/31 09:30:08, Ivan Podogov wrote:
> On 2015/03/31 09:16:09, Mattias Nissler wrote:
> > On 2015/03/31 09:01:21, Ivan Podogov wrote:
> > > On 2015/03/31 08:56:03, Mattias Nissler wrote:
> > > > On 2015/03/31 08:51:12, Ivan Podogov wrote:
> > > > > On 2015/03/31 08:44:48, Mattias Nissler wrote:
> > > > > > On 2015/03/31 08:40:00, Ivan Podogov wrote:
> > > > > > > On 2015/03/30 14:45:46, Mattias Nissler wrote:
> > > > > > > > This is not going to mix well with our goal to remove
> > > > > > > > CrosSettingsProvider::Set()
> > > > > > > 
> > > > > > > As doesn't the direct use of CrosSettingsProvider::Set() in all
the
> > > > tests...
> > > > > > I'm
> > > > > > > not sure what better option there is.
> > > > > > 
> > > > > > Ah, so we had our wires crossed here - I think all the tests have a
> > > > > > StubCrosSettingsProvider instance. We can have a Set() on that that
we
> > can
> > > > > call
> > > > > > from tests, but still remove CrosSettignsProvider::Set(). Or am I
> > missing
> > > > > > something?
> > > > > 
> > > > > Hm, this should be very well possible, although it would require
> > > > > FakeOwnerSettingsService to have the StubCrosSettingsProvider instance
> > > access
> > > > > here. We could either add some SetProvider() to
FakeOwnerSettingsService
> > > class
> > > > > (or require to pass a provider in the constructor), or just expect
> > > > > CrosSettings::GetProvider() to always return a
StubCrosSettingsProvider
> > > here.
> > > > > What do you think works better?
> > > > 
> > > > Why would FakeOwnerSettingsService need to write settings at all?
> > > 
> > > For example, because KioskAppsManager (or KioskAppsHelper, which doesn't
> > really
> > > matter) writes settings in AddApp/RemoveApp through the
> OwnerSettingsService.
> > > There may also be some other cases where OwnerSettingsService is used.
> > > Basically, for the same reason the OwnerSettingsService itself has the
write
> > > path: shouldn't it become the default way to write settings?
> > 
> > OwnerSettingsServiceChromeOS writes via DeviceSettingsService::Store().
> > CrosSettings will update its view of the world once the write has propagated
> > through the system. OwnerSettingsServiceChromeOS has no reason to push
values
> > directly to CrosSettings.
> > 
> > For short-circuiting this in testing code, we might be able to find a way to
> > instantiate CrosSettings with a StubSettingsProvider and
> > FakeOwnerSettingsService s.t. we can pass a pointer to the former to the
> > latter's constructor? Anything is acceptable here IMHO, as long as we can
get
> > rid of the write path in production code.
> 
> Well, that's what I was asking in the first place: what is the better way to
> pass StubSettingsProvider to FakeOwnerSettingsService for tests, since we
don't
> want to use the whole heavyweight device settings implementation like in PS#8.
> If I'm going to use a TestingProfile::Builder to create all
> FakeOwnerSettingsService instances anyway, then I guess I'll try passing the
> provider in its constructor, that should be the easiest way.

Ah, sorry for the confusion then. What you suggest sounds great!

[bartfab (slow), 2015-03-31 14:12:42]

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:21: #include
"chrome/browser/chromeos/settings/cros_settings.h"
Nit: No longer used.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:201: new
chromeos::FakeOwnerSettingsService(browser()->profile()));
Nit: s/chromeos:://

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:350:
scoped_ptr<chromeos::FakeOwnerSettingsService> owner_settings_service_;
Nit 1: #include "base/memory/scoped_ptr.h"
Nit 2: s/chromeos:://

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(left):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:15:
#include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
Nit: Still used in line 84.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:79:
public chromeos::SettingsProviderTestBase {
Nit: s/chromeos:://

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc
(right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc:134:
public chromeos::SettingsProviderTestBase {
Nit: s/chromeos:://

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc:160:
void TearDown() { RestoreProvider(); }
Nit: The style guide mandates that you break after { and before }.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
(left):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:25:
#include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
Nit: Still used in line 440.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
(right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:131:
public chromeos::SettingsProviderTestBase {
Nit: s/chromeos:://

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:134: :
chromeos::SettingsProviderTestBase(false),
Nit: s/chromeos:://

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:504:
owner_settings_service_.reset(new chromeos::FakeOwnerSettingsService(
Nit: s/chromeos:://

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:801:
scoped_ptr<chromeos::FakeOwnerSettingsService> owner_settings_service_;
Nit: s/chromeos:://

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/user_manager_unittest.cc (left):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/user_manager_unittest.cc:21: #include
"chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
Nit: Still used in line 128 and others.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/user_manager_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/user_manager_unittest.cc:14: #include
"base/values.h"
Nit: No longer used.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:38:
CrosSettingsProvider* provider = CrosSettings::Get()->GetProvider(setting);
Nit: #include "chromeos/settings/cros_settings_provider.h"

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account.h (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account.h:68: // Stores a list of
device-local accounts in |cros_settings|. The accounts are
Nit: Update the comment.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(left):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:26:
#include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
Nit: Still used in line 292.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/extension_cache_unittest.cc (left):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/extension_cache_unittest.cc:15: #include
"chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
Nit: Still used in line 68.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/extension_cache_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/extension_cache_unittest.cc:60: void SetUp()
override { ReplaceProvider(chromeos::kExtensionCacheSize); }
Nit: Per style guide, this must be broken into three lines.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/extension_cache_unittest.cc:62: void TearDown()
override { RestoreProvider(); }
Nit: Per style guide, this must be broken into three lines.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (left):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:11: #include
"chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
Nit: Still used in line 106 and others.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc:64: void SetUp()
override { ReplaceProvider(chromeos::kHeartbeatEnabled); }
Nit: Per style guide, this must be broken into three lines.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/status_uploader_unittest.cc (left):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:11: #include
"chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
Nit: Still used in line 120 and others.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/settings_provider_test_base.cc (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.cc:14: if
(!DeviceSettingsService::IsInitialized()) {
Nit: #include "chrome/browser/chromeos/settings/device_settings_service.h"

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.cc:23:
test_device_settings_service_.reset(new ScopedTestDeviceSettingsService());
Nit: Move this common code out to a helper method.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.cc:35:
device_settings_provider_ = cros_settings->GetProvider(path);
Nit: #include "chromeos/settings/cros_settings_provider.h"

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.cc:42: if
(device_settings_provider_) {
Why is this conditional here? Do you expect RestoreProvider() to be called
multiple times? If so, you should clear |device_settings_provider_| once you
have restored the real settings provider. Otherwise, a subsequent call will take
the same branch and fail at the EXPECT_TRUE() below.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/settings_provider_test_base.h (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.h:12: #include
"chrome/browser/chromeos/settings/device_settings_provider.h"
Nit: Not used.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.h:23: class
SettingsProviderTestBase {
You are using a strange paradigm here. Even though the class has "TestBase" in
its name, it does not pull in |InProcessBrowserTest|. Test fixtures are expected
to inherit from this class and another one that actually provides the base
|InProcessBrowserTest|. If you really do not want to be pulling
|InProcessBrowserTest| into this class, then I think it should be turned into a
helper you instantiate, not a base you inherit from (i.e. composition, not
inheritance).

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.h:43:
CrosSettingsProvider* device_settings_provider_ = nullptr;
Nit: Forward-declare CrosSettingProvider.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/settings_provider_test_base.h:46:
scoped_ptr<chromeos::FakeOwnerSettingsService> owner_settings_service_;
Nit: s/chromeos:://

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc:9: #include
"base/memory/scoped_ptr.h"
Nit: No longer used.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc:11:
#include "base/values.h"
Nit: No longer used.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/shutdown_policy_handler_unittest.cc:48:
stub_settings_provider_.SetBoolean(kRebootOnShutdown, reboot_on_shutdown);
Nit: #include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/stub_cros_settings_provider.cc (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.cc:50:
base::FundamentalValue value(in_value);
Nit: const.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.cc:56:
base::FundamentalValue value(in_value);
Nit: const.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.cc:62:
base::FundamentalValue value(in_value);
Nit: const.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.cc:68:
base::StringValue value(in_value);
Nit: const.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(left):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:16:
#include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
Nit: Still used, e.g. in line 253.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:172:
virtual ~EPKPChallengeKeyTestBase() { RestoreProvider(); }
Nit: Per style guide, you must break this into three lines.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:178:
InitOwnerSettingsService(browser()->profile());
Nit: #include "chrome/browser/ui/browser.h"

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:253:
stub_settings_provider_.SetBoolean(chromeos::kDeviceAttestationEnabled,
Why is it that you sometimes use owner_settings_service_->Set*() and other
times, use stub_setting_provider_.Set*()? In some tests you use one, in other
tests the other. And here, you mix both.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1017: Profile* profile =
ProfileManager::GetActiveUserProfile();
Nit: const pointer.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1018:
OwnerSettingsServiceChromeOS* service =
Nit: const pointer.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1122: Profile* profile =
ProfileManager::GetActiveUserProfile();
Nit: const pointer.

https://codereview.chromium.org/1019283004/diff/180001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1123:
OwnerSettingsServiceChromeOS* service =
Nit: const pointer.

[Ivan Podogov, 2015-03-31 14:40:22]

bartfab@, thank you very much for your review, as thorough as always! :)

https://codereview.chromium.org/1019283004/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:429:
DeviceSettingsService::Get()->AddObserver(this);
I was overly optimistic on using TestingProfile::Builder after all: in all the
browsertests I've edited here there is no Profile building as such, only the
default one is created at startup (this is also the case in almost all of the
browsertests, actually). It is not a problem for two out of three test packs (I
just add a StubCrosSettingsProvider anyway), but this one is harder: it expects
the DeviceSettingsService to be really used and to change its trusted status...

So, for this iteration I had to leave CrosSettingsProvider write in the
FakeOwnerSettingsService::Set...

[Ivan Podogov, 2015-04-03 14:17:27]

https://codereview.chromium.org/1019283004/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1320:
settings_helper_.StoreDeviceSetting(kAccountsPrefDeviceLocalAccounts);
So, the road was long and bumpy, but it seems that I have more or less
succeeded. This is the only thing I'm not sure of: I need a way to somehow store
the value between PRE and TEST, so that it could be used on the next run, before
the SetUpOnMainThread is called (default CrosSettings provider is created
earlier).

This is probably the only place where I still had to rely on
DeviceSettingsProvider's values cache in local state. Although it should be
possible to use DeviceSettingsService's file storage instead. Anyway, that's
also under the hood of ScopedCrosSettingsTestHelper now.

https://codereview.chromium.org/1019283004/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1836:
settings_helper_.SetCurrentUserIsOwner(false);
This is another place where I had some problems: the test is designed so that
every CrosSettings::Set call would fail (because the current user is not the
owner), but some values have to be set anyway.
I've removed the protobuf encoding, because to set the values in
StubCrosSettingsProvider I would require DeviceSettingsProvider's protobuf
decoder, and I wanted to avoid using it... So the simple solution was to
temporary allow the changes and lock them again after that.

BTW, in this place it might be better to use owner_settings_service_ to set the
values, and call SetDeviceLocalAccounts to actually generate and set the list
(again, using owner_settings_service_).
We could also make it so that the Set* from settings helper would fail depending
on the ownership status, and the Set* from the (fake) owner settings service
would always succeed. That would make this function somehow cleaner.

[Ivan Podogov, 2015-04-06 11:52:46]

New patchset added.

https://codereview.chromium.org/1019283004/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1836:
settings_helper_.SetCurrentUserIsOwner(false);
On 2015/04/03 14:17:27, Ivan Podogov wrote:
> This is another place where I had some problems: the test is designed so that
> every CrosSettings::Set call would fail (because the current user is not the
> owner), but some values have to be set anyway.
> I've removed the protobuf encoding, because to set the values in
> StubCrosSettingsProvider I would require DeviceSettingsProvider's protobuf
> decoder, and I wanted to avoid using it... So the simple solution was to
> temporary allow the changes and lock them again after that.
> 
> BTW, in this place it might be better to use owner_settings_service_ to set
the
> values, and call SetDeviceLocalAccounts to actually generate and set the list
> (again, using owner_settings_service_).
> We could also make it so that the Set* from settings helper would fail
depending
> on the ownership status, and the Set* from the (fake) owner settings service
> would always succeed. That would make this function somehow cleaner.

In fact, scratch that, owner settings service calls StubCrosSettingsProvider
anyway, so that won't help. Still, calling SetDeviceLocalAccounts is actually
more compact and simple (see new PS).

[Mattias Nissler (ping if slow), 2015-04-07 12:59:19]

This looks pretty good now - the only major remaining issue is the duplicate
device settings mocking in kiosk_browsertest.cc.

https://codereview.chromium.org/1019283004/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1320:
settings_helper_.StoreDeviceSetting(kAccountsPrefDeviceLocalAccounts);
On 2015/04/03 14:17:27, Ivan Podogov wrote:
> So, the road was long and bumpy, but it seems that I have more or less
> succeeded. This is the only thing I'm not sure of: I need a way to somehow
store
> the value between PRE and TEST, so that it could be used on the next run,
before
> the SetUpOnMainThread is called (default CrosSettings provider is created
> earlier).
> 
> This is probably the only place where I still had to rely on
> DeviceSettingsProvider's values cache in local state. Although it should be
> possible to use DeviceSettingsService's file storage instead. Anyway, that's
> also under the hood of ScopedCrosSettingsTestHelper now.

I think what you have here is fine.

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1830:
policy::DevicePolicyCrosTestHelper device_policy_test_helper_;
It doesn't make sense for a test to have both your test helper and a full-blown
DevicePolicyCrosTestHelper installed as they provide overlapping functionality
using different means. The former mocks out a settings provider, while the
latter mocks out the settings service on the DBus level.

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h
(right):

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:67: void
StoreDeviceSetting(const std::string& path);
Let's rename this to StoreCachedDeviceSetting to make it clear this doesn't
actually touch the underlying device settings.

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:73:
CrosSettingsProvider* device_settings_provider_ = nullptr;
I think you no longer assume anywhere that you're actually swapping out
DeviceSettingsProvider, and in fact ReplaceProvider swaps out whatever provider
is present for the provided |path|. Maybe rename this member to reflect that?

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/stub_cros_settings_provider.h (right):

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.h:46: // Default
value is TRUSTED
nit: Missing period.

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h (right):

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h:35: explicit
KioskAppsHandler(Profile* profile);
Might as well just pass OwnerSettingsService* here to avoid passing the much
heavier-weight Profile dependency?

[Ivan Podogov, 2015-04-07 14:14:48]

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1830:
policy::DevicePolicyCrosTestHelper device_policy_test_helper_;
On 2015/04/07 12:59:18, Mattias Nissler wrote:
> It doesn't make sense for a test to have both your test helper and a
full-blown
> DevicePolicyCrosTestHelper installed as they provide overlapping functionality
> using different means. The former mocks out a settings provider, while the
> latter mocks out the settings service on the DBus level.

Had to leave it here for now (though moved it out to local variable): without
its InstallOwnerKey() I get "Unchecked runtime.lastError while running
identity.getAuthToken: The user is not signed in.".

[Ivan Podogov, 2015-04-07 14:31:08]

Other comments addressed, new patchset uploaded.

[Mattias Nissler (ping if slow), 2015-04-07 14:50:57]

https://codereview.chromium.org/1019283004/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1698:
policy::DevicePolicyCrosTestHelper device_policy_test_helper;
It doesn't make sense to use both DevicePolicyCrosTestHelper and
ScopedCrosSettingsTestHelper in the same test, as they're mocking at completely
different levels. Please make this test use either one or the other.

If you just need MarkAsEnterpriseOwned(), then feel free to move that to a place
where you can call it without a DevicePolicyCrosTestHelper.

If you need InstallOwnerKey() as well, then ScopedCrosSettingsTestHelper is
probably not appropriate to use here, as it already is supposed to mock the
owner key, but fails to do it properly to match test expectations?

[Ivan Podogov, 2015-04-07 15:16:13]

https://codereview.chromium.org/1019283004/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1698:
policy::DevicePolicyCrosTestHelper device_policy_test_helper;
On 2015/04/07 14:50:57, Mattias Nissler wrote:
> It doesn't make sense to use both DevicePolicyCrosTestHelper and
> ScopedCrosSettingsTestHelper in the same test, as they're mocking at
completely
> different levels. Please make this test use either one or the other.
> 
> If you just need MarkAsEnterpriseOwned(), then feel free to move that to a
place
> where you can call it without a DevicePolicyCrosTestHelper.
> 
> If you need InstallOwnerKey() as well, then ScopedCrosSettingsTestHelper is
> probably not appropriate to use here, as it already is supposed to mock the
> owner key, but fails to do it properly to match test expectations?

OK, using just the MarkAsEnterpriseOwned() does the trick, so it should be easy
to move it out, although I'm not sure where to, since DevicePolicyCrosTestHelper
is already pretty compact as it is, and MarkAsEnterpriseOwned() uses at least
one its method (OverridePaths) and PolicyBuilder::kFakeUsername.

[Mattias Nissler (ping if slow), 2015-04-08 11:38:18]

https://codereview.chromium.org/1019283004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1249:
settings_helper_.CopyStoredValue(kAccountsPrefDeviceLocalAccounts);
Can you please put a comment here explaining why this is needed, also below in
TearDownOnMainThread()?

Also, it's a bit weird to rely on the actual device settings implementation to
persist the value across PRE_ test phases, even though you've mocked out the
owner settings side of things. Why not just have TearDownOnMainThread() capture
the value and re-inject it for the next SetUpOnMainThread() as long as we're in
the same test group?

https://codereview.chromium.org/1019283004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h
(right):

https://codereview.chromium.org/1019283004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:15: class
KeyedService;
not needed?

[Ivan Podogov, 2015-04-08 11:59:42]

https://codereview.chromium.org/1019283004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1249:
settings_helper_.CopyStoredValue(kAccountsPrefDeviceLocalAccounts);
On 2015/04/08 11:38:17, Mattias Nissler wrote:
> Can you please put a comment here explaining why this is needed, also below in
> TearDownOnMainThread()?
> 
> Also, it's a bit weird to rely on the actual device settings implementation to
> persist the value across PRE_ test phases, even though you've mocked out the
> owner settings side of things. Why not just have TearDownOnMainThread()
capture
> the value and re-inject it for the next SetUpOnMainThread() as long as we're
in
> the same test group?

Because the values are stored so that they would be used long before the next
SetUpOnMainThread() actually happens: the default CrosSettings provider is
created and used earlier, by KioskAppManager::UpdateAppData, somewhere around
its creation. If I don't have the values ready by that time (and I don't really
have control over that, since it would require changing the base classes'
behavior), then the external extension cache will be empty on creation, and the
whole PRE part (which only does app caching) becomes meaningless, and the test
breaks.

Another (and arguably better) way to do this would be by moving the default
settings provider creation out of the CrosSettings constructor (it's somewhat
clumsy right now) by setting some global variable, to be able to inject a custom
provider right from the test's constructor (or, better yet, from the
ScopedCrosSettingsTestHelper's constructor).

Still, I would require something to store my settings to a file in
TearDownOnMainThread, or maybe still use StoreCachedDeviceSetting, as I do now,
just by reading those values from the cache in constructor.

[Mattias Nissler (ping if slow), 2015-04-10 07:07:53]

OK, LGTM. Thanks for doing this!

[bartfab (slow), 2015-04-10 08:38:14]

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:206: void
TearDownOnMainThread() override { settings_helper_.RestoreProvider(); }
Nit: Per style guide, you must split this into three lines.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:17: #include
"base/single_thread_task_runner.h"
Nit: No longer used.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:21: #include
"base/thread_task_runner_handle.h"
Nit: No longer used.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:937:
CrosSettings::Get()->SetBoolean(
Can you not switch over this call as well?

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1751:
std::vector<policy::DeviceLocalAccount> accounts;
Nit: #include <vector>

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:41:
CHECK(settings_provider_);
Nit: #include "base/logging.h"

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc:7: #include
<string>
Nit: Already included by header.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:305: void
TearDown() override { settings_helper_.RestoreProvider(); }
Nit: Per style guide, this has to be wrapped over 3 lines.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc
(right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:16:
#include "chromeos/settings/cros_settings_provider.h"
Nit: This should be included by the header.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:46:
real_settings_provider_ = cros_settings->GetProvider(path);
What if someone calls ReplaceProvider() multiple times, for different paths?

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:99: const
base::Value* value = stub_settings_provider_.Get(path);
Nit: Const pointer.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:104:
CHECK(settings.SerializeToString(data.mutable_policy_value()));
Nit: #include "base/logging.h"

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:105:
CHECK(device_settings_cache::Store(data, g_browser_process->local_state()));
Will this overwrite any previously serialized settings blob? If so, each
StoreCachedDeviceSetting() call will overwrite the result of the previous call.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:113: const
base::Value* value = provider->Get(path);
Nit: Const pointer.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:120: if
(create_settings_service && !DeviceSettingsService::IsInitialized()) {
Would it not be an error to pass |create_settings_service = true| when
|DeviceSettingsService::IsInitialized() == true|?

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h
(right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:1: //
Copyright (c) 2015 The Chromium Authors. All rights reserved.
Nit: s/(c) //

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:12: #include
"base/values.h"
Nit: Forward-declare |base::Value| instead.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:19: class
CrosSettingsProvider;
Nit: Not needed once you include "chromeos/settings/cros_settings_provider.h".

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:29: // if
the test already has a device settings service and/or CrosSettings set
Nit: s/if/If/

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:30: // up by
another bundle, creating another one causes crash.
Nit: What is a "bundle"?

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:32: virtual
~ScopedCrosSettingsTestHelper();
Nit: s/virtual //

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:41:
FakeOwnerSettingsService* CreateOwnerSettingsService(Profile* profile);
Nit: This appears to pass ownership. Please use scoped_ptr when passing
ownership.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:44: void
SetTrustedStatus(CrosSettingsProvider::TrustedStatus status);
Nit: #include "chromeos/settings/cros_settings_provider.h"

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/stub_cros_settings_provider.h (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.h:40: // By
default, every value from Set is written. However, in some cases (e.g.
I find this comment very difficult to parse. Rewording it would help. However, I
also think that the functionality you describe here is fishy: You are abusing
Set() as a non-owner to not set but send out a notification only. If you just
want to send a notification, you should add a testing method that triggers the
notification directly.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.h:44: bool
current_user_is_owner_;
Nit: Initialize this.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.h:47: TrustedStatus
trusted_status_;
Nit: Initialize this.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1024: Profile* const
profile = ProfileManager::GetActiveUserProfile();
ProfileManager::GetActiveUserProfile() is dangerous (it will return a "suitable"
profile, which may not be the profile you expect) and is documented as "avoid
using it at all cost." Please retrieve the current user from UserManager and
then get the profile for that user.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1129: Profile* const
profile = ProfileManager::GetActiveUserProfile();
As above, please do not use ProfileManager::GetActiveUserProfile().

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h:73:
OwnerSettingsServiceChromeOS* owner_settings_service_;
Nit: Const pointer.

[Ivan Podogov, 2015-04-10 08:41:22]

ginkage@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Ivan Podogov, 2015-04-10 09:50:54]

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:206: void
TearDownOnMainThread() override { settings_helper_.RestoreProvider(); }
On 2015/04/10 08:38:12, bartfab wrote:
> Nit: Per style guide, you must split this into three lines.

This was caused by "git cl format", and I get a presubmit warning once I unwrap
this to 3 lines.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:17: #include
"base/single_thread_task_runner.h"
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:21: #include
"base/thread_task_runner_handle.h"
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:937:
CrosSettings::Get()->SetBoolean(
On 2015/04/10 08:38:13, bartfab wrote:
> Can you not switch over this call as well?

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1751:
std::vector<policy::DeviceLocalAccount> accounts;
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: #include <vector>

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/ownership/fake_owner_settings_service.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/ownership/fake_owner_settings_service.cc:41:
CHECK(settings_provider_);
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc:7: #include
<string>
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: Already included by header.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:305: void
TearDown() override { settings_helper_.RestoreProvider(); }
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: Per style guide, this has to be wrapped over 3 lines.

This was caused by "git cl format", and I get a presubmit warning once I unwrap
this to 3 lines.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc
(right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:16:
#include "chromeos/settings/cros_settings_provider.h"
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: This should be included by the header.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:46:
real_settings_provider_ = cros_settings->GetProvider(path);
On 2015/04/10 08:38:13, bartfab wrote:
> What if someone calls ReplaceProvider() multiple times, for different paths?

Added CHECK.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:99: const
base::Value* value = stub_settings_provider_.Get(path);
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: Const pointer.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:104:
CHECK(settings.SerializeToString(data.mutable_policy_value()));
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:105:
CHECK(device_settings_cache::Store(data, g_browser_process->local_state()));
On 2015/04/10 08:38:13, bartfab wrote:
> Will this overwrite any previously serialized settings blob? If so, each
> StoreCachedDeviceSetting() call will overwrite the result of the previous
call.

Added deserialization.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:113: const
base::Value* value = provider->Get(path);
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: Const pointer.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc:120: if
(create_settings_service && !DeviceSettingsService::IsInitialized()) {
On 2015/04/10 08:38:13, bartfab wrote:
> Would it not be an error to pass |create_settings_service = true| when
> |DeviceSettingsService::IsInitialized() == true|?

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h
(right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:1: //
Copyright (c) 2015 The Chromium Authors. All rights reserved.
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: s/(c) //

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:12: #include
"base/values.h"
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: Forward-declare |base::Value| instead.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:19: class
CrosSettingsProvider;
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: Not needed once you include "chromeos/settings/cros_settings_provider.h".

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:29: // if
the test already has a device settings service and/or CrosSettings set
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: s/if/If/

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:30: // up by
another bundle, creating another one causes crash.
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: What is a "bundle"?

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:32: virtual
~ScopedCrosSettingsTestHelper();
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: s/virtual //

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:41:
FakeOwnerSettingsService* CreateOwnerSettingsService(Profile* profile);
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: This appears to pass ownership. Please use scoped_ptr when passing
> ownership.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:44: void
SetTrustedStatus(CrosSettingsProvider::TrustedStatus status);
On 2015/04/10 08:38:13, bartfab wrote:
> Nit: #include "chromeos/settings/cros_settings_provider.h"

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/stub_cros_settings_provider.h (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.h:40: // By
default, every value from Set is written. However, in some cases (e.g.
On 2015/04/10 08:38:13, bartfab wrote:
> I find this comment very difficult to parse. Rewording it would help. However,
I
> also think that the functionality you describe here is fishy: You are abusing
> Set() as a non-owner to not set but send out a notification only. If you just
> want to send a notification, you should add a testing method that triggers the
> notification directly.

This behavior is actually something that some tests depend on, so I left it that
way.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.h:44: bool
current_user_is_owner_;
On 2015/04/10 08:38:14, bartfab wrote:
> Nit: Initialize this.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.h:47: TrustedStatus
trusted_status_;
On 2015/04/10 08:38:14, bartfab wrote:
> Nit: Initialize this.

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1024: Profile* const
profile = ProfileManager::GetActiveUserProfile();
On 2015/04/10 08:38:14, bartfab wrote:
> ProfileManager::GetActiveUserProfile() is dangerous (it will return a
"suitable"
> profile, which may not be the profile you expect) and is documented as "avoid
> using it at all cost." Please retrieve the current user from UserManager and
> then get the profile for that user.

OK, it was changed from that some iterations ago because of another comment. :)

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1129: Profile* const
profile = ProfileManager::GetActiveUserProfile();
On 2015/04/10 08:38:14, bartfab wrote:
> As above, please do not use ProfileManager::GetActiveUserProfile().

Done.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h:73:
OwnerSettingsServiceChromeOS* owner_settings_service_;
On 2015/04/10 08:38:14, bartfab wrote:
> Nit: Const pointer.

Done.

[Ivan Podogov, 2015-04-13 12:02:00]

Missing owner reviews:

oshima@
chrome/browser/chromeos/*
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc

nkostylev@
chrome/browser/chromeos/*
chrome/browser/ui/webui/*

[Nikita (slow), 2015-04-13 12:26:36]

nkostylev@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Nikita (slow), 2015-04-13 12:26:37]

+Xiyuan for kiosk specific changes

[Nikita (slow), 2015-04-13 12:49:11]

On 2015/04/13 12:02:00, Ivan Podogov wrote:
> nkostylev@
> chrome/browser/chromeos/*
> chrome/browser/ui/webui/*

lgtm

[Nikita (slow), 2015-04-13 12:49:24]

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h:73:
OwnerSettingsServiceChromeOS* const owner_settings_service_;
nit: // not owned.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extensions_ui.cc (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extensions_ui.cc:71: profile));
web_ui->GetWebContents()->GetBrowserContext() to avoid explicit dependency on
Profile.

[xiyuan, 2015-04-13 17:15:12]

LGTM with nits

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:45: #include
"chrome/browser/chromeos/settings/device_settings_service.h"
Can this go now?

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h:33: explicit
KioskAppsHandler(OwnerSettingsServiceChromeOS* service);
Can we use OwnerSettingsServiceChromeOS::FromWebUI instead of passing and
storing a pointer?

[Ivan Podogov, 2015-04-14 08:04:18]

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h:33: explicit
KioskAppsHandler(OwnerSettingsServiceChromeOS* service);
On 2015/04/13 17:15:12, xiyuan wrote:
> Can we use OwnerSettingsServiceChromeOS::FromWebUI instead of passing and
> storing a pointer?

Using OwnerSettingsServiceChromeOS::FromWebUI is not a problem (one of the first
patchsets did just that), I just wanted it to be available since construction.
The alternative is to set it in the RegisterMessages() call triggered by
"web_ui->AddMessageHandler(kiosk_app_handler)" (as web_ui() doesn't have any
value before that), which is not that obvious.
As for storing the pointer, it actually makes code cleaner: there are six calls
that use it right now, and adding copy-pasted FromWebUI call to all of them
wouldn't help in readability. Basically, it is here for the same reason the
kiosk_app_manager_ is.

[bartfab (slow), 2015-04-14 14:10:09]

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:206: void
TearDownOnMainThread() override { settings_helper_.RestoreProvider(); }
On 2015/04/10 09:50:52, Ivan Podogov wrote:
> On 2015/04/10 08:38:12, bartfab wrote:
> > Nit: Per style guide, you must split this into three lines.
> 
> This was caused by "git cl format", and I get a presubmit warning once I
unwrap
> this to 3 lines.

Sounds like a bug in clang-format as it violates the style guide.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:305: void
TearDown() override { settings_helper_.RestoreProvider(); }
On 2015/04/10 09:50:53, Ivan Podogov wrote:
> On 2015/04/10 08:38:13, bartfab wrote:
> > Nit: Per style guide, this has to be wrapped over 3 lines.
> 
> This was caused by "git cl format", and I get a presubmit warning once I
unwrap
> this to 3 lines.

This must be a bug in clang-format.

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/stub_cros_settings_provider.h (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.h:40: // By
default, every value from Set is written. However, in some cases (e.g.
On 2015/04/10 09:50:53, Ivan Podogov wrote:
> On 2015/04/10 08:38:13, bartfab wrote:
> > I find this comment very difficult to parse. Rewording it would help.
However,
> I
> > also think that the functionality you describe here is fishy: You are
abusing
> > Set() as a non-owner to not set but send out a notification only. If you
just
> > want to send a notification, you should add a testing method that triggers
the
> > notification directly.
> 
> This behavior is actually something that some tests depend on, so I left it
that
> way.

This still looks very hacky. Anyone reading such a test will have no way to know
why the value passed is not actually being applied. Maybe you could at least add
a check that verifies the value being passed in matches the current value? That
way, if someone tries to modify such a test to use Set() for changing values,
they will get a CHECK() and will be pointed at this comment?

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:42: #include
"chrome/browser/chromeos/settings/cros_settings.h"
Nit: No longer used.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h
(right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:33: // up by
another (included or base) class, creating another one causes crash.
Nit: s/included/instantiated/

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1024: user_manager::User*
user =
Nit: Const pointer.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1027: Profile* profile =
ProfileHelper::Get()->GetProfileByUser(user);
Nit: Const pointer.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1029:
OwnerSettingsServiceChromeOS* service =
Nit: Const pointer.

[Ivan Podogov, 2015-04-14 14:40:37]

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/stub_cros_settings_provider.h (right):

https://codereview.chromium.org/1019283004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/stub_cros_settings_provider.h:40: // By
default, every value from Set is written. However, in some cases (e.g.
On 2015/04/14 14:10:09, bartfab wrote:
> On 2015/04/10 09:50:53, Ivan Podogov wrote:
> > On 2015/04/10 08:38:13, bartfab wrote:
> > > I find this comment very difficult to parse. Rewording it would help.
> However,
> > I
> > > also think that the functionality you describe here is fishy: You are
> abusing
> > > Set() as a non-owner to not set but send out a notification only. If you
> just
> > > want to send a notification, you should add a testing method that triggers
> the
> > > notification directly.
> > 
> > This behavior is actually something that some tests depend on, so I left it
> that
> > way.
> 
> This still looks very hacky. Anyone reading such a test will have no way to
know
> why the value passed is not actually being applied. Maybe you could at least
add
> a check that verifies the value being passed in matches the current value?
That
> way, if someone tries to modify such a test to use Set() for changing values,
> they will get a CHECK() and will be pointed at this comment?

This is actually a mimic of the current DeviceSettingsProvider::DoSet behavior
(right in the beginnning of it). The hacky thing is that KioskTest actually
depends on this behavior (and tries to set different values, so the CHECK
wouldn't help). That test should probably be rewritten from scratch anyway, but
for now that is not the task that I'm trying to accomplish...

As a compromise, I've added a warning log message (again, like in
DeviceSettingsProvider::DoSet) for such cases, so that the tests would still
work and the test author would have at least some information.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:42: #include
"chrome/browser/chromeos/settings/cros_settings.h"
On 2015/04/14 14:10:09, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/login/kiosk_browsertest.cc:45: #include
"chrome/browser/chromeos/settings/device_settings_service.h"
On 2015/04/13 17:15:11, xiyuan wrote:
> Can this go now?

Done.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h
(right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h:33: // up by
another (included or base) class, creating another one causes crash.
On 2015/04/14 14:10:09, bartfab wrote:
> Nit: s/included/instantiated/

Done.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1024: user_manager::User*
user =
On 2015/04/14 14:10:09, bartfab wrote:
> Nit: Const pointer.

Done.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1027: Profile* profile =
ProfileHelper::Get()->GetProfileByUser(user);
On 2015/04/14 14:10:09, bartfab wrote:
> Nit: Const pointer.

Done.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1029:
OwnerSettingsServiceChromeOS* service =
On 2015/04/14 14:10:09, bartfab wrote:
> Nit: Const pointer.

Done.

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h (right):

https://codereview.chromium.org/1019283004/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h:73:
OwnerSettingsServiceChromeOS* const owner_settings_service_;
On 2015/04/13 12:49:23, Nikita Kostylev wrote:
> nit: // not owned.

Done.

[oshima, 2015-04-15 19:29:43]

https://codereview.chromium.org/1019283004/diff/380001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1026: if (user) {
can this be NULL when IsCurrentUserOwner is true? If not, DCHECK would be
better.

https://codereview.chromium.org/1019283004/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1028: if (profile) {
same here

https://codereview.chromium.org/1019283004/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1031: if (service)
same here

[Ivan Podogov, 2015-04-16 12:30:11]

https://codereview.chromium.org/1019283004/diff/380001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/380001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1026: if (user) {
On 2015/04/15 19:29:43, oshima wrote:
> can this be NULL when IsCurrentUserOwner is true? If not, DCHECK would be
> better.

Done.

[oshima, 2015-04-16 17:56:40]

c/b/ui/ash lgtm

[bartfab (slow), 2015-04-22 15:12:32]

https://codereview.chromium.org/1019283004/diff/400001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/400001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1136: DCHECK(user);
Nit: As above, use CHECK() throughout, not DCHECK().

[bartfab (slow), 2015-04-22 15:14:59]

Eh, Rietveld ate my comments :(. Second try.

LGTM with a couple final nits.

https://codereview.chromium.org/1019283004/diff/400001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/400001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1026: DCHECK(user);
Nit: Here and below: Use CHECK(), not DCHECK(), as you will crash if you get a
|nullptr|.

[Ivan Podogov, 2015-04-22 15:16:40]

https://codereview.chromium.org/1019283004/diff/400001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/1019283004/diff/400001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1026: DCHECK(user);
On 2015/04/22 15:14:58, bartfab wrote:
> Nit: Here and below: Use CHECK(), not DCHECK(), as you will crash if you get a
> |nullptr|.

Done.

https://codereview.chromium.org/1019283004/diff/400001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1136: DCHECK(user);
On 2015/04/22 15:12:32, bartfab wrote:
> Nit: As above, use CHECK() throughout, not DCHECK().

Done.

[Ivan Podogov, 2015-04-22 15:19:46]

The CQ bit was checked by ginkage@chromium.org

[Ivan Podogov, 2015-04-22 15:19:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnissler@chromium.org, nkostylev@chromium.org, xiyuan@chromium.org,
bartfab@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1019283004/#ps420001
(title: "Replace DCHECK with CHECK.")

[commit-bot: I haz the power, 2015-04-22 15:20:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1019283004/420001

[commit-bot: I haz the power, 2015-04-22 15:28:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-22 15:28:56]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ivan Podogov, 2015-04-22 15:32:31]

The CQ bit was checked by ginkage@chromium.org

[Ivan Podogov, 2015-04-22 15:32:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, nkostylev@chromium.org, mnissler@chromium.org,
xiyuan@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1019283004/#ps440001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-22 15:33:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1019283004/440001

[commit-bot: I haz the power, 2015-04-22 17:55:02]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2015-04-22 17:56:15]

Patchset 23 (id:??) landed as
https://crrev.com/3a599661a49fa5ebfe63cb4016bb8efcd2d02712
Cr-Commit-Position: refs/heads/master@{#326338}