Add support for public account policy to CloudPolicyClient.

Add support for public account policy to CloudPolicyClient.

BUG=chromium:152937
TEST=Manual, user policy still works.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170991

[Mattias Nissler (ping if slow), 2012-11-30 13:12:31]

Hey folks, please review. This is a part of an earlier CL that you might have
looked at. I've changed plans a bit and reshuffled stuff though, sorry about the
overhead.

[Mattias Nissler (ping if slow), 2012-12-03 11:23:18]

+Joao

[pastarmovj, 2012-12-03 11:47:40]

This was a Deja Vu for me and is still lgtm with one nit.

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_client.cc (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/cloud_policy_client.cc:174: LOG(FATAL) << "Cannot register
for public account policy.";
Can this be NOTREACHED() too? Or simply remove the return and it will reach the
one below.

[bartfab (slow), 2012-12-03 12:28:40]

lgtm

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_client.cc (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/cloud_policy_client.cc:177: NOTREACHED() << "Invalid
policy scope " << type_;
Nit: You renamed scope to type but the debug output still says "scope".

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/cloud_policy_client.cc:192: NOTREACHED() << "Invalid
policy scope " << type_;
Nit: as above, scope <-> type mismatch in debug output.

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_client.h (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/cloud_policy_client.h:15: #include
"chrome/browser/policy/policy_types.h"
Nit: No longer needed.

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
File chrome/browser/policy/proto/device_management_backend.proto (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/proto/device_management_backend.proto:264: // The unit is
milli-second.
This comment seems unrelated to the CL. But since you are adding it:
milliseconds should be in plural and without a dash.

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
File chrome/browser/policy/user_cloud_policy_manager_chromeos.cc (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/user_cloud_policy_manager_chromeos.cc:10: #include
"chrome/browser/policy/policy_types.h"
Nit: No longer needed.

https://chromiumcodereview.appspot.com/11434053/diff/3001/net/tools/testserve...
File net/tools/testserver/device_management.py (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/net/tools/testserve...
net/tools/testserver/device_management.py:439: policy_type += '/' +
msg.settings_entity_id
Should we document that in order to set mock public account policy, you need to
specify it under google/chromeos/publicaccount/${user} ?

Also, at that point, |policy_type| does not really fit as a variable name
anymore. Something like |policy_key| or |policy_path| would seem more
appropriate.

[Mattias Nissler (ping if slow), 2012-12-03 13:50:39]

Thanks for the review. I've made some changes to device_management.py to play
nicer with public accounts, can you please take another look?

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_client.cc (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/cloud_policy_client.cc:174: LOG(FATAL) << "Cannot register
for public account policy.";
On 2012/12/03 11:47:40, pastarmovj wrote:
> Can this be NOTREACHED() too? Or simply remove the return and it will reach
the
> one below.

LOG(FATAL) is abort(), i.e. NOTREACHED() in release configuration. I think
that's what you were asking for?

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/cloud_policy_client.cc:177: NOTREACHED() << "Invalid
policy scope " << type_;
On 2012/12/03 12:28:40, bartfab wrote:
> Nit: You renamed scope to type but the debug output still says "scope".

Done.

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/cloud_policy_client.cc:192: NOTREACHED() << "Invalid
policy scope " << type_;
On 2012/12/03 12:28:40, bartfab wrote:
> Nit: as above, scope <-> type mismatch in debug output.

Done.

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
File chrome/browser/policy/proto/device_management_backend.proto (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/proto/device_management_backend.proto:264: // The unit is
milli-second.
On 2012/12/03 12:28:40, bartfab wrote:
> This comment seems unrelated to the CL. But since you are adding it:
> milliseconds should be in plural and without a dash.

True. This is the text they have on the google3 side though, and I was just
syncing with that. Note that my original reason for the syncing (adding
settings_entity_id to PolicyData) Joao has meanwhile committed in a CL of his
own, but I was thorough and re-synced our version of this file with google3 and
he was not :)

If you feel strong, I can just drop this file.

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
File chrome/browser/policy/user_cloud_policy_manager_chromeos.cc (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/user_cloud_policy_manager_chromeos.cc:10: #include
"chrome/browser/policy/policy_types.h"
On 2012/12/03 12:28:40, bartfab wrote:
> Nit: No longer needed.

Done.

https://chromiumcodereview.appspot.com/11434053/diff/3001/net/tools/testserve...
File net/tools/testserver/device_management.py (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/net/tools/testserve...
net/tools/testserver/device_management.py:439: policy_type += '/' +
msg.settings_entity_id
On 2012/12/03 12:28:40, bartfab wrote:
> Should we document that in order to set mock public account policy, you need
to
> specify it under google/chromeos/publicaccount/${user} ?

Added an example at the beginning of the file.

> 
> Also, at that point, |policy_type| does not really fit as a variable name
> anymore. Something like |policy_key| or |policy_path| would seem more
> appropriate.

Done.

[bartfab (slow), 2012-12-03 14:01:05]

still lgtm

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
File chrome/browser/policy/proto/device_management_backend.proto (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/chrome/browser/poli...
chrome/browser/policy/proto/device_management_backend.proto:264: // The unit is
milli-second.
I think being in sync with the server trumps any spelling nits. Thanks for
clarifying.

On 2012/12/03 13:50:39, Mattias Nissler wrote:
> On 2012/12/03 12:28:40, bartfab wrote:
> > This comment seems unrelated to the CL. But since you are adding it:
> > milliseconds should be in plural and without a dash.
> 
> True. This is the text they have on the google3 side though, and I was just
> syncing with that. Note that my original reason for the syncing (adding
> settings_entity_id to PolicyData) Joao has meanwhile committed in a CL of his
> own, but I was thorough and re-synced our version of this file with google3
and
> he was not :)
> 
> If you feel strong, I can just drop this file.

https://chromiumcodereview.appspot.com/11434053/diff/3001/net/tools/testserve...
File net/tools/testserver/device_management.py (right):

https://chromiumcodereview.appspot.com/11434053/diff/3001/net/tools/testserve...
net/tools/testserver/device_management.py:439: policy_type += '/' +
msg.settings_entity_id
Thanks, that's excellent. I had completely missed it that we have an example
section at the top.

On 2012/12/03 13:50:39, Mattias Nissler wrote:
> On 2012/12/03 12:28:40, bartfab wrote:
> > Should we document that in order to set mock public account policy, you need
> to
> > specify it under google/chromeos/publicaccount/${user} ?
> 
> Added an example at the beginning of the file.
> 
> > 
> > Also, at that point, |policy_type| does not really fit as a variable name
> > anymore. Something like |policy_key| or |policy_path| would seem more
> > appropriate.
> 
> Done.

https://chromiumcodereview.appspot.com/11434053/diff/1015/net/tools/testserve...
File net/tools/testserver/device_management.py (right):

https://chromiumcodereview.appspot.com/11434053/diff/1015/net/tools/testserve...
net/tools/testserver/device_management.py:483: # There is no way for the
testserver to know the user name belonging to
Nit: Update this comment to say that it applies to non-public-accounts only?

[Mattias Nissler (ping if slow), 2012-12-03 16:49:31]

Thanks for the reviews, I'll submit this as time permits if there are no further
comments.

https://codereview.chromium.org/11434053/diff/1015/net/tools/testserver/devic...
File net/tools/testserver/device_management.py (right):

https://codereview.chromium.org/11434053/diff/1015/net/tools/testserver/devic...
net/tools/testserver/device_management.py:483: # There is no way for the
testserver to know the user name belonging to
On 2012/12/03 14:01:05, bartfab wrote:
> Nit: Update this comment to say that it applies to non-public-accounts only?

Technically, the comment is still accurate ;)

Updated the code to only use settings_entity_id in the public accounts case.

[commit-bot: I haz the power, 2012-12-04 10:23:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/11434053/14019

[commit-bot: I haz the power, 2012-12-04 12:52:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/11434053/6007

[commit-bot: I haz the power, 2012-12-04 15:37:21]

Retried try job too often on win_rel for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-12-04 16:02:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/11434053/6007

[Joao da Silva, 2012-12-04 16:33:30]

lgtm

(I hope this doesn't break the CQ...)

https://codereview.chromium.org/11434053/diff/6007/net/tools/testserver/devic...
File net/tools/testserver/device_management.py (right):

https://codereview.chromium.org/11434053/diff/6007/net/tools/testserver/devic...
net/tools/testserver/device_management.py:45: }
nitty nit: comma

[Mattias Nissler (ping if slow), 2012-12-04 16:58:36]

The CQ is a joke btw, I can't believe anybody doing serious work is still using
it.

https://codereview.chromium.org/11434053/diff/6007/net/tools/testserver/devic...
File net/tools/testserver/device_management.py (right):

https://codereview.chromium.org/11434053/diff/6007/net/tools/testserver/devic...
net/tools/testserver/device_management.py:45: }
On 2012/12/04 16:33:31, Joao da Silva wrote:
> nitty nit: comma

Done.