Expose kDeviceLoginScreenDomainAutoComplete through CrosSettings

Expose kDeviceLoginScreenDomainAutoComplete through CrosSettings

BUG=471242
Committed: https://crrev.com/4266e4cb376444ab767ee39c1a96267626bd5da9
Cr-Commit-Position: refs/heads/master@{#324048}

[peletskyi, 2015-04-01 13:14:46]

peletskyi@chromium.org changed reviewers:
+ atwilson@chromium.org

[peletskyi, 2015-04-01 13:14:47]

Please take a look

[Andrew T Wilson (Slow), 2015-04-01 13:33:22]

Also, we need a test for this. See
DeviceSettingsProviderTest.DecodeHeartbeatSettings for an example.

https://codereview.chromium.org/1052673002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (left):

https://codereview.chromium.org/1052673002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:603:
!container.login_screen_domain_auto_complete().empty()) {
I think you may still want some code like this to set the policy in the policy
map regardless. Maybe ping mattias about whether we actually should remove this
entirely?

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

https://codereview.chromium.org/1052673002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:433:
policy.has_login_screen_domain_auto_complete()
This doesn't look like it compiles. I wonder if we should change the attribute
names so they aren't identical, because this is hard to read.

https://codereview.chromium.org/1052673002/diff/20001/chromeos/settings/cros_...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/1052673002/diff/20001/chromeos/settings/cros_...
chromeos/settings/cros_settings_names.cc:169: // An string pref that specifies a
domain name for autocomplete option during
nit: A string pref

nit: for the autocomplete option

[peletskyi, 2015-04-01 13:55:18]

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

[peletskyi, 2015-04-01 13:55:19]

PTAL

https://codereview.chromium.org/1052673002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (left):

https://codereview.chromium.org/1052673002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:603:
!container.login_screen_domain_auto_complete().empty()) {
On 2015/04/01 13:33:21, Andrew T Wilson wrote:
> I think you may still want some code like this to set the policy in the policy
> map regardless. Maybe ping mattias about whether we actually should remove
this
> entirely?

I will add Mattias as a reviewer.

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

https://codereview.chromium.org/1052673002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:433:
policy.has_login_screen_domain_auto_complete()
On 2015/04/01 13:33:21, Andrew T Wilson wrote:
> This doesn't look like it compiles. I wonder if we should change the attribute
> names so they aren't identical, because this is hard to read.

I've fixed this. About names - I just follow the common pattern in
chrome_device_policy.proto

https://codereview.chromium.org/1052673002/diff/20001/chromeos/settings/cros_...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/1052673002/diff/20001/chromeos/settings/cros_...
chromeos/settings/cros_settings_names.cc:169: // An string pref that specifies a
domain name for autocomplete option during
On 2015/04/01 13:33:22, Andrew T Wilson wrote:
> nit: A string pref
> 
> nit: for the autocomplete option

Done.

[Andrew T Wilson (Slow), 2015-04-01 14:05:15]

mostly lg - still want a test, and want to have mnissler chime in about whether
populating the PolicyMap is still important given that this policy is exposed
via cros settings (I think it probably is).

[peletskyi, 2015-04-01 15:32:54]

On 2015/04/01 14:05:15, Andrew T Wilson wrote:
> mostly lg - still want a test, and want to have mnissler chime in about
whether
> populating the PolicyMap is still important given that this policy is exposed
> via cros settings (I think it probably is).

I've added the test.

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

https://codereview.chromium.org/1052673002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (left):

https://codereview.chromium.org/1052673002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:601: // value
is not an empty string.
The rule is to show whatever is present in policy as accurately as possible in
chrome://policy. Thus, no cleverness required, please make the code dumb.

https://codereview.chromium.org/1052673002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:609: }
This block should stay, so the policy gets displayed correctly on
chrome://policy

https://codereview.chromium.org/1052673002/diff/80001/chromeos/settings/cros_...
File chromeos/settings/cros_settings_names.h (right):

https://codereview.chromium.org/1052673002/diff/80001/chromeos/settings/cros_...
chromeos/settings/cros_settings_names.h:92: CHROMEOS_EXPORT extern const char
kDeviceLoginScreenDomainAutoComplete[];
I think this should be prefixed with kAccountsPref to group it with other device
settings affecting the login screen.

[peletskyi, 2015-04-07 12:06:28]

peletskyi@chromium.org changed reviewers:
+ derat@chromium.org, stevenjb@chromium.org

[peletskyi, 2015-04-07 12:06:29]

PTAL

stevenjb, derat: can you take a look at 
chromeos/settings/cros_settings_names.*

Thanks :)

https://codereview.chromium.org/1052673002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (left):

https://codereview.chromium.org/1052673002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:601: // value
is not an empty string.
On 2015/04/02 08:29:38, Mattias Nissler wrote:
> The rule is to show whatever is present in policy as accurately as possible in
> chrome://policy. Thus, no cleverness required, please make the code dumb.

Done.

https://codereview.chromium.org/1052673002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:609: }
On 2015/04/02 08:29:38, Mattias Nissler wrote:
> This block should stay, so the policy gets displayed correctly on
> chrome://policy

Done.

https://codereview.chromium.org/1052673002/diff/80001/chromeos/settings/cros_...
File chromeos/settings/cros_settings_names.h (right):

https://codereview.chromium.org/1052673002/diff/80001/chromeos/settings/cros_...
chromeos/settings/cros_settings_names.h:92: CHROMEOS_EXPORT extern const char
kDeviceLoginScreenDomainAutoComplete[];
On 2015/04/02 08:29:38, Mattias Nissler wrote:
> I think this should be prefixed with kAccountsPref to group it with other
device
> settings affecting the login screen.

Done.

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

This LGTM from my side, just some documentation and edge case nits that don't
require re-review.

https://codereview.chromium.org/1052673002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/1052673002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:619: // no
autocomplete option during user sign-in flow will be shown.
Shouldn't it default to consumer gaia (i.e. @gmail.com) if not configured?

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

https://codereview.chromium.org/1052673002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_provider.cc:252: // is the
same. Thus lets add policy only if it is set and its value is not
Question: Is there a use case for turning off auto-default to @gmail.com? If so,
the natural way to do this would be to configure an empty auto-completion
domain? Do we know what GAIA does if we give them an empty string as parameter?

https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
chromeos/settings/cros_settings_names.cc:40:
"cros.accounts.transferSAMLCookies";
nit: blank line before comment.

[Daniel Erat, 2015-04-07 13:46:47]

lgtm for chromeos/ with a naming question

https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
chromeos/settings/cros_settings_names.cc:43: const char
kAccountsPrefDeviceLoginScreenDomainAutoComplete[] =
i'm trying to parse the names of all of these constants. does "device" indicate
that it's a device-level, rather than user-level, pref? the earlier constants
with crazy camel-cased values, like
kAccountsPrefDeviceLocalAccountPromptForNetworkWhenOffline, look like they
additionally contain "device" in their values, which this one doesn't.

should it "just" be:

  kAccountsPrefLoginScreenDomainAutoComplete

, i.e. no "device" in the name?

[peletskyi, 2015-04-07 13:48:13]

Hi Mattias,
I provided answers to your questions based on my knowledge
and my understanding.
Also I've fixed the nit.

https://codereview.chromium.org/1052673002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/1052673002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:619: // no
autocomplete option during user sign-in flow will be shown.
On 2015/04/07 12:29:45, Mattias Nissler wrote:
> Shouldn't it default to consumer gaia (i.e. @gmail.com) if not configured?
According to design doc: "Empty String (Not configured) - No auto-complete
option presented to the user [DEFAULT] "

go/cros-prd-autocomplete

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

https://codereview.chromium.org/1052673002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_provider.cc:252: // is the
same. Thus lets add policy only if it is set and its value is not
On 2015/04/07 12:29:45, Mattias Nissler wrote:
> Question: Is there a use case for turning off auto-default to @gmail.com? If
so,
> the natural way to do this would be to configure an empty auto-completion
> domain? Do we know what GAIA does if we give them an empty string as
parameter?

This use case is not mentioned in the design doc go/cros-prd-autocomplete
Maybe more senior developers can provide more information

https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
File chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
chromeos/settings/cros_settings_names.cc:40:
"cros.accounts.transferSAMLCookies";
On 2015/04/07 12:29:45, Mattias Nissler wrote:
> nit: blank line before comment.

Done.

[peletskyi, 2015-04-07 13:59:37]

The CQ bit was checked by peletskyi@chromium.org

[peletskyi, 2015-04-07 13:59:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1052673002/#ps120001
(title: "nit fix + rebase")

[commit-bot: I haz the power, 2015-04-07 14:00:02]

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

[peletskyi, 2015-04-07 14:10:01]

On 2015/04/07 13:46:47, Daniel Erat wrote:
> lgtm for chromeos/ with a naming question
> 
>
https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
> File chromeos/settings/cros_settings_names.cc (right):
> 
>
https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
> chromeos/settings/cros_settings_names.cc:43: const char
> kAccountsPrefDeviceLoginScreenDomainAutoComplete[] =
> i'm trying to parse the names of all of these constants. does "device"
indicate
> that it's a device-level, rather than user-level, pref? the earlier constants
> with crazy camel-cased values, like
> kAccountsPrefDeviceLocalAccountPromptForNetworkWhenOffline, look like they
> additionally contain "device" in their values, which this one doesn't.
> 
> should it "just" be:
> 
>   kAccountsPrefLoginScreenDomainAutoComplete
> 
> , i.e. no "device" in the name?

Agree. From my point of view the names are too long. But anyway to fix it is out
of the scope of the issue.

[Daniel Erat, 2015-04-07 14:13:14]

On 2015/04/07 14:10:01, peletskyi wrote:
> On 2015/04/07 13:46:47, Daniel Erat wrote:
> > lgtm for chromeos/ with a naming question
> > 
> >
>
https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
> > File chromeos/settings/cros_settings_names.cc (right):
> > 
> >
>
https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
> > chromeos/settings/cros_settings_names.cc:43: const char
> > kAccountsPrefDeviceLoginScreenDomainAutoComplete[] =
> > i'm trying to parse the names of all of these constants. does "device"
> indicate
> > that it's a device-level, rather than user-level, pref? the earlier
constants
> > with crazy camel-cased values, like
> > kAccountsPrefDeviceLocalAccountPromptForNetworkWhenOffline, look like they
> > additionally contain "device" in their values, which this one doesn't.
> > 
> > should it "just" be:
> > 
> >   kAccountsPrefLoginScreenDomainAutoComplete
> > 
> > , i.e. no "device" in the name?
> 
> Agree. From my point of view the names are too long. But anyway to fix it is
out
> of the scope of the issue.

i was asking if you can remove "device" from the name of the constant that
you're adding.

[peletskyi, 2015-04-07 14:32:20]

The CQ bit was checked by peletskyi@chromium.org

[peletskyi, 2015-04-07 14:32:20]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1052673002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-04-07 14:32:27]

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

[peletskyi, 2015-04-07 14:33:23]

On 2015/04/07 14:13:14, Daniel Erat wrote:
> On 2015/04/07 14:10:01, peletskyi wrote:
> > On 2015/04/07 13:46:47, Daniel Erat wrote:
> > > lgtm for chromeos/ with a naming question
> > > 
> > >
> >
>
https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
> > > File chromeos/settings/cros_settings_names.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1052673002/diff/100001/chromeos/settings/cros...
> > > chromeos/settings/cros_settings_names.cc:43: const char
> > > kAccountsPrefDeviceLoginScreenDomainAutoComplete[] =
> > > i'm trying to parse the names of all of these constants. does "device"
> > indicate
> > > that it's a device-level, rather than user-level, pref? the earlier
> constants
> > > with crazy camel-cased values, like
> > > kAccountsPrefDeviceLocalAccountPromptForNetworkWhenOffline, look like they
> > > additionally contain "device" in their values, which this one doesn't.
> > > 
> > > should it "just" be:
> > > 
> > >   kAccountsPrefLoginScreenDomainAutoComplete
> > > 
> > > , i.e. no "device" in the name?
> > 
> > Agree. From my point of view the names are too long. But anyway to fix it is
> out
> > of the scope of the issue.
> 
> i was asking if you can remove "device" from the name of the constant that
> you're adding.

yes, I can :) now it is kAccountsPrefLoginScreenDomainAutoComplete

[Daniel Erat, 2015-04-07 15:13:47]

lgtm

thanks

[commit-bot: I haz the power, 2015-04-07 15:57:21]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-07 15:58:20]

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