Refactored the ComponentCloudPolicyService.

Refactored the ComponentCloudPolicyService.

The ComponentCloudPolicyService is used to fetch cloud policy for components
of Chrome, such as extensions. This feature is currently disabled behind a
flag, on ChromeOS.

This refactor builds on recent changes to the policy code that make it possible
to significantly simplify this code:

- the list of components now comes from a SchemaRegistry
- all the dependencies come in the constructor (no more late Connect())
- cancel pending updates when components change

This is still a WIP; the next step is to make the ComponentCloudPolicyService
a PKS and decouple it from the UserCloudPolicyManagerChromeOS.

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

[Joao da Silva, 2013-11-12 22:32:37]

Note that component_cloud_policy_browsertest.cc didn't have any changes and
still works end-to-end with an extension using the API :-)

PTAL, thanks!

[bartfab (slow), 2013-11-14 14:13:12]

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:24:
#include "chrome/browser/policy/cloud/resource_cache.h"
Nit: No longer used.

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

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:76: // Invoked on
the background |task_runner_| to load the current store and
Nit 1: There is no need to mention that this is invoked via |task_runner_|. The
comment above the constructor already states that *all* methods may be invoked
via the |task_runner_| only.

Nit 2: s/store/store_/

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:77: // start
updating remote data.
Nit: "Start updating remote data" sounds like this class will be sending updates
to a remote place. It does the opposite of course, pulling updates from a remote
place. Something like "start downloading updates" would be clearer.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:108:
scoped_ptr<ComponentCloudPolicyStore> store_;
Nit: This can be a plain member now. No need for a scoped_ptr anymore.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:308: // Send the
current credentials to the backend; do this whenever the store
Nit: The comment is wrong. You do not send the credentials to the backend
whenever the store updates. You check for credential availability whenever the
store updates but you only send the credentials at most once.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:349: // When an
extension is reloaded or update it triggers an unregister quickly
Nit: s/update/updated, /

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:354: // new schema
will equal the older version in case of extension updates.
That's a bit hacky :(. Is there no way to detect at the Extension* level whether
an extension is being reloaded/updated or uninstalled?

Also, with this implementation, there will always be a delay between a registry
change and the fetch based on the new schema - even if the registry change is
not the result of a reload/update.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:362:
base::MessageLoopProxy::current()->PostDelayedTask(
Instead of a CancelableCallback and a delayed task, why not use a OneShotTimer?

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:370: if
(!schema_registry_->IsReady())
Nit: These two if-return statements could be combined into one.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:386: // Send the
current schema to the backend, in the case that it has changed
Nit: s/in the case that it has/in case it/

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_service.h (left):

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.h:34: class
SchemaMap;
Nit: This is still used in the declaration of SetCurrentSchema().

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

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.h:16: #include
"chrome/browser/policy/cloud/cloud_policy_constants.h"
Nit: Move to implementation file. No longer needed in the header.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_service_unittest.cc
(right):

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service_unittest.cc:26:
#include "chrome/browser/policy/schema_registry.h"
Nit: Already included by header file now.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_updater_unittest.cc
(right):

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_updater_unittest.cc:7:
#include <string>
Nit: Already included by header now.

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

Thanks for the review. PTAL

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:24:
#include "chrome/browser/policy/cloud/resource_cache.h"
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: No longer used.

Done.

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

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:76: // Invoked on
the background |task_runner_| to load the current store and
On 2013/11/14 14:13:12, bartfab wrote:
> Nit 1: There is no need to mention that this is invoked via |task_runner_|.
The
> comment above the constructor already states that *all* methods may be invoked
> via the |task_runner_| only.
> 
> Nit 2: s/store/store_/

Done.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:77: // start
updating remote data.
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: "Start updating remote data" sounds like this class will be sending
updates
> to a remote place. It does the opposite of course, pulling updates from a
remote
> place. Something like "start downloading updates" would be clearer.

Done.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:108:
scoped_ptr<ComponentCloudPolicyStore> store_;
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: This can be a plain member now. No need for a scoped_ptr anymore.

Done.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:308: // Send the
current credentials to the backend; do this whenever the store
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: The comment is wrong. You do not send the credentials to the backend
> whenever the store updates. You check for credential availability whenever the
> store updates but you only send the credentials at most once.

Done.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:349: // When an
extension is reloaded or update it triggers an unregister quickly
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: s/update/updated, /

Done.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:354: // new schema
will equal the older version in case of extension updates.
On 2013/11/14 14:13:12, bartfab wrote:
> That's a bit hacky :(. Is there no way to detect at the Extension* level
whether
> an extension is being reloaded/updated or uninstalled?
> 

I think that's by design, and we're not going to change it. Notice that the
reloaded extension may have different behavior and a different manifest, so
maybe it won't support policy anymore. Getting an UNLOAD possibly followed by a
LOAD handles this and other cases without introducing a transient state.

> Also, with this implementation, there will always be a delay between a
registry
> change and the fetch based on the new schema - even if the registry change is
> not the result of a reload/update.

That's fine. It only matters the first time that an extension is installed; from
then on it will keep getting cached policy. Note that a delay here or a delay on
the network during the download is indistinguishable from the extension's POV.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:362:
base::MessageLoopProxy::current()->PostDelayedTask(
On 2013/11/14 14:13:12, bartfab wrote:
> Instead of a CancelableCallback and a delayed task, why not use a
OneShotTimer?

That would make more sense semantically. Unfortunately it doesn't support
binding arguments.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:370: if
(!schema_registry_->IsReady())
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: These two if-return statements could be combined into one.

Done.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:386: // Send the
current schema to the backend, in the case that it has changed
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: s/in the case that it has/in case it/

Done.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_service.h (left):

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.h:34: class
SchemaMap;
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: This is still used in the declaration of SetCurrentSchema().

Done.

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

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.h:16: #include
"chrome/browser/policy/cloud/cloud_policy_constants.h"
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: Move to implementation file. No longer needed in the header.

Done.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_service_unittest.cc
(right):

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service_unittest.cc:26:
#include "chrome/browser/policy/schema_registry.h"
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: Already included by header file now.

Done.

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/component_cloud_policy_updater_unittest.cc
(right):

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_updater_unittest.cc:7:
#include <string>
On 2013/11/14 14:13:12, bartfab wrote:
> Nit: Already included by header now.

Done.

[bartfab (slow), 2013-11-14 15:02:21]

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

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:354: // new schema
will equal the older version in case of extension updates.
On 2013/11/14 14:56:41, Joao da Silva wrote:
> On 2013/11/14 14:13:12, bartfab wrote:
> > That's a bit hacky :(. Is there no way to detect at the Extension* level
> whether
> > an extension is being reloaded/updated or uninstalled?
> > 
> 
> I think that's by design, and we're not going to change it. Notice that the
> reloaded extension may have different behavior and a different manifest, so
> maybe it won't support policy anymore. Getting an UNLOAD possibly followed by
a
> LOAD handles this and other cases without introducing a transient state.
> 
> > Also, with this implementation, there will always be a delay between a
> registry
> > change and the fetch based on the new schema - even if the registry change
is
> > not the result of a reload/update.
> 
> That's fine. It only matters the first time that an extension is installed;
from
> then on it will keep getting cached policy. Note that a delay here or a delay
on
> the network during the download is indistinguishable from the extension's POV.

I agree that there will be a delay while policy is downloaded over the network
and that delay can be of arbitrary length (up to infinity, if the download fails
for good) but it is not elegant to introduce another delay *if* there is a
better way. Hence my question whether the UNLOAD includes some hint whether a
LOAD will follow shortly (in which case, holding off on the update makes sense)
or not (in which case, the update can be processed immediately).

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:362:
base::MessageLoopProxy::current()->PostDelayedTask(
On 2013/11/14 14:56:41, Joao da Silva wrote:
> On 2013/11/14 14:13:12, bartfab wrote:
> > Instead of a CancelableCallback and a delayed task, why not use a
> OneShotTimer?
> 
> That would make more sense semantically. Unfortunately it doesn't support
> binding arguments.

Sure it does. OneShotTimer is a BaseTimerMethodPointer is a Timer, which has the
following method:

void Start(const tracked_objects::Location& posted_from,
           TimeDelta delay,
           const base::Closure& user_task);

[Joao da Silva, 2013-11-15 09:28:15]

Replaced the CancellableCallback with a Timer. The delay is currently 0 and I've
left the TODO; the next CL will improve this, by checking if namespaces have
been removed or not before delaying the update.

PTAL, thanks!

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

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:354: // new schema
will equal the older version in case of extension updates.
On 2013/11/14 15:02:21, bartfab wrote:
> On 2013/11/14 14:56:41, Joao da Silva wrote:
> > On 2013/11/14 14:13:12, bartfab wrote:
> > > That's a bit hacky :(. Is there no way to detect at the Extension* level
> > whether
> > > an extension is being reloaded/updated or uninstalled?
> > > 
> > 
> > I think that's by design, and we're not going to change it. Notice that the
> > reloaded extension may have different behavior and a different manifest, so
> > maybe it won't support policy anymore. Getting an UNLOAD possibly followed
by
> a
> > LOAD handles this and other cases without introducing a transient state.
> > 
> > > Also, with this implementation, there will always be a delay between a
> > registry
> > > change and the fetch based on the new schema - even if the registry change
> is
> > > not the result of a reload/update.
> > 
> > That's fine. It only matters the first time that an extension is installed;
> from
> > then on it will keep getting cached policy. Note that a delay here or a
delay
> on
> > the network during the download is indistinguishable from the extension's
POV.
> 
> I agree that there will be a delay while policy is downloaded over the network
> and that delay can be of arbitrary length (up to infinity, if the download
fails
> for good) but it is not elegant to introduce another delay *if* there is a
> better way. Hence my question whether the UNLOAD includes some hint whether a
> LOAD will follow shortly (in which case, holding off on the update makes
sense)
> or not (in which case, the update can be processed immediately).

I'll definitely revised this later, so I'll leave a TODO here. For now there's
no good signal for that; UNLOAD just indicates which extension is being
unloaded, not why.

Come to think of it, OnSchemaRegistryUpdated() could indicate whether schemas
have been removed (togethe with has_new_schemas), and then we could do an
immediate up if nothing was removed. I'll come back to this in a subsequent CL
:-)

https://codereview.chromium.org/70863002/diff/720001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:362:
base::MessageLoopProxy::current()->PostDelayedTask(
On 2013/11/14 15:02:21, bartfab wrote:
> On 2013/11/14 14:56:41, Joao da Silva wrote:
> > On 2013/11/14 14:13:12, bartfab wrote:
> > > Instead of a CancelableCallback and a delayed task, why not use a
> > OneShotTimer?
> > 
> > That would make more sense semantically. Unfortunately it doesn't support
> > binding arguments.
> 
> Sure it does. OneShotTimer is a BaseTimerMethodPointer is a Timer, which has
the
> following method:
> 
> void Start(const tracked_objects::Location& posted_from,
>            TimeDelta delay,
>            const base::Closure& user_task);

Doh, that's right; done. Somehow I only noticed OneShotTimer<> and
RepeatingTimer<> in timer.h.

[bartfab (slow), 2013-11-15 12:20:31]

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

https://codereview.chromium.org/70863002/diff/410003/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:219:
schema_update_timer_(false, false),
Nit: If you used a OneShotTimer instead of a Timer, the constructor would not
have these nondescript bool arguments.

https://codereview.chromium.org/70863002/diff/410003/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:309: // Send the
current credentials to the backend, if they haven't been sent
Nit: This comment says that credentials are sent if not sent before. In reality,
there are two conditions: The credentials were not sent before *and* the
credentials are actually available. Either update the comment or simply move it
inside the if block which only gets executed when credentials are available.

[bartfab (slow), 2013-11-15 12:21:36]

Forgot to say, LGTM.

[Joao da Silva, 2013-11-15 14:58:08]

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

https://codereview.chromium.org/70863002/diff/410003/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:219:
schema_update_timer_(false, false),
On 2013/11/15 12:20:31, bartfab wrote:
> Nit: If you used a OneShotTimer instead of a Timer, the constructor would not
> have these nondescript bool arguments.

Dude, I can't read code. I thought again that OneShotTimer only worked with
method pointers :-(

Anyway, done now. Thanks for noticing, repeatedly :-)

https://codereview.chromium.org/70863002/diff/410003/chrome/browser/policy/cl...
chrome/browser/policy/cloud/component_cloud_policy_service.cc:309: // Send the
current credentials to the backend, if they haven't been sent
On 2013/11/15 12:20:31, bartfab wrote:
> Nit: This comment says that credentials are sent if not sent before. In
reality,
> there are two conditions: The credentials were not sent before *and* the
> credentials are actually available. Either update the comment or simply move
it
> inside the if block which only gets executed when credentials are available.

Done.

[commit-bot: I haz the power, 2013-11-15 15:00:52]

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

[commit-bot: I haz the power, 2013-11-15 22:22:39]

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

[commit-bot: I haz the power, 2013-11-16 01:02:16]

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

[commit-bot: I haz the power, 2013-11-17 20:11:09]

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

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

Change committed as 235638