Added a PolicyDomainDescriptor.

Added a PolicyDomainDescriptor.

Policies are scoped by namespace keys, which indicate whether they're meant for
the browser itself or for a component, such as an extension. The descriptor
additionally describes the types of the known policies, for each component. This
allows the policy loaders to known which policies to look for, and to filter out
unknown or invalid policies.

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

[Joao da Silva, 2013-05-14 16:44:41]

This depends on https://codereview.chromium.org/15133005.

@mnissler: please review the policy changes

@kalman: please review the extensions changes

Thanks!

[Mattias Nissler (ping if slow), 2013-05-15 10:24:06]

https://codereview.chromium.org/15061007/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:75:
EXPECT_CALL(provider_, RegisterPolicyDomain(_)).Times(AnyNumber());
It seems like there are a large number of tests that use
MockConfigurationPolicyProvider but actually are not interested in tracking call
expectations. I guess it makes sense to have another mock implementation lacking
the gmock bits. WDYT? If you agree, can you file a bug?

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/async_p...
File chrome/browser/policy/async_policy_loader.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/async_p...
chrome/browser/policy/async_policy_loader.cc:61:
it->second->FilterBundle(bundle.get());
I don't think this is a good idea - the idea was that chrome://policy does show
misconfigured policy. I guess a better point for filtering would be at the
PolicyService or ConfigurationPolicyPrefStore level.

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:294: POLICY_DOMAIN_CHROME);
Any plans to pass the Chrome schema here as well? FWIW, the Win loader already
uses schemas internally... I guess we should use a single one - is that planned
for a follow-up CL?

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/component_cloud_policy_service.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:161:
i->second->FilterBundle(bundle.get());
Similar comment regarding surfacing typos/unsupported policies. Note that if you
don't filter, you don't need to keep a pointer to the entire descriptor in
|domain_map_|.

Btw.: Should about:policy list extension policy as well?

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_domain_descriptor.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor.cc:41: // be removed for M30.
http://crbug.com/240704
Have you made sure that the extension gets updated well ahead of time?

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor.cc:50:
type_map.find(it_map->first);
Given these are both indexed by name, we might be able to just start at
type_map.begin() and advance it_type appropriately?

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_domain_descriptor.h (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor.h:21: class
PolicyDomainDescriptor : public base::RefCounted<PolicyDomainDescriptor> {
This should have a class comment explaining what it's good for. The CL
description might be a good start ;)

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor.h:30: void AddComponent(const
std::string& component_id,
Given the implementation, this should probably be SetComponent(), or
alternatively needs a comment explaining that it replaces existing entries.

[not at google - send to devlin, 2013-05-16 16:37:16]

extensions lgtm

[Joao da Silva, 2013-05-19 13:17:54]

@bartfab: passing this one to you too :-) Please have a look, thanks!

https://codereview.chromium.org/15061007/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:75:
EXPECT_CALL(provider_, RegisterPolicyDomain(_)).Times(AnyNumber());
On 2013/05/15 10:24:06, Mattias Nissler wrote:
> It seems like there are a large number of tests that use
> MockConfigurationPolicyProvider but actually are not interested in tracking
call
> expectations. I guess it makes sense to have another mock implementation
lacking
> the gmock bits. WDYT? If you agree, can you file a bug?

Agreed, filed http://crbug.com/242087 and left a TODO in
mock_configuration_policy_provider.h.

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/async_p...
File chrome/browser/policy/async_policy_loader.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/async_p...
chrome/browser/policy/async_policy_loader.cc:61:
it->second->FilterBundle(bundle.get());
On 2013/05/15 10:24:06, Mattias Nissler wrote:
> I don't think this is a good idea - the idea was that chrome://policy does
show
> misconfigured policy. I guess a better point for filtering would be at the
> PolicyService or ConfigurationPolicyPrefStore level.

Note that this only applies to non-CHROME policy, so typos will still show up.

Typos for non-CHROME policy are trickier to find, since the loaders use the
schema information to determine which names to lookup. That will be figured out
in future work, since about:policy doesn't show extension policy for now.

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:294: POLICY_DOMAIN_CHROME);
On 2013/05/15 10:24:06, Mattias Nissler wrote:
> Any plans to pass the Chrome schema here as well? FWIW, the Win loader already
> uses schemas internally... I guess we should use a single one - is that
planned
> for a follow-up CL?

Yes it is :-) Having a complete PolicySchema for Chrome removes the need for the
schemas-in-registry hack, and also makes support for structured policy easy.

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/component_cloud_policy_service.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:161:
i->second->FilterBundle(bundle.get());
On 2013/05/15 10:24:06, Mattias Nissler wrote:
> Similar comment regarding surfacing typos/unsupported policies. Note that if
you
> don't filter, you don't need to keep a pointer to the entire descriptor in
> |domain_map_|.
> 

This only applies to component policy, and the bundle must be filtered since
extensions should never see policies that are not declared in their schema.

> Btw.: Should about:policy list extension policy as well?

I'd like to have that eventually. For now I see two issues with that: the
loaders use the descriptor info to lookup policy so it's harder to locate typos,
and we don't know which policy values are "sensitive" and shouldn't be shown to
the user (e.g. passwords or tokens).

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_domain_descriptor.cc (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor.cc:41: // be removed for M30.
http://crbug.com/240704
On 2013/05/15 10:24:06, Mattias Nissler wrote:
> Have you made sure that the extension gets updated well ahead of time?

Yes, I'm in sync with Julian here.

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor.cc:50:
type_map.find(it_map->first);
On 2013/05/15 10:24:06, Mattias Nissler wrote:
> Given these are both indexed by name, we might be able to just start at
> type_map.begin() and advance it_type appropriately?

This has changed after the PolicySchema changes.

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_domain_descriptor.h (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor.h:21: class
PolicyDomainDescriptor : public base::RefCounted<PolicyDomainDescriptor> {
On 2013/05/15 10:24:06, Mattias Nissler wrote:
> This should have a class comment explaining what it's good for. The CL
> description might be a good start ;)

I had that in some commit before uploading but lost it in a rebase ._. Done.

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor.h:30: void AddComponent(const
std::string& component_id,
On 2013/05/15 10:24:06, Mattias Nissler wrote:
> Given the implementation, this should probably be SetComponent(), or
> alternatively needs a comment explaining that it replaces existing entries.

Done.

[bartfab (slow), 2013-05-21 12:10:56]

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:7: #include
<set>
Nit: No longer used.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/asy...
File chrome/browser/policy/async_policy_loader.h (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/asy...
chrome/browser/policy/async_policy_loader.h:15: #include
"chrome/browser/policy/policy_domain_descriptor.h"
Nit: Would a forward-declaration of PolicyDomainDescriptor not be sufficient
here?

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/component_cloud_policy_service.cc (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:18: #include
"chrome/browser/policy/policy_domain_descriptor.h"
Nit: This should be included in the header file here.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:160: for
(DomainMap::iterator i = domain_map_.begin(); i != domain_map_.end(); ++i)
|it| is the established short name for an iterator. |i| sounds like an integer
to me.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:179: for
(StringSet::iterator it = ids.begin(); it != ids.end(); ++it)
Why is the iterator no longer const?

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:308: previous_ids
= current_ids;
I think |registered_ids| was clearer here. Now the line looks like you are
modifying the previous set of IDs when in fact, you are modifying the entry in
the |registered_components_| map.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/component_cloud_policy_service_unittest.cc
(right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service_unittest.cc:8:
#include "base/memory/ref_counted.h"
Nit: Already included by component_cloud_policy_service.h.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_provider.h (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_provider.h:8: #include <set>
Nit: No longer used.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_provider.h:9: #include <string>
Nit: No longer used.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_provider.h:16: #include
"chrome/browser/policy/policy_domain_descriptor.h"
Nit: A forward-declaration would be sufficient here.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_domain_descriptor.cc (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:16: bool Matches(const
PolicySchema* schema, const base::Value& value) {
#include "base/values.h"

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:53: previous =
schema.release();
Could you rename |previous| to something like |entry| or |component_schema|
maybe? "previous = ..." looks like you are modifying the previous schema instead
of replacing it with the new schema.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:58:
DCHECK_NE(POLICY_DOMAIN_CHROME, domain_);
#include "base/logging.h"

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:69: bundle->Get(ns).Clear();
Why not make |it_bundle| a non-const iterator, allowing you to
it_bundle.second->Clear()

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:81: PolicyMap& map =
bundle->Get(ns);
As above, if |it_bundle| were non-const, you could use it_bundle.second here.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:82: PolicyMap::const_iterator
it_map = map.begin();
for and while are equivalent of course. In terms of readability, I think a for
loop would be clearer here:

for (PolicyMap::const_iterator it_map = map.begin();
     it_map != map.end(); ) {

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:88: ++it_map;
Alternatively (and using the STL more efficiently):

if (!policy_value || !Matches(policy_schema, *policy_value))
  it_map = map.Erase(policy_name);
else
  ++it_map;

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_domain_descriptor.h (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.h:34: void SetComponent(const
std::string& component_id,
Nit: Maybe better |SetComponentSchema| or |AddComponent|? "Setting a component"
does not convey any meaning; "setting the schema for a component" or "adding a
component to the list" does.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_domain_descriptor_unittest.cc (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor_unittest.cc:56: // Other
namespace aren't filtered.
Nit: This part of the test only verifies that POLICY_DOMAIN_CHROME is not
getting filtered. How about testing that another extension's policy is left
alone?

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor_unittest.cc:62:
base::Value::CreateStringValue("value"));
#include "base/values.h"

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor_unittest.cc:68: PolicyMap& map =
expected_bundle.Get(extension_ns);
#include "chrome/browser/policy/policy_map.h"

[bartfab (slow), 2013-05-21 12:14:19]

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_domain_descriptor.h (right):

https://codereview.chromium.org/15061007/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_domain_descriptor.h:30: void AddComponent(const
std::string& component_id,
On 2013/05/19 13:17:55, Joao da Silva wrote:
> On 2013/05/15 10:24:06, Mattias Nissler wrote:
> > Given the implementation, this should probably be SetComponent(), or
> > alternatively needs a comment explaining that it replaces existing entries.
> 
> Done.

I just noticed that my suggestion of |AddComponent| would reverse the change
Mattias asked you to do :). How about adding an explanation in a comment to
address both Mattias' and my requests?

[Joao da Silva, 2013-05-21 17:50:14]

@bartfab: PTAL

@jhawkins: please review chrome/browser/ui/webui/

Thanks!

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:7: #include
<set>
On 2013/05/21 12:10:56, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/asy...
File chrome/browser/policy/async_policy_loader.h (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/asy...
chrome/browser/policy/async_policy_loader.h:15: #include
"chrome/browser/policy/policy_domain_descriptor.h"
On 2013/05/21 12:10:56, bartfab wrote:
> Nit: Would a forward-declaration of PolicyDomainDescriptor not be sufficient
> here?

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/component_cloud_policy_service.cc (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:18: #include
"chrome/browser/policy/policy_domain_descriptor.h"
On 2013/05/21 12:10:56, bartfab wrote:
> Nit: This should be included in the header file here.

Added a forward decl.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:160: for
(DomainMap::iterator i = domain_map_.begin(); i != domain_map_.end(); ++i)
On 2013/05/21 12:10:56, bartfab wrote:
> |it| is the established short name for an iterator. |i| sounds like an integer
> to me.

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:179: for
(StringSet::iterator it = ids.begin(); it != ids.end(); ++it)
On 2013/05/21 12:10:56, bartfab wrote:
> Why is the iterator no longer const?

I don't know why this changed, reverted.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:308: previous_ids
= current_ids;
On 2013/05/21 12:10:56, bartfab wrote:
> I think |registered_ids| was clearer here. Now the line looks like you are
> modifying the previous set of IDs when in fact, you are modifying the entry in
> the |registered_components_| map.

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/component_cloud_policy_service_unittest.cc
(right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/component_cloud_policy_service_unittest.cc:8:
#include "base/memory/ref_counted.h"
On 2013/05/21 12:10:56, bartfab wrote:
> Nit: Already included by component_cloud_policy_service.h.

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_provider.h (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_provider.h:8: #include <set>
On 2013/05/21 12:10:56, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_provider.h:9: #include <string>
On 2013/05/21 12:10:56, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_provider.h:16: #include
"chrome/browser/policy/policy_domain_descriptor.h"
On 2013/05/21 12:10:56, bartfab wrote:
> Nit: A forward-declaration would be sufficient here.

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_domain_descriptor.cc (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:16: bool Matches(const
PolicySchema* schema, const base::Value& value) {
On 2013/05/21 12:10:56, bartfab wrote:
> #include "base/values.h"

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:53: previous =
schema.release();
On 2013/05/21 12:10:56, bartfab wrote:
> Could you rename |previous| to something like |entry| or |component_schema|
> maybe? "previous = ..." looks like you are modifying the previous schema
instead
> of replacing it with the new schema.

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:58:
DCHECK_NE(POLICY_DOMAIN_CHROME, domain_);
On 2013/05/21 12:10:56, bartfab wrote:
> #include "base/logging.h"

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:69: bundle->Get(ns).Clear();
On 2013/05/21 12:10:56, bartfab wrote:
> Why not make |it_bundle| a non-const iterator, allowing you to
> it_bundle.second->Clear()

PolicyBundle only has const_iterators, but there's no reason why it couldn't
have iterators too. Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:81: PolicyMap& map =
bundle->Get(ns);
On 2013/05/21 12:10:56, bartfab wrote:
> As above, if |it_bundle| were non-const, you could use it_bundle.second here.

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:82: PolicyMap::const_iterator
it_map = map.begin();
On 2013/05/21 12:10:56, bartfab wrote:
> for and while are equivalent of course. In terms of readability, I think a for
> loop would be clearer here:
> 
> for (PolicyMap::const_iterator it_map = map.begin();
>      it_map != map.end(); ) {

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.cc:88: ++it_map;
On 2013/05/21 12:10:56, bartfab wrote:
> Alternatively (and using the STL more efficiently):
> 
> if (!policy_value || !Matches(policy_schema, *policy_value))
>   it_map = map.Erase(policy_name);
> else
>   ++it_map;

PolicyMap is not an std::map: it doesn't have iterators (const_iterators can be
used to mutate a map), and its Erase() method doesn't return an iterator. I'll
leave this as is.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_domain_descriptor.h (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor.h:34: void SetComponent(const
std::string& component_id,
On 2013/05/21 12:10:56, bartfab wrote:
> Nit: Maybe better |SetComponentSchema| or |AddComponent|? "Setting a
component"
> does not convey any meaning; "setting the schema for a component" or "adding a
> component to the list" does.

Went for RegisterComponent(), I think that conveys more clearly what this is
about, and hopefully apeases you and Mattias :-)

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_domain_descriptor_unittest.cc (right):

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor_unittest.cc:56: // Other
namespace aren't filtered.
On 2013/05/21 12:10:56, bartfab wrote:
> Nit: This part of the test only verifies that POLICY_DOMAIN_CHROME is not
> getting filtered. How about testing that another extension's policy is left
> alone?

Ah, but it's not left alone :-) Undeclared components are filtered out. Added a
test for that.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor_unittest.cc:62:
base::Value::CreateStringValue("value"));
On 2013/05/21 12:10:56, bartfab wrote:
> #include "base/values.h"

Done.

https://codereview.chromium.org/15061007/diff/32001/chrome/browser/policy/pol...
chrome/browser/policy/policy_domain_descriptor_unittest.cc:68: PolicyMap& map =
expected_bundle.Get(extension_ns);
On 2013/05/21 12:10:56, bartfab wrote:
> #include "chrome/browser/policy/policy_map.h"

Done.

[James Hawkins, 2013-05-21 18:25:10]

lgtm

[bartfab (slow), 2013-05-22 09:23:21]

lgtm

[commit-bot: I haz the power, 2013-05-23 08:01:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/15061007/110001

[commit-bot: I haz the power, 2013-05-23 12:46:36]

Change committed as 201765