Introduced a ForwardingPolicyProvider.

Introduced a ForwardingPolicyProvider.

This policy provider forwards its methods to a delegate policy provider, but
keeps its own state of initialization for each policy domain. This allows the
Profile PolicyService to have a ForwardingPolicyProvider instead of directly
reading from the platform provider, so that each Profile can be in a different
initialization state.

This means that the IsInitializationComplete() signal is now reliable and
never goes from "ready" to "not ready".

Also introduced a readiness signal for each domain at the SchemaRegistry.
This allows policy providers to determine when to reload based on the
availability of schemas for each component that supports policy.

All of that together means that the ManagedValueStoreCache (which implements
the API that allows extensions to read policy) can be greatly simplified, by
simply waiting for the IsInitializationComplete() signal before starting to
read policy values.

The SchemaRegistry updates also enable a similar simplification in the
ComponentCloudPolicyService, which will come later.

BUG=270667, 108992, 171477
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235005

[Joao da Silva, 2013-11-09 21:37:55]

Please have a look. This uses the recent refactorings in the policy code to
significantly simplify the implementation of the storage.managed API; note that
the current browser_tests and test extensions still work with a much simpler
implementation.

@Benjamin: extensions/.  This is the last change to that code and depends on
https://codereview.chromium.org/56623005/

@Bartosz: policy/.  The next change after this one is to similarly simplify the
cloud policy for extensions code, and enable it by default on M33 (with a flag).
There are also some cleanups to do on the Windows policy provider, and then this
stuff should Finally Be Ready :-)

[bartfab (slow), 2013-11-11 14:40:08]

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

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:18: #include
"chrome/browser/extensions/extension_system.h"
Nit: No longer used.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:215: //
POLICY_DOMAIN_EXTENSIONS, and OnPolicyServiceInitialized() will be invoked.
Nit: Maybe add a sentence that explains any further SetReady() calls are no-ops.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:283: const
policy::ComponentMap* map = registry->schema_map()->GetComponents(
Nit: #include "chrome/browser/policy/schema_map.h"

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:291:
policy::PolicyNamespace ns(policy::POLICY_DOMAIN_EXTENSIONS, it->first);
Nit: Const - or just create it inline.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:305: //
OnPolicyUpdated is updated whenever a policy changes, but it doesn't
Nit: s/updated/called/

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store.h (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store.h:36: void
SetCurrentPolicy(const policy::PolicyMap& policy);
Nit: |notify_if_changed| no longer exists but the method still does send
noifications of there are changes, so that part of the comment still makes
sense.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store_unittest.cc
(right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:172:
changes.push_back(ValueStoreChange("ccc", NULL, value.DeepCopy()));
Nit: I guess this could be "bbb" now since you snipped away the old "bbb"...

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:207: //
Don't notify when there aren't changes.
Nit: s/arent't/aren't any/

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
File chrome/browser/policy/forwarding_policy_provider.cc (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider.cc:25: if (registry->IsReady())
Nit: #include "chrome/browser/policy/schema_registry.h"

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider.cc:81: PolicyNamespace
chrome_ns(POLICY_DOMAIN_CHROME, "");
Nit: const - or simply construct it inline.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
File chrome/browser/policy/forwarding_policy_provider.h (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider.h:28: // Note that Init(),
Shutdown(), OnSchemaRegistryUpdated() and
I think it would be clearer to list the calls that *are* forwarded to the
|delegate_|: RefreshPolicies() and IsInitializationComplete() for the Chrome
domain.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
File chrome/browser/policy/forwarding_policy_provider_unittest.cc (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:7: #include
"base/basictypes.h"
Nit: Already included by header file.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:25: class
ForwardingPolicyProviderTest : public testing::Test {
Nit: The style guide forbids inline method definitions, except for "trivial or
performance-critical, and very short, methods." It is a common misconception
that some kind of exception to this rule applies in tests.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:62: PolicyNamespace
chrome_ns(POLICY_DOMAIN_CHROME, "");
Nit: const - or simply construct it inline.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:67:
scoped_ptr<PolicyBundle> copy(new PolicyBundle);
Nit 1: I would not call this |copy| because its main function is not to be an
identical copy but a policy bundle that gets sent to the provider. It is
specifically *not* an identical copy of |bundle| once you add the component
policy.

Nit 2: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:109:
schema_registry_.SetReady(POLICY_DOMAIN_CHROME);
Nit: You could additionally verify that this SetReady() alone does not cause a
RefreshPolicies().

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:165: // Keeps
forwarding from now on.
Nit: No need for "from now on" - the forwarding was happening before already.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/mo...
File chrome/browser/policy/mock_configuration_policy_provider.cc (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/mo...
chrome/browser/policy/mock_configuration_policy_provider.cc:38:
scoped_ptr<PolicyBundle> bundle(new PolicyBundle());
Nit: No need for ().

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/sc...
File chrome/browser/policy/schema_registry_unittest.cc (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_unittest.cc:112:
registry.SetReady(POLICY_DOMAIN_EXTENSIONS);
Nit 1: It would be useful to have an observer in this test, verifying not only
that IsReady() does not misbehave when the same domain becomes ready but also
OnSchemaRegistryReady() does not fire too often.

Nit 2: Why not use POLICY_DOMAIN_CHROME as the domain that you mark as ready
twice? That way, marking a component as ready twice would get tested even if
ENABLE_EXTENSIONS is not defined.

[Joao da Silva, 2013-11-12 15:26:33]

@Bartosz: PTAL

@Benjamin: ping :-)

All the dependencies are in, so this CL is ready to go after getting reviewed.
Thanks!

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

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:18: #include
"chrome/browser/extensions/extension_system.h"
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: No longer used.

It is, see lines 112 and 120

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:215: //
POLICY_DOMAIN_EXTENSIONS, and OnPolicyServiceInitialized() will be invoked.
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: Maybe add a sentence that explains any further SetReady() calls are
no-ops.

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:283: const
policy::ComponentMap* map = registry->schema_map()->GetComponents(
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_map.h"

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:291:
policy::PolicyNamespace ns(policy::POLICY_DOMAIN_EXTENSIONS, it->first);
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: Const - or just create it inline.

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:305: //
OnPolicyUpdated is updated whenever a policy changes, but it doesn't
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: s/updated/called/

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store.h (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store.h:36: void
SetCurrentPolicy(const policy::PolicyMap& policy);
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: |notify_if_changed| no longer exists but the method still does send
> noifications of there are changes, so that part of the comment still makes
> sense.

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/policy_value_store_unittest.cc
(right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:172:
changes.push_back(ValueStoreChange("ccc", NULL, value.DeepCopy()));
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: I guess this could be "bbb" now since you snipped away the old "bbb"...

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/extension...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:207: //
Don't notify when there aren't changes.
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: s/arent't/aren't any/

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
File chrome/browser/policy/forwarding_policy_provider.cc (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider.cc:25: if (registry->IsReady())
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: #include "chrome/browser/policy/schema_registry.h"

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider.cc:81: PolicyNamespace
chrome_ns(POLICY_DOMAIN_CHROME, "");
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: const - or simply construct it inline.

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
File chrome/browser/policy/forwarding_policy_provider.h (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider.h:28: // Note that Init(),
Shutdown(), OnSchemaRegistryUpdated() and
On 2013/11/11 14:40:08, bartfab wrote:
> I think it would be clearer to list the calls that *are* forwarded to the
> |delegate_|: RefreshPolicies() and IsInitializationComplete() for the Chrome
> domain.

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
File chrome/browser/policy/forwarding_policy_provider_unittest.cc (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:7: #include
"base/basictypes.h"
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: Already included by header file.

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:25: class
ForwardingPolicyProviderTest : public testing::Test {
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: The style guide forbids inline method definitions, except for "trivial or
> performance-critical, and very short, methods." It is a common misconception
> that some kind of exception to this rule applies in tests.

There is an exception for tests. I dug the latest thread where this topic was
discussed:

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/X8D176Uzp6w/dis...

The conclusion was added to a companion doc of the style guide:

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-What-ab...

"For testing code, most of these factors are reduced or eliminated.  Test
framework classes don't tend to be instantiated separately and passed around as
objects; they're effectively just bundles of file-scope functionality coupled
with a mechanism to reset state between tests.  In these cases, defining the
test functions inline at their declaration sites has little negative effect and
can reduce the amount of "boilerplate" in the test file.  Ultimately, test file
inlining should be a readability-focused judgment call on the part of the test
author and reviewer."

I'm on the side that finds inlining in tests more readable, so I'd prefer to
keep this. I don't feel strongly though, if you prefer then I'll change it.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:62: PolicyNamespace
chrome_ns(POLICY_DOMAIN_CHROME, "");
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: const - or simply construct it inline.

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:67:
scoped_ptr<PolicyBundle> copy(new PolicyBundle);
On 2013/11/11 14:40:08, bartfab wrote:
> Nit 1: I would not call this |copy| because its main function is not to be an
> identical copy but a policy bundle that gets sent to the provider. It is
> specifically *not* an identical copy of |bundle| once you add the component
> policy.
> 
> Nit 2: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:109:
schema_registry_.SetReady(POLICY_DOMAIN_CHROME);
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: You could additionally verify that this SetReady() alone does not cause a
> RefreshPolicies().

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/fo...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:165: // Keeps
forwarding from now on.
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: No need for "from now on" - the forwarding was happening before already.

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/mo...
File chrome/browser/policy/mock_configuration_policy_provider.cc (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/mo...
chrome/browser/policy/mock_configuration_policy_provider.cc:38:
scoped_ptr<PolicyBundle> bundle(new PolicyBundle());
On 2013/11/11 14:40:08, bartfab wrote:
> Nit: No need for ().

Done.

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/sc...
File chrome/browser/policy/schema_registry_unittest.cc (right):

https://codereview.chromium.org/60823003/diff/690001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_unittest.cc:112:
registry.SetReady(POLICY_DOMAIN_EXTENSIONS);
On 2013/11/11 14:40:08, bartfab wrote:
> Nit 1: It would be useful to have an observer in this test, verifying not only
> that IsReady() does not misbehave when the same domain becomes ready but also
> OnSchemaRegistryReady() does not fire too often.
> 
> Nit 2: Why not use POLICY_DOMAIN_CHROME as the domain that you mark as ready
> twice? That way, marking a component as ready twice would get tested even if
> ENABLE_EXTENSIONS is not defined.

Done.

[not at google - send to devlin, 2013-11-13 00:30:17]

lgtm, nice cleanup indeed.

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

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:277:
policy::PolicyDomain domain) {
for consistency with this file: add a BrowserThread::UI assertion.

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:297:
OnPolicyUpdated(ns, empty_map, policy_service_->GetPolicies(ns));
I don't know what a "component" is here, but just checking it's not a problem
that you're going to be posting a bunch of stuff to the FILE thread like this,
when it could be done all at once.

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:307:
policy::POLICY_DOMAIN_EXTENSIONS)) {
Why not check that ns != POLICY_DOMAIN_EXTENSIONS instead?

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/policy_value_store_unittest.cc
(right):

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:171:
changes.clear();
I prefer scoping these sorts of thing explicitly using like

{
  ValueStoreChangeList changes;
  ... etc
}

{
  ValueStoreChaneList changes;
}

but I feel very weakly about it.

[Joao da Silva, 2013-11-13 08:56:47]

@Bartosz: ping :-)

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

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:277:
policy::PolicyDomain domain) {
On 2013/11/13 00:30:18, kalman wrote:
> for consistency with this file: add a BrowserThread::UI assertion.

Done.

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:297:
OnPolicyUpdated(ns, empty_map, policy_service_->GetPolicies(ns));
On 2013/11/13 00:30:18, kalman wrote:
> I don't know what a "component" is here, but just checking it's not a problem
> that you're going to be posting a bunch of stuff to the FILE thread like this,
> when it could be done all at once.

This for() loop is iterating over the list of extensions that declare a
managed-storage-schema in their manifest. For non-managed installations this is
most likely 0, in an enterprise that loves managed extensions it could be around
10 or so.

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:307:
policy::POLICY_DOMAIN_EXTENSIONS)) {
On 2013/11/13 00:30:18, kalman wrote:
> Why not check that ns != POLICY_DOMAIN_EXTENSIONS instead?

On line 234 this starts observing updates to POLICY_DOMAIN_EXTENSIONS, so
|ns.domain| is always going to have that value.

This check is meant to skip OnPolicyUpdated() notifications that come before all
the policy providers are ready (e.g. if a provider loads policy for an extension
an publishes it, but before a 2nd provider is ready). Once all of them are ready
then the policies are merged, and some providers may override the others.

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/policy_value_store_unittest.cc
(right):

https://codereview.chromium.org/60823003/diff/1080001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:171:
changes.clear();
On 2013/11/13 00:30:18, kalman wrote:
> I prefer scoping these sorts of thing explicitly using like
> 
> {
>   ValueStoreChangeList changes;
>   ... etc
> }
> 
> {
>   ValueStoreChaneList changes;
> }
> 
> but I feel very weakly about it.

Done.

[bartfab (slow), 2013-11-13 11:56:34]

lgtm

https://chromiumcodereview.appspot.com/60823003/diff/690001/chrome/browser/po...
File chrome/browser/policy/forwarding_policy_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/60823003/diff/690001/chrome/browser/po...
chrome/browser/policy/forwarding_policy_provider_unittest.cc:25: class
ForwardingPolicyProviderTest : public testing::Test {
On 2013/11/12 15:26:33, Joao da Silva wrote:
> On 2013/11/11 14:40:08, bartfab wrote:
> > Nit: The style guide forbids inline method definitions, except for "trivial
or
> > performance-critical, and very short, methods." It is a common misconception
> > that some kind of exception to this rule applies in tests.
> 
> There is an exception for tests. I dug the latest thread where this topic was
> discussed:
> 
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/X8D176Uzp6w/dis...
> 
> The conclusion was added to a companion doc of the style guide:
> 
>
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-What-ab...
> 
> "For testing code, most of these factors are reduced or eliminated.  Test
> framework classes don't tend to be instantiated separately and passed around
as
> objects; they're effectively just bundles of file-scope functionality coupled
> with a mechanism to reset state between tests.  In these cases, defining the
> test functions inline at their declaration sites has little negative effect
and
> can reduce the amount of "boilerplate" in the test file.  Ultimately, test
file
> inlining should be a readability-focused judgment call on the part of the test
> author and reviewer."
> 
> I'm on the side that finds inlining in tests more readable, so I'd prefer to
> keep this. I don't feel strongly though, if you prefer then I'll change it.

Interesting. I had followed a different thread that concluded there was no valid
exception :). But I do not feel strongly either way.

https://chromiumcodereview.appspot.com/60823003/diff/1300001/chrome/browser/e...
File chrome/browser/extensions/api/storage/policy_value_store_unittest.cc
(right):

https://chromiumcodereview.appspot.com/60823003/diff/1300001/chrome/browser/e...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:157:
base::StringValue value("111");
Nit: const.

https://chromiumcodereview.appspot.com/60823003/diff/1300001/chrome/browser/e...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:191:
base::StringValue new_value("222");
Nit: const.

https://chromiumcodereview.appspot.com/60823003/diff/1300001/chrome/browser/p...
File chrome/browser/policy/forwarding_policy_provider.h (right):

https://chromiumcodereview.appspot.com/60823003/diff/1300001/chrome/browser/p...
chrome/browser/policy/forwarding_policy_provider.h:41: // except
POLICY_DOMAIN_CHROME, which is always forwarded to the |delegate_|.
Nit: I think something implying status pulling, not pushing, would be clearer -
i.e. replace "which is always forwarded to" with "whose status is always queried
from."

[Joao da Silva, 2013-11-13 17:30:48]

@Bartosz: addressed the previous comments. The latest patch set has a fix for a
crash on Android, and the profile_policy_connector also had to be rebased on a
refactoring from Philipp.

Please have one last look :-) Thanks!

https://codereview.chromium.org/60823003/diff/1300001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/policy_value_store_unittest.cc
(right):

https://codereview.chromium.org/60823003/diff/1300001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:157:
base::StringValue value("111");
On 2013/11/13 11:56:35, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/60823003/diff/1300001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc:191:
base::StringValue new_value("222");
On 2013/11/13 11:56:35, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/60823003/diff/1300001/chrome/browser/policy/f...
File chrome/browser/policy/forwarding_policy_provider.h (right):

https://codereview.chromium.org/60823003/diff/1300001/chrome/browser/policy/f...
chrome/browser/policy/forwarding_policy_provider.h:41: // except
POLICY_DOMAIN_CHROME, which is always forwarded to the |delegate_|.
On 2013/11/13 11:56:35, bartfab wrote:
> Nit: I think something implying status pulling, not pushing, would be clearer
-
> i.e. replace "which is always forwarded to" with "whose status is always
queried
> from."

Done.

https://codereview.chromium.org/60823003/diff/1480001/chrome/browser/policy/p...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://codereview.chromium.org/60823003/diff/1480001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector.cc:54: if
(connector->GetPlatformProvider()) {
This was the fix the Android, because it doesn't have a platform provider

[bartfab (slow), 2013-11-13 18:23:34]

lgtm

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
File chrome/browser/policy/profile_policy_connector.h (right):

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector.h:63: #endif
Nit: Add // defined(OS_CHROMEOS) to make it clearer which #if is ending here.

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector.h:66: #endif
Nit: Add // defined(ENABLE_CONFIGURATION_POLICY) to make it clearer which #if is
ending here.

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
File chrome/browser/policy/profile_policy_connector_factory.cc (right):

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector_factory.cc:108: #endif
Nit: Add // defined(OS_CHROMEOS) to make it clearer which #if is ending here.

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector_factory.cc:112: #endif
Nit: Add // defined(ENABLE_CONFIGURATION_POLICY) to make it clearer which #if is
ending here.

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

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

[commit-bot: I haz the power, 2013-11-14 01:23:45]

Change committed as 235005

[Joao da Silva, 2013-11-14 08:23:41]

Forgot to publish these drafts. Thanks for the reviews!

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
File chrome/browser/policy/profile_policy_connector.h (right):

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector.h:63: #endif
On 2013/11/13 18:23:35, bartfab wrote:
> Nit: Add // defined(OS_CHROMEOS) to make it clearer which #if is ending here.

Done.

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector.h:66: #endif
On 2013/11/13 18:23:35, bartfab wrote:
> Nit: Add // defined(ENABLE_CONFIGURATION_POLICY) to make it clearer which #if
is
> ending here.

Done.

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
File chrome/browser/policy/profile_policy_connector_factory.cc (right):

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector_factory.cc:108: #endif
On 2013/11/13 18:23:35, bartfab wrote:
> Nit: Add // defined(OS_CHROMEOS) to make it clearer which #if is ending here.

Done.

https://codereview.chromium.org/60823003/diff/1590001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector_factory.cc:112: #endif
On 2013/11/13 18:23:35, bartfab wrote:
> Nit: Add // defined(ENABLE_CONFIGURATION_POLICY) to make it clearer which #if
is
> ending here.

Done.