New policy protobuf protocol.

New policy protobuf protocol.

(Third attempt to land http://codereview.chromium.org/6409040/ -- now without memory leaks)

- cloud_policy.proto autogenerated from policy_templates.json
- C++ method decoding the protobuf also 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=75259

[Jakob Kummerow, 2011-02-17 08:53:18]

One LGTM is enough, let's see who gets there first ;-)

Patch set 1: Included for your reviewing convenience, that's what I tried to
land last time. It leaks memory.

Patch set 2: Updated version: contains new class "PolicyMap" that wraps a
std::map<ConfigurationPolicyType, Value*> and cleans up after itself when going
out of scope.

[Mattias Nissler (ping if slow), 2011-02-17 09:27:20]

here you go.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
File chrome/browser/policy/policy_map.cc (right):

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.cc:5: 
excess whitespace

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
File chrome/browser/policy/policy_map.h (right):

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.h:4: 
excess whitespace

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.h:18: // scoped_ptr). Exposes interesting
methods of the underlying std::map.
I feel the scoped_ptr reference here creates more confusion than it clarifies
things. I leave the decision to you though.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.h:27: const Value* Get(ConfigurationPolicyType
policy) const;
Please mention ownership handling in a comment to Get() and Set(). Also,
document what happens if there is no value set or the value already present.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.h:39: void clear();
By the style guide, this should be named Clear(), since it is a non-trivial
operation.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
File chrome/browser/policy/policy_map_unittest.cc (right):

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map_unittest.cc:13: class PolicyMapTest : public
testing::Test {
If you don't need the fixture, you can just remove it and replace TEST_F() with
TEST() below.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map_unittest.cc:21: scoped_ptr<StringValue>
expected(Value::CreateStringValue("aaa"));
Why use scoped_ptrs? You can just declare a StringValue local.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map_unittest.cc:59: scoped_ptr<Value>
expected(Value::CreateStringValue("bbb"));
same here.

[Jakob Kummerow, 2011-02-17 09:47:14]

Thanks for the comments. I addressed them all.

No new try jobs since there were no functional changes. (Btw, for patch set 2,
try jobs on win, mac, linux, linux_valgrind, mac_valgrind are on their way;
rietveld just fails to display them, as usual.)

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
File chrome/browser/policy/policy_map.cc (right):

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.cc:5: 
On 2011/02/17 09:27:20, Mattias Nissler wrote:
> excess whitespace

Done.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
File chrome/browser/policy/policy_map.h (right):

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.h:4: 
On 2011/02/17 09:27:20, Mattias Nissler wrote:
> excess whitespace

Done.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.h:18: // scoped_ptr). Exposes interesting
methods of the underlying std::map.
On 2011/02/17 09:27:20, Mattias Nissler wrote:
> I feel the scoped_ptr reference here creates more confusion than it clarifies
> things. I leave the decision to you though.

Done (removed it).

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.h:27: const Value* Get(ConfigurationPolicyType
policy) const;
On 2011/02/17 09:27:20, Mattias Nissler wrote:
> Please mention ownership handling in a comment to Get() and Set(). Also,
> document what happens if there is no value set or the value already present.

Done.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map.h:39: void clear();
On 2011/02/17 09:27:20, Mattias Nissler wrote:
> By the style guide, this should be named Clear(), since it is a non-trivial
> operation.

Done.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
File chrome/browser/policy/policy_map_unittest.cc (right):

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map_unittest.cc:13: class PolicyMapTest : public
testing::Test {
On 2011/02/17 09:27:20, Mattias Nissler wrote:
> If you don't need the fixture, you can just remove it and replace TEST_F()
with
> TEST() below.

Done.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map_unittest.cc:21: scoped_ptr<StringValue>
expected(Value::CreateStringValue("aaa"));
On 2011/02/17 09:27:20, Mattias Nissler wrote:
> Why use scoped_ptrs? You can just declare a StringValue local.

Done.

http://codereview.chromium.org/6532019/diff/2001/chrome/browser/policy/policy...
chrome/browser/policy/policy_map_unittest.cc:59: scoped_ptr<Value>
expected(Value::CreateStringValue("bbb"));
On 2011/02/17 09:27:20, Mattias Nissler wrote:
> same here.

Done.

[danno, 2011-02-17 11:05:44]

LGTM with a nit.

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

http://codereview.chromium.org/6532019/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache.cc:180:
mandatory_policy_.Swap(&mandatory_policy);
Comment about using Swap to cleanup to old value of mandatory_policy_?