Implement cloud policy invalidations using the invalidation service framework.

Implement cloud policy invalidations using the invalidation service framework.

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

[rlarocque, 2013-07-23 17:43:08]

I'm still hoping to temporarily disable the acknowledgement functionality when
adding trickles support, though this CL might force me to reconsider.  Prior to
this change, no one was actually taking advantage of local acknowledgements.

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:115: void
CloudPolicyInvalidator::HandleInvalidation(
The invalidations framework currently repeatedly re-sends invalidations with an
exponential backoff if the clients don't acknowledge the invalidation right
away.

I'd like to remove this feature, but it seems likely your change will land
before mine does.  If so, you should be prepared for the repeated invalidations.
 Most clients currently just acknowledge them as soon as they're received, in
part to avoid dealing with repeated re-sends.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:120:
AcknowledgeInvalidation();
For example, this probably doesn't work so well if an invalidation is
redelivered.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:152:
task_runner_->PostDelayedTask(
Would you be interested in cancelling any existing tasks before posting a new
one?

If so, you might want to look at base/timer/timer.h's OneShotTimer.  We found it
very useful for implementing sync's delayed fetch logic.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:237: invalidate_.Run();
From where does AcknowledgeInvalidation() get called when an invalidation is
successfully handled?

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:246:
weak_factory_.InvalidateWeakPtrs();
That's a neat trick.  I had no idea that InvalidateWeakPtrs() could be called
several times on the same object.  I think I still prefer the OneShotTimer,
though.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:34: public
CloudPolicyStore::Observer {
nit: indentation.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:122: int64
registered_timestamp_;
Make this a base::Time?

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.cc:236: // Make sure
the refresh does not occur before the first refresh time.
Did cloud policy refresh within minutes of restarting the browser prior to this
change?  If so, where did that call come from?  The refresh delays hard-coded at
the top of this file seem to be significantly longer.

[Joao da Silva, 2013-07-23 20:44:46]

First pass at this, I need to look again at the invalidator but overall looks
awesome! :-)

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_local_account_policy_store.cc
(right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:82:
validator->hash_value());
See comments in CloudPolicyStore: if the hash is computed at NotifyStoreLoaded()
(which is called whenever the policy changes, like at the end of this function)
then you won't have to change InstallPolicy().

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:282: if
(CommandLine::ForCurrentProcess()->HasSwitch(
Check the command line flag on CreateInvalidator instead.

You may #ifdef OS_CHROMEOS, OS_ANDROID, etc as appropriate.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:149:
new UserCloudPolicyManagerChromeOS(profile,
Instead of passing in the Profile, pass in the real dependency directly here
(the InvalidationService)

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:337:
validator->hash_value());
This can be removed (and the change below too) if you compute the hash in
NotifyStoreLoaded (which is always called from these 2 places as well).

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
File chrome/browser/invalidation/fake_invalidation_service.cc (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
chrome/browser/invalidation/fake_invalidation_service.cc:41: for (handle =
begin; handle != end; handle++)
++handle

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
chrome/browser/invalidation/fake_invalidation_service.cc:86: for
(AckHandleList::const_iterator handle = begin; handle != end; handle++)
++handle

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
File chrome/browser/invalidation/fake_invalidation_service.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
chrome/browser/invalidation/fake_invalidation_service.h:58: typedef
std::list<std::pair<syncer::AckHandle, invalidation::ObjectId> >
We tend to prefer vector to list, but I leave that to an owner of this code.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:8: #include <string>
Not used

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:12: #include
"base/files/file_path.h"
Not used

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:30: class
CloudPolicyStore;
cloud_policy_store.h already included

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:63:
~CloudPolicyInvalidator();
virtual

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:154: base::ThreadChecker
thread_checker_;
It's more common to inherit from NonThreadSafe and then assert
CalledOnValidThread()

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_manager.cc:80:
invalidation::InvalidationService* service) {
You can check the command line flag here.

Also, skip that command line flag on OS_ANDROID and always initialize this on
that platform.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_manager.cc:87:
&CloudPolicyRefreshScheduler::RefreshForInvalidation,
CloudPolicyRefreshScheduler::RefreshSoon

Actually, the invalidator should also tell the refresh_scheduler when the
invalidation_service is online/offline, so we could remove this callback and
just pass a handle to the scheduler directly here. WDYT?

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.cc:236: // Make sure
the refresh does not occur before the first refresh time.
On 2013/07/23 17:43:08, rlarocque wrote:
> Did cloud policy refresh within minutes of restarting the browser prior to
this
> change?  If so, where did that call come from?  The refresh delays hard-coded
at
> the top of this file seem to be significantly longer.

On the desktop, yes (on Android we will delay it depending on the age of the
cache).

Basically |last_refresh_| won't be initialized at startup for managed users, so
the first schedule call will get a huge cache age, and always refreshes. This is
intentional, and is not delayed here (but the networking backend of this code
delays all policy requests by 15 seconds at startup).

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h:51: bool
invalidations_enabled);
As discussed offline, you can revert the changes to this class.

It will have a method to set invalidations enabled/disabled. If they are
disabled from the command line then they should never be enabled from the
outside instead.

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_service.cc:71:
store_->set_invalidation_version(client->fetched_invalidation_version());
It'd be nicer to pass this value in the Store() call.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_store.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_store.h:110: void
set_invalidation_version(int64 invalidation_version) {
Can this be part of the Store() method? Or maybe have a new
StoreWithVersion(policy, version) method?

If this is combined with Store() then it can't be used in the wrong order.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_store.h:121: void
SetPolicyHashValue(uint32 hash_value);
A subclass may forget to call this, or do it in the wrong order.

You can just compute the hash of the current |policy_| at NotifyStoreLoaded(),
which is called whenever the policy changes, and then compare that to the
previous hash. If you do that then this function isn't needed, nor the
hash_value changes in the CloudPolicyValidator.

You may have to serialize |policy_| to do that.

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_validator.cc:363: hash_value_ =
base::Hash(policy_data_->policy_value());
This isn't a validation of the policy (it can't fail).

I think this can be done on the CloudPolicyStore, see the comments there.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
File chrome/browser/policy/cloud/user_cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_manager.cc:44:
switches::kEnableCloudPolicyPush)) {
Move the flag check to CreateInvalidator

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
File chrome/browser/policy/cloud/user_cloud_policy_store.cc (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_store.cc:73: const
em::PolicyFetchResponse& policy) {
This is chromium style :-)

You may give clang-format a try, it has a chromium-style flag:

http://clang.llvm.org/docs/ClangFormat.html

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_store.cc:130:
SetPolicyHashValue(0);
NotifyStoreLoaded() could compute this

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_store.cc:195:
validator->hash_value());
NotifyStoreLoaded can do this :-)

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_store.cc:261:
validator->hash_value());
Same

https://codereview.chromium.org/19733003/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.before.pretty-print.xml (right):

https://codereview.chromium.org/19733003/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.before.pretty-print.xml:1: <!--
This file shouldn't be included.

[rlarocque, 2013-07-23 20:55:05]

One of Joao's comments just reminded me about mobile platforms.  The Android and
iOS implementations of invalidations are different.

I'm pretty sure that your attempt to register for certain object IDs on Android
will have no effect.  Those registrations are hard-coded somewhere else.

I know almost nothing about iOS's implementation, so I'm not sure what will
happen on that platform.

We might need to start some separate discussions if you want to get this working
on mobile platforms.

[Joao da Silva, 2013-07-23 21:34:10]

Richard, thanks for the heads up. I'm currently working on policy on Android,
and we're planning to have invalidations working on that platform.

I'll ping you and Tommy over email to discuss that. Cheers!

[stepco1, 2013-07-23 21:36:50]

Yes, Tommy told me a while ago we'd need to do some extra work to get
arbitrary object registration to work on Android. That will be our next
step.


On Tue, Jul 23, 2013 at 2:34 PM, <joaodasilva@chromium.org> wrote:

> Richard, thanks for the heads up. I'm currently working on policy on
> Android,
> and we're planning to have invalidations working on that platform.
>
> I'll ping you and Tommy over email to discuss that. Cheers!
>
>
https://codereview.chromium.**org/19733003/<https://codereview.chromium.org/1...
>

[Steve Condie, 2013-07-24 01:42:04]

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_local_account_policy_store.cc
(right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:82:
validator->hash_value());
On 2013/07/23 20:44:47, Joao da Silva wrote:
> See comments in CloudPolicyStore: if the hash is computed at
NotifyStoreLoaded()
> (which is called whenever the policy changes, like at the end of this
function)
> then you won't have to change InstallPolicy().

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:282: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2013/07/23 20:44:47, Joao da Silva wrote:
> Check the command line flag on CreateInvalidator instead.
> 
> You may #ifdef OS_CHROMEOS, OS_ANDROID, etc as appropriate.

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:149:
new UserCloudPolicyManagerChromeOS(profile,
On 2013/07/23 20:44:47, Joao da Silva wrote:
> Instead of passing in the Profile, pass in the real dependency directly here
> (the InvalidationService)

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:337:
validator->hash_value());
On 2013/07/23 20:44:47, Joao da Silva wrote:
> This can be removed (and the change below too) if you compute the hash in
> NotifyStoreLoaded (which is always called from these 2 places as well).

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
File chrome/browser/invalidation/fake_invalidation_service.cc (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
chrome/browser/invalidation/fake_invalidation_service.cc:41: for (handle =
begin; handle != end; handle++)
On 2013/07/23 20:44:47, Joao da Silva wrote:
> ++handle

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
chrome/browser/invalidation/fake_invalidation_service.cc:86: for
(AckHandleList::const_iterator handle = begin; handle != end; handle++)
On 2013/07/23 20:44:47, Joao da Silva wrote:
> ++handle

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
File chrome/browser/invalidation/fake_invalidation_service.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
chrome/browser/invalidation/fake_invalidation_service.h:58: typedef
std::list<std::pair<syncer::AckHandle, invalidation::ObjectId> >
On 2013/07/23 20:44:47, Joao da Silva wrote:
> We tend to prefer vector to list, but I leave that to an owner of this code.

I like list in this case for fast deletion in AcknowledgeInvalidation. What do
you think Richard?

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:115: void
CloudPolicyInvalidator::HandleInvalidation(
On 2013/07/23 17:43:08, rlarocque wrote:
> The invalidations framework currently repeatedly re-sends invalidations with
an
> exponential backoff if the clients don't acknowledge the invalidation right
> away.
> 
> I'd like to remove this feature, but it seems likely your change will land
> before mine does.  If so, you should be prepared for the repeated
invalidations.
>  Most clients currently just acknowledge them as soon as they're received, in
> part to avoid dealing with repeated re-sends.

Thanks for pointing this out. My solution is to ignore the "new" invalidation if
the ack handle is the same as my currently outstanding ack handle.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:120:
AcknowledgeInvalidation();
On 2013/07/23 17:43:08, rlarocque wrote:
> For example, this probably doesn't work so well if an invalidation is
> redelivered.

Agreed, this was erroneous in that case.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:152:
task_runner_->PostDelayedTask(
On 2013/07/23 17:43:08, rlarocque wrote:
> Would you be interested in cancelling any existing tasks before posting a new
> one?
> 
> If so, you might want to look at base/timer/timer.h's OneShotTimer.  We found
it
> very useful for implementing sync's delayed fetch logic.

The previous tasks are cancelled by virtue of calling AcknowledgeInvalidation. I
did it in this way based how a similar scenario is solved in my team's
CloudPolicyRefreshScheduler class, although OneShotTimer looks like a good
alternative. For now I'd rather leave it since I've manually tested and written
unit tests for it unless you guys have a strong preference.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:237: invalidate_.Run();
On 2013/07/23 17:43:08, rlarocque wrote:
> From where does AcknowledgeInvalidation() get called when an invalidation is
> successfully handled?

The OnStoreLoaded method. That method is invoked after fetching, validating,
storing, and publishing a new policy. At that point we consider the invalidation
handled.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:8: #include <string>
On 2013/07/23 20:44:47, Joao da Silva wrote:
> Not used

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:12: #include
"base/files/file_path.h"
On 2013/07/23 20:44:47, Joao da Silva wrote:
> Not used

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:30: class
CloudPolicyStore;
On 2013/07/23 20:44:47, Joao da Silva wrote:
> cloud_policy_store.h already included

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:34: public
CloudPolicyStore::Observer {
On 2013/07/23 17:43:08, rlarocque wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:63:
~CloudPolicyInvalidator();
On 2013/07/23 20:44:47, Joao da Silva wrote:
> virtual

Done. Trybot caught this for me as well.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:122: int64
registered_timestamp_;
On 2013/07/23 17:43:08, rlarocque wrote:
> Make this a base::Time?

Since this variable is used to do an equality comparison to a proto buffer field
of type int64 and to check for zero/non-zero, and there are no time-specific
operations, I prefer leaving as int64.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:154: base::ThreadChecker
thread_checker_;
On 2013/07/23 20:44:47, Joao da Silva wrote:
> It's more common to inherit from NonThreadSafe and then assert
> CalledOnValidThread()

From thread_checker.h:
// The choice between having a
// ThreadChecker member and inheriting from base::NonThreadSafe should be based
// on whether:
//  - Derived classes need to know the thread they belong to, as opposed to
//    having that functionality fully encapsulated in the base class.
//  - Derived classes should be able to reassign the base class to another
//    thread, via DetachFromThread.
//
// If neither of these are true, then having a ThreadChecker member and calling
// CalledOnValidThread is the preferable solution.

Following this comment, it sounds like ThreadChecker member is preferable, but
of course I can change it if you'd like.

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_manager.cc:80:
invalidation::InvalidationService* service) {
On 2013/07/23 20:44:47, Joao da Silva wrote:
> You can check the command line flag here.
> 
> Also, skip that command line flag on OS_ANDROID and always initialize this on
> that platform.

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_manager.cc:87:
&CloudPolicyRefreshScheduler::RefreshForInvalidation,
On 2013/07/23 20:44:47, Joao da Silva wrote:
> CloudPolicyRefreshScheduler::RefreshSoon
> 
> Actually, the invalidator should also tell the refresh_scheduler when the
> invalidation_service is online/offline, so we could remove this callback and
> just pass a handle to the scheduler directly here. WDYT?

Changed this so that instead of passing in a single closure, you pass in a
reference to a CloudPolicyInvalidationHandler, which provides both the
invalidation and the state changed functions.

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.cc:236: // Make sure
the refresh does not occur before the first refresh time.
On 2013/07/23 20:44:47, Joao da Silva wrote:
> On 2013/07/23 17:43:08, rlarocque wrote:
> > Did cloud policy refresh within minutes of restarting the browser prior to
> this
> > change?  If so, where did that call come from?  The refresh delays
hard-coded
> at
> > the top of this file seem to be significantly longer.
> 
> On the desktop, yes (on Android we will delay it depending on the age of the
> cache).
> 
> Basically |last_refresh_| won't be initialized at startup for managed users,
so
> the first schedule call will get a huge cache age, and always refreshes. This
is
> intentional, and is not delayed here (but the networking backend of this code
> delays all policy requests by 15 seconds at startup).

As we discussed I reverted all my changes to CloudPolicyRefreshScheduler as you
will handle it in your larger Android change to the class.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h:51: bool
invalidations_enabled);
On 2013/07/23 20:44:47, Joao da Silva wrote:
> As discussed offline, you can revert the changes to this class.
> 
> It will have a method to set invalidations enabled/disabled. If they are
> disabled from the command line then they should never be enabled from the
> outside instead.

Done.

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_service.cc:71:
store_->set_invalidation_version(client->fetched_invalidation_version());
On 2013/07/23 20:44:47, Joao da Silva wrote:
> It'd be nicer to pass this value in the Store() call.

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_store.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_store.h:110: void
set_invalidation_version(int64 invalidation_version) {
On 2013/07/23 20:44:47, Joao da Silva wrote:
> Can this be part of the Store() method? Or maybe have a new
> StoreWithVersion(policy, version) method?
> 
> If this is combined with Store() then it can't be used in the wrong order.

I agree it's better to make it one call. I considered doing it but resisted
because it's a virtual method and would seem to dictate replacing the public
method and making the virtual method a protected member which would involve
touching all the subclasses, etc. However, I like your idea of just adding a new
public method to avoid touching subclasses so I did that.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_store.h:121: void
SetPolicyHashValue(uint32 hash_value);
On 2013/07/23 20:44:47, Joao da Silva wrote:
> A subclass may forget to call this, or do it in the wrong order.
> 
> You can just compute the hash of the current |policy_| at NotifyStoreLoaded(),
> which is called whenever the policy changes, and then compare that to the
> previous hash. If you do that then this function isn't needed, nor the
> hash_value changes in the CloudPolicyValidator.
> 
> You may have to serialize |policy_| to do that.

Done.

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

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_validator.cc:363: hash_value_ =
base::Hash(policy_data_->policy_value());
On 2013/07/23 20:44:47, Joao da Silva wrote:
> This isn't a validation of the policy (it can't fail).
> 
> I think this can be done on the CloudPolicyStore, see the comments there.

I agree the way you suggested is cleaner. The only reason I put it here in the
first place was so that the hash computation would be performed on the file
thread, just in case for a large policy we didn't want to do that computation on
the UI thread.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
File chrome/browser/policy/cloud/user_cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_manager.cc:44:
switches::kEnableCloudPolicyPush)) {
On 2013/07/23 20:44:47, Joao da Silva wrote:
> Move the flag check to CreateInvalidator

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
File chrome/browser/policy/cloud/user_cloud_policy_store.cc (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_store.cc:73: const
em::PolicyFetchResponse& policy) {
On 2013/07/23 20:44:47, Joao da Silva wrote:
> This is chromium style :-)
> 
> You may give clang-format a try, it has a chromium-style flag:
> 
> http://clang.llvm.org/docs/ClangFormat.html

Whoops, didn't mean to change that. In a previous version of the code I modified
this function to have a return value (so that OnStoreLoaded would only be called
after a successful disk store). When I reverted that apparently I didn't fully
revert the spacing.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_store.cc:130:
SetPolicyHashValue(0);
On 2013/07/23 20:44:47, Joao da Silva wrote:
> NotifyStoreLoaded() could compute this

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_store.cc:195:
validator->hash_value());
On 2013/07/23 20:44:47, Joao da Silva wrote:
> NotifyStoreLoaded can do this :-)

Done.

https://codereview.chromium.org/19733003/diff/1/chrome/browser/policy/cloud/u...
chrome/browser/policy/cloud/user_cloud_policy_store.cc:261:
validator->hash_value());
On 2013/07/23 20:44:47, Joao da Silva wrote:
> Same

Done.

https://codereview.chromium.org/19733003/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.before.pretty-print.xml (right):

https://codereview.chromium.org/19733003/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.before.pretty-print.xml:1: <!--
On 2013/07/23 20:44:47, Joao da Silva wrote:
> This file shouldn't be included.

Done.

[Joao da Silva, 2013-07-24 15:34:07]

This is almost there, but I just realized that the CloudPolicyManager can't
depend on any other BrowserContextKeyedServices. In this case, it can't depend
on the InvalidationService.

The reason for that is that the CloudPolicyManager is the very first service
created, because the preferences depend on it; anything created before will
*not* see managed preferences, so it can't depend on anything else.

The right way to do this is to make the CloudPolicyInvalidator a
BrowserContextKeyedService itself, and introduce a
CloudPolicyInvalidatorFactory.

That factory should override ServiceIsCreatedWithBrowserContext() to make sure
the service is always created at profile creation time (see
user_cloud_policy_token_forwarder_factory.h for an example).

That factory then depends on the InvalidationServiceFactory, and the
UserCloudPolicyManagerFactory or the UserCloudPolicyManagerFactoryChromeOS
(depending on OS_CHROMEOS).

The CloudPolicyInvalidator can then be constructed with a direct reference to
the InvalidationService, and to the CloudPolicyInvalidatorHandler (which is the
CloudPolicyManager).

Finally, the CloudPolicyInvalidator can figure out whether to connect or not by
testing the state of the store (and should react to OnStoreLoaded and
OnStoreError events):

- if the store has policy then connect or stay connected;
- if the store doesn't  have policy then disconnect.

The store is guaranteed to have the cache loaded by the time that the
CloudPolicyInvalidator gets created, so you can rely on this check on the
constructor too.

You just can't DCHECK that the refresh_scheduler exists on the
cloud_policy_manager on the overridden methods from the
CloudPolicyInvalidatorHandler; instead test that it exists before forwarding the
call.

https://codereview.chromium.org/19733003/diff/24001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:2442: 'supported_on': ['chrome.*:30-',
'chrome_os:0.30-'],
Yikes, nice catch

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:73:
DependsOn(invalidation::InvalidationServiceFactory::GetInstance());
I just realized that we can't do this.

Basically the cloud policy manager is the very first PKS created, because the
PrefService depends on it (and ALL the other PKSs depend on having prefs ready).
So if we do this then the InvalidationService gets created before the cloud
policy manager is ready, and before the prefs are ready.

Fortunately there's an easy fix: just make the cloud_policy_invalidator a PKS as
well. I'll comment more on this elsewhere.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client.cc:197:
fetched_invalidation_version_ = invalidation_version_;
This should be at OnPolicyFetchCompleted, and done only if the request
succeeded.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager.cc:112: if (invalidator_.get())
{
.get() not needed to test scoped_ptrs

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_store.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_store.cc:42: if (policy_.get() &&
policy_->has_policy_value())
.get() is not needed to test the policy_ scoped_ptr

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_store.cc:43: new_hash_value =
base::Hash(policy_->policy_value());
Note that base::Hash does a single pass over the string, so it's rather fast :-)
it's not a problem to do this on UI

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_manager.cc:56:
UnregisterInvalidator();
This call is not really needed, because store_->Clear() will trigger an
OnStoreLoaded notification, and the CloudPolicyInvalidator already Unregisters()
when that happens.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_manager_factory.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_manager_factory.cc:40:
DependsOn(invalidation::InvalidationServiceFactory::GetInstance());
We can't do this :-/

[rlarocque, 2013-07-24 16:40:44]

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
File chrome/browser/invalidation/fake_invalidation_service.h (right):

https://codereview.chromium.org/19733003/diff/1/chrome/browser/invalidation/f...
chrome/browser/invalidation/fake_invalidation_service.h:58: typedef
std::list<std::pair<syncer::AckHandle, invalidation::ObjectId> >
On 2013/07/24 01:42:04, stepco wrote:
> On 2013/07/23 20:44:47, Joao da Silva wrote:
> > We tend to prefer vector to list, but I leave that to an owner of this code.
> 
> I like list in this case for fast deletion in AcknowledgeInvalidation. What do
> you think Richard?

This is test code, so I don't care much about algorithmic efficiency one way or
the other.  Either one looks OK to me.

[Steve Condie, 2013-07-25 01:18:08]

I haven't tested this patch yet, so it probably doesn't work, but I thought I'd
upload the latest for the day so you can see the direction I'm going with
refactoring for the service dependency issue.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:73:
DependsOn(invalidation::InvalidationServiceFactory::GetInstance());
On 2013/07/24 15:34:07, Joao da Silva wrote:
> I just realized that we can't do this.
> 
> Basically the cloud policy manager is the very first PKS created, because the
> PrefService depends on it (and ALL the other PKSs depend on having prefs
ready).
> So if we do this then the InvalidationService gets created before the cloud
> policy manager is ready, and before the prefs are ready.
> 
> Fortunately there's an easy fix: just make the cloud_policy_invalidator a PKS
as
> well. I'll comment more on this elsewhere.

Done.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client.cc:197:
fetched_invalidation_version_ = invalidation_version_;
On 2013/07/24 15:34:07, Joao da Silva wrote:
> This should be at OnPolicyFetchCompleted, and done only if the request
> succeeded.

That actually wouldn't do what I want because it's possible that in between the
call to FetchPolicy and the call to OnPolicyFetchCompleted, another invalidation
was received, but due to "jittering", FetchPolicy hasn't been called again. In
that case, fetched_invalidation_version_ would be set to the latest invalidation
instead of the true invalidation which was fetched which would then cause the
invalidator to cancel the refresh for the latter invalidation.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager.cc:112: if (invalidator_.get())
{
On 2013/07/24 15:34:07, Joao da Silva wrote:
> .get() not needed to test scoped_ptrs

This function is now removed anyway.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_store.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_store.cc:42: if (policy_.get() &&
policy_->has_policy_value())
On 2013/07/24 15:34:07, Joao da Silva wrote:
> .get() is not needed to test the policy_ scoped_ptr

Done.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_manager.cc:56:
UnregisterInvalidator();
On 2013/07/24 15:34:07, Joao da Silva wrote:
> This call is not really needed, because store_->Clear() will trigger an
> OnStoreLoaded notification, and the CloudPolicyInvalidator already
Unregisters()
> when that happens.

True, although this would subtlely violate the contract of the invalidator class
since core()->Disconnect() would be called before the invalidator was destroyed,
causing invalidator's pointer to the cloud policy client to be invalid.

To solve this problem in the new refactoring I ended up removing the direct
dependency from invalidator to cloud policy client. Now the Clear() should
correctly cause unregistration without violating any dependencies.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_manager_factory.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_manager_factory.cc:40:
DependsOn(invalidation::InvalidationServiceFactory::GetInstance());
On 2013/07/24 15:34:07, Joao da Silva wrote:
> We can't do this :-/

Done.

[Joao da Silva, 2013-07-25 18:03:53]

I think the code structure is right, and from my side this is ready to go after
fixing the remaining comments.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client.cc:197:
fetched_invalidation_version_ = invalidation_version_;
On 2013/07/25 01:18:08, stepco wrote:
> On 2013/07/24 15:34:07, Joao da Silva wrote:
> > This should be at OnPolicyFetchCompleted, and done only if the request
> > succeeded.
> 
> That actually wouldn't do what I want because it's possible that in between
the
> call to FetchPolicy and the call to OnPolicyFetchCompleted, another
invalidation
> was received, but due to "jittering", FetchPolicy hasn't been called again. In
> that case, fetched_invalidation_version_ would be set to the latest
invalidation
> instead of the true invalidation which was fetched which would then cause the
> invalidator to cancel the refresh for the latter invalidation.

I see, thanks for clarifying. Please add a comment explaining that here.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:9: #include
"base/message_loop/message_loop.h"
not used

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:18: #include
"chrome/browser/policy/cloud/cloud_policy_store.h"
alread included in the header

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:234: max_fetch_delay_ =
kMaxFetchDelayDefault;
For consistency:

  set_max_fetch_delay(kMaxFetchDelayDefault);

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:172: public:
virtual ~CloudPolicyInvalidatorHandler() {}

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:15:
CloudPolicyManager* policy_manager) : CloudPolicyInvalidator(
break the ": CloudPolicyInvalidator(" into a newline

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc
(right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:8: #if
defined(OS_CHROMEOS)
Put OS_CHROMEOS #includes after the unconditional #includes

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:14: #if
!defined(OS_CHROMEOS)
Same for these

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:30: }  //
namespace
no need for the namespace for typedefs.

Also, it's clearer to have the #ifdef on lines 43 and 62, even if that seems
like a bit of duplication.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:56: #endif
After discussing this with Mattias we decided it's not a good idea; Android
should respect the flag as well, and have this disabled by default.

We'll enable it after the branch point, and if it proves stable then we might
enable it again in beta.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.h
(right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.h:13: }*/
Remove this

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.h:17: class
UserCloudPolicyInvalidatorFactory
Document the class

[rlarocque, 2013-07-25 19:23:39]

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/invalidati...
File chrome/browser/invalidation/fake_invalidation_service.h (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/invalidati...
chrome/browser/invalidation/fake_invalidation_service.h:63: AckHandleList
unacknowledged_handles_;
The way ack tracking currently works is that it only keeps track of the most
recently acked handle and the handle of the most most recently emitted
invalidation for each object.  If you acknowledge the handle with version N,
that implicitly acknowledges all invalidations with versions < N.

I suspect this test class' behavior does not match that of the original.

The behavior will change in the future, but this CL should still be written in
accordance with the code as it exists today.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:461:
ExpectInvalidationInfo(12, "test_payload");
When you wrap your expectations in functions, the test failure messages won't
include useful line numbers.  You could make test failures easier to read by
making this function return a ::testing::AssertionResult, then wrapping these
calls in an EXPECT_TRUE.

See
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_a_Function_That...

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager.h:92:
CloudPolicyInvalidationReplayer invalidation_replayer_;
Are you sure this is necessary?  The InvalidationService should buffer
invalidations while waiting for clients to register.  Why not just neglect to
register until you're ready to process the invalidations?

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.h (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.h:21: class
UserCloudPolicyInvalidator : public CloudPolicyInvalidator,
I'm curious: Why does this class exist?  Why not make the CloudPolicyInvalidator
inherit from BrowserContextKeyedService?

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc
(right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:60:
invalidation::InvalidationServiceFactory::GetForProfile(profile);
I'm just catching up on the discussions of service lifetime.

Does this mean that we'll be instantiating an InvalidationService before the
PrefService?  I wouldn't expect that to work.  The InvalidationService depends
on the PrefService.

[Steve Condie, 2013-07-26 01:39:25]

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_client.cc:197:
fetched_invalidation_version_ = invalidation_version_;
On 2013/07/25 18:03:53, Joao da Silva wrote:
> On 2013/07/25 01:18:08, stepco wrote:
> > On 2013/07/24 15:34:07, Joao da Silva wrote:
> > > This should be at OnPolicyFetchCompleted, and done only if the request
> > > succeeded.
> > 
> > That actually wouldn't do what I want because it's possible that in between
> the
> > call to FetchPolicy and the call to OnPolicyFetchCompleted, another
> invalidation
> > was received, but due to "jittering", FetchPolicy hasn't been called again.
In
> > that case, fetched_invalidation_version_ would be set to the latest
> invalidation
> > instead of the true invalidation which was fetched which would then cause
the
> > invalidator to cancel the refresh for the latter invalidation.
> 
> I see, thanks for clarifying. Please add a comment explaining that here.

Done.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:9: #include
"base/message_loop/message_loop.h"
On 2013/07/25 18:03:53, Joao da Silva wrote:
> not used

Done.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:18: #include
"chrome/browser/policy/cloud/cloud_policy_store.h"
On 2013/07/25 18:03:53, Joao da Silva wrote:
> alread included in the header

Done.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:234: max_fetch_delay_ =
kMaxFetchDelayDefault;
On 2013/07/25 18:03:53, Joao da Silva wrote:
> For consistency:
> 
>   set_max_fetch_delay(kMaxFetchDelayDefault);
> 

Done.

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/19733003/diff/24001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:172: public:
On 2013/07/25 18:03:53, Joao da Silva wrote:
> virtual ~CloudPolicyInvalidatorHandler() {}

Done.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/invalidati...
File chrome/browser/invalidation/fake_invalidation_service.h (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/invalidati...
chrome/browser/invalidation/fake_invalidation_service.h:63: AckHandleList
unacknowledged_handles_;
On 2013/07/25 19:23:40, rlarocque wrote:
> The way ack tracking currently works is that it only keeps track of the most
> recently acked handle and the handle of the most most recently emitted
> invalidation for each object.  If you acknowledge the handle with version N,
> that implicitly acknowledges all invalidations with versions < N.
> 
> I suspect this test class' behavior does not match that of the original.
> 
> The behavior will change in the future, but this CL should still be written in
> accordance with the code as it exists today.

As we discussed, clients are not required to acknowledge invalidations with
versions < N; however, it is not a violation of the contract to do so. Keeping
track of all of the emitted invalidations allows the FakeInvalidationService to
ensure that AcknowledgeInvalidation is only called with previously emitted ack
handle's, but this won't force callers to call AcknowledgeInvalidation for every
emitted ack handle.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:461:
ExpectInvalidationInfo(12, "test_payload");
On 2013/07/25 19:23:40, rlarocque wrote:
> When you wrap your expectations in functions, the test failure messages won't
> include useful line numbers.  You could make test failures easier to read by
> making this function return a ::testing::AssertionResult, then wrapping these
> calls in an EXPECT_TRUE.
> 
> See
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_a_Function_That...

Thanks for the suggestion. For the most part, these functions are only checking
one main thing, so I changed all the Expect* functions to Check*, made the
return type bool, and changed the call sites to use EXPECT_TRUE.

The few remaining expectations in the Check* functions are things that aren't
related to a particular test case, so if any of those fail they will probably
fail in multiple test cases, so I'm fine with seeing the same line number for
all the failures.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager.h:92:
CloudPolicyInvalidationReplayer invalidation_replayer_;
On 2013/07/25 19:23:40, rlarocque wrote:
> Are you sure this is necessary?  The InvalidationService should buffer
> invalidations while waiting for clients to register.  Why not just neglect to
> register until you're ready to process the invalidations?

With the new way of managing the lifetime of the UserCloudPolicyInvalidator, the
object is created when the profile creation is complete. There is a race
condition between that and the UserCloudPolicyManager being ready to handle the
invalidations, but there is no notification system to indicate when either one
is done, so the invalidator has no way of being alerted when it is time to
register for invalidations.

Joao, do you view the replayer as necessary? In your big comment about the
dependency management you mentioned that CloudPolicyManager would no longer be
able to assume that the refresh scheduler was started when an invalidation was
received, so it seems that you were acknowledging that an invalidation could be
received before the refresh scheduler is started.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:15:
CloudPolicyManager* policy_manager) : CloudPolicyInvalidator(
On 2013/07/25 18:03:53, Joao da Silva wrote:
> break the ": CloudPolicyInvalidator(" into a newline

Done.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.h (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.h:21: class
UserCloudPolicyInvalidator : public CloudPolicyInvalidator,
On 2013/07/25 19:23:40, rlarocque wrote:
> I'm curious: Why does this class exist?  Why not make the
CloudPolicyInvalidator
> inherit from BrowserContextKeyedService?

The reason is that there are other types of cloud policy that aren't tied to a
user profile (for example, device-level policy, public account policy,
kiosk-mode policy). The intent is for CloudPolicyInvalidator to be generic
enough to handle all types of policy (even though for now it is only used for
user policy). So the user-policy-specific functionality of managing the object
lifetime around the user profile I think is better off going in a subclass.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc
(right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:8: #if
defined(OS_CHROMEOS)
On 2013/07/25 18:03:53, Joao da Silva wrote:
> Put OS_CHROMEOS #includes after the unconditional #includes

Done.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:14: #if
!defined(OS_CHROMEOS)
On 2013/07/25 18:03:53, Joao da Silva wrote:
> Same for these

Done.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:30: }  //
namespace
On 2013/07/25 18:03:53, Joao da Silva wrote:
> no need for the namespace for typedefs.
> 
> Also, it's clearer to have the #ifdef on lines 43 and 62, even if that seems
> like a bit of duplication.

Done.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:56: #endif
On 2013/07/25 18:03:53, Joao da Silva wrote:
> After discussing this with Mattias we decided it's not a good idea; Android
> should respect the flag as well, and have this disabled by default.
> 
> We'll enable it after the branch point, and if it proves stable then we might
> enable it again in beta.

Done.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.cc:60:
invalidation::InvalidationServiceFactory::GetForProfile(profile);
On 2013/07/25 19:23:40, rlarocque wrote:
> I'm just catching up on the discussions of service lifetime.
> 
> Does this mean that we'll be instantiating an InvalidationService before the
> PrefService?  I wouldn't expect that to work.  The InvalidationService depends
> on the PrefService.

This will be solved by not attempting to get the InvalidationService until the
NOTIFICATION_PROFILE_ADDED notification is sent by the profile (See latest
version of UserCloudPolicyInvalidator). Since that notification is sent at the
very end of profile initialization I assume it should be safe at that point.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.h
(right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.h:13: }*/
On 2013/07/25 18:03:53, Joao da Silva wrote:
> Remove this

Done.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator_factory.h:17: class
UserCloudPolicyInvalidatorFactory
On 2013/07/25 18:03:53, Joao da Silva wrote:
> Document the class

Done.

[Steve Condie, 2013-07-26 08:47:27]

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager.h:92:
CloudPolicyInvalidationReplayer invalidation_replayer_;
On 2013/07/26 01:39:25, stepco wrote:
> On 2013/07/25 19:23:40, rlarocque wrote:
> > Are you sure this is necessary?  The InvalidationService should buffer
> > invalidations while waiting for clients to register.  Why not just neglect
to
> > register until you're ready to process the invalidations?
> 
> With the new way of managing the lifetime of the UserCloudPolicyInvalidator,
the
> object is created when the profile creation is complete. There is a race
> condition between that and the UserCloudPolicyManager being ready to handle
the
> invalidations, but there is no notification system to indicate when either one
> is done, so the invalidator has no way of being alerted when it is time to
> register for invalidations.
> 
> Joao, do you view the replayer as necessary? In your big comment about the
> dependency management you mentioned that CloudPolicyManager would no longer be
> able to assume that the refresh scheduler was started when an invalidation was
> received, so it seems that you were acknowledging that an invalidation could
be
> received before the refresh scheduler is started.

After thinking about it more, there is a fairly straightforward way I could
coordinate the events in order to delay invalidation registration until the
refresh scheduler is started. I'll upload a better solution in the morning.

[Steve Condie, 2013-07-26 19:17:23]

Fixed some problems found by trybot
Removed invalidation replayer in favor of simpler solution

[Steve Condie, 2013-07-26 20:12:25]

Synced forward so trybot can apply patch

[rlarocque, 2013-07-29 18:29:53]

I don't think I have anything more to add, so LGTM with nits.

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:461:
ExpectInvalidationInfo(12, "test_payload");
On 2013/07/26 01:39:25, stepco wrote:
> On 2013/07/25 19:23:40, rlarocque wrote:
> > When you wrap your expectations in functions, the test failure messages
won't
> > include useful line numbers.  You could make test failures easier to read by
> > making this function return a ::testing::AssertionResult, then wrapping
these
> > calls in an EXPECT_TRUE.
> > 
> > See
> >
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_a_Function_That...
> 
> Thanks for the suggestion. For the most part, these functions are only
checking
> one main thing, so I changed all the Expect* functions to Check*, made the
> return type bool, and changed the call sites to use EXPECT_TRUE.
> 
> The few remaining expectations in the Check* functions are things that aren't
> related to a particular test case, so if any of those fail they will probably
> fail in multiple test cases, so I'm fine with seeing the same line number for
> all the failures.

You've gained better line numbers, but now the tests won't be able to print the
values that failed comparison.  Unless you don't expect that information to be
useful, I still think you should consider returning an assertion result.

https://codereview.chromium.org/19733003/diff/79001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/79001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager.cc:95: // Enable this code once
method is added to CloudPolicyRefreshScheduler.
nit: Add a TODO with your name on it.

[Steve Condie, 2013-07-29 18:53:07]

Thanks Richard

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/19733003/diff/42001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:461:
ExpectInvalidationInfo(12, "test_payload");
On 2013/07/29 18:29:54, rlarocque wrote:
> On 2013/07/26 01:39:25, stepco wrote:
> > On 2013/07/25 19:23:40, rlarocque wrote:
> > > When you wrap your expectations in functions, the test failure messages
> won't
> > > include useful line numbers.  You could make test failures easier to read
by
> > > making this function return a ::testing::AssertionResult, then wrapping
> these
> > > calls in an EXPECT_TRUE.
> > > 
> > > See
> > >
> >
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Using_a_Function_That...
> > 
> > Thanks for the suggestion. For the most part, these functions are only
> checking
> > one main thing, so I changed all the Expect* functions to Check*, made the
> > return type bool, and changed the call sites to use EXPECT_TRUE.
> > 
> > The few remaining expectations in the Check* functions are things that
aren't
> > related to a particular test case, so if any of those fail they will
probably
> > fail in multiple test cases, so I'm fine with seeing the same line number
for
> > all the failures.
> 
> You've gained better line numbers, but now the tests won't be able to print
the
> values that failed comparison.  Unless you don't expect that information to be
> useful, I still think you should consider returning an assertion result.

I actually think seeing the name of the function is more useful than seeing the
actual comparison that failed. For example, for CheckInvalidateCalled, it's more
intuitive that the failure is because the invalidate callback wasn't made if the
output for the failed check is "CheckInvalidateCalled()" as opposed to
"task_runner_->GetPendingTasks().empty()". The latter is how the test fixture
implements that check internally, but that detail is abstracted away from the
test case code.

https://codereview.chromium.org/19733003/diff/79001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/79001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager.cc:95: // Enable this code once
method is added to CloudPolicyRefreshScheduler.
On 2013/07/29 18:29:54, rlarocque wrote:
> nit: Add a TODO with your name on it.

The code in question got checked in, so these lines can be uncommented now.

[Joao da Silva, 2013-07-29 19:44:32]

Only mechanical changes left, so lgtm.

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager.cc:35: const base::Closure&
initialize_invalidator) {
DCHECK(!initialize_invalidator.is_null());
DCHECK(initialize_invalidator_.is_null());

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc (right):

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc:218:
&CloudPolicyInvalidator::Initialize;
Don't do this. Rename the Initialize() methods instead (e.g.
InitializeWithProfile and InitializeWithService)

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc:425: 
nit: remove extra newline

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:22:
base::MessageLoop::current()->message_loop_proxy()),
base::MessageLoopProxy::current()

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:44:
&CloudPolicyInvalidator::Initialize;
Rename

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:48:
base::Unretained(this),
This should be a WeakPtr, because |this| might have been deleted by the time
that the manager invokes this (since this depends on the manager, so the manager
lives longer).

The CloudPolicyInvalidator already has a weak_factory_ and the callback method
is part of it, so you can just make it protected and grab a WeakPtr here.

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:49:
base::Unretained(profile_)));
This is fine though; if the WeakPtr is invalidated then the callback won't ever
be invoked, and |this| is destroyed before the Profile

https://codereview.chromium.org/19733003/diff/89001/chrome/test/data/policy/p...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/19733003/diff/89001/chrome/test/data/policy/p...
chrome/test/data/policy/policy_test_cases.json:1500: "os": ["chromeos"],
On the template this is also enabled on the desktop platforms

[Steve Condie, 2013-07-29 20:44:50]

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager.cc:35: const base::Closure&
initialize_invalidator) {
On 2013/07/29 19:44:32, Joao da Silva wrote:
> DCHECK(!initialize_invalidator.is_null());
> DCHECK(initialize_invalidator_.is_null());

Done.

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc (right):

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc:218:
&CloudPolicyInvalidator::Initialize;
On 2013/07/29 19:44:32, Joao da Silva wrote:
> Don't do this. Rename the Initialize() methods instead (e.g.
> InitializeWithProfile and InitializeWithService)

Done.

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc:425: 
On 2013/07/29 19:44:32, Joao da Silva wrote:
> nit: remove extra newline

Done.

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:22:
base::MessageLoop::current()->message_loop_proxy()),
On 2013/07/29 19:44:32, Joao da Silva wrote:
> base::MessageLoopProxy::current()

Done.

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:44:
&CloudPolicyInvalidator::Initialize;
On 2013/07/29 19:44:32, Joao da Silva wrote:
> Rename

Done.

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:48:
base::Unretained(this),
On 2013/07/29 19:44:32, Joao da Silva wrote:
> This should be a WeakPtr, because |this| might have been deleted by the time
> that the manager invokes this (since this depends on the manager, so the
manager
> lives longer).
> 
> The CloudPolicyInvalidator already has a weak_factory_ and the callback method
> is part of it, so you can just make it protected and grab a WeakPtr here.

Done.

https://codereview.chromium.org/19733003/diff/89001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:49:
base::Unretained(profile_)));
On 2013/07/29 19:44:32, Joao da Silva wrote:
> This is fine though; if the WeakPtr is invalidated then the callback won't
ever
> be invoked, and |this| is destroyed before the Profile

Done.

https://codereview.chromium.org/19733003/diff/89001/chrome/test/data/policy/p...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/19733003/diff/89001/chrome/test/data/policy/p...
chrome/test/data/policy/policy_test_cases.json:1500: "os": ["chromeos"],
On 2013/07/29 19:44:32, Joao da Silva wrote:
> On the template this is also enabled on the desktop platforms

Done.

[Joao da Silva, 2013-07-29 20:50:02]

lgtm! Good stuff :-)

[commit-bot: I haz the power, 2013-07-29 22:19:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/19733003/118002

[Steve Condie, 2013-07-29 23:32:56]

Added Ilya for new histogram LGTM.
Added Elliot for LGTM for new profile-keyed service added to
chrome_browser_main_extra_parts_profiles.cc.

[Ilya Sherman, 2013-07-30 01:23:15]

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:193:
UMA_HISTOGRAM_ENUMERATION(
nit: An UMA_HISTOGRAM_BOOLEAN would probably serve you better here.

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/enterprise_metrics.cc (right):

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/enterprise_metrics.cc:13: const char*
kMetricPolicyInvalidations = "Enterprise.PolicyInvalidations";
nit: These constants should all be of type "const char* kMetricName[]".

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/enterprise_metrics.h (right):

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/enterprise_metrics.h:180: kMetricPolicyRefreshSize 
// Must be the last.
nit: I see that this isn't the style being used in this file; but the normal
style in Chromium code is to define enumerated constants in ALL_CAPS

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:2409: <summary>Events for receiving
different types of invalidations.</summary>
This summary is very vague.  This histogram is really just a boolean histogram
tracking whether or not the invalidation contained a payload or not, right? 
(Incidentally, IMO it would be nice to explain a bit further what it means for a
policy invalidation to be issued, and what it means for it to contain a payload
or not.)

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:16158: </summary>
nit: This summary section is probably not needed.  It will only ever be shown in
the dashboard on hover, so it's generally better to simple make sure that the
histogram description includes any relevant info.  Ditto below.

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:16160: <int value="1"
label="NO_PAYLOAD">The invalidation contains no payload.</int>
nit: I'd recommend writing this simply as:

<int value="0" label="Payload"/>
<int value="1" label="No payload"/>

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:16213: </int>
Optional nit: I'd suggest labels more like "Changed, invalidations enabled",
"Changed, invalidations disabled", "Unchanged", "Invalidated, changed", and
"Invalidated, unchanged".

[commit-bot: I haz the power, 2013-07-30 01:25:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/19733003/118002

[Steve Condie, 2013-07-30 06:12:31]

Response to Ilya's review of new histograms and usage.

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:193:
UMA_HISTOGRAM_ENUMERATION(
On 2013/07/30 01:23:16, Ilya Sherman wrote:
> nit: An UMA_HISTOGRAM_BOOLEAN would probably serve you better here.

Done.

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/enterprise_metrics.cc (right):

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/enterprise_metrics.cc:13: const char*
kMetricPolicyInvalidations = "Enterprise.PolicyInvalidations";
On 2013/07/30 01:23:16, Ilya Sherman wrote:
> nit: These constants should all be of type "const char* kMetricName[]".

Done.

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/enterprise_metrics.h (right):

https://codereview.chromium.org/19733003/diff/118002/chrome/browser/policy/cl...
chrome/browser/policy/cloud/enterprise_metrics.h:180: kMetricPolicyRefreshSize 
// Must be the last.
On 2013/07/30 01:23:16, Ilya Sherman wrote:
> nit: I see that this isn't the style being used in this file; but the normal
> style in Chromium code is to define enumerated constants in ALL_CAPS

Done.

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:2409: <summary>Events for receiving
different types of invalidations.</summary>
On 2013/07/30 01:23:16, Ilya Sherman wrote:
> This summary is very vague.  This histogram is really just a boolean histogram
> tracking whether or not the invalidation contained a payload or not, right? 
> (Incidentally, IMO it would be nice to explain a bit further what it means for
a
> policy invalidation to be issued, and what it means for it to contain a
payload
> or not.)

Done.

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:16158: </summary>
On 2013/07/30 01:23:16, Ilya Sherman wrote:
> nit: This summary section is probably not needed.  It will only ever be shown
in
> the dashboard on hover, so it's generally better to simple make sure that the
> histogram description includes any relevant info.  Ditto below.

Done.

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:16160: <int value="1"
label="NO_PAYLOAD">The invalidation contains no payload.</int>
On 2013/07/30 01:23:16, Ilya Sherman wrote:
> nit: I'd recommend writing this simply as:
> 
> <int value="0" label="Payload"/>
> <int value="1" label="No payload"/>

Done.

https://codereview.chromium.org/19733003/diff/118002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:16213: </int>
On 2013/07/30 01:23:16, Ilya Sherman wrote:
> Optional nit: I'd suggest labels more like "Changed, invalidations enabled",
> "Changed, invalidations disabled", "Unchanged", "Invalidated, changed", and
> "Invalidated, unchanged".

Done.

[commit-bot: I haz the power, 2013-07-30 07:18:05]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[Ilya Sherman, 2013-07-30 07:55:01]

histograms LGTM, thanks.

[Steve Condie, 2013-07-30 18:07:22]

Not sure if Elliot is available, so added some more owners from
chrome/browser/profiles for the change to 
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc

[Elliot Glaysher, 2013-07-30 18:10:10]

profiles lgtm

[commit-bot: I haz the power, 2013-07-30 20:39:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/19733003/140001

[Steve Condie, 2013-07-30 21:18:34]

Synced to resolve conflict in policy_templates.json

[commit-bot: I haz the power, 2013-07-30 22:34:03]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-07-30 22:35:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/19733003/155001

[commit-bot: I haz the power, 2013-07-31 04:15:33]

Change committed as 214555

[Mattias Nissler (ping if slow), 2013-08-28 14:20:39]

Steve, I got interested in this CL when debugging crashes, and I've added a
couple comments that we may want to discuss and address in follow-up CLs.

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:56: void
CloudPolicyInvalidator::InitializeWithProfile(Profile* profile) {
Why do we need two initializers here? We'd preferably pass the
InvalidationService* dependency in the constructor.

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
File chrome/browser/policy/cloud/cloud_policy_manager.cc (right):

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
chrome/browser/policy/cloud/cloud_policy_manager.cc:86:
DCHECK(core()->client());
This DCHECK is optimistic, in fact there are crashes due to core()->client() ==
NULL: https://crash.corp.google.com/reportdetail?reportid=7b1a443bd435ccff

CloudPolicyManager shouldn't rely on client code to only call it when
appropriate.

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
chrome/browser/policy/cloud/cloud_policy_manager.cc:91:
DCHECK(core()->refresh_scheduler());
Same here.

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
chrome/browser/policy/cloud/cloud_policy_manager.cc:96:
DCHECK(core()->refresh_scheduler());
and here.

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
File chrome/browser/policy/cloud/cloud_policy_manager.h (right):

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
chrome/browser/policy/cloud/cloud_policy_manager.h:47: void
EnableInvalidations(const base::Closure& initialize_invalidator);
This really shouldn't be here. The invalidator should listen for a proper signal
and then initialize itself.

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
File chrome/browser/policy/cloud/cloud_policy_store.h (right):

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
chrome/browser/policy/cloud/cloud_policy_store.h:78: bool policy_changed() const
{
AFAICS, the only user of this is CloudPolicyInvalidator::GetRefreshMetric. Thus,
it'd be preferable to do change detection there instead of complicating the
public interface of CloudPolicyStore.

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://chromiumcodereview.appspot.com/19733003/diff/155001/chrome/browser/po...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:27: // Register for
notification that profile creation is complete.
Why is this delayed? Should at least have a comment explaining if there is a
good reason and no better way of addressing the issue.

[Steve Condie, 2013-09-07 01:11:19]

https://codereview.chromium.org/19733003/diff/155001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_store.h (right):

https://codereview.chromium.org/19733003/diff/155001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_store.h:78: bool policy_changed() const
{
On 2013/08/28 14:20:40, Mattias Nissler wrote:
> AFAICS, the only user of this is CloudPolicyInvalidator::GetRefreshMetric.
Thus,
> it'd be preferable to do change detection there instead of complicating the
> public interface of CloudPolicyStore.

Opened bug 286613 to track refactoring the policy change code as suggested by
this comment.

I believe all the remaining comments from Mattias have been addressed in
https://codereview.chromium.org/23592017/