Implemented OwnerSettingsService::Set() method.

Implemented OwnerSettingsService::Set() method.

BUG=230018
TEST=unit_tests:OwnerSettingsServiceChromeOSTest.*

Committed: https://crrev.com/5fa0ee22683b92d67e07c060e8c7a3c4cced51ee
Cr-Commit-Position: refs/heads/master@{#301132}

Committed: https://crrev.com/0e5fbf43b4ea78a8f2848b0b7bb1fe69b0c2892e
Cr-Commit-Position: refs/heads/master@{#301328}

[ygorshenin1, 2014-10-15 15:19:43]

ygorshenin@chromium.org changed reviewers:
+ mnissler@chromium.org, pastarmovj@chromium.org

[ygorshenin1, 2014-10-17 09:59:56]

Mattias, Julian, could you please take a look?

[Mattias Nissler (ping if slow), 2014-10-17 11:01:29]

Sorry, looking now.

-----------------------------------------------
Google Germany GmbH
Dienerstr. 12
80331 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores

Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370

On Fri, Oct 17, 2014 at 11:59 AM, <ygorshenin@chromium.org> wrote:

> Mattias, Julian, could you please take a look?
>
> https://codereview.chromium.org/654263003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mattias Nissler (ping if slow), 2014-10-17 12:05:27]

I've put some suggestions to switch more stuff over to
OwnerSettingsServiceChromeOS in this CL already, please take a look at them.
Haven't done a detailed pass over tests yet.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:149: class
OwnerSettingsServiceChromeOS::DeviceSettingsServiceLock {
Calling this a lock is bit misleading, nothing gets blocked actually.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:188: if
(DeviceSettingsService::IsInitialized())
I assume this is just for testing? A better way might be to pass in the
DeviceSettingsService pointer via the constructor, so you can pass mocks for
tests.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:329:
BrowserThread::PostTask(
If you're on the right thread anyways, you can just use
MessageLoop::current()->PostTask(). Is there a good reason to not just call this
directly?

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:340: if
(set_requests_callback_factory_.HasWeakPtrs())
Could just unify the conditionals in lines 338, 340, 342-343 into a single if
statement.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:351: //
crbug.com/230018
Have you tested how the device behaves (in particular, owner settings in the
settings page) with just the changes you have in this CL? I'm curious :)

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:358:
DeviceSettingsService::UpdateDeviceSettings(setting, *value, new_settings);
Is there a good reason to have the UpdateDeviceSettings and AssemblePolicy
functions in DeviceSettingsService and not here in OwnerSettingsServiceChromeOS?

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
(right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:106: void
ProcessNextSetRequestAsync();
The functions here could use some documentation.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:130: typedef
std::pair<std::string, linked_ptr<base::Value>> SetRequest;
Looking at this, I can only guess what the first component of the pair is used
for. I suggest making a simple struct type with proper names to help
readability.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:462: const
base::Value& in_value) {
Can we perhaps re-wire this function to go through
OwnershipSettingsServiceChromeOS already in this CL? Doing so would allow you to
get rid of most of the complications in DeviceSettingsService, in particular the
need to stop the processing loop. I think you can get the owner user via
DeviceSettingsService->GetUsername and then locate the right profile via
ProfileHelper. If it's not there, just bail.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:549:
DeviceSettingsService::UpdateDeviceSettings(prop, *value, device_settings_);
We should probably put a comment somewhere in this file or in the header saying
that the write path is in the process of being migrated to
OwnerSettingsServiceChromeOS

[ygorshenin1, 2014-10-20 11:36:10]

PTAL

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:149: class
OwnerSettingsServiceChromeOS::DeviceSettingsServiceLock {
On 2014/10/17 12:05:26, Mattias Nissler wrote:
> Calling this a lock is bit misleading, nothing gets blocked actually.

Renamed to ProcessingLoopAutoBlocker. Not sure that this is a good name.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:188: if
(DeviceSettingsService::IsInitialized())
On 2014/10/17 12:05:27, Mattias Nissler wrote:
> I assume this is just for testing? A better way might be to pass in the
> DeviceSettingsService pointer via the constructor, so you can pass mocks for
> tests.

Done.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:329:
BrowserThread::PostTask(
On 2014/10/17 12:05:27, Mattias Nissler wrote:
> If you're on the right thread anyways, you can just use
> MessageLoop::current()->PostTask(). Is there a good reason to not just call
this
> directly?

The reason is that the method is called when DeviceSettingsService notifies
their observers about changes in settings or ownership status while
DeviceSettingsService::weak_factory_ still have active weak pointers. So direct
call to ProcessNextSetRequest() will have no effect because
DeviceSettingsService::HasPendingOperations() will return true.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:340: if
(set_requests_callback_factory_.HasWeakPtrs())
On 2014/10/17 12:05:27, Mattias Nissler wrote:
> Could just unify the conditionals in lines 338, 340, 342-343 into a single if
> statement.

Done.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:351: //
crbug.com/230018
On 2014/10/17 12:05:27, Mattias Nissler wrote:
> Have you tested how the device behaves (in particular, owner settings in the
> settings page) with just the changes you have in this CL? I'm curious :)

Yes, I've tested the CL with small additional change -
CoreChromeOSOptionsHandler::SetPref() used OwnerSettingsServiceChromeOS instead
of CrosSettings - and didn't found any regressions. But I'm going to make the
latter change in an individual CL.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:358:
DeviceSettingsService::UpdateDeviceSettings(setting, *value, new_settings);
On 2014/10/17 12:05:26, Mattias Nissler wrote:
> Is there a good reason to have the UpdateDeviceSettings and AssemblePolicy
> functions in DeviceSettingsService and not here in
OwnerSettingsServiceChromeOS?

The reason is to not introduce dependency on
chrome/browser/chromeos/ownership/*. If it's not a big issue to introduce such
dependencies in chrome/browser/chromeos/settings/*, I'll move AssemblePolicy and
UpdateDeviceSettings to OwnerSettingsServiceChromeOS.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
(right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:106: void
ProcessNextSetRequestAsync();
On 2014/10/17 12:05:27, Mattias Nissler wrote:
> The functions here could use some documentation.

Done.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:130: typedef
std::pair<std::string, linked_ptr<base::Value>> SetRequest;
On 2014/10/17 12:05:27, Mattias Nissler wrote:
> Looking at this, I can only guess what the first component of the pair is used
> for. I suggest making a simple struct type with proper names to help
> readability.

Done.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:462: const
base::Value& in_value) {
On 2014/10/17 12:05:27, Mattias Nissler wrote:
> Can we perhaps re-wire this function to go through
> OwnershipSettingsServiceChromeOS already in this CL? Doing so would allow you
to
> get rid of most of the complications in DeviceSettingsService, in particular
the
> need to stop the processing loop. I think you can get the owner user via
> DeviceSettingsService->GetUsername and then locate the right profile via
> ProfileHelper. If it's not there, just bail. 

Good idea, but I have a question to you: what is the best way for owner's
instance of OwnerSettingsServiceChromeOS to notify DeviceSettingsProvider about
tentative changes in policy?

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:549:
DeviceSettingsService::UpdateDeviceSettings(prop, *value, device_settings_);
On 2014/10/17 12:05:27, Mattias Nissler wrote:
> We should probably put a comment somewhere in this file or in the header
saying
> that the write path is in the process of being migrated to
> OwnerSettingsServiceChromeOS

Done.

[Mattias Nissler (ping if slow), 2014-10-20 12:41:31]

Mostly replies and suggestions.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:329:
BrowserThread::PostTask(
On 2014/10/20 11:36:10, ygorshenin1 wrote:
> On 2014/10/17 12:05:27, Mattias Nissler wrote:
> > If you're on the right thread anyways, you can just use
> > MessageLoop::current()->PostTask(). Is there a good reason to not just call
> this
> > directly?
> 
> The reason is that the method is called when DeviceSettingsService notifies
> their observers about changes in settings or ownership status while
> DeviceSettingsService::weak_factory_ still have active weak pointers. So
direct
> call to ProcessNextSetRequest() will have no effect because
> DeviceSettingsService::HasPendingOperations() will return true.

Is there a good reason DeviceSettingsService is implemented that way? Sounds
like clearing the weak pointers earlier might be a better fix that doesn't
require quirks in consuming code.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:358:
DeviceSettingsService::UpdateDeviceSettings(setting, *value, new_settings);
On 2014/10/20 11:36:10, ygorshenin1 wrote:
> On 2014/10/17 12:05:26, Mattias Nissler wrote:
> > Is there a good reason to have the UpdateDeviceSettings and AssemblePolicy
> > functions in DeviceSettingsService and not here in
> OwnerSettingsServiceChromeOS?
> 
> The reason is to not introduce dependency on
> chrome/browser/chromeos/ownership/*. If it's not a big issue to introduce such
> dependencies in chrome/browser/chromeos/settings/*, I'll move AssemblePolicy
and
> UpdateDeviceSettings to OwnerSettingsServiceChromeOS.

What dependencies are we talking about? IMHO, the two functions logically belong
in OwnerSettingsServiceChromeOS, and I'd be willing to accept additional
dependencies for that.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:462: const
base::Value& in_value) {
On 2014/10/20 11:36:10, ygorshenin1 wrote:
> On 2014/10/17 12:05:27, Mattias Nissler wrote:
> > Can we perhaps re-wire this function to go through
> > OwnershipSettingsServiceChromeOS already in this CL? Doing so would allow
you
> to
> > get rid of most of the complications in DeviceSettingsService, in particular
> the
> > need to stop the processing loop. I think you can get the owner user via
> > DeviceSettingsService->GetUsername and then locate the right profile via
> > ProfileHelper. If it's not there, just bail. 
> 
> Good idea, but I have a question to you: what is the best way for owner's
> instance of OwnerSettingsServiceChromeOS to notify DeviceSettingsProvider
about
> tentative changes in policy?

Hm, I'm trying to remember why we need that actually. I guess one reason is the
temporary storage of settings when there's no owner yet and get them persisted
to the signed settings cache in local state? If that is still a thing, then the
best way would probably be to add a public interface that lets one call
UpdateValuesCache(policy_data, settings, TEMPORARILY_UNTRUSTED).

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:228:
set_requests_.push(
It just occurred to me that instead of keeping a queue of individual settings to
updated, we could just as well keep a ChromeDeviceSettingsProto that we write
all tentative device settings to. Then, when we get the chance to call
DeviceSettingsService::Store() we can send the updated device settings in one
go. WDYT?

This would also conveniently give us the right settings to send to the proposed
DeviceSettingsProvider call to update tentative settings.

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:343: //
crbug.com/230018
Shouldn't this already happen in Set()?

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
(right):

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:109: // with
session manager operations. If it's the case, temprorary stops
*temporarily

[ygorshenin1, 2014-10-20 13:35:17]

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:228:
set_requests_.push(
On 2014/10/20 12:41:30, Mattias Nissler wrote:
> It just occurred to me that instead of keeping a queue of individual settings
to
> updated, we could just as well keep a ChromeDeviceSettingsProto that we write
> all tentative device settings to. Then, when we get the chance to call
> DeviceSettingsService::Store() we can send the updated device settings in one
> go. WDYT?
> 
> This would also conveniently give us the right settings to send to the
proposed
> DeviceSettingsProvider call to update tentative settings.

But we always can call DeviceSettingsService::Store(), right? Or, you mean, that
if DeviceSettingsService is currently busy with load/store, we should wait until
it'll complete it's operations, then merge our device settings with
DeviceSettingsService::device_settings_, sign them and send for storage?

[Mattias Nissler (ping if slow), 2014-10-20 13:46:49]

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:228:
set_requests_.push(
On 2014/10/20 13:35:16, ygorshenin1 wrote:
> On 2014/10/20 12:41:30, Mattias Nissler wrote:
> > It just occurred to me that instead of keeping a queue of individual
settings
> to
> > updated, we could just as well keep a ChromeDeviceSettingsProto that we
write
> > all tentative device settings to. Then, when we get the chance to call
> > DeviceSettingsService::Store() we can send the updated device settings in
one
> > go. WDYT?
> > 
> > This would also conveniently give us the right settings to send to the
> proposed
> > DeviceSettingsProvider call to update tentative settings.
> 
> But we always can call DeviceSettingsService::Store(), right? Or, you mean,
that
> if DeviceSettingsService is currently busy with load/store, we should wait
until
> it'll complete it's operations, then merge our device settings with
> DeviceSettingsService::device_settings_, sign them and send for storage?

DeviceSettingsService doesn't impose any restrictions on when we can call Store,
it'll happily queue up multiple Store operations for us. However, that'd make it
hard for OwnerSettingsServiceChromeOS to keep track of what to do in
DeviceSettingsUpdated(), as we'd have to wait for all Store() operations to
finish until we'd get a consistent picture again. And if we'd generate more
Store() operations using the current device settings published by
DeviceSettingsService while we still have Store()s in progress, we might
inadvertently roll back the effect of the pending Store()s because our fresh
Store() would pick up old values.

Hence, I think OwnerSettingsServiceChromeOS should have at most one pending
Store() operation at a given point in time. We can accumulate all changes that
come in via Set() in a local ChromeDeviceSettingsProto instance and fire off a
new Store() operation once the currently-pending Store() has finished. Makes
sense?

[ygorshenin1, 2014-10-20 13:49:06]

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:228:
set_requests_.push(
On 2014/10/20 13:46:49, Mattias Nissler wrote:
> On 2014/10/20 13:35:16, ygorshenin1 wrote:
> > On 2014/10/20 12:41:30, Mattias Nissler wrote:
> > > It just occurred to me that instead of keeping a queue of individual
> settings
> > to
> > > updated, we could just as well keep a ChromeDeviceSettingsProto that we
> write
> > > all tentative device settings to. Then, when we get the chance to call
> > > DeviceSettingsService::Store() we can send the updated device settings in
> one
> > > go. WDYT?
> > > 
> > > This would also conveniently give us the right settings to send to the
> > proposed
> > > DeviceSettingsProvider call to update tentative settings.
> > 
> > But we always can call DeviceSettingsService::Store(), right? Or, you mean,
> that
> > if DeviceSettingsService is currently busy with load/store, we should wait
> until
> > it'll complete it's operations, then merge our device settings with
> > DeviceSettingsService::device_settings_, sign them and send for storage?
> 
> DeviceSettingsService doesn't impose any restrictions on when we can call
Store,
> it'll happily queue up multiple Store operations for us. However, that'd make
it
> hard for OwnerSettingsServiceChromeOS to keep track of what to do in
> DeviceSettingsUpdated(), as we'd have to wait for all Store() operations to
> finish until we'd get a consistent picture again. And if we'd generate more
> Store() operations using the current device settings published by
> DeviceSettingsService while we still have Store()s in progress, we might
> inadvertently roll back the effect of the pending Store()s because our fresh
> Store() would pick up old values.
> 
> Hence, I think OwnerSettingsServiceChromeOS should have at most one pending
> Store() operation at a given point in time. We can accumulate all changes that
> come in via Set() in a local ChromeDeviceSettingsProto instance and fire off a
> new Store() operation once the currently-pending Store() has finished. Makes
> sense?

Sure, thanks for the explanation!

[ygorshenin1, 2014-10-22 09:07:36]

Patchset #4 (id:60001) has been deleted

[ygorshenin1, 2014-10-22 09:13:33]

Patchset #4 (id:80001) has been deleted

[ygorshenin1, 2014-10-22 09:20:12]

Mattias, could you please take a look again? I removed all classes and methods
related to blocking of DeviceSettingsService's processing loop - because, as you
proposed, there are non need in them if DeviceSettingsProvider::DoSet() will use
OwnerSettingsService::Set().

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:329:
BrowserThread::PostTask(
On 2014/10/20 12:41:30, Mattias Nissler wrote:
> On 2014/10/20 11:36:10, ygorshenin1 wrote:
> > On 2014/10/17 12:05:27, Mattias Nissler wrote:
> > > If you're on the right thread anyways, you can just use
> > > MessageLoop::current()->PostTask(). Is there a good reason to not just
call
> > this
> > > directly?
> > 
> > The reason is that the method is called when DeviceSettingsService notifies
> > their observers about changes in settings or ownership status while
> > DeviceSettingsService::weak_factory_ still have active weak pointers. So
> direct
> > call to ProcessNextSetRequest() will have no effect because
> > DeviceSettingsService::HasPendingOperations() will return true.
> 
> Is there a good reason DeviceSettingsService is implemented that way? Sounds
> like clearing the weak pointers earlier might be a better fix that doesn't
> require quirks in consuming code.

Done.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:358:
DeviceSettingsService::UpdateDeviceSettings(setting, *value, new_settings);
On 2014/10/20 12:41:30, Mattias Nissler wrote:
> On 2014/10/20 11:36:10, ygorshenin1 wrote:
> > On 2014/10/17 12:05:26, Mattias Nissler wrote:
> > > Is there a good reason to have the UpdateDeviceSettings and AssemblePolicy
> > > functions in DeviceSettingsService and not here in
> > OwnerSettingsServiceChromeOS?
> > 
> > The reason is to not introduce dependency on
> > chrome/browser/chromeos/ownership/*. If it's not a big issue to introduce
such
> > dependencies in chrome/browser/chromeos/settings/*, I'll move AssemblePolicy
> and
> > UpdateDeviceSettings to OwnerSettingsServiceChromeOS.
> 
> What dependencies are we talking about? IMHO, the two functions logically
belong
> in OwnerSettingsServiceChromeOS, and I'd be willing to accept additional
> dependencies for that.

Done.

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/654263003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_provider.cc:462: const
base::Value& in_value) {
On 2014/10/20 12:41:30, Mattias Nissler wrote:
> On 2014/10/20 11:36:10, ygorshenin1 wrote:
> > On 2014/10/17 12:05:27, Mattias Nissler wrote:
> > > Can we perhaps re-wire this function to go through
> > > OwnershipSettingsServiceChromeOS already in this CL? Doing so would allow
> you
> > to
> > > get rid of most of the complications in DeviceSettingsService, in
particular
> > the
> > > need to stop the processing loop. I think you can get the owner user via
> > > DeviceSettingsService->GetUsername and then locate the right profile via
> > > ProfileHelper. If it's not there, just bail. 
> > 
> > Good idea, but I have a question to you: what is the best way for owner's
> > instance of OwnerSettingsServiceChromeOS to notify DeviceSettingsProvider
> about
> > tentative changes in policy?
> 
> Hm, I'm trying to remember why we need that actually. I guess one reason is
the
> temporary storage of settings when there's no owner yet and get them persisted
> to the signed settings cache in local state? If that is still a thing, then
the
> best way would probably be to add a public interface that lets one call
> UpdateValuesCache(policy_data, settings, TEMPORARILY_UNTRUSTED).

I'd prefer to make the change you're proposing in a separate CL, if you don't
mind, because current one is already quite complex.

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:343: //
crbug.com/230018
On 2014/10/20 12:41:30, Mattias Nissler wrote:
> Shouldn't this already happen in Set()?

Done.

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h
(right):

https://codereview.chromium.org/654263003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h:109: // with
session manager operations. If it's the case, temprorary stops
On 2014/10/20 12:41:30, Mattias Nissler wrote:
> *temporarily

Acknowledged.

[ygorshenin1, 2014-10-23 13:10:45]

Friendly ping.

[Mattias Nissler (ping if slow), 2014-10-23 13:26:16]

Sorry, got a sick child to take care of. Will hopefully get to it in the
evening.
On Oct 23, 2014 3:10 PM, <ygorshenin@chromium.org> wrote:

> Friendly ping.
>
> https://codereview.chromium.org/654263003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mattias Nissler (ping if slow), 2014-10-24 08:29:56]

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:193:
device_settings_service_->RemoveObserver(this);
nit: should probably do this first in order to match inverse initialization
order.

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:592:
has_pending_changes_ = true;
Won't this cause an infinite store loop in the error case? I'd rather not retry
immediately and only do so when the next write comes in. If I'm not missing
something, moving the StoreDeviceSettings line before the if (!success) check
should have the desired effect.

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
File
chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc
(right):

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:55:
CHECK(request.second->Equals(value))
Why not ASSERT_EQ?

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:150: void
DeviceSettingsService::SetManagementSettings(
This should probably be re-routed through OwnerSettingsServiceChromeOS as well
eventually.

https://codereview.chromium.org/654263003/diff/100001/components/ownership/ow...
File components/ownership/owner_settings_service.h (right):

https://codereview.chromium.org/654263003/diff/100001/components/ownership/ow...
components/ownership/owner_settings_service.h:45: // not signed and stored.
Can we rather invert the dependency here instead of providing an observer
interface? IMHO, it's more natural for OwnerSettingsService to depend on and
make a direct call to DeviceSettingsProvider, instead of DeviceSettingsProvider
(which is a browser-global object) to depend on and observe a profile-keyed
service.

[ygorshenin1, 2014-10-24 10:33:01]

PTAL

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
(right):

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:193:
device_settings_service_->RemoveObserver(this);
On 2014/10/24 08:29:56, Mattias Nissler wrote:
> nit: should probably do this first in order to match inverse initialization
> order.

Done.

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:592:
has_pending_changes_ = true;
Done, thanks!

On 2014/10/24 08:29:56, Mattias Nissler wrote:
> Won't this cause an infinite store loop in the error case? I'd rather not
retry
> immediately and only do so when the next write comes in. If I'm not missing
> something, moving the StoreDeviceSettings line before the if (!success) check
> should have the desired effect.

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
File
chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc
(right):

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:55:
CHECK(request.second->Equals(value))
On 2014/10/24 08:29:56, Mattias Nissler wrote:
> Why not ASSERT_EQ?

I'm not sure that operator==() behaves properly on base::Value and it's
subclasses. That's why I've used Value::Equals().

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:150: void
DeviceSettingsService::SetManagementSettings(
On 2014/10/24 08:29:56, Mattias Nissler wrote:
> This should probably be re-routed through OwnerSettingsServiceChromeOS as well
> eventually.

I've added a comment to the DeviceSettingsService::SetManagementSettings()
declaration.

https://codereview.chromium.org/654263003/diff/100001/components/ownership/ow...
File components/ownership/owner_settings_service.h (right):

https://codereview.chromium.org/654263003/diff/100001/components/ownership/ow...
components/ownership/owner_settings_service.h:45: // not signed and stored.
On 2014/10/24 08:29:56, Mattias Nissler wrote:
> Can we rather invert the dependency here instead of providing an observer
> interface? IMHO, it's more natural for OwnerSettingsService to depend on and
> make a direct call to DeviceSettingsProvider, instead of
DeviceSettingsProvider
> (which is a browser-global object) to depend on and observe a profile-keyed
> service.

How about to declare OwnerSettingsService::Delegate class, and
DeviceSettingsProvider will implement it? Or it would be better to directly
store a pointer to DeviceSettingsProvider in OwnerSettingsService instances?

[ygorshenin1, 2014-10-24 10:37:12]

https://codereview.chromium.org/666363002/ is merged to the CL.

[Mattias Nissler (ping if slow), 2014-10-24 10:48:13]

OK, let's get this in then. LGTM with nits.

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
File
chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc
(right):

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:55:
CHECK(request.second->Equals(value))
On 2014/10/24 10:33:01, ygorshenin1 wrote:
> On 2014/10/24 08:29:56, Mattias Nissler wrote:
> > Why not ASSERT_EQ?
> 
> I'm not sure that operator==() behaves properly on base::Value and it's
> subclasses. That's why I've used Value::Equals().

Sorry, I meant ASSERT_TRUE().

https://codereview.chromium.org/654263003/diff/100001/components/ownership/ow...
File components/ownership/owner_settings_service.h (right):

https://codereview.chromium.org/654263003/diff/100001/components/ownership/ow...
components/ownership/owner_settings_service.h:45: // not signed and stored.
On 2014/10/24 10:33:01, ygorshenin1 wrote:
> On 2014/10/24 08:29:56, Mattias Nissler wrote:
> > Can we rather invert the dependency here instead of providing an observer
> > interface? IMHO, it's more natural for OwnerSettingsService to depend on and
> > make a direct call to DeviceSettingsProvider, instead of
> DeviceSettingsProvider
> > (which is a browser-global object) to depend on and observe a profile-keyed
> > service.
> 
> How about to declare OwnerSettingsService::Delegate class, and
> DeviceSettingsProvider will implement it? Or it would be better to directly
> store a pointer to DeviceSettingsProvider in OwnerSettingsService instances?

My goal would be to completely remove knowledge of OwnerSettingsService from
DeviceSettingsService and DeviceSettingsProvider eventually. You can't do that
right now, since we still need to accept writes coming in via
DeviceSettingsProvider until we've fixed all callers. But once we've done so,
the only interaction that I'm aware of will be for OwnerSettingsServiceChromeOS
to supply the tentative settings to serve until the write fully propagates. And
that is much simpler to understand by a direct call. Thinking about it, we could
actually just make a copy of the tentative settings when DeviceSettingsService
receives them via Store(), at which point it could notify its observers that new
tentative settings are available. WDYT?

Anyhow, I'm OK to get this CL landed now and then look into this in a follow-up.

https://codereview.chromium.org/654263003/diff/120001/components/ownership/ow...
File components/ownership/owner_settings_service.h (right):

https://codereview.chromium.org/654263003/diff/120001/components/ownership/ow...
components/ownership/owner_settings_service.h:92: // Note that this is a
temporary solution and should be removed soon.
nit: reference the bug

[ygorshenin1, 2014-10-24 12:10:01]

Many thanks! The CL will be landed after the branch point.

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
File
chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc
(right):

https://codereview.chromium.org/654263003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc:55:
CHECK(request.second->Equals(value))
On 2014/10/24 10:48:12, Mattias Nissler wrote:
> On 2014/10/24 10:33:01, ygorshenin1 wrote:
> > On 2014/10/24 08:29:56, Mattias Nissler wrote:
> > > Why not ASSERT_EQ?
> > 
> > I'm not sure that operator==() behaves properly on base::Value and it's
> > subclasses. That's why I've used Value::Equals().
> 
> Sorry, I meant ASSERT_TRUE().

Done.

https://codereview.chromium.org/654263003/diff/100001/components/ownership/ow...
File components/ownership/owner_settings_service.h (right):

https://codereview.chromium.org/654263003/diff/100001/components/ownership/ow...
components/ownership/owner_settings_service.h:45: // not signed and stored.
On 2014/10/24 10:48:12, Mattias Nissler wrote:
> On 2014/10/24 10:33:01, ygorshenin1 wrote:
> > On 2014/10/24 08:29:56, Mattias Nissler wrote:
> > > Can we rather invert the dependency here instead of providing an observer
> > > interface? IMHO, it's more natural for OwnerSettingsService to depend on
and
> > > make a direct call to DeviceSettingsProvider, instead of
> > DeviceSettingsProvider
> > > (which is a browser-global object) to depend on and observe a
profile-keyed
> > > service.
> > 
> > How about to declare OwnerSettingsService::Delegate class, and
> > DeviceSettingsProvider will implement it? Or it would be better to directly
> > store a pointer to DeviceSettingsProvider in OwnerSettingsService instances?
> 
> My goal would be to completely remove knowledge of OwnerSettingsService from
> DeviceSettingsService and DeviceSettingsProvider eventually. You can't do that
> right now, since we still need to accept writes coming in via
> DeviceSettingsProvider until we've fixed all callers. But once we've done so,
> the only interaction that I'm aware of will be for
OwnerSettingsServiceChromeOS
> to supply the tentative settings to serve until the write fully propagates.
And
> that is much simpler to understand by a direct call. Thinking about it, we
could
> actually just make a copy of the tentative settings when DeviceSettingsService
> receives them via Store(), at which point it could notify its observers that
new
> tentative settings are available. WDYT?
> 
> Anyhow, I'm OK to get this CL landed now and then look into this in a
follow-up.

Your proposal about making a copy of device settings in
DeviceSettingsService::Store() sounds good, I'll implement it in a subsequent
CL.

https://codereview.chromium.org/654263003/diff/120001/components/ownership/ow...
File components/ownership/owner_settings_service.h (right):

https://codereview.chromium.org/654263003/diff/120001/components/ownership/ow...
components/ownership/owner_settings_service.h:92: // Note that this is a
temporary solution and should be removed soon.
On 2014/10/24 10:48:13, Mattias Nissler wrote:
> nit: reference the bug

Done.

[ygorshenin1, 2014-10-24 12:28:09]

Mattias, would you mind if we will try to land the CL in M39?

[Mattias Nissler (ping if slow), 2014-10-24 12:39:27]

On 2014/10/24 12:28:09, ygorshenin1 wrote:
> Mattias, would you mind if we will try to land the CL in M39?

I don't mind, you're the one who has to handle any fallout anyways :-P

On a more serious note, my major motivation here is to move the code in the
right direction and doing so in a series of steps that don't create broken
intermediate states. So no concerns from my side, feel free to land in M39.

[ygorshenin1, 2014-10-24 12:46:51]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-10-24 12:47:31]

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

[commit-bot: I haz the power, 2014-10-24 15:22:08]

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

[commit-bot: I haz the power, 2014-10-24 15:22:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ygorshenin1, 2014-10-24 16:31:52]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-10-24 16:33:15]

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

[commit-bot: I haz the power, 2014-10-24 17:23:31]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2014-10-24 17:24:09]

Patchset 7 (id:??) landed as
https://crrev.com/5fa0ee22683b92d67e07c060e8c7a3c4cced51ee
Cr-Commit-Position: refs/heads/master@{#301132}

[Avi (use Gerrit), 2014-10-24 19:03:01]

A revert of this CL (patchset #7 id:160001) has been created in
https://codereview.chromium.org/640063008/ by avi@chromium.org.

The reason for reverting is: ASAN bot unhappy

http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20....

[ygorshenin1, 2014-10-27 08:15:02]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-10-27 08:15:55]

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

[commit-bot: I haz the power, 2014-10-27 09:11:11]

Committed patchset #8 (id:170001)

[commit-bot: I haz the power, 2014-10-27 09:11:57]

Patchset 8 (id:??) landed as
https://crrev.com/0e5fbf43b4ea78a8f2848b0b7bb1fe69b0c2892e
Cr-Commit-Position: refs/heads/master@{#301328}