Removed policy::PolicyNamespaceKey.

Removed policy::PolicyNamespaceKey.

The policy code maps policy namespaces (which are pairs of policy type
and an optional settings entity ID, or component ID) to their policy
internally. The namespace spilled into the CloudPolicyClient though
the cloud policy protocol doesn't have this concept; this change
clears that up.

This is a cleanup of the policy code and doesn't make any functional
changes.

BUG=435987
Committed: https://crrev.com/fa667f7aa445c85d915050ee93d01b4a738348f4
Cr-Commit-Position: refs/heads/master@{#306079}

[Joao da Silva, 2014-11-26 16:44:07]

joaodasilva@chromium.org changed reviewers:
+ mnissler@chromium.org

[Joao da Silva, 2014-11-26 16:44:07]

Here's the cleanup we've discussed. You may want to start at
cloud_policy_client.h. If there's something there that is very wrong then you
can skip the rest during the first round :-)

[Mattias Nissler (ping if slow), 2014-11-27 08:34:11]

I think this is good. One idea before I do a full review pass: Does it make
sense to pass the type and settings_entity_id strings from manager to service to
core to client, or should we maybe just have the manager implementation/creation
logic make the call to the client? Or is the passing-through-all-ctors required
for correctness?

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_client.cc:177: if
(IsChromePolicy(type_to_fetch.first)) {
Side note: We may eventually change this to not hardcode the if-is-chrome-policy
here, but keep the necessary fields in a map keyed by the policy type.

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_client.h:231: typedef
std::set<std::pair<std::string, std::string>> TypeToFetchSet;
nit: Let's rename this to PolicyTypeSet or something - that describes better
what the type is, which I think is more important than what it's used for.

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_core_unittest.cc (right):

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_core_unittest.cc:25: "",
why not std::string()

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_manager.cc:31: :
core_(policy_type, "", cloud_policy_store, task_runner),
std::string() instead of ""?

[Joao da Silva, 2014-11-27 12:19:31]

The (policy_type, settings_entity_id) info is passed around because the service
must know it to forward the "main" policy type from the client to the store. It
comes from the manager because it's the manager that determines its value.

I agree this doesn't look very good though. I've removed that from the ctors and
it's now passed from the manager to the core via the Connect() call, where the
service gets created. WDYT?

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_client.cc:177: if
(IsChromePolicy(type_to_fetch.first)) {
On 2014/11/27 08:34:10, Mattias Nissler wrote:
> Side note: We may eventually change this to not hardcode the
if-is-chrome-policy
> here, but keep the necessary fields in a map keyed by the policy type.

Ack

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_client.h:231: typedef
std::set<std::pair<std::string, std::string>> TypeToFetchSet;
On 2014/11/27 08:34:10, Mattias Nissler wrote:
> nit: Let's rename this to PolicyTypeSet or something - that describes better
> what the type is, which I think is more important than what it's used for.

Done.

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_core_unittest.cc (right):

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_core_unittest.cc:25: "",
On 2014/11/27 08:34:10, Mattias Nissler wrote:
> why not std::string()

Done.

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/cloud_policy_manager.cc:31: :
core_(policy_type, "", cloud_policy_store, task_runner),
On 2014/11/27 08:34:10, Mattias Nissler wrote:
> std::string() instead of ""?

Done.

[Mattias Nissler (ping if slow), 2014-11-28 10:42:39]

On 2014/11/27 12:19:31, Joao da Silva wrote:
> The (policy_type, settings_entity_id) info is passed around because the
service
> must know it to forward the "main" policy type from the client to the store.
It
> comes from the manager because it's the manager that determines its value.
> 
> I agree this doesn't look very good though. I've removed that from the ctors
and
> it's now passed from the manager to the core via the Connect() call, where the
> service gets created. WDYT?

Ah, so I was missing the detail that CloudPolicyService needs to know the policy
type / entity ID to do its thing. In that sense, CloudPolicyCore needs to be
configured to work for that specific type/ID, and it's expected for this to not
ever change in the lifetime of the object. I think that's good motivation to
keep the ctor parameters after all. Sorry for not seeing through this yesterday.


> 
>
https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
> File components/policy/core/common/cloud/cloud_policy_client.cc (right):
> 
>
https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
> components/policy/core/common/cloud/cloud_policy_client.cc:177: if
> (IsChromePolicy(type_to_fetch.first)) {
> On 2014/11/27 08:34:10, Mattias Nissler wrote:
> > Side note: We may eventually change this to not hardcode the
> if-is-chrome-policy
> > here, but keep the necessary fields in a map keyed by the policy type.
> 
> Ack
> 
>
https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
> File components/policy/core/common/cloud/cloud_policy_client.h (right):
> 
>
https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
> components/policy/core/common/cloud/cloud_policy_client.h:231: typedef
> std::set<std::pair<std::string, std::string>> TypeToFetchSet;
> On 2014/11/27 08:34:10, Mattias Nissler wrote:
> > nit: Let's rename this to PolicyTypeSet or something - that describes better
> > what the type is, which I think is more important than what it's used for.
> 
> Done.
> 
>
https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
> File components/policy/core/common/cloud/cloud_policy_core_unittest.cc
(right):
> 
>
https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
> components/policy/core/common/cloud/cloud_policy_core_unittest.cc:25: "",
> On 2014/11/27 08:34:10, Mattias Nissler wrote:
> > why not std::string()
> 
> Done.
> 
>
https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
> File components/policy/core/common/cloud/cloud_policy_manager.cc (right):
> 
>
https://codereview.chromium.org/762863002/diff/1/components/policy/core/commo...
> components/policy/core/common/cloud/cloud_policy_manager.cc:31: :
> core_(policy_type, "", cloud_policy_store, task_runner),
> On 2014/11/27 08:34:10, Mattias Nissler wrote:
> > std::string() instead of ""?
> 
> Done.

[Joao da Silva, 2014-11-28 13:28:59]

Alright, reverted those changes again. PTAL

[Mattias Nissler (ping if slow), 2014-11-28 13:47:19]

LGTM. Ship it!

[Joao da Silva, 2014-11-28 13:49:26]

Thanks! https://codereview.chromium.org/756133002/ has already  been rebased on
this and is ready for a review too :-)

[Joao da Silva, 2014-11-28 13:49:32]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-11-28 13:50:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/762863002/40001

[commit-bot: I haz the power, 2014-11-28 14:11:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-28 14:13:39]

Patchset 3 (id:??) landed as
https://crrev.com/fa667f7aa445c85d915050ee93d01b4a738348f4
Cr-Commit-Position: refs/heads/master@{#306079}