Policy providers all get a SchemaRegistry to work with.

Policy providers all get a SchemaRegistry to work with.

The SchemaRegistry contains the components that the policy provider should load
policy for, as well as the expected policies and their types for each component.

Also killed the PolicyDomainDescriptor, which was a similar way of doing this
but had the problem of overlapping descriptors on shared policy providers.

BUG=270667
R=bartfab@chromium.org, jochen@chromium.org, kalman@chromium.org

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

[Joao da Silva, 2013-11-04 14:52:16]

Please review. This fixes the problem and ties up all the other changes, and
removes dead code.

[Joao da Silva, 2013-11-04 19:32:59]

+cc Daniel

[bartfab (slow), 2013-11-05 15:53:04]

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:142:
SchemaRegistry schema_registry_;
Nit: #include "chrome/browser/policy/schema_registry.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:779:
SchemaRegistry schema_registry_;
Nit: #include "chrome/browser/policy/schema_registry.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/proxy_policy_provider.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/proxy_policy_provider.cc:62: // Ignore, and don't
call RefreshPolicies. Policies only have to be refreshes
Nit: s/refreshes/refreshed/

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/proxy_policy_provider_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/proxy_policy_provider_unittest.cc:31:
SchemaRegistry schema_registry_;
#include "chrome/browser/policy/schema_registry.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:160: //
component that have been removed can be dropped from the cache.
Nit 1: s/component/components/
Nit 2: s/drooped/dropped/

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:39: using
testing::AnyNumber;
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:35:
using testing::AnyNumber;
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:22: #include
"chrome/browser/policy/schema_registry_service.h"
Nit: As commented elsewhere, in line with your usual include style, you should
leave this one out because you never dereference the type.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:81:
policy::SchemaRegistry* schema_registry_;
Nit: #include "chrome/browser/policy/schema_registry.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:114:
schema_registry_->UnregisterComponent(policy::PolicyNamespace(
Nit: #include "components/policy/core/common/policy_namespace.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:120:
scoped_ptr<ExtensionSet> set(new ExtensionSet());
Nit: No need for ().

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:167:
policy::ComponentMap* components = new policy::ComponentMap();
Nit 1: No need for ().
Nit 2: Use a scoped_ptr.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_apitest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:32: #include
"chrome/browser/policy/policy_service.h"
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:33: #include
"chrome/browser/policy/profile_policy_connector.h"
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:34: #include
"chrome/browser/policy/profile_policy_connector_factory.h"
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:36: #include
"chrome/browser/policy/schema_registry_service.h"
Nit: As commented elsewhere, in line with your usual include style, you should
leave this one out because you never dereference the type.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:49: using
testing::AnyNumber;
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:107: class
MockSchemaRegistryObserver : public policy::SchemaRegistry::Observer {
Nit: #include "chrome/browser/policy/schema_registry.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:497:
registry->schema_map()->GetSchema(policy::PolicyNamespace(
Nit: #include "components/policy/core/common/policy_namespace.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:502:
ASSERT_EQ(base::Value::TYPE_DICTIONARY, schema->type());
Nit: #include "base/values.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
File chrome/browser/policy/async_policy_loader.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
chrome/browser/policy/async_policy_loader.h:8: #include <map>
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
chrome/browser/policy/async_policy_loader.h:16: #include
"components/policy/core/common/policy_namespace.h"
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
File chrome/browser/policy/async_policy_provider.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
chrome/browser/policy/async_policy_provider.h:31: // The AsyncPolicyProvider
does a synchronous load on its constructor, and
Nit: s/on/in/

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
File chrome/browser/policy/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
chrome/browser/policy/async_policy_provider_unittest.cc:87: SchemaRegistry
schema_registry_;
Nit: #include "chrome/browser/policy/schema_registry.h

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc:201: SchemaRegistry
schema_registry_;
#include "chrome/browser/policy/schema_registry.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_service.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:174: // No schemas
have been registered yet, so keep serving the initial policy.
Is the initial policy just blank? Could you document that here?

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:221:
scoped_ptr<PolicyNamespaceKeys> keys(new PolicyNamespaceKeys());
No: No need for ().

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:232:
keys->insert(std::make_pair(it->first, id));
Nit: #include <utility>

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:348: if (client_
&& is_initialized()) {
Why the nested conditional?

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_service.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.h:9: #include
<string>
Nit: No longer used (move to implementation file, where it is used).

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.h:113: typedef
std::set<PolicyNamespaceKey> PolicyNamespaceKeys;
Nit: #include "chrome/browser/policy/cloud/cloud_policy_constants.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_store.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_store.h:92: // Deletes the
storage of all components of |domain| that pass then given
It is kind of weird that those which pass the test get deleted. As commented in
resource_cache.h itself, I think renaming the clean-up method and/or its
argument and/or its type will help to make this more understandable.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_store_unittest.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_store_unittest.cc:51: bool
NotEquals(const std::string& expected, const std::string& key) {
Nit: s/Equals/Equal/ (x.Equals(y) reads like proper English, x.NotEquals(y) does
not but x.NotEqual(y) sounds fine again)

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_store_unittest.cc:307:
store_->Purge(POLICY_DOMAIN_EXTENSIONS, base::Bind(&NotEquals, "obsolete"));
The fact that "obsolete" does not match any of the extension ids is a bit hack.
Maybe better add another predicate method for this test which simply returns
true?

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_provider.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.cc:85: // Unit tests don't
initialize the browser_policy_connector but call
Written with underscores like this, browser_policy_connector clearly refers to
symbol. But there is insufficient context to figure out what symbol is meant
exactly. Is this a member named browser_policy_connector? Or a getter? Which
class does that member/symbol belong to? I think it would be better to refer to
the human-readable concept here ("browser policy connector," without
underscores) rather than a specific symbol name.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.cc:98: const
scoped_refptr<SchemaMap>& current_map,
General question: Does anyone ever use the current_map argument of
OnSchemaRegistryUpdated? If it is generally discarded, would it make sense to
remove this argument and have everyone call schema_map() if needed?

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_provider.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.h:43: // updated.
* "whenever the registry issues updated"? This statement does not compute. Is it
missing a few words maybe?

* It used to be that a ConfigurationPolicyProvider could ignore any parts of the
PolicyDomainDescriptor that were not applicable to it. Is this true of the
SchemaRegistry as well? If so, that should be documented.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.h:73: // The base
implementation just calls RefreshPolicies.
Nit: Maybe better s/The base implementation/This base implementation/ to make it
clear which implementation exactly you are talking about?

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.h:75: const
scoped_refptr<SchemaMap>& current_map,
Nit: SchemaMap:
* Forward-declare here.
* #include "chrome/browser/policy/schema_map.h" in implementation.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_provider_test.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider_test.cc:144:
schema_registry_.RegisterComponent(PolicyNamespace(POLICY_DOMAIN_CHROME, ""),
Nit: #include "components/policy/core/common/policy_namespace.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/mo...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/mo...
chrome/browser/policy/mock_configuration_policy_provider.h:45:
DISALLOW_COPY_AND_ASSIGN(MockConfigurationPolicyProvider);
Nit: #include "base/basictypes.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:155: using testing::AnyNumber;
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_mac.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.cc:184: const ComponentMap* components =
schema_map()->GetComponents(domain);
Nit: #include "chrome/browser/policy/schema_map.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.cc:201: // TODO(joaodasilva): extensions
may be registered in a ComponentMap
Nit: Capitalize start of sentence.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_mac.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.h:55: // Loads policies for the
components described in the current SchemaMap,
Nit 1: s/SchemaMap/schema_map_/ (reference the member, not its type).
Nit 2: The comma is wrong. It should not be here.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.h:58: PolicyDomain domain,
Nit: #include "components/policy/core/common/policy_namespace.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.h:60: PolicyBundle* bundle);
Nit: Forward-declare PolicyBundle.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_prefs_browsertest.cc:40: using testing::AnyNumber;
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_service.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_service.h:13: #include "base/memory/ref_counted.h"
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_service_impl_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_service_impl_unittest.cc:10: #include
"base/memory/ref_counted.h"
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_service_impl_unittest.cc:17: #include
"components/policy/core/common/schema.h"
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/prefs/syn...
File chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/prefs/syn...
chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc:33: using
testing::AnyNumber;
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:60: using
testing::AnyNumber;
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/certificate_manager_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/certificate_manager_browsertest.cc:30: using
testing::AnyNumber;
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/preferences_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/preferences_browsertest.cc:56: using
testing::AnyNumber;
Nit: No longer used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/policy_ui.cc:36: #include
"chrome/browser/policy/schema_registry_service.h"
Nit: Not used (it appears as a return type in
SchemaRegistryServiceFactory::GetForContext() but to be consistent with
yourself, you should not be including a header for a type that you never
dereference).

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/policy_ui.cc:567: policy::SchemaRegistry* registry =
Nit: #include "chrome/browser/policy/schema_registry.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/policy_ui.cc:583: policy::POLICY_DOMAIN_EXTENSIONS,
extension->id()));
Nit: #include "components/policy/core/common/policy_namespace.h"

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/policy_ui_browsertest.cc:29: using testing::AnyNumber;
Nit: No longer used.

[Joao da Silva, 2013-11-07 13:14:59]

@Bartfab: PTAL

@Jochen: this removes a line of dead code in some tests all over the place, and
you stamp those changes?

@Benjamin: please review extensions/. I'm working on improving the
implementation of managed_value_store_cache to make it way simpler to
understand, more changes coming soon :-)

Thanks!

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:142:
SchemaRegistry schema_registry_;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_registry.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:779:
SchemaRegistry schema_registry_;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_registry.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/proxy_policy_provider.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/proxy_policy_provider.cc:62: // Ignore, and don't
call RefreshPolicies. Policies only have to be refreshes
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: s/refreshes/refreshed/

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/proxy_policy_provider_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/proxy_policy_provider_unittest.cc:31:
SchemaRegistry schema_registry_;
On 2013/11/05 15:53:04, bartfab wrote:
> #include "chrome/browser/policy/schema_registry.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:160: //
component that have been removed can be dropped from the cache.
On 2013/11/05 15:53:04, bartfab wrote:
> Nit 1: s/component/components/

Done

> Nit 2: s/drooped/dropped/

?

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/chromeos/...
chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:39: using
testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:35:
using testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:22: #include
"chrome/browser/policy/schema_registry_service.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: As commented elsewhere, in line with your usual include style, you should
> leave this one out because you never dereference the type.

It's needed so that this unit sees that a SchemaRegistryService is a
SchemaRegistry, which is used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:81:
policy::SchemaRegistry* schema_registry_;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_registry.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:114:
schema_registry_->UnregisterComponent(policy::PolicyNamespace(
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_namespace.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:120:
scoped_ptr<ExtensionSet> set(new ExtensionSet());
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No need for ().

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:167:
policy::ComponentMap* components = new policy::ComponentMap();
On 2013/11/05 15:53:04, bartfab wrote:
> Nit 1: No need for ().
> Nit 2: Use a scoped_ptr.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_apitest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:32: #include
"chrome/browser/policy/policy_service.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:33: #include
"chrome/browser/policy/profile_policy_connector.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:34: #include
"chrome/browser/policy/profile_policy_connector_factory.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:36: #include
"chrome/browser/policy/schema_registry_service.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: As commented elsewhere, in line with your usual include style, you should
> leave this one out because you never dereference the type.

Same as before, it needs to see it's derived from SchemaRegistry.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:49: using
testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:107: class
MockSchemaRegistryObserver : public policy::SchemaRegistry::Observer {
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_registry.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:497:
registry->schema_map()->GetSchema(policy::PolicyNamespace(
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_namespace.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_apitest.cc:502:
ASSERT_EQ(base::Value::TYPE_DICTIONARY, schema->type());
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "base/values.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
File chrome/browser/policy/async_policy_loader.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
chrome/browser/policy/async_policy_loader.h:8: #include <map>
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
chrome/browser/policy/async_policy_loader.h:16: #include
"components/policy/core/common/policy_namespace.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
File chrome/browser/policy/async_policy_provider.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
chrome/browser/policy/async_policy_provider.h:31: // The AsyncPolicyProvider
does a synchronous load on its constructor, and
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: s/on/in/

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
File chrome/browser/policy/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/as...
chrome/browser/policy/async_policy_provider_unittest.cc:87: SchemaRegistry
schema_registry_;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_registry.h

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc:201: SchemaRegistry
schema_registry_;
On 2013/11/05 15:53:04, bartfab wrote:
> #include "chrome/browser/policy/schema_registry.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_service.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:174: // No schemas
have been registered yet, so keep serving the initial policy.
On 2013/11/05 15:53:04, bartfab wrote:
> Is the initial policy just blank? Could you document that here?

Done. It's not blank, it's loaded synchronously in FinalizeInit.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:221:
scoped_ptr<PolicyNamespaceKeys> keys(new PolicyNamespaceKeys());
On 2013/11/05 15:53:04, bartfab wrote:
> No: No need for ().

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:232:
keys->insert(std::make_pair(it->first, id));
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include <utility>

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:348: if (client_
&& is_initialized()) {
On 2013/11/05 15:53:04, bartfab wrote:
> Why the nested conditional?

No reason for it, simplified.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_service.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.h:9: #include
<string>
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used (move to implementation file, where it is used).

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.h:113: typedef
std::set<PolicyNamespaceKey> PolicyNamespaceKeys;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "chrome/browser/policy/cloud/cloud_policy_constants.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_store.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_store.h:92: // Deletes the
storage of all components of |domain| that pass then given
On 2013/11/05 15:53:04, bartfab wrote:
> It is kind of weird that those which pass the test get deleted. As commented
in
> resource_cache.h itself, I think renaming the clean-up method and/or its
> argument and/or its type will help to make this more understandable.

It has been renamed to SubkeyFilter.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_store_unittest.cc
(right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_store_unittest.cc:51: bool
NotEquals(const std::string& expected, const std::string& key) {
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: s/Equals/Equal/ (x.Equals(y) reads like proper English, x.NotEquals(y)
does
> not but x.NotEqual(y) sounds fine again)

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_store_unittest.cc:307:
store_->Purge(POLICY_DOMAIN_EXTENSIONS, base::Bind(&NotEquals, "obsolete"));
On 2013/11/05 15:53:04, bartfab wrote:
> The fact that "obsolete" does not match any of the extension ids is a bit
hack.
> Maybe better add another predicate method for this test which simply returns
> true?

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_provider.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.cc:85: // Unit tests don't
initialize the browser_policy_connector but call
On 2013/11/05 15:53:04, bartfab wrote:
> Written with underscores like this, browser_policy_connector clearly refers to
> symbol. But there is insufficient context to figure out what symbol is meant
> exactly. Is this a member named browser_policy_connector? Or a getter? Which
> class does that member/symbol belong to? I think it would be better to refer
to
> the human-readable concept here ("browser policy connector," without
> underscores) rather than a specific symbol name.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.cc:98: const
scoped_refptr<SchemaMap>& current_map,
On 2013/11/05 15:53:04, bartfab wrote:
> General question: Does anyone ever use the current_map argument of
> OnSchemaRegistryUpdated? If it is generally discarded, would it make sense to
> remove this argument and have everyone call schema_map() if needed?

That's right, removed.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_provider.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.h:43: // updated.
On 2013/11/05 15:53:04, bartfab wrote:
> * "whenever the registry issues updated"? This statement does not compute. Is
it
> missing a few words maybe?
> 
> * It used to be that a ConfigurationPolicyProvider could ignore any parts of
the
> PolicyDomainDescriptor that were not applicable to it. Is this true of the
> SchemaRegistry as well? If so, that should be documented.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.h:73: // The base
implementation just calls RefreshPolicies.
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: Maybe better s/The base implementation/This base implementation/ to make
it
> clear which implementation exactly you are talking about?

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.h:75: const
scoped_refptr<SchemaMap>& current_map,
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: SchemaMap:
> * Forward-declare here.
> * #include "chrome/browser/policy/schema_map.h" in implementation.

It's part of the interface that this is overriding, so it must already be
declared; and in the implementation it's not actually used.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_provider_test.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider_test.cc:144:
schema_registry_.RegisterComponent(PolicyNamespace(POLICY_DOMAIN_CHROME, ""),
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_namespace.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/mo...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/mo...
chrome/browser/policy/mock_configuration_policy_provider.h:45:
DISALLOW_COPY_AND_ASSIGN(MockConfigurationPolicyProvider);
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "base/basictypes.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_browsertest.cc:155: using testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_mac.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.cc:184: const ComponentMap* components =
schema_map()->GetComponents(domain);
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_map.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.cc:201: // TODO(joaodasilva): extensions
may be registered in a ComponentMap
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: Capitalize start of sentence.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_mac.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.h:55: // Loads policies for the
components described in the current SchemaMap,
On 2013/11/05 15:53:04, bartfab wrote:
> Nit 1: s/SchemaMap/schema_map_/ (reference the member, not its type).
> Nit 2: The comma is wrong. It should not be here.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.h:58: PolicyDomain domain,
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_namespace.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.h:60: PolicyBundle* bundle);
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: Forward-declare PolicyBundle.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_prefs_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_prefs_browsertest.cc:40: using testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_service.h (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_service.h:13: #include "base/memory/ref_counted.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
File chrome/browser/policy/policy_service_impl_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_service_impl_unittest.cc:10: #include
"base/memory/ref_counted.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/policy/po...
chrome/browser/policy/policy_service_impl_unittest.cc:17: #include
"components/policy/core/common/schema.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/prefs/syn...
File chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/prefs/syn...
chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc:33: using
testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:60: using
testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/certificate_manager_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/certificate_manager_browsertest.cc:30: using
testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/preferences_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/preferences_browsertest.cc:56: using
testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/policy_ui.cc:36: #include
"chrome/browser/policy/schema_registry_service.h"
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: Not used (it appears as a return type in
> SchemaRegistryServiceFactory::GetForContext() but to be consistent with
> yourself, you should not be including a header for a type that you never
> dereference).

It's needed to see that it's  derived from SchemaRegistry.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/policy_ui.cc:567: policy::SchemaRegistry* registry =
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_registry.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/policy_ui.cc:583: policy::POLICY_DOMAIN_EXTENSIONS,
extension->id()));
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_namespace.h"

Done.

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/56623005/diff/140002/chrome/browser/ui/webui/...
chrome/browser/ui/webui/policy_ui_browsertest.cc:29: using testing::AnyNumber;
On 2013/11/05 15:53:04, bartfab wrote:
> Nit: No longer used.

Done.

[jochen (gone - plz use gerrit), 2013-11-07 13:40:14]

what part do you want me to review?

[Joao da Silva, 2013-11-07 13:43:14]

@Jochen: anything non-policy and non-extensions:

chrome/browser/chromeos/system
chrome/browser/prefs
chrome/browser/ui (except policy_ui.cc)


On Thu, Nov 7, 2013 at 2:40 PM, <jochen@chromium.org> wrote:

> what part do you want me to review?
>
> https://codereview.chromium.org/56623005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2013-11-08 11:49:57]

*browsertest.cc where you just delete stuff lgtm

[bartfab (slow), 2013-11-08 13:50:16]

lgtm

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_provider.h (right):

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.h:72: // This base
implementation just calls RefreshPolicies.
Nit: "[...] calls RefreshPolicies if |has_new_schemas| is true".

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_mac.cc (right):

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.cc:87: // TODO(joaodasilva): use the
Chrome schema instead of the
Nit: Capitalize start of sentence.

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_win_unittest.cc:807: ComponentMap
components;
Nit: #include "chrome/browser/policy/schema_map.h"

[Joao da Silva, 2013-11-08 14:18:46]

@Benjamin: friendly ping.

Note that I'm about to slash a lot of the ManagedValueStoreCache code soon, this
CL is a necessary intermediate state; it'll become much simpler after this :-)

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/co...
File chrome/browser/policy/configuration_policy_provider.h (right):

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/co...
chrome/browser/policy/configuration_policy_provider.h:72: // This base
implementation just calls RefreshPolicies.
On 2013/11/08 13:50:17, bartfab wrote:
> Nit: "[...] calls RefreshPolicies if |has_new_schemas| is true".

Done.

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_mac.cc (right):

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_mac.cc:87: // TODO(joaodasilva): use the
Chrome schema instead of the
On 2013/11/08 13:50:17, bartfab wrote:
> Nit: Capitalize start of sentence.

Done.

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/po...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/56623005/diff/850001/chrome/browser/policy/po...
chrome/browser/policy/policy_loader_win_unittest.cc:807: ComponentMap
components;
On 2013/11/08 13:50:17, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_map.h"

Done.

[not at google - send to devlin, 2013-11-09 23:52:42]

Sorry for taking so long. basically just nits + that OTR question which would be
easy to resolve, so lgtm.

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:109: if
(!ExtensionSystem::Get(profile_)->ready().is_signaled())
(how can this be false if we're getting extension load and unload events? bleh)

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:112: if (type
== chrome::NOTIFICATION_EXTENSION_UNLOADED) {
use switch. easier to read. I think you could make it look something like this
and it'd be a little less noisy-looking:

scoped_ptr<ExtensionSet> added(new ExtensionSet());
const Extension* removed = NULL;

switch (..) {
  case UNLOADED:
    removed = ..
    break
  case LOADED:
    added.add(...)
    break
   case READ:
    added.addall(...)
}

if (removed && IsEnterpriseExtension(removed)) {
  ...
  return;
}

for (set)
  ...
if (set)
  load

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:151: bool
ManagedValueStoreCache::ExtensionTracker::IsEnterpriseExtension(
"IsEnterpriseExtension" is a confusing name. It makes me think it's an extension
that was installed due to enterprise policy, which isn't true.

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:226:
extension_tracker_.reset(new ExtensionTracker(profile_));
Are we even creating one of these with an OTR profile in the first place? Check
that. I would expect that this object is owned by an ExtensionService which is
shared between main/OTR profiles, so this line of code is moot.

If not, ideally it would be like that.

[Joao da Silva, 2013-11-11 08:46:25]

Thanks for the reviews!

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:109: if
(!ExtensionSystem::Get(profile_)->ready().is_signaled())
On 2013/11/09 23:52:43, kalman wrote:
> (how can this be false if we're getting extension load and unload events?
bleh)

Left a comment explaining why this is here:

all the extensions of the Profile are loaded on startup and each triggers a
NOTIFICATION_EXTENSION_LOADED before the ExtensionSystem becomes ready (which
triggers NOTIFICATION_EXTENSIONS_READY). This check prevents repeated policy
reloads for each extension loaded on startup, and coalesces those updates into
at most one.

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:112: if (type
== chrome::NOTIFICATION_EXTENSION_UNLOADED) {
On 2013/11/09 23:52:43, kalman wrote:
> use switch. easier to read. I think you could make it look something like this
> and it'd be a little less noisy-looking:
> 
> scoped_ptr<ExtensionSet> added(new ExtensionSet());
> const Extension* removed = NULL;
> 
> switch (..) {
>   case UNLOADED:
>     removed = ..
>     break
>   case LOADED:
>     added.add(...)
>     break
>    case READ:
>     added.addall(...)
> }
> 
> if (removed && IsEnterpriseExtension(removed)) {
>   ...
>   return;
> }
> 
> for (set)
>   ...
> if (set)
>   load

Done.

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:151: bool
ManagedValueStoreCache::ExtensionTracker::IsEnterpriseExtension(
On 2013/11/09 23:52:43, kalman wrote:
> "IsEnterpriseExtension" is a confusing name. It makes me think it's an
extension
> that was installed due to enterprise policy, which isn't true.

Renamed to UsesManagedStorage.

https://codereview.chromium.org/56623005/diff/1410001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:226:
extension_tracker_.reset(new ExtensionTracker(profile_));
On 2013/11/09 23:52:43, kalman wrote:
> Are we even creating one of these with an OTR profile in the first place?
Check
> that. I would expect that this object is owned by an ExtensionService which is
> shared between main/OTR profiles, so this line of code is moot.
> 
> If not, ideally it would be like that.

That's right, removed this test.

[commit-bot: I haz the power, 2013-11-11 08:46:44]

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

[commit-bot: I haz the power, 2013-11-11 18:12:27]

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

[commit-bot: I haz the power, 2013-11-11 19:49:55]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-11 22:47:24]

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

[Joao da Silva, 2013-11-12 14:50:25]

Committed patchset #12 manually as r234514 (presubmit successful).