[cros] Add checkbox for toggling data roaming setting.

[cros] Add checkbox for toggling data roaming setting.

BUG=chromium-os:12008
TEST=manual.

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

[Nikita (slow), 2011-03-16 16:45:17]

Erik, please review HTML/JS/CSS bits.

Mattias, we probably want to make this option managed by policy. I'll address
that in a separate CL then.

[Nikita (slow), 2011-03-16 16:46:10]

+mnissler

Mattias, we probably want to make this option managed by policy. I'll address
that in a separate CL then.

[arv (Not doing code reviews), 2011-03-16 17:04:12]

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos_internet_options.html (right):

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_internet_options.html:11: <button
id="enableWifi" class="hidden"
Use hidden attribute instead?

IDs should use dashes

<button id="enable-wifi" hidden i18n-content"enableWifi"></button>

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_internet_options.html:22: <span
i18n-content="owner_only"></span>
i18n names should use camelCase

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_internet_options.html:25: <table
class="option-control-table">
Why the table?

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos_internet_options.js (right):

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_internet_options.js:170:
$('internetOwnerOnlyWarning').classList.remove('hidden');
Or if you used hidden attribute

$('internetOwnerOnlyWarning').hidden = false;

I see that the rest of the code here uses a class name but code cleanup is
always welcome.

[Denis Lagno, 2011-03-16 17:09:35]

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings_names.cc (right):

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_names.cc:17: const char
kSignedDataRoamingEnabled[] = "cros.signed.data_roaming_enabled";
is cros.signed.* a good namespace?  it makes thinking that other settings are
not signed?

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:40:
kSignedDataRoamingEnabled,
I think it should default to false.  But in this case it contradicts comment
above.

[Mattias Nissler (ping if slow), 2011-03-16 17:38:32]

On 2011/03/16 16:46:10, Nikita Kostylev wrote:
> +mnissler
> 
> Mattias, we probably want to make this option managed by policy. I'll address
> that in a separate CL then.

Makes sense. Is this a per-user or per-device setting?

[Nikita (slow), 2011-03-16 17:42:39]

On Wed, Mar 16, 2011 at 8:38 PM, <mnissler@chromium.org> wrote:

> Makes sense. Is this a per-user or per-device setting?


Per device, controllable by owner, stored in signed settings.

-- 
Nikita

[Mattias Nissler (ping if slow), 2011-03-16 17:44:57]

On Wed, Mar 16, 2011 at 6:40 PM, Nikita Kostylev <nkostylev@chromium.org>wrote:

> On Wed, Mar 16, 2011 at 8:38 PM, <mnissler@chromium.org> wrote:
>
>> Makes sense. Is this a per-user or per-device setting?
>
>
> Per device, controllable by owner, stored in signed settings.
>

OK, so we probably want to create a device policy for this. Don't worry
about the implementation for now, we need to put the infrastructure into
place first. How about creating a bug for tracking this?

[Nikita (slow), 2011-03-17 10:45:57]

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings_names.cc (right):

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_names.cc:17: const char
kSignedDataRoamingEnabled[] = "cros.signed.data_roaming_enabled";
On 2011/03/16 17:09:35, Denis Lagno wrote:
> is cros.signed.* a good namespace?  it makes thinking that other settings are
> not signed?

Added comment about that.

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:40:
kSignedDataRoamingEnabled,
On 2011/03/16 17:09:35, Denis Lagno wrote:
> I think it should default to false.  But in this case it contradicts comment
> above.

Done.

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos_internet_options.html (right):

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_internet_options.html:11: <button
id="enableWifi" class="hidden"
On 2011/03/16 17:04:12, arv wrote:
> Use hidden attribute instead?
> 
> IDs should use dashes
> 
> <button id="enable-wifi" hidden i18n-content"enableWifi"></button>

Done. Did some cleanup.

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_internet_options.html:22: <span
i18n-content="owner_only"></span>
On 2011/03/16 17:04:12, arv wrote:
> i18n names should use camelCase

Done.

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_internet_options.html:25: <table
class="option-control-table">
On 2011/03/16 17:04:12, arv wrote:
> Why the table?

Done.

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos_internet_options.js (right):

http://codereview.chromium.org/6679040/diff/2002/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_internet_options.js:170:
$('internetOwnerOnlyWarning').classList.remove('hidden');
On 2011/03/16 17:04:12, arv wrote:
> Or if you used hidden attribute
> 
> $('internetOwnerOnlyWarning').hidden = false;
> 
> I see that the rest of the code here uses a class name but code cleanup is
> always welcome.

Done.

http://codereview.chromium.org/6679040/diff/2004/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos_accounts_options_page.css
(right):

http://codereview.chromium.org/6679040/diff/2004/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_accounts_options_page.css:78:
padding-bottom: 1px;
warning.png was cut by 1px at the bottom.

http://codereview.chromium.org/6679040/diff/2004/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos_internet_options_page.css
(right):

http://codereview.chromium.org/6679040/diff/2004/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos_internet_options_page.css:20:
-webkit-margin-start: 10px;
Align checkbox with the rest of the section.

[Denis Lagno, 2011-03-17 11:05:27]

LGTM

http://codereview.chromium.org/6679040/diff/2004/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/6679040/diff/2004/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:57: (pref_path ==
kAccountsPrefAllowGuest) ||
nit: is this correct indentation?

[Nikita (slow), 2011-03-18 10:53:17]

Erik, please take a look,
I've addressed your HTML/JS comments.

http://codereview.chromium.org/6679040/diff/2004/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/6679040/diff/2004/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:57: (pref_path ==
kAccountsPrefAllowGuest) ||
On 2011/03/17 11:05:27, Denis Lagno wrote:
> nit: is this correct indentation?

Done.

[arv (Not doing code reviews), 2011-03-18 18:07:32]

LGTM