Please, lgtm again. The last commit broke some tests because of some other commit which ...
8 years, 4 months ago
(2012-08-09 13:32:40 UTC)
#1
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
lgtm after fixing the BrowserPolicyConnector. https://chromiumcodereview.appspot.com/10832222/diff/1/chrome/browser/policy/browser_policy_connector.cc File chrome/browser/policy/browser_policy_connector.cc (right): https://chromiumcodereview.appspot.com/10832222/diff/1/chrome/browser/policy/browser_policy_connector.cc#newcode153 chrome/browser/policy/browser_policy_connector.cc:153: if (MessageLoop::current()) { Don't ...
8 years, 4 months ago
(2012-08-09 13:39:47 UTC)
#2
jcivelli, please take a look at the tests, that I want to add, and the ...
8 years, 4 months ago
(2012-08-10 07:03:39 UTC)
#4
jcivelli, please take a look at the tests, that I want to add, and the small
change to pyauto.py.
Thanks.
pneubeck (no reviews)
mnissler, please take a look at the changes to settings/*. Thanks.
8 years, 4 months ago
(2012-08-10 07:13:12 UTC)
#5
mnissler, please take a look at the changes to settings/*.
Thanks.
Mattias Nissler (ping if slow)
couple nits https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/app/policy/policy_templates.json File chrome/app/policy/policy_templates.json (right): https://chromiumcodereview.appspot.com/10832222/diff/6002/chrome/app/policy/policy_templates.json#newcode3167 chrome/app/policy/policy_templates.json:3167: If additionally no user ever set a ...
8 years, 4 months ago
(2012-08-10 07:45:16 UTC)
#6
chrome/{app,browser}/policy LGTM https://chromiumcodereview.appspot.com/10832222/diff/11003/chrome/app/policy/policy_templates.json File chrome/app/policy/policy_templates.json (right): https://chromiumcodereview.appspot.com/10832222/diff/11003/chrome/app/policy/policy_templates.json#newcode3169 chrome/app/policy/policy_templates.json:3169: The specified value is valid if it ...
8 years, 4 months ago
(2012-08-13 07:49:46 UTC)
#8
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)
Fixed the policy description. https://chromiumcodereview.appspot.com/10832222/diff/11003/chrome/app/policy/policy_templates.json File chrome/app/policy/policy_templates.json (right): https://chromiumcodereview.appspot.com/10832222/diff/11003/chrome/app/policy/policy_templates.json#newcode3169 chrome/app/policy/policy_templates.json:3169: The specified value is valid ...
8 years, 4 months ago
(2012-08-13 12:26:45 UTC)
#9
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)
Nirnimesh, could you please teak a look at the changes affecting chrome/test/**. Thanks. darin already ...
8 years, 4 months ago
(2012-08-14 08:23:57 UTC)
#10
Nirnimesh, could you please teak a look at the changes affecting chrome/test/**.
Thanks.
darin already approved the changes to automation.
Nirnimesh
chrome/test/ LGTM http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chromeos_device_policy.py File chrome/test/functional/chromeos_device_policy.py (right): http://codereview.chromium.org/10832222/diff/10003/chrome/test/functional/chromeos_device_policy.py#newcode206 chrome/test/functional/chromeos_device_policy.py:206: self.SetDevicePolicy({'timezone':self._timezones[0]}, refresh=True) nit: need space after : ...
8 years, 4 months ago
(2012-08-14 18:09:37 UTC)
#11
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 ...
8 years, 4 months ago
(2012-08-16 09:40:41 UTC)
#14
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
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/10832222/6021
8 years, 4 months ago
(2012-08-16 09:48:45 UTC)
#15
Presubmit check for 10832222-6021 failed and returned exit status 1. Running presubmit commit checks ...
8 years, 4 months ago
(2012-08-16 09:48:50 UTC)
#16
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
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/10832222/6021
8 years, 4 months ago
(2012-08-16 09:52:25 UTC)
#17
Issue 10832222: Added a timezone policy and pyauto tests for it.
(Closed)
Created 8 years, 4 months ago by pneubeck (no reviews)
Modified 8 years, 4 months ago
Reviewers: Joao da Silva, darin (slow to review), Mattias Nissler (ping if slow), Nirnimesh
Base URL: http://git.chromium.org/chromium/src.git@master
Comments: 16