ChromeOS: Add SystemTimezoneAutomaticDetection policy.

ChromeOS: Add SystemTimezoneAutomaticDetection policy.

This CL adds SystemTimezoneAutomaticDetection device policy
and modifies all necessary components to support it.

BUG=596690
TEST=unittest

[Alexander Alekseev, 2016-03-25 11:36:22]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-25 11:36:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836433003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836433003/1

[Alexander Alekseev, 2016-03-25 11:42:14]

alemate@chromium.org changed reviewers:
+ stevenjb@chromium.org, tnagel@chromium.org

[Alexander Alekseev, 2016-03-25 11:42:15]

Please review:

tnagel:
  *policy*

stevenjb:
  all the rest

[commit-bot: I haz the power, 2016-03-25 12:46:01]

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

[commit-bot: I haz the power, 2016-03-25 12:46:02]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-03-28 23:23:18]

This is another really large CL and it's hard to follow all of the changes. It
would be much better broken into two or three parts:
a) The addition of timezone_resolver_manager.cc TimeZoneResolver::Delegate (and
related re-factoring)
b) The new policy code
c) The UI

Maybe b and c need to be combined, but even splitting this in two would help.

Also, it is not clear to me that just adding the test case to
policy_test_cases.json provides adequate test coverage?

https://codereview.chromium.org/1836433003/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:2355: Enable
EnableSystemTimezoneAutomaticDetection device policy.
nit: 'Enable system timezone automatic detection device policy.'

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:888:
.has_timezone_detection_type()) {
Combine ifs

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/timezone_resolver_manager.cc (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.cc:22: // time zone
resolve process.
nit: processes

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.cc:48:
ResolveDeviceTimezoneByGeolocationPolicyValues::NUM_ELEMENTS);
Instead, before the default return add:
NOTREACHED() << "Unrecognized policy value: " << policy_value;

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.cc:98: }
nit: no {}

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.cc:158: }
nit: no {}

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/timezone_resolver_manager.h (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.h:28: // Starts or
stops TimezoneResolver if according to currect settings.
s/if//

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.h:33: bool
IfShouldApplyResolvedTimezone();
Just ShouldApplyResolvedTimezone()

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.h:36: // Returns truw
if TimeZoneResolver should be running taking in account
s/truw/true/
s/running taking in/running and taking into/

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1703:
$('resolve-timezone-by-geolocation').checked = null;
false

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1709: .checked =
this.resolveTimezoneByGeolocation_ ? true : null;
true : false ?

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1718: null;
Shouldn't this just be (this.systemTimezoneAutomaticDetectionValue_ != 1 /*
DISABLED */) ?

https://codereview.chromium.org/1836433003/diff/1/chromeos/timezone/timezone_...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/1836433003/diff/1/chromeos/timezone/timezone_...
chromeos/timezone/timezone_resolver.cc:188:
resolver_->ShouldSendWiFiGeolocationData(),
just ShouldSendWiFiGeolocationData() ?

[Alexander Alekseev, 2016-03-29 00:29:26]

On 2016/03/28 23:23:18, stevenjb wrote:
> This is another really large CL and it's hard to follow all of the changes. It
> would be much better broken into two or three parts:
> a) The addition of timezone_resolver_manager.cc TimeZoneResolver::Delegate
(and
> related re-factoring)
> b) The new policy code
> c) The UI
> 
> Maybe b and c need to be combined, but even splitting this in two would help.

Well, I'll split this.

> Also, it is not clear to me that just adding the test case to
> policy_test_cases.json provides adequate test coverage?

This is not adequate, but I decoded to make this test in another CL. This is
actually too large.

https://codereview.chromium.org/1836433003/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:2355: Enable
EnableSystemTimezoneAutomaticDetection device policy.
On 2016/03/28 23:23:17, stevenjb wrote:
> nit: 'Enable system timezone automatic detection device policy.'

Done.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:888:
.has_timezone_detection_type()) {
On 2016/03/28 23:23:17, stevenjb wrote:
> Combine ifs

Done.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/timezone_resolver_manager.cc (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.cc:22: // time zone
resolve process.
On 2016/03/28 23:23:17, stevenjb wrote:
> nit: processes

Done.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.cc:48:
ResolveDeviceTimezoneByGeolocationPolicyValues::NUM_ELEMENTS);
On 2016/03/28 23:23:17, stevenjb wrote:
> Instead, before the default return add:
> NOTREACHED() << "Unrecognized policy value: " << policy_value;

Done.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.cc:98: }
On 2016/03/28 23:23:17, stevenjb wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.cc:158: }
On 2016/03/28 23:23:17, stevenjb wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/timezone_resolver_manager.h (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.h:28: // Starts or
stops TimezoneResolver if according to currect settings.
On 2016/03/28 23:23:17, stevenjb wrote:
> s/if//

Done.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.h:33: bool
IfShouldApplyResolvedTimezone();
On 2016/03/28 23:23:17, stevenjb wrote:
> Just ShouldApplyResolvedTimezone()

Done.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/timezone_resolver_manager.h:36: // Returns truw
if TimeZoneResolver should be running taking in account
On 2016/03/28 23:23:17, stevenjb wrote:
> s/truw/true/
> s/running taking in/running and taking into/

Done.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1703:
$('resolve-timezone-by-geolocation').checked = null;
On 2016/03/28 23:23:17, stevenjb wrote:
> false

This doesn't work. It will stay checked unless "checked" attribute is deleted.
Setting it value to false will not help.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1709: .checked =
this.resolveTimezoneByGeolocation_ ? true : null;
On 2016/03/28 23:23:17, stevenjb wrote:
> true : false ?

null is required.

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1718: null;
On 2016/03/28 23:23:18, stevenjb wrote:
> Shouldn't this just be (this.systemTimezoneAutomaticDetectionValue_ != 1 /*
> DISABLED */) ?

Null is required here.

https://codereview.chromium.org/1836433003/diff/1/chromeos/timezone/timezone_...
File chromeos/timezone/timezone_resolver.cc (right):

https://codereview.chromium.org/1836433003/diff/1/chromeos/timezone/timezone_...
chromeos/timezone/timezone_resolver.cc:188:
resolver_->ShouldSendWiFiGeolocationData(),
On 2016/03/28 23:23:18, stevenjb wrote:
> just ShouldSendWiFiGeolocationData() ?

TZRequest doesn't have ShouldSendWiFiGeolocationData() .
Only TimeZoneResolver / TimeZoneResolverImpl do.

[Alexander Alekseev, 2016-03-29 00:30:38]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 00:31:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836433003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836433003/20001

[commit-bot: I haz the power, 2016-03-29 00:34:21]

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

[commit-bot: I haz the power, 2016-03-29 00:34:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)

[stevenjb, 2016-03-29 00:39:41]

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1703:
$('resolve-timezone-by-geolocation').checked = null;
On 2016/03/29 00:29:26, Alexander Alekseev wrote:
> On 2016/03/28 23:23:17, stevenjb wrote:
> > false
> 
> This doesn't work. It will stay checked unless "checked" attribute is deleted.
> Setting it value to false will not help.

That's strange, are you certain? The type of input.checked is Boolean. I don't
see us doing that elsewhere.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox

[Alexander Alekseev, 2016-03-29 01:11:47]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 01:12:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836433003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836433003/40001

[Alexander Alekseev, 2016-03-29 02:08:28]

This patch was split into two:

https://codereview.chromium.org/1837893003/
https://codereview.chromium.org/1843523002/

Closing this one.

[Alexander Alekseev, 2016-03-29 02:08:41]

Description was changed from

==========
ChromeOS: Add SystemTimezoneAutomaticDetection policy.

This CL adds SystemTimezoneAutomaticDetection device policy
and modifies all necessary components to support it.

BUG=596690
TEST=unittest
==========

to

==========
ChromeOS: Add SystemTimezoneAutomaticDetection policy.

This CL adds SystemTimezoneAutomaticDetection device policy
and modifies all necessary components to support it.

BUG=596690
TEST=unittest
==========

[Alexander Alekseev, 2016-03-29 03:59:24]

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:1703:
$('resolve-timezone-by-geolocation').checked = null;
On 2016/03/29 00:39:41, stevenjb wrote:
> On 2016/03/29 00:29:26, Alexander Alekseev wrote:
> > On 2016/03/28 23:23:17, stevenjb wrote:
> > > false
> > 
> > This doesn't work. It will stay checked unless "checked" attribute is
deleted.
> > Setting it value to false will not help.
> 
> That's strange, are you certain? The type of input.checked is Boolean. I don't
> see us doing that elsewhere.
> 
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox

" the presence of this Boolean attribute indicates that the control is selected
by default; otherwise it is ignored."

This turned out to be the exact description. It took me a while to figure out
why setting this value to false doesn't work. The key is "the presence of this
attribute".

[stevenjb, 2016-03-29 16:49:52]

So, you are correct in that in the HTML the presence of the attribute
determines whether or not the element is checked, however in javascript you
can set the checked property to false, which is what we do elsewhere in the
code. I should have directed you here:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement


On Mon, Mar 28, 2016 at 8:59 PM, <alemate@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
> File chrome/browser/resources/options/browser_options.js (right):
>
>
>
https://codereview.chromium.org/1836433003/diff/1/chrome/browser/resources/op...
> chrome/browser/resources/options/browser_options.js:1703:
> $('resolve-timezone-by-geolocation').checked = null;
> On 2016/03/29 00:39:41, stevenjb wrote:
> > On 2016/03/29 00:29:26, Alexander Alekseev wrote:
> > > On 2016/03/28 23:23:17, stevenjb wrote:
> > > > false
> > >
> > > This doesn't work. It will stay checked unless "checked" attribute
> is deleted.
> > > Setting it value to false will not help.
> >
> > That's strange, are you certain? The type of input.checked is Boolean.
> I don't
> > see us doing that elsewhere.
> >
> >
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox
>
> " the presence of this Boolean attribute indicates that the control is
> selected by default; otherwise it is ignored."
>
> This turned out to be the exact description. It took me a while to
> figure out why setting this value to false doesn't work. The key is "the
> presence of this attribute".
>
> https://codereview.chromium.org/1836433003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.