Added a timezone policy and pyauto tests for it.

Added a timezone policy and pyauto tests for it.

Relanding this CL
  https://chromiumcodereview.appspot.com/10827050/
after a memory leak was fixed in the master.

TBR=darin@chromium.org
BUG=chromium-os:27206
TEST=
Test active policy:
1. Set the timezone policy to X.
2. Login, set timezone to Y (!= X)
3. Logout, check that the timezone is reset to X

Test inactive policy:
1. Set the timezone policy to X.
2. Login, set timezone to Y (!= X)
3. Logout, check that the timezone is still Y


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

[pneubeck (no reviews), 2012-08-09 13:32:40]

Please, lgtm again.
The last commit broke some tests because of some other commit which is fixed in
the tree now.
My patch did not change except rebasing.

[Joao da Silva, 2012-08-09 13:39:47]

lgtm after fixing the BrowserPolicyConnector.

https://chromiumcodereview.appspot.com/10832222/diff/1/chrome/browser/policy/...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://chromiumcodereview.appspot.com/10832222/diff/1/chrome/browser/policy/...
chrome/browser/policy/browser_policy_connector.cc:153: if
(MessageLoop::current()) {
Don't add this test, just run the inner stuff directly.

[pneubeck (no reviews), 2012-08-09 13:45:34]

Removed the test.

http://codereview.chromium.org/10832222/diff/1/chrome/browser/policy/browser_...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/10832222/diff/1/chrome/browser/policy/browser_...
chrome/browser/policy/browser_policy_connector.cc:153: if
(MessageLoop::current()) {
On 2012/08/09 13:39:47, Joao da Silva wrote:
> Don't add this test, just run the inner stuff directly.

Done.

[pneubeck (no reviews), 2012-08-10 07:03:39]

jcivelli, please take a look at the tests, that I want to add, and the small
change to pyauto.py.
Thanks.

[pneubeck (no reviews), 2012-08-10 07:13:12]

mnissler, please take a look at the changes to settings/*.
Thanks.

[Mattias Nissler (ping if slow), 2012-08-10 07:45:16]

couple nits

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/app/policy/p...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/app/policy/p...
chrome/app/policy/policy_templates.json:3167: If additionally no user ever set a
timezone, a default value (currently "US/Pacific") is used.''',
Nit: additionally? In addition to what? I think yo just want to drop
additionally.

Also, formatting in this file uses a newline between paragraphs, and you should
follow that style.

Finally, you should let the reader know what the valid values for the timezone
setting are.

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/browser/chro...
File chrome/browser/chromeos/settings/cros_settings_names.cc (right):

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/browser/chro...
chrome/browser/chromeos/settings/cros_settings_names.cc:29: // the description
in policy_templates.json).
It would be good to say what it doesn't support instead of "required behavior".

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/browser/poli...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/browser/poli...
chrome/browser/policy/proto/chrome_device_policy.proto:210: // Specifies the
timezone which is applied if this value is provided.
Please document which format this is in.

[pneubeck (no reviews), 2012-08-10 15:32:58]

Addressed comments and fixed a bug reported on linux_chromeos.

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/app/policy/p...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/app/policy/p...
chrome/app/policy/policy_templates.json:3167: If additionally no user ever set a
timezone, a default value (currently "US/Pacific") is used.''',
On 2012/08/10 07:45:16, Mattias Nissler wrote:
> Nit: additionally? In addition to what? I think yo just want to drop
> additionally.
> 
> Also, formatting in this file uses a newline between paragraphs, and you
should
> follow that style.
> 
> Finally, you should let the reader know what the valid values for the timezone
> setting are.

Done.

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/browser/chro...
File chrome/browser/chromeos/settings/cros_settings_names.cc (right):

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/browser/chro...
chrome/browser/chromeos/settings/cros_settings_names.cc:29: // the description
in policy_templates.json).
On 2012/08/10 07:45:16, Mattias Nissler wrote:
> It would be good to say what it doesn't support instead of "required
behavior".

Done.

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/browser/poli...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/browser/poli...
chrome/browser/policy/proto/chrome_device_policy.proto:210: // Specifies the
timezone which is applied if this value is provided.
On 2012/08/10 07:45:16, Mattias Nissler wrote:
> Please document which format this is in.

Done.

[Mattias Nissler (ping if slow), 2012-08-13 07:49:46]

chrome/{app,browser}/policy LGTM

https://chromiumcodereview.appspot.com/10832222/diff/11003/chrome/app/policy/...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/10832222/diff/11003/chrome/app/policy/...
chrome/app/policy/policy_templates.json:3169: The specified value is valid if it
is a name of a timezone listed in the "IANA Time Zone Database" (see
"http://en.wikipedia.org/wiki/List_of_tz_database_time"). In particular, most
timezones can be referred to by "continent/large_city" or "ocean/large_city".
Currently, not all of the timezones may be supported.''',
Nitty nit: This is somewhat contradictory given that you first say all
IANA-defined time zones are valid and then mention that not all are support.
Just say "The format of the value follows IANA's Time Zone Database" or
somesuch?

[pneubeck (no reviews), 2012-08-13 12:26:45]

Fixed the policy description.

https://chromiumcodereview.appspot.com/10832222/diff/11003/chrome/app/policy/...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/10832222/diff/11003/chrome/app/policy/...
chrome/app/policy/policy_templates.json:3169: The specified value is valid if it
is a name of a timezone listed in the "IANA Time Zone Database" (see
"http://en.wikipedia.org/wiki/List_of_tz_database_time"). In particular, most
timezones can be referred to by "continent/large_city" or "ocean/large_city".
Currently, not all of the timezones may be supported.''',
On 2012/08/13 07:49:46, Mattias Nissler wrote:
> Nitty nit: This is somewhat contradictory given that you first say all
> IANA-defined time zones are valid and then mention that not all are support.
> Just say "The format of the value follows IANA's Time Zone Database" or
> somesuch?

Done.

[pneubeck (no reviews), 2012-08-14 08:23:57]

Nirnimesh, could you please teak a look at the changes affecting chrome/test/**.
Thanks.

darin already approved the changes to automation.

[Nirnimesh, 2012-08-14 18:09:37]

chrome/test/ LGTM

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
File chrome/test/functional/chromeos_device_policy.py (right):

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
chrome/test/functional/chromeos_device_policy.py:206:
self.SetDevicePolicy({'timezone':self._timezones[0]}, refresh=True)
nit: need space after :

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
chrome/test/functional/chromeos_device_policy.py:221: 
nit: need another blank line here

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
File chrome/test/functional/chromeos_time.py (right):

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
chrome/test/functional/chromeos_time.py:91: self.WaitUntil(lambda: timezone ==
self.GetTimeInfo()['timezone']),
use expect_retval arg to WaitUntil

self.WaitUntil(self.GetTimeInfo()['timezeon'], expect_retval=timezone,
msg='...')

This is so that in debug mode (-v) it prints out the actual vs expected for
every poll attempt.

[pneubeck (no reviews), 2012-08-16 06:47:28]

Fixed nits.

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
File chrome/test/functional/chromeos_device_policy.py (right):

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
chrome/test/functional/chromeos_device_policy.py:206:
self.SetDevicePolicy({'timezone':self._timezones[0]}, refresh=True)
On 2012/08/14 18:09:37, Nirnimesh wrote:
> nit: need space after :

Done.

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
chrome/test/functional/chromeos_device_policy.py:221: 
On 2012/08/14 18:09:37, Nirnimesh wrote:
> nit: need another blank line here

Done.

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
File chrome/test/functional/chromeos_time.py (right):

http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chr...
chrome/test/functional/chromeos_time.py:91: self.WaitUntil(lambda: timezone ==
self.GetTimeInfo()['timezone']),
On 2012/08/14 18:09:37, Nirnimesh wrote:
> use expect_retval arg to WaitUntil
> 
> self.WaitUntil(self.GetTimeInfo()['timezeon'], expect_retval=timezone,
> msg='...')
> 
> This is so that in debug mode (-v) it prints out the actual vs expected for
> every poll attempt.

Done.

[commit-bot: I haz the power, 2012-08-16 09:40:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/10832222/10004

[commit-bot: I haz the power, 2012-08-16 09:40:41]

Failed to apply patch for chrome/app/policy/policy_templates.json:
While running patch -p1 --forward --force;
patching file chrome/app/policy/policy_templates.json
Hunk #1 FAILED at 112.
Hunk #2 succeeded at 3165 (offset 15 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej

[commit-bot: I haz the power, 2012-08-16 09:48:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/10832222/6021

[commit-bot: I haz the power, 2012-08-16 09:48:50]

Presubmit check for 10832222-6021 failed and returned exit status 1.


Running presubmit commit checks ...
Finished checking
/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
errors, 0 warnings.

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/automation

Presubmit checks took 1.7s to calculate.

[commit-bot: I haz the power, 2012-08-16 09:52:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/10832222/6021

[commit-bot: I haz the power, 2012-08-16 13:42:39]

Change committed as 151884