New policy protobuf protocol.

New policy protobuf protocol.

- cloud_policy.proto autogenerated from policy_templates.json
- C++ method decoding the protobuf autogenerated from policy_templates.json
- changed policy fetching mechanism to fetch new-style policy protobufs

BUG=68309, chromium-os:11253, chromium-os:11255
TEST=CloudPolicyCacheTest.*; also manual test against python testserver

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

[Jakob Kummerow, 2011-02-01 09:41:56]

FYI -- preview release. Not quite finished yet.

[Jakob Kummerow, 2011-02-01 16:15:51]

It compiles, and CloudPolicyCacheTests are green :-)
Please review.

[gfeher, 2011-02-02 08:42:45]

Here is my first round, its mostly nits.

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:104: 'protobuf_id': 1,
For better readability, I suggest moving 'protobuf_id' next to 'name', or at
least over the caption and description texts.

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/syntax_che...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:91: if protobuf_id in
self.group_protobuf_ids:
How about making |self.group_protobuf_ids| / |is_in_group| a function parameter?
(Both here and in _CheckPolicy())

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:106: of entries (i.e. no
holes).
Isn't it possible that we will have holes later as deprecated policies are
removed?

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.cc:159: it != mandatory_policy_->end();
++it) {
Nit: indent.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.cc:208: if (*timestamp >
base::Time::NowFromSystemTime()) {
Allow a small difference for safety?

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.h:36: PolicyMapType* mandatory,
PolicyMapType* recommended);
Please hide this in an anonymous namespace.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.h:57: // to the caller.
If I understand correctly, there is no way to get recommended policies out from
this cache, right? If that's intentional, please add a (TODO) comment.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.h:95: // Protects |policy_|.
Please update comment.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_unittest.cc:46: const base::Time&
timestamp) {
Nit: indent.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.h:156:
scoped_ptr<CloudPolicyCache> cache2_;
I suggest calling this cache_ and naming the previous as deprecated_cache_,
dm_cache_ or etc.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/cloud_policy.proto (left):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/cloud_policy.proto:194: optional HomepageProto
Homepage = 1;
Bad news: we will have to start numbering at 3 here so that we don't clash with
the current ChromeSettingsProto on the server side.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_backend.proto:122: // username?
(currently this is also sent as an HTTP POST parameter)
You'll probably need to sync this file to the latest protocol version before
committing.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:157: // Only included in serialized
form in the PBs below.
Please update comment.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:291: code += ('      PolicyMapType*
result = mandatory;\n'
Nit: rename |result| to |destination|?

[Mattias Nissler (ping if slow), 2011-02-02 12:27:55]

feedback! sorry for the delay.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.h:17: // policy definitions.
I'm no fan of repeating that comment over and over again. Let's just drop it,
people who are curious will learn by looking at the header of that file.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.h:22: typedef
std::map<ConfigurationPolicyType, Value*> PolicyMapType;
That should really be a typedef inside ConfigurationPolicyProvider, no?

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.h:73: void
ApplyPolicyMap(const PolicyMapType* policies,
Can we name that similar to DecodePolicyValueTree? It does the same thing, just
on a different data structure.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_store_interface.h:11: // policy
definitions.
same here...

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend.h:79: const
em::DevicePolicyResponse& response) = 0;  // deprecated.
Please explain why it's deprecated, what is used instead and what our plan is to
clean it up.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend_impl.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend_impl.h:39: static const char
kValueRequestPolicy[];  // deprecated.
same here, maybe put a reference to the other explanation.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend_impl.h:71:
DevicePolicyResponseDelegate* response_delegate);  // deprecated
dito

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.cc:45:
FILE_PATH_LITERAL("CloudPolicy");
See my other comment about having only one cache.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.h:39: bool
cloud_policy_enabled);
Is this the way to go? I think we should switch our client to first ask for
policy using the new request and if the server complains, fall back to the old
one.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.h:156:
scoped_ptr<CloudPolicyCache> cache2_;
I'm also not too sure about this. Thing is that the unmanaged bit we should be
able to migrate from the old cache file. So can't we just have a single cache
object here, add a new field in the cache protobuf, support caching both types
of policy for a while and eventually remove support for the old thing?

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/device_management_local.proto (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_local.proto:30: optional uint64
timestamp = 3;
See other comment about single cache.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:36: help="generate CPP code
decoding the policy protobuf",
In my experience CPP most of the time refers to the C PreProcessor and most
people use CXX to refer to C++. But why not just say C++? ;)

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:58: #------------------ shared
helpers ---------------------------------#
Is that commonly used comment style in chromium python code?

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:157: // Only included in serialized
form in the PBs below.
and can't we just switch to repeated string instead of wrapping this in
StringList?

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:171: 
Can we have this in a separate .proto that includes the generated stuff?

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:188: def _WriteProtobuf(data, args,
outfilepath):
What's data? Can we have a better name?

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:202: sub_policy['name'],
(sub_policy['protobuf_id'] + 1)))
hm, we might want to name the id just ID and not protobuf_id, because of the +1
(and the json file shouldn't know it's being used for proto generation).

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:205:
(_PolicyTypeToProtobufType(policy['type']), policy['name']))
Seems like you could use a _WritePolicy function and call it from the two
branches.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:262: '''
I wouldn't worry to much about this boilerplate header because you need to
generate the proper #includes anyway. Another approach I can see would be to
generate only the DecodePolicy function definition and #include that in some .cc
file that contains the boilerplate.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:287: protoname = "%s_proto" %
policy['name'].lower()
can use membername instead of the repeated policy['name'].lower()

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:294:
'em::PolicyOptions::RECOMMENDED)) {\n' %
I think I would rather have used a switch(%s.policy_options().mode()) here,
which is future-proof in a way that the compiler will notice if we ever add a
third enum value. WDYT?

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:312: protoname = "%s_proto" %
policyname.lower()
why not use membername?

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:318:
f.write(_GeneratePolicyCode(sub_policy, protoname))
Why not just pass f to the function instead of building a string and returning
it?

[Jakob Kummerow, 2011-02-03 14:36:52]

Addressed your feedback. Please have another look.

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:104: 'protobuf_id': 1,
On 2011/02/02 08:42:45, gfeher wrote:
> For better readability, I suggest moving 'protobuf_id' next to 'name', or at
> least over the caption and description texts.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/syntax_che...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:91: if protobuf_id in
self.group_protobuf_ids:
On 2011/02/02 08:42:45, gfeher wrote:
> How about making |self.group_protobuf_ids| / |is_in_group| a function
parameter?
> (Both here and in _CheckPolicy())

Done. Good idea.

http://codereview.chromium.org/6409040/diff/2001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:106: of entries (i.e. no
holes).
On 2011/02/02 08:42:45, gfeher wrote:
> Isn't it possible that we will have holes later as deprecated policies are
> removed?

As discussed offline, we don't want to allow holes. Ever. (Or if we do, we'll
change the checker.)

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.cc:159: it != mandatory_policy_->end();
++it) {
On 2011/02/02 08:42:45, gfeher wrote:
> Nit: indent.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.cc:208: if (*timestamp >
base::Time::NowFromSystemTime()) {
On 2011/02/02 08:42:45, gfeher wrote:
> Allow a small difference for safety?

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.h:36: PolicyMapType* mandatory,
PolicyMapType* recommended);
On 2011/02/02 08:42:45, gfeher wrote:
> Please hide this in an anonymous namespace.

Can't. See the comment: the implementation is generated code, i.e. lives in
another file. The method *has* to be in the policy namespace or it wouldn't be
accessible.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.h:57: // to the caller.
On 2011/02/02 08:42:45, gfeher wrote:
> If I understand correctly, there is no way to get recommended policies out
from
> this cache, right? If that's intentional, please add a (TODO) comment.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.h:95: // Protects |policy_|.
On 2011/02/02 08:42:45, gfeher wrote:
> Please update comment.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_unittest.cc:46: const base::Time&
timestamp) {
On 2011/02/02 08:42:45, gfeher wrote:
> Nit: indent.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.h:17: // policy definitions.
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> I'm no fan of repeating that comment over and over again. Let's just drop it,
> people who are curious will learn by looking at the header of that file.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.h:22: typedef
std::map<ConfigurationPolicyType, Value*> PolicyMapType;
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> That should really be a typedef inside ConfigurationPolicyProvider, no?

Done. As you wish.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.h:73: void
ApplyPolicyMap(const PolicyMapType* policies,
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> Can we name that similar to DecodePolicyValueTree? It does the same thing,
just
> on a different data structure.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_store_interface.h:11: // policy
definitions.
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> same here...

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend.h:79: const
em::DevicePolicyResponse& response) = 0;  // deprecated.
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> Please explain why it's deprecated, what is used instead and what our plan is
to
> clean it up.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_backend_impl.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend_impl.h:39: static const char
kValueRequestPolicy[];  // deprecated.
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> same here, maybe put a reference to the other explanation.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_backend_impl.h:71:
DevicePolicyResponseDelegate* response_delegate);  // deprecated
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> dito

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.cc:45:
FILE_PATH_LITERAL("CloudPolicy");
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> See my other comment about having only one cache.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.h:39: bool
cloud_policy_enabled);
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> Is this the way to go? I think we should switch our client to first ask for
> policy using the new request and if the server complains, fall back to the old
> one.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.h:156:
scoped_ptr<CloudPolicyCache> cache2_;
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> I'm also not too sure about this. Thing is that the unmanaged bit we should be
> able to migrate from the old cache file. So can't we just have a single cache
> object here, add a new field in the cache protobuf, support caching both types
> of policy for a while and eventually remove support for the old thing?

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/cloud_policy.proto (left):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/cloud_policy.proto:194: optional HomepageProto
Homepage = 1;
On 2011/02/02 08:42:45, gfeher wrote:
> Bad news: we will have to start numbering at 3 here so that we don't clash
with
> the current ChromeSettingsProto on the server side.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_backend.proto:122: // username?
(currently this is also sent as an HTTP POST parameter)
On 2011/02/02 08:42:45, gfeher wrote:
> You'll probably need to sync this file to the latest protocol version before
> committing.

It's on my TODO list :-)

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/device_management_local.proto (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_local.proto:30: optional uint64
timestamp = 3;
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> See other comment about single cache.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:36: help="generate CPP code
decoding the policy protobuf",
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> In my experience CPP most of the time refers to the C PreProcessor and most
> people use CXX to refer to C++. But why not just say C++? ;)

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:58: #------------------ shared
helpers ---------------------------------#
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> Is that commonly used comment style in chromium python code?

No, I guess it's not. I just felt that this file would benefit from having clear
visual separations of its sections.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:157: // Only included in serialized
form in the PBs below.
On 2011/02/02 08:42:45, gfeher wrote:
> Please update comment.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:157: // Only included in serialized
form in the PBs below.
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> and can't we just switch to repeated string instead of wrapping this in
> StringList?

As discussed offline: Yes, we could, but we would lose the ability to
differentiate between empty lists and non-existing lists, so we don't.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:171: 
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> Can we have this in a separate .proto that includes the generated stuff?

As discussed offline: Yes, we could, but since it's not much code, it's unclear
whether that would be better, so we're leaving it in here for now. If we ever
need more static code here, we can always factor it out into its own file.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:188: def _WriteProtobuf(data, args,
outfilepath):
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> What's data? Can we have a better name?

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:202: sub_policy['name'],
(sub_policy['protobuf_id'] + 1)))
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> hm, we might want to name the id just ID and not protobuf_id, because of the
+1
> (and the json file shouldn't know it's being used for proto generation).

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:205:
(_PolicyTypeToProtobufType(policy['type']), policy['name']))
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> Seems like you could use a _WritePolicy function and call it from the two
> branches.

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:262: '''
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> I wouldn't worry to much about this boilerplate header because you need to
> generate the proper #includes anyway. Another approach I can see would be to
> generate only the DecodePolicy function definition and #include that in some
.cc
> file that contains the boilerplate.

Same as above: as discussed offline, we'll leave this as is for now.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:287: protoname = "%s_proto" %
policy['name'].lower()
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> can use membername instead of the repeated policy['name'].lower()

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:291: code += ('      PolicyMapType*
result = mandatory;\n'
On 2011/02/02 08:42:45, gfeher wrote:
> Nit: rename |result| to |destination|?

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:294:
'em::PolicyOptions::RECOMMENDED)) {\n' %
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> I think I would rather have used a switch(%s.policy_options().mode()) here,
> which is future-proof in a way that the compiler will notice if we ever add a
> third enum value. WDYT?

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:312: protoname = "%s_proto" %
policyname.lower()
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> why not use membername?

Done.

http://codereview.chromium.org/6409040/diff/2001/chrome/tools/build/generate_...
chrome/tools/build/generate_policy_source.py:318:
f.write(_GeneratePolicyCode(sub_policy, protoname))
On 2011/02/02 12:27:55, Mattias Nissler wrote:
> Why not just pass f to the function instead of building a string and returning
> it?

Done.

[danno, 2011-02-03 15:31:37]

LGTM with a few comments

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.gypi (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.gypi:68: 'hard_dependency': 1,
you don't need this, because it is not a shared library.

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:83: #
Add doc for id?

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:93: 'id': 1,
maybe use a different id label, like group_id? or the other id should be
"policy_id".

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:304: for
(ConfigurationPolicyProvider::PolicyMapType::const_iterator it =
nit: move the declaration to another line. easier to read.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:313: for
(ConfigurationPolicyProvider::PolicyMapType::const_iterator it =
nit: move the declaration to another line. easier to read.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:327: for
(ConfigurationPolicyProvider::PolicyMapType::const_iterator it =
nit: move the declaration to another line. easier to read. elsewhere in this
file probably more.

[Mattias Nissler (ping if slow), 2011-02-03 16:23:41]

a couple of comments

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/syntax_ch...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:98: of entries (i.e. no
holes).
can you elaborate here please on why we don't want holes? Maybe also in the
error message.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:166:
scoped_ptr<ConfigurationPolicyProvider::PolicyMapType> mandatory_policy_map(
why are all maps wrapped in a scoped_ptr? I think just declaring regular members
and relying on swap() would simplify all this quite a bit.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:227: return
CopyPolicyMap(mandatory_policy_.get());
Can't we just return a const pointer instead of copying? Also, this method
should probably be const.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:232: return
CopyPolicyMap(recommended_policy_.get());
same here.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:280:
base::TimeDelta::FromMinutes(1)) {
I would do this only for data that comes from the cache file. That way, if the
server's clock is really off, we just refetch early on startup and don't ignore
policy at all.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:8: #include <map>
do you need this?

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:32: // recommended settings,
respectively. The implementation is generated code.
maybe also add the file name to look at?

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:54: bool SetPolicy(const
em::DevicePolicyResponse& policy);
we generally discourage overloads (see the style guide). It might even sense to
rename the DevicePolicyResponse variant to SetLegacyPolicy() or something in
order to indicate this is not the code path you should add new features to.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:58: DictionaryValue*
GetDevicePolicy();
Why do we still need this?

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:74: bool has_device_policy() const {
Why would we need this? Can't the provider just ask for both in sequence and
take the first one that is non-null?

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:100: const
ConfigurationPolicyProvider::PolicyMapType* b);
can you use std::equal from <algorithm> instead?

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:104: const
ConfigurationPolicyProvider::PolicyMapType* original);
also looks like std::copy could help here.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:125: base::Lock lock_;
do we still need locking here?

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:130: scoped_ptr<DictionaryValue>
device_policy_;
why is this still needed?

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:143:
SendPolicyRequest();
You can do SetState(STATE_TOKEN_VALID) instead for consistency with the rest of
the code (i.e. after each transition we go through the state machine code).

http://codereview.chromium.org/6409040/diff/10003/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:186: (PROTOBUF_TYPE[policy_type],
policy_name, field_id))
We crash if policy_type is not in PROTOBUF_TYPE. Is that OK?

[Jakob Kummerow, 2011-02-08 16:15:43]

Finally, another iteration :-)
Please review. 

Note: I tested the protocol fallback mechanism manually against the production
DMServer. It needed a small change, but now it works.

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.gypi (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.gypi:68: 'hard_dependency': 1,
On 2011/02/03 15:31:38, danno wrote:
> you don't need this, because it is not a shared library.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:83: #
On 2011/02/03 15:31:38, danno wrote:
> Add doc for id?

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:93: 'id': 1,
On 2011/02/03 15:31:38, danno wrote:
> maybe use a different id label, like group_id? or the other id should be
> "policy_id". 

According to our latest discussions, the ID ranges have been merged (i.e. it's
only one range now).

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/syntax_ch...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:98: of entries (i.e. no
holes).
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> can you elaborate here please on why we don't want holes? Maybe also in the
> error message.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:166:
scoped_ptr<ConfigurationPolicyProvider::PolicyMapType> mandatory_policy_map(
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> why are all maps wrapped in a scoped_ptr? I think just declaring regular
members
> and relying on swap() would simplify all this quite a bit.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:227: return
CopyPolicyMap(mandatory_policy_.get());
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> Can't we just return a const pointer instead of copying? Also, this method
> should probably be const.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:232: return
CopyPolicyMap(recommended_policy_.get());
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> same here.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:280:
base::TimeDelta::FromMinutes(1)) {
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> I would do this only for data that comes from the cache file. That way, if the
> server's clock is really off, we just refetch early on startup and don't
ignore
> policy at all.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:304: for
(ConfigurationPolicyProvider::PolicyMapType::const_iterator it =
On 2011/02/03 15:31:38, danno wrote:
> nit: move the declaration to another line. easier to read.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:313: for
(ConfigurationPolicyProvider::PolicyMapType::const_iterator it =
On 2011/02/03 15:31:38, danno wrote:
> nit: move the declaration to another line. easier to read.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:327: for
(ConfigurationPolicyProvider::PolicyMapType::const_iterator it =
On 2011/02/03 15:31:38, danno wrote:
> nit: move the declaration to another line. easier to read. elsewhere in this
> file probably more.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:8: #include <map>
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> do you need this?

No, I don't. Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:32: // recommended settings,
respectively. The implementation is generated code.
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> maybe also add the file name to look at?

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:54: bool SetPolicy(const
em::DevicePolicyResponse& policy);
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> we generally discourage overloads (see the style guide). It might even sense
to
> rename the DevicePolicyResponse variant to SetLegacyPolicy() or something in
> order to indicate this is not the code path you should add new features to.

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:58: DictionaryValue*
GetDevicePolicy();
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> Why do we still need this?

Because for the time being, DMServer only gives us old-style device policy, and
we need to access that somehow after having cached it :-)

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:74: bool has_device_policy() const {
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> Why would we need this? Can't the provider just ask for both in sequence and
> take the first one that is non-null?

Currently no policy ever returns null; it'll just be an empty list if no policy
is set. I guess we could change it, but it wouldn't be easier than maintaining
this flag.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:100: const
ConfigurationPolicyProvider::PolicyMapType* b);
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> can you use std::equal from <algorithm> instead?

No, because I need Value::Equals(). |operator=| isn't defined for Values.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:104: const
ConfigurationPolicyProvider::PolicyMapType* original);
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> also looks like std::copy could help here.

No, because Value::DeepCopy() was needed. However, I was able to avoid copying
policy maps altogether.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:125: base::Lock lock_;
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> do we still need locking here?

I guess we don't (would need to take a closer look to give a definitive answer),
but it's a moot point since your own CL restructures all of this and removes the
lock anyway.

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:130: scoped_ptr<DictionaryValue>
device_policy_;
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> why is this still needed?

To store old-style policy until support for that is removed.
(In theory, it would be possible to put that into mandatory_policy_, but due to
the different mapping type that approach would require a throw-away
implementation involving more generated code, which I don't think would be an
efficient thing to do in terms of engineering effort.)

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:143:
SendPolicyRequest();
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> You can do SetState(STATE_TOKEN_VALID) instead for consistency with the rest
of
> the code (i.e. after each transition we go through the state machine code).

Done.

http://codereview.chromium.org/6409040/diff/10003/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6409040/diff/10003/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:186: (PROTOBUF_TYPE[policy_type],
policy_name, field_id))
On 2011/02/03 16:23:41, Mattias Nissler wrote:
> We crash if policy_type is not in PROTOBUF_TYPE. Is that OK?

There's not really a way around it. We could throw an explicit
NotImplementedError (which is another way of crashing), but there's no way to
silently ignore the error and still produce valid output.

[gfeher, 2011-02-09 17:50:43]

Just some nits.

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:88: void
CloudPolicyCache::LoadPolicyFromFile() {
Here and everywhere elese: please add
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::XYZ) checks to document which
thread are the methods running on.

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:69: // Returns the time as which the
policy was last fetched.
Nit: at which

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:177: LOG(WARNING) << "Token
fetcher error:" << code;
Is this intentional?

http://codereview.chromium.org/6409040/diff/14001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6409040/diff/14001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:191: (policy['name'],
policy['name'], policy['id'] + 2)]
Nit: please extract 2 as a constant.

http://codereview.chromium.org/6409040/diff/14001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:320: #        (proto_type,
protoname, membername))
Please clean comments.

[Jakob Kummerow, 2011-02-10 12:10:15]

Fixed the build on Mac and Win (let's hope the try bots agree) and addressed
gfeher's comments. Please take another look. 

mark: Please take a look at the .gyp[i] changes. 

Quick background info about what's going on in cloud_policy_codegen.gyp:
1. generate_policy_source.py generates two header files, two .cc source files,
and a .proto file (target "cloud_policy_code_generate").
2. the autogenerated .proto as well as two manually defined .proto files get
"compiled" into C++/Python code by protoc (targets "cloud_policy_proto_compile"
and "cloud_policy_backend_header_compile").
3. the autogenerated code is compiled into a library: target "policy" (on all
platforms) and "policy_win64" (required on *all* windows builds as a dependency
of 64bit NaCl).
(4. a bunch of other targets in the chrome/ and net/ root dirs depend on the
generated stuff)

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:88: void
CloudPolicyCache::LoadPolicyFromFile() {
On 2011/02/09 17:50:43, gfeher wrote:
> Here and everywhere elese: please add
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::XYZ) checks to document which
> thread are the methods running on.

Done.

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:69: // Returns the time as which the
policy was last fetched.
On 2011/02/09 17:50:43, gfeher wrote:
> Nit: at which

Done.

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6409040/diff/14001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:177: LOG(WARNING) << "Token
fetcher error:" << code;
On 2011/02/09 17:50:43, gfeher wrote:
> Is this intentional?

No. Done.

http://codereview.chromium.org/6409040/diff/14001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/6409040/diff/14001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:191: (policy['name'],
policy['name'], policy['id'] + 2)]
On 2011/02/09 17:50:43, gfeher wrote:
> Nit: please extract 2 as a constant.

Done.

http://codereview.chromium.org/6409040/diff/14001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:320: #        (proto_type,
protoname, membername))
On 2011/02/09 17:50:43, gfeher wrote:
> Please clean comments.

Sure. Done.

[Mark Mentovai, 2011-02-10 18:42:18]

The .gyp and .gypi files look OK otherwise.

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/cloud_pol...
File chrome/app/policy/cloud_policy_codegen.gyp (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/cloud_pol...
chrome/app/policy/cloud_policy_codegen.gyp:10: 'generate_policy_source_script':
Everything in here that’s a path needs to be in a variable whose name ends in
_path, _file, _dir, _paths, _files, or _dirs. It looks like that means doing
this for each variable from policy_constant_header through proto_rel_path.

generate_policy_source_script has one non-path and one path. I think you should
change it to just be a path-based string variable, and not a list. Take the
'python' out and just write it below in the generate_policy_source action
specification.

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/cloud_pol...
chrome/app/policy/cloud_policy_codegen.gyp:26: {
Weird indent here. It should only be two spaces in from the ' beginning
'actions' on the preceding line.

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/cloud_pol...
chrome/app/policy/cloud_policy_codegen.gyp:57: },
Looks like this is where the weird indentation ends. See how the } on this line
doesn’t look right considering the } on the next?

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome.gyp#newcode41
chrome/chrome.gyp:41: #'app/policy/cloud_policy_codegen.gyp:policy_win64',
?

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:33: 'app/policy/cloud_policy_codegen.gyp:policy',
Looks like you were trying to sort things, but this one isn’t sorted. app/ <
browser/.

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:45: 'app/policy/cloud_policy_codegen.gyp:policy',
And for this one, renderer < app/. Usually these get sorted so that
“local” targets unqualified by a .gyp file name and colon come before “remote”
filename-qualified targets.

http://codereview.chromium.org/6409040/diff/20001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/6409040/diff/20001/net/net.gyp#newcode1187
net/net.gyp:1187:
'../chrome/app/policy/cloud_policy_codegen.gyp:cloud_policy_proto_compile',
Sort

[danno, 2011-02-11 11:00:42]

LGTM with a few comments, and as discussed, please don't throw away policy on
responses from the future.

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/syntax_ch...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:149: if is_in_group:
Check that there are no ids in the group?

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:57:
cached_policy.mutable_cloud_policy()->CopyFrom(*cloud_policy_response_);
Why do you only set the time stamp in the device policy case?

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:137: } else if
(cached_response.has_cloud_policy()) {
assert !has_device_policy()?

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:146: scoped_ptr<DictionaryValue>
value(
assert !has_cloud_policy()?

[gfeher, 2011-02-11 20:29:42]

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:138: bool has_device_policy_;
It looks to me that you are not reading this variable anywhere.

[Jakob Kummerow, 2011-02-14 16:14:26]

Thank you for your reviews, everyone. 

Danno: are you OK with my responses to your comments in cloud_policy_cache.cc?

Gabor, Mattias: does this LGTY?

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/cloud_pol...
File chrome/app/policy/cloud_policy_codegen.gyp (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/cloud_pol...
chrome/app/policy/cloud_policy_codegen.gyp:10: 'generate_policy_source_script':
On 2011/02/10 18:42:19, Mark Mentovai wrote:
> Everything in here that’s a path needs to be in a variable whose name ends in
> _path, _file, _dir, _paths, _files, or _dirs. It looks like that means doing
> this for each variable from policy_constant_header through proto_rel_path.
> 
> generate_policy_source_script has one non-path and one path. I think you
should
> change it to just be a path-based string variable, and not a list. Take the
> 'python' out and just write it below in the generate_policy_source action
> specification.

Done. (Except for 'proto_path_substr' which is used as a component of other
_path named variables and would actually break stuff if it ended it _path
itself.)

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/cloud_pol...
chrome/app/policy/cloud_policy_codegen.gyp:26: {
On 2011/02/10 18:42:19, Mark Mentovai wrote:
> Weird indent here. It should only be two spaces in from the ' beginning
> 'actions' on the preceding line.

Done. I'm surprised that I didn't notice that myself ;-)

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/cloud_pol...
chrome/app/policy/cloud_policy_codegen.gyp:57: },
On 2011/02/10 18:42:19, Mark Mentovai wrote:
> Looks like this is where the weird indentation ends. See how the } on this
line
> doesn’t look right considering the } on the next?

Done.

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/syntax_ch...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:149: if is_in_group:
On 2011/02/11 11:00:42, danno wrote:
> Check that there are no ids in the group?

Done.

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:57:
cached_policy.mutable_cloud_policy()->CopyFrom(*cloud_policy_response_);
On 2011/02/11 11:00:42, danno wrote:
> Why do you only set the time stamp in the device policy case?

Because new-style cloud policy contains its own timestamp, created and signed by
the server, whereas old-style device policy doesn't.

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:137: } else if
(cached_response.has_cloud_policy()) {
On 2011/02/11 11:00:42, danno wrote:
> assert !has_device_policy()?

Not necessary. With the current implementation, device policy can't ever be
there, but even if it were, we'd ignore it, no harm done.

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:146: scoped_ptr<DictionaryValue>
value(
On 2011/02/11 11:00:42, danno wrote:
> assert !has_cloud_policy()?

Not necessary: we're in the "else" clause of precisely that "if".

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:138: bool has_device_policy_;
On 2011/02/11 20:29:43, gfeher wrote:
> It looks to me that you are not reading this variable anywhere.

I *am* reading it in |bool CloudPolicyCache::has_device_policy()|, which is
defined inline in the .h file, and is itself called by the
DeviceManagementPolicyProvider.

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome.gyp#newcode41
chrome/chrome.gyp:41: #'app/policy/cloud_policy_codegen.gyp:policy_win64',
On 2011/02/10 18:42:19, Mark Mentovai wrote:
> ?

Done. Forgot to remove it earlier.

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:33: 'app/policy/cloud_policy_codegen.gyp:policy',
On 2011/02/10 18:42:19, Mark Mentovai wrote:
> Looks like you were trying to sort things, but this one isn’t sorted. app/ <
> browser/.

Done.

http://codereview.chromium.org/6409040/diff/20001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:45: 'app/policy/cloud_policy_codegen.gyp:policy',
On 2011/02/10 18:42:19, Mark Mentovai wrote:
> And for this one, renderer < app/. Usually these get sorted so that
> “local”&nbsp;targets unqualified by a .gyp file name and colon come before
“remote”
> filename-qualified targets.

Done.

http://codereview.chromium.org/6409040/diff/20001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/6409040/diff/20001/net/net.gyp#newcode1187
net/net.gyp:1187:
'../chrome/app/policy/cloud_policy_codegen.gyp:cloud_policy_proto_compile',
On 2011/02/10 18:42:19, Mark Mentovai wrote:
> Sort

Done.

[Mattias Nissler (ping if slow), 2011-02-15 10:46:39]

my 2 cents.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:310: }
Is there anything wrong with the more std-like solution?

bool MapEntryEquals(const PolicyMapType::value_type& a, const
PolicyMapType::value_type& b) {
  return a.first == a.second && Value::Equals(a.second, b.second);
}

bool CloudPolicyCache::Equals(const PolicyMapType& a, const PolicyMapType& b) {
  return a.size() == b.size() && std::equal(a.begin(), a.end(), b.begin(),
MapEntryEquals);
}

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:35:
ConfigurationPolicyProvider::PolicyMapType* recommended);
Do we need this declaration in the header? I think it would suffice to put the
declaration into the .cc file.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:122: // |device_policy_|.
both ... as well as?

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_unittest.cc:22: Value*
DecodeIntegerValue(google::protobuf::int64 value);
Where are the implementations? Put a comment helping to find them?

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_unittest.cc:331: 
AFAICS, these tests only test the new format. Should we have some tests that
check whether the cache handles old policy format correctly?

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache_unittest.cc:24: // from
CPC. The new interface is tested in cloud_policy_cache_unittest.cc.
Ah, the tests are here. Can we just move this code to the other file? Breaking
the convention of the implementation and test file names matching doesn't seem
like a good idea.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:139: LOG(WARNING) <<
"Device manager doesn't understand new protocol, falling "
s/Device manager/device management server/

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/proto...
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
Didn't look at this file in detail, it'll get reworked anyway.

[Jakob Kummerow, 2011-02-15 14:22:00]

Mattias: LGTY?
Gabor: ping?

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:310: }
On 2011/02/15 10:46:39, Mattias Nissler wrote:
> Is there anything wrong with the more std-like solution?
> 
> bool MapEntryEquals(const PolicyMapType::value_type& a, const
> PolicyMapType::value_type& b) {
>   return a.first == a.second && Value::Equals(a.second, b.second);
> }
> 
> bool CloudPolicyCache::Equals(const PolicyMapType& a, const PolicyMapType& b)
{
>   return a.size() == b.size() && std::equal(a.begin(), a.end(), b.begin(),
> MapEntryEquals);
> }

Done.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:35:
ConfigurationPolicyProvider::PolicyMapType* recommended);
On 2011/02/15 10:46:39, Mattias Nissler wrote:
> Do we need this declaration in the header? I think it would suffice to put the
> declaration into the .cc file.

Done.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:122: // |device_policy_|.
On 2011/02/15 10:46:39, Mattias Nissler wrote:
> both ... as well as?

(both ... and ...) as well as (...) ;-)
But both |lock_| and this comment will be removed in our next CL anyway, so no
reason to fight about it.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_unittest.cc:22: Value*
DecodeIntegerValue(google::protobuf::int64 value);
On 2011/02/15 10:46:39, Mattias Nissler wrote:
> Where are the implementations? Put a comment helping to find them?

Done.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_unittest.cc:331: 
On 2011/02/15 10:46:39, Mattias Nissler wrote:
> AFAICS, these tests only test the new format. Should we have some tests that
> check whether the cache handles old policy format correctly?

We still have the old tests in device_management_policy_cache_unittest.cc.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_cache_unittest.cc:24: // from
CPC. The new interface is tested in cloud_policy_cache_unittest.cc.
On 2011/02/15 10:46:39, Mattias Nissler wrote:
> Ah, the tests are here. Can we just move this code to the other file? Breaking
> the convention of the implementation and test file names matching doesn't seem
> like a good idea.

Well, it was only supposed to be a temporary solution for a few weeks, but...
Done.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:139: LOG(WARNING) <<
"Device manager doesn't understand new protocol, falling "
On 2011/02/15 10:46:39, Mattias Nissler wrote:
> s/Device manager/device management server/

Done.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/proto...
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2011/02/15 10:46:39, Mattias Nissler wrote:
> Didn't look at this file in detail, it'll get reworked anyway.

Yes. OK.

[Mattias Nissler (ping if slow), 2011-02-15 14:55:19]

LGTM pending trybot happiness and the declaration change you missed.

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:35:
ConfigurationPolicyProvider::PolicyMapType* recommended);
On 2011/02/15 14:22:00, jkummerow wrote:
> On 2011/02/15 10:46:39, Mattias Nissler wrote:
> > Do we need this declaration in the header? I think it would suffice to put
the
> > declaration into the .cc file.
> 
> Done.

not done?

http://codereview.chromium.org/6409040/diff/38001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:122: // |device_policy_|.
On 2011/02/15 14:22:00, jkummerow wrote:
> On 2011/02/15 10:46:39, Mattias Nissler wrote:
> > both ... as well as?
> 
> (both ... and ...) as well as (...) ;-)
> But both |lock_| and this comment will be removed in our next CL anyway, so no
> reason to fight about it.

I can see that from a logic formula point of view you're right, but I find the
sentence structure weird.

[gfeher, 2011-02-15 14:56:12]

I have no more comments on this.