Added a timezone policy and pyauto tests for it.

Added a timezone policy and pyauto tests for it.

BUG=chromium-os:27206
TEST=See included tests.


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

[pneubeck (no reviews), 2012-07-27 05:27:08]

Please review, thanks.

[Joao da Silva, 2012-07-27 13:40:56]

I'm mostly cool with this change except for the introduction of SetCrosSettings
in automation, please see the comments about that.

http://codereview.chromium.org/10827050/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10827050/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:2787: If this policy is not used or set
to an invalid value, always the timezone set by the last user is active also
after logging-out.
Maybe it's just me, but the 2nd part of the sentence sounds a bit weird;
rephrase?

http://codereview.chromium.org/10827050/diff/1/chrome/browser/automation/test...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/10827050/diff/1/chrome/browser/automation/test...
chrome/browser/automation/testing_automation_provider.h:1610: void
SetCrosSetting(base::DictionaryValue* args, IPC::Message* reply_message);
This is a big hammer. We previously had a similar call to SetPolicies() and
ultimately decided to remove it.

Notice that there's already a SetTimezone() automation call, isn't it enough to
cover your test?

http://codereview.chromium.org/10827050/diff/1/chrome/browser/chromeos/cros_s...
File chrome/browser/chromeos/cros_settings_names.cc (right):

http://codereview.chromium.org/10827050/diff/1/chrome/browser/chromeos/cros_s...
chrome/browser/chromeos/cros_settings_names.cc:27: const char kSystemTimezone[]
= "cros.system.timezone";
Please comment why there are two similar settings for the timezone.

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

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/browser_...
chrome/browser/policy/browser_policy_connector.cc:16: #include
"chrome/browser/chromeos/system/timezone_settings.h"
Move these to the #if defined(OS_CHROMEOS) below

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/browser_...
chrome/browser/policy/browser_policy_connector.cc:575:
SetTimezoneIfPolicyAvailable();
Notice that CompleteInitialization() is only executed if device policy is
enabled. Please do this refactorings:

- move the posting of CompleteInitialization from InitializeDevicePolicy to
Init, and make it unconditional (and execute on every platform). It should be
the last thing done in Init.

- move the posting of GetAppPackUpdater from Init to CompleteInitialization

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/browser_...
chrome/browser/policy/browser_policy_connector.cc:579: #if defined(OS_CHROMEOS)
Move the #ifdef into the method, and make the method available on every platform
i.e. remove the #ifdef from the header. See InitializeDevicePolicy for an
example.

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/browser_...
chrome/browser/policy/browser_policy_connector.cc:588: result ==
Provider::PERMANENTLY_UNTRUSTED)
Nit: indent of the 2nd line
Nit: braces when the conditional or the body span more than 1 line

I'd rather write this as

if (result != Provider::TRUSTED)
  return;

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/proto/ch...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:210: optional string
timezone = 1;
Please document the field, even if it's obvious :-)

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
File chrome/test/functional/chromeos_device_policy.py (right):

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
chrome/test/functional/chromeos_device_policy.py:238: def
SetTimezoneInCrosSettings(self, timezone):
Private methods' names start with _.

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
chrome/test/functional/chromeos_device_policy.py:267:
self.SetTimezoneInCrosSettings(self._timezones[1])
It seems to me that both uses of SetTimezoneInCrosSettings here can be replaced
with self.SetTimezone (from chrome/test/pyautolib/pyauto.py)

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
File chrome/test/functional/chromeos_time.py (right):

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
chrome/test/functional/chromeos_time.py:77: #self.Login(user_index=1,
expect_success=True)
Remove?

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
chrome/test/functional/chromeos_time.py:81: self.WaitUntil(lambda: timezone ==
self.GetTimeInfo()['timezone']),
Nit: indent

[pneubeck (no reviews), 2012-07-31 22:21:05]

Please see my comments.

http://codereview.chromium.org/10827050/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10827050/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:2787: If this policy is not used or set
to an invalid value, always the timezone set by the last user is active also
after logging-out.
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Maybe it's just me, but the 2nd part of the sentence sounds a bit weird;
> rephrase?

Done.

http://codereview.chromium.org/10827050/diff/1/chrome/browser/automation/test...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/10827050/diff/1/chrome/browser/automation/test...
chrome/browser/automation/testing_automation_provider.h:1610: void
SetCrosSetting(base::DictionaryValue* args, IPC::Message* reply_message);
On 2012/07/27 13:40:57, Joao da Silva wrote:
> This is a big hammer. We previously had a similar call to SetPolicies() and
> ultimately decided to remove it.
> 
> Notice that there's already a SetTimezone() automation call, isn't it enough
to
> cover your test?
The idea of this definition was to change chromeos settings (controlled by
CrosSettings) in the same fashion as normal settings are set using the function
SetPrefs. I see, that this means that policies can also be changed via the owner
of the device because these are also controlled by CrosSettings.
I don't see in which way this is problematic. Surely it is not really necessary
as long as the timezone test is the only use case. But the difference of the
generic function i provided and the a special function for timezone isn't that
huge... so why should I provide the special one?

The major difference between setting the timezone with this function and the
existing SetTimezone is, that this one talks to CrosSettings and the other to
the underlying chromeos::system::TimezoneSettings.

Depending on what we like to have, more a functional, integration or unit test,
both may be more appropriate. For functional tests, which I wanted to implement
and many pyauto test seem to be, the new function should be proper.

Do you have any other suggestions or which of the variants do you think fits
better?

http://codereview.chromium.org/10827050/diff/1/chrome/browser/chromeos/cros_s...
File chrome/browser/chromeos/cros_settings_names.cc (right):

http://codereview.chromium.org/10827050/diff/1/chrome/browser/chromeos/cros_s...
chrome/browser/chromeos/cros_settings_names.cc:27: const char kSystemTimezone[]
= "cros.system.timezone";
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Please comment why there are two similar settings for the timezone.

Done.

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

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/browser_...
chrome/browser/policy/browser_policy_connector.cc:16: #include
"chrome/browser/chromeos/system/timezone_settings.h"
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Move these to the #if defined(OS_CHROMEOS) below

Done.

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/browser_...
chrome/browser/policy/browser_policy_connector.cc:575:
SetTimezoneIfPolicyAvailable();
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Notice that CompleteInitialization() is only executed if device policy is
> enabled. Please do this refactorings:
> 
> - move the posting of CompleteInitialization from InitializeDevicePolicy to
> Init, and make it unconditional (and execute on every platform). It should be
> the last thing done in Init.
> 
> - move the posting of GetAppPackUpdater from Init to CompleteInitialization

The only thing, I don't like about the "execute on every platform" is, that
GetAppPackUpdater is also ChromeOS only. Thus if !chromeos we post an empty
task.
Therefore I made it conditional on the chromeos platform.

Is that ok?

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/browser_...
chrome/browser/policy/browser_policy_connector.cc:579: #if defined(OS_CHROMEOS)
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Move the #ifdef into the method, and make the method available on every
platform
> i.e. remove the #ifdef from the header. See InitializeDevicePolicy for an
> example.

Done.

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/browser_...
chrome/browser/policy/browser_policy_connector.cc:588: result ==
Provider::PERMANENTLY_UNTRUSTED)
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Nit: indent of the 2nd line
> Nit: braces when the conditional or the body span more than 1 line
> 
> I'd rather write this as
> 
> if (result != Provider::TRUSTED)
>   return;

Done.

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/proto/ch...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:210: optional string
timezone = 1;
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Please document the field, even if it's obvious :-)
Did so. However, I not happy with providing a description relating to the
implementation or repeating the policy behavior. This would be redundant and
surely be out-of-date shortly.

http://codereview.chromium.org/10827050/diff/1/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:210: optional string
timezone = 1;
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Please document the field, even if it's obvious :-)

Done.

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
File chrome/test/functional/chromeos_device_policy.py (right):

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
chrome/test/functional/chromeos_device_policy.py:238: def
SetTimezoneInCrosSettings(self, timezone):
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Private methods' names start with _.

Done.

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
File chrome/test/functional/chromeos_time.py (right):

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
chrome/test/functional/chromeos_time.py:77: #self.Login(user_index=1,
expect_success=True)
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Remove?

Done.

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
chrome/test/functional/chromeos_time.py:81: self.WaitUntil(lambda: timezone ==
self.GetTimeInfo()['timezone']),
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Nit: indent

ah. My emacs mode is not configured correctly :-)

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
chrome/test/functional/chromeos_time.py:81: self.WaitUntil(lambda: timezone ==
self.GetTimeInfo()['timezone']),
On 2012/07/27 13:40:57, Joao da Silva wrote:
> Nit: indent

Done.

[Joao da Silva, 2012-08-03 14:02:38]

Did you forget to upload the new patch set?

[pneubeck (no reviews), 2012-08-06 14:50:48]

Ben, could you please also review this CL.
Thanks.

http://codereview.chromium.org/10827050/diff/1/chrome/browser/automation/test...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/10827050/diff/1/chrome/browser/automation/test...
chrome/browser/automation/testing_automation_provider.h:1610: void
SetCrosSetting(base::DictionaryValue* args, IPC::Message* reply_message);
On 2012/07/31 22:21:05, pneubeck1 wrote:
> On 2012/07/27 13:40:57, Joao da Silva wrote:
> > This is a big hammer. We previously had a similar call to SetPolicies() and
> > ultimately decided to remove it.
> > 
> > Notice that there's already a SetTimezone() automation call, isn't it enough
> to
> > cover your test?
> The idea of this definition was to change chromeos settings (controlled by
> CrosSettings) in the same fashion as normal settings are set using the
function
> SetPrefs. I see, that this means that policies can also be changed via the
owner
> of the device because these are also controlled by CrosSettings.
> I don't see in which way this is problematic. Surely it is not really
necessary
> as long as the timezone test is the only use case. But the difference of the
> generic function i provided and the a special function for timezone isn't that
> huge... so why should I provide the special one?
> 
> The major difference between setting the timezone with this function and the
> existing SetTimezone is, that this one talks to CrosSettings and the other to
> the underlying chromeos::system::TimezoneSettings.
> 
> Depending on what we like to have, more a functional, integration or unit
test,
> both may be more appropriate. For functional tests, which I wanted to
implement
> and many pyauto test seem to be, the new function should be proper.
> 
> Do you have any other suggestions or which of the variants do you think fits
> better?

Done.

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
File chrome/test/functional/chromeos_device_policy.py (right):

http://codereview.chromium.org/10827050/diff/1/chrome/test/functional/chromeo...
chrome/test/functional/chromeos_device_policy.py:267:
self.SetTimezoneInCrosSettings(self._timezones[1])
On 2012/07/27 13:40:57, Joao da Silva wrote:
> It seems to me that both uses of SetTimezoneInCrosSettings here can be
replaced
> with self.SetTimezone (from chrome/test/pyautolib/pyauto.py)

Done.

[Joao da Silva, 2012-08-06 15:33:49]

lgtm. Please fix the remaining nits :-)

http://codereview.chromium.org/10827050/diff/8001/chrome/browser/chromeos/set...
File chrome/browser/chromeos/settings/cros_settings_names.cc (right):

http://codereview.chromium.org/10827050/diff/8001/chrome/browser/chromeos/set...
chrome/browser/chromeos/settings/cros_settings_names.cc:29: // the description
in policy_template.json).
Nit: policy_templates.json

http://codereview.chromium.org/10827050/diff/8001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/10827050/diff/8001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:164: // Set the timezone as
soon as the policies are avaiblable.
Nit: *available

http://codereview.chromium.org/10827050/diff/8001/chrome/test/functional/chro...
File chrome/test/functional/chromeos_time.py (right):

http://codereview.chromium.org/10827050/diff/8001/chrome/test/functional/chro...
chrome/test/functional/chromeos_time.py:83: def _SetTimezoneInUI(self,
timezone):
Nit: put the auxiliary function before the test that uses it

[Ben Goodger (Google), 2012-08-06 19:39:41]

+darin,jam

[darin (slow to review), 2012-08-06 20:53:08]

LGTM for automation changes.

[pneubeck (no reviews), 2012-08-07 06:56:11]

Addressed all comments.

http://codereview.chromium.org/10827050/diff/8001/chrome/browser/chromeos/set...
File chrome/browser/chromeos/settings/cros_settings_names.cc (right):

http://codereview.chromium.org/10827050/diff/8001/chrome/browser/chromeos/set...
chrome/browser/chromeos/settings/cros_settings_names.cc:29: // the description
in policy_template.json).
On 2012/08/06 15:33:49, Joao da Silva wrote:
> Nit: policy_templates.json

Done.

http://codereview.chromium.org/10827050/diff/8001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/10827050/diff/8001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:164: // Set the timezone as
soon as the policies are avaiblable.
On 2012/08/06 15:33:49, Joao da Silva wrote:
> Nit: *available

Done.

http://codereview.chromium.org/10827050/diff/8001/chrome/test/functional/chro...
File chrome/test/functional/chromeos_time.py (right):

http://codereview.chromium.org/10827050/diff/8001/chrome/test/functional/chro...
chrome/test/functional/chromeos_time.py:83: def _SetTimezoneInUI(self,
timezone):
On 2012/08/06 15:33:49, Joao da Silva wrote:
> Nit: put the auxiliary function before the test that uses it

Done.

[commit-bot: I haz the power, 2012-08-08 08:48:18]

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

[commit-bot: I haz the power, 2012-08-08 08:48:25]

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 3150 (offset 50 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej

[pneubeck (no reviews), 2012-08-08 09:24:09]

Joao, please look at the resolve of the conflict.

[Joao da Silva, 2012-08-08 09:35:45]

Please address the issues in the comments before landing.

http://codereview.chromium.org/10827050/diff/1022/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/10827050/diff/1022/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:156: if
(!MessageLoop::current()) {
This should be if (MessageLoop::current()) { ... }

http://codereview.chromium.org/10827050/diff/1022/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:557: if
(!MessageLoop::current()) {
This test is not necessary, since it was already checked when the task was
posted.

[pneubeck (no reviews), 2012-08-08 09:47:29]

Addressed comments.

http://codereview.chromium.org/10827050/diff/1022/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/10827050/diff/1022/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:156: if
(!MessageLoop::current()) {
On 2012/08/08 09:35:46, Joao da Silva wrote:
> This should be if (MessageLoop::current()) { ... }

Done.

http://codereview.chromium.org/10827050/diff/1022/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:557: if
(!MessageLoop::current()) {
On 2012/08/08 09:35:46, Joao da Silva wrote:
> This test is not necessary, since it was already checked when the task was
> posted.

Done.

[Joao da Silva, 2012-08-08 09:48:59]

lgtm

[commit-bot: I haz the power, 2012-08-08 09:49:27]

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

[commit-bot: I haz the power, 2012-08-08 11:13:04]

Change committed as 150536