DeviceSettingsProvider's write path uses OwnerSettingsService now.

DeviceSettingsProvider's write path uses OwnerSettingsService now.

This changelist follows https://codereview.chromium.org/654263003/.

BUG=230018
TEST=existing unit_tests

[ygorshenin1, 2014-10-22 14:38:01]

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

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

This LGTM. Given that this has a net-negative impact on # source lines, can you
just include these changes in CL https://codereview.chromium.org/654263003?

https://codereview.chromium.org/666363002/diff/1/chrome/browser/chromeos/sett...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/666363002/diff/1/chrome/browser/chromeos/sett...
chrome/browser/chromeos/settings/device_settings_provider.cc:543:
scoped_ptr<em::PolicyData> policy(new em::PolicyData());
Assuming that we want to get rid of the Set() code path in
DeviceSettingsProvider entirely, we probably want to break out the code for
caching settings before ownership gets established. Perhaps we can get away with
a static SetTentativeDeviceSetting function in OwnershipSettingsServiceChromeOS
that just updates the cache in local_state?

Let's do that in a subsequent CL though.

https://codereview.chromium.org/666363002/diff/1/components/ownership/owner_s...
File components/ownership/owner_settings_service.h (right):

https://codereview.chromium.org/666363002/diff/1/components/ownership/owner_s...
components/ownership/owner_settings_service.h:89: 
Can we put a comment here saying that this function is a temporary way to deal
with the problem of getting the tentative device settings from before ownership
gets established into OwnershipSettingsServiceChromeOS? Might also be good to
rename this to CommitTentativeDeviceSettings or somesuch to make this more
clear.

[ygorshenin1, 2014-10-24 09:06:05]

Patchset #2 (id:20001) has been deleted

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

Would you mind if I'll not merge the CL to
https://codereview.chromium.org/654263003? M40 will be soon (in less than two
weeks) and the change is quite dangerous from my point of view. In case of
issues it would be much simpler and safer to revert only this CL.

https://codereview.chromium.org/666363002/diff/1/chrome/browser/chromeos/sett...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/666363002/diff/1/chrome/browser/chromeos/sett...
chrome/browser/chromeos/settings/device_settings_provider.cc:543:
scoped_ptr<em::PolicyData> policy(new em::PolicyData());
On 2014/10/24 08:08:02, Mattias Nissler wrote:
> Assuming that we want to get rid of the Set() code path in
> DeviceSettingsProvider entirely, we probably want to break out the code for
> caching settings before ownership gets established. Perhaps we can get away
with
> a static SetTentativeDeviceSetting function in
OwnershipSettingsServiceChromeOS
> that just updates the cache in local_state?
> 
> Let's do that in a subsequent CL though.

Sounds good, I'll implement that in a subsequent CL.

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

IIUC, if you don't merge, then we have two write paths to device settings that
are not coordinating, which we know can result in lost writes. Admittedly, the
OnwnerSettingsService path is currently unused, but I'm uneasy about LGTMing and
committing stuff that's known to be broken. If you're worried about M40, why not
commit all of it only after the branch point?

On 2014/10/24 09:13:28, ygorshenin1 wrote:
> Would you mind if I'll not merge the CL to
> <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">https://codereview.chromium.org/654263003</crxref>
M40 will be soon (in less than two
> weeks) and the change is quite dangerous from my point of view. In case of
> issues it would be much simpler and safer to revert only this CL.
> 
>
https://codereview.chromium.org/666363002/diff/1/chrome/browser/chromeos/sett...
> File chrome/browser/chromeos/settings/device_settings_provider.cc (right):
> 
>
https://codereview.chromium.org/666363002/diff/1/chrome/browser/chromeos/sett...
> chrome/browser/chromeos/settings/device_settings_provider.cc:543:
> scoped_ptr<em::PolicyData> policy(new em::PolicyData());
> On 2014/10/24 08:08:02, Mattias Nissler wrote:
> > Assuming that we want to get rid of the Set() code path in
> > DeviceSettingsProvider entirely, we probably want to break out the code for
> > caching settings before ownership gets established. Perhaps we can get away
> with
> > a static SetTentativeDeviceSetting function in
> OwnershipSettingsServiceChromeOS
> > that just updates the cache in local_state?
> > 
> > Let's do that in a subsequent CL though.
> 
> Sounds good, I'll implement that in a subsequent CL.

[Nikita (slow), 2014-10-24 10:15:49]

On 2014/10/24 10:13:33, Mattias Nissler wrote:
> IIUC, if you don't merge, then we have two write paths to device settings that
> are not coordinating, which we know can result in lost writes. Admittedly, the
> OnwnerSettingsService path is currently unused, but I'm uneasy about LGTMing
and
> committing stuff that's known to be broken. If you're worried about M40, why
not
> commit all of it only after the branch point?

Since there's a risk if only one CL is part of M40 this sounds like both should
be postponed to M41.

> 
> On 2014/10/24 09:13:28, ygorshenin1 wrote:
> > Would you mind if I'll not merge the CL to
> > <crxref style="background-image:
>
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">https://codereview.chromium.org/654263003</crxref>
> M40 will be soon (in less than two
> > weeks) and the change is quite dangerous from my point of view. In case of
> > issues it would be much simpler and safer to revert only this CL.
> > 
> >
>
https://codereview.chromium.org/666363002/diff/1/chrome/browser/chromeos/sett...
> > File chrome/browser/chromeos/settings/device_settings_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/666363002/diff/1/chrome/browser/chromeos/sett...
> > chrome/browser/chromeos/settings/device_settings_provider.cc:543:
> > scoped_ptr<em::PolicyData> policy(new em::PolicyData());
> > On 2014/10/24 08:08:02, Mattias Nissler wrote:
> > > Assuming that we want to get rid of the Set() code path in
> > > DeviceSettingsProvider entirely, we probably want to break out the code
for
> > > caching settings before ownership gets established. Perhaps we can get
away
> > with
> > > a static SetTentativeDeviceSetting function in
> > OwnershipSettingsServiceChromeOS
> > > that just updates the cache in local_state?
> > > 
> > > Let's do that in a subsequent CL though.
> > 
> > Sounds good, I'll implement that in a subsequent CL.

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

The CL is merged to https://codereview.chromium.org/654263003/.