Added ConfigurationPolicyProvider::RefreshPolicies.

Added ConfigurationPolicyProvider::RefreshPolicies.

This call has guaranteed behavior regarding notifications of observers, which
can be relied upon to build test infrastructure that sets up policy.

Added the provider as an argument to OnUpdatePolicy.
Added BrowserPolicyConnector::FetchCloudPolicy and ResetCloudPolicy.

TEST=All works as before


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110977

[Joao da Silva, 2011-11-17 15:48:23]

Here's a monster CL for your reviewing, uh, pleasure :-)

I'll write tests for the new stuff if we reach an agreement on whether this is
desirable.

Changes:
- ConfigurationPolicyProvider::RefreshPolicies that always notifies the
observers (unless it's deleted)
- AsynchronousPolicyLoader has callbacks on Reload
- BrowserPolicyConnector::FetchCloudPolicy
- CloudPolicyProviderImpl relies on the caches notifying it of every update;
using SetFetchingDone for this

[Mattias Nissler (ping if slow), 2011-11-18 11:00:25]

Here are my comments, I think I also found an issue with RefreshPolicies() not
synchronizing properly in the cloud policy case.

http://codereview.chromium.org/8586030/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:943: void
TestingAutomationProvider::FetchEnterprisePolicy(
Wouldn't this now be covered by a regular refresh?

http://codereview.chromium.org/8586030/diff/1/chrome/browser/chromeos/login/e...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc
(right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/chromeos/login/e...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc:102:
g_browser_process->browser_policy_connector()->ResetCloudPolicy();
I dislike this. At this point we actually want to only reset device policy, and
leave user policy alone. Same below.

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/browser_p...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:87: void ResetCloudPolicy();
As mentioned in the other comment, the enrollment screen should have a way of
only resetting device policy and leave user policy alone.

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_provider_unittest.cc:64:
g_browser_process->browser_policy_connector(),
shouldn't we rather mock that thing?

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_provider.h:32: virtual void
OnProviderGoingAway();
Pass provider also to OnProviderGoingAway() for symmetry?

http://codereview.chromium.org/8586030/diff/1/chrome/browser/ui/webui/policy_...
File chrome/browser/ui/webui/policy_ui.cc (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/ui/webui/policy_...
chrome/browser/ui/webui/policy_ui.cc:120: connector->FetchCloudPolicy();
Can we just issue a RefreshPolicies() call here? Maybe rename
BrowserPolicyConnector::FetchCloudPolicy() to
BrowserPolicyConnector::RefreshPolicy()? That way, we also get the reload
functionality on win and mac for easy testing :)

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:42:
browser_policy_connector_->FetchCloudPolicy();
I think there's still an issue here. This will notify CloudPolicyController but
it will only post a task to the message loop that will then reset the backend
and issue a new policy fetch. In the mean time, an earlier policy fetch can
potentially post back it's results...

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:53: for (ListType::iterator
i = caches_.begin(); i != caches_.end(); ++i) {
can we please use std::find + erase here? :) Also, it's basically the same as
RemoveFromPending, so you could share code.

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.h:32: virtual void
OnProviderGoingAway();
For symmetry, we might also want to pass the parameter to OnProviderGoingAway?

[Joao da Silva, 2011-11-18 14:03:30]

Please have another look!

http://codereview.chromium.org/8586030/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:943: void
TestingAutomationProvider::FetchEnterprisePolicy(
On 2011/11/18 11:00:25, Mattias Nissler wrote:
> Wouldn't this now be covered by a regular refresh?

Yes. I'll update the CloudPolicyObserver in the automation RefreshPolicies() CL.

http://codereview.chromium.org/8586030/diff/1/chrome/browser/chromeos/login/e...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc
(right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/chromeos/login/e...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc:102:
g_browser_process->browser_policy_connector()->ResetCloudPolicy();
On 2011/11/18 11:00:25, Mattias Nissler wrote:
> I dislike this. At this point we actually want to only reset device policy,
and
> leave user policy alone. Same below.

Reverted to just have ResetDevicePolicy.

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/browser_p...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:87: void ResetCloudPolicy();
On 2011/11/18 11:00:25, Mattias Nissler wrote:
> As mentioned in the other comment, the enrollment screen should have a way of
> only resetting device policy and leave user policy alone.

Done.

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_provider_unittest.cc:64:
g_browser_process->browser_policy_connector(),
On 2011/11/18 11:00:25, Mattias Nissler wrote:
> shouldn't we rather mock that thing?

It is "mocked" in unit_tests (as in, it has no providers and doesn't initialize
anything; see BrowserPolicyConnector::CreateForTests).

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_provider.h:32: virtual void
OnProviderGoingAway();
On 2011/11/18 11:00:25, Mattias Nissler wrote:
> Pass provider also to OnProviderGoingAway() for symmetry?

Done.

http://codereview.chromium.org/8586030/diff/1/chrome/browser/ui/webui/policy_...
File chrome/browser/ui/webui/policy_ui.cc (right):

http://codereview.chromium.org/8586030/diff/1/chrome/browser/ui/webui/policy_...
chrome/browser/ui/webui/policy_ui.cc:120: connector->FetchCloudPolicy();
On 2011/11/18 11:00:25, Mattias Nissler wrote:
> Can we just issue a RefreshPolicies() call here? Maybe rename
> BrowserPolicyConnector::FetchCloudPolicy() to
> BrowserPolicyConnector::RefreshPolicy()? That way, we also get the reload
> functionality on win and mac for easy testing :)

Done.

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:42:
browser_policy_connector_->FetchCloudPolicy();
On 2011/11/18 11:00:25, Mattias Nissler wrote:
> I think there's still an issue here. This will notify CloudPolicyController
but
> it will only post a task to the message loop that will then reset the backend
> and issue a new policy fetch. In the mean time, an earlier policy fetch can
> potentially post back it's results...

FetchCloudPolicy() triggers CloudPolicyController::OnDeviceTokenChanged on the
same iteration of the loop, which in turn calls SetState() where the backend_
(and any pending reply it might have) is immediately dropped.

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:53: for (ListType::iterator
i = caches_.begin(); i != caches_.end(); ++i) {
On 2011/11/18 11:00:25, Mattias Nissler wrote:
> can we please use std::find + erase here? :) Also, it's basically the same as
> RemoveFromPending, so you could share code.

Done.

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/8586030/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.h:32: virtual void
OnProviderGoingAway();
On 2011/11/18 11:00:25, Mattias Nissler wrote:
> For symmetry, we might also want to pass the parameter to OnProviderGoingAway?

Done.

[Mattias Nissler (ping if slow), 2011-11-18 14:49:32]

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc
(right):

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_screen.cc:289:
connector->FetchCloudPolicy();
This is not quite what we want, but user policy shouldn't care, so OK.

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/asynch...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/asynch...
chrome/browser/policy/asynchronous_policy_provider.h:47: static void
PostReloadOnFILE(scoped_refptr<AsynchronousPolicyLoader> loader);
In these cases we usually s/FILE/FileThread/:

$ git grep 'FileThread' | wc -l
335
$ git grep 'FILE(' | wc -l
29

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/asynch...
chrome/browser/policy/asynchronous_policy_provider.h:47: static void
PostReloadOnFILE(scoped_refptr<AsynchronousPolicyLoader> loader);
Pass loader as plain pointer and base::Bind will take care of the refcounting.

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:222:
recommended_cloud_provider_->RefreshPolicies();
So here is where we have both cloud providers call FetchCloudPolicy immediately?

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_impl.h (right):

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:59: // Removes |cache| from
|caches|, if contained therein.
can be static.

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:64:
g_browser_process->browser_policy_connector(),
I see. Globals suck though, your call.

[Joao da Silva, 2011-11-18 15:39:18]

It keeps growing! :-) PTAL

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/asynch...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/asynch...
chrome/browser/policy/asynchronous_policy_provider.h:47: static void
PostReloadOnFILE(scoped_refptr<AsynchronousPolicyLoader> loader);
On 2011/11/18 14:49:32, Mattias Nissler wrote:
> In these cases we usually s/FILE/FileThread/:
> 
> $ git grep 'FileThread' | wc -l
> 335
> $ git grep 'FILE(' | wc -l
> 29

Done.

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/asynch...
chrome/browser/policy/asynchronous_policy_provider.h:47: static void
PostReloadOnFILE(scoped_refptr<AsynchronousPolicyLoader> loader);
On 2011/11/18 14:49:32, Mattias Nissler wrote:
> Pass loader as plain pointer and base::Bind will take care of the refcounting.

Done.

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:222:
recommended_cloud_provider_->RefreshPolicies();
On 2011/11/18 14:49:32, Mattias Nissler wrote:
> So here is where we have both cloud providers call FetchCloudPolicy
immediately?

Yes. The 2nd call will cancel the posting of DoWork of the 1st. We can also use
"internal knowledge" of the connector and assume that either both
cloud_providers exist or both don't, and just RefreshPolicies on one of them.
That seems like asking for trouble in the future, though. WDYT?

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_impl.h (right):

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:59: // Removes |cache| from
|caches|, if contained therein.
On 2011/11/18 14:49:32, Mattias Nissler wrote:
> can be static.

Done.

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8586030/diff/7001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:64:
g_browser_process->browser_policy_connector(),
On 2011/11/18 14:49:32, Mattias Nissler wrote:
> I see. Globals suck though, your call.

Removed the static-ness out of BrowserPolicyConnector, making it easier to
create an instance for tests.

[Mattias Nissler (ping if slow), 2011-11-18 16:14:11]

I still don't like the Refresh-All-Providers-Cancel-Everything-But-One-DoWork,
but don't have a better suggestion at this point. Once you get it running, LGTM.

http://codereview.chromium.org/8586030/diff/6038/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8586030/diff/6038/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:63:
browser_policy_connector_.reset(new BrowserPolicyConnector());
No need to wrap in a scoped_ptr.

[Joao da Silva, 2011-11-21 14:55:04]

Please have another look; this has been more thoroughly tested, and unit tests
have been added.

Changes since last review:
- fixed LoginUtilsTest (wasn't initializing connector_)
- fixed RefreshPolices in the connector, was calling the wrong providers
- extended AsynchronousPolicyTestBase.ProvideAfterRefresh
- extended AsynchronousPolicyTestBase.ProviderRefresh
- added CloudPolicyProviderTest.RefreshPolicies
- added ConfigurationPolicyProviderTest.RefreshPolicies

Thanks!

http://codereview.chromium.org/8586030/diff/6038/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8586030/diff/6038/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:63:
browser_policy_connector_.reset(new BrowserPolicyConnector());
On 2011/11/18 16:14:11, Mattias Nissler wrote:
> No need to wrap in a scoped_ptr.

Done.

[Mattias Nissler (ping if slow), 2011-11-21 16:09:05]

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:9: #include
"chrome/browser/browser_process.h"
needed?

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:76:
g_browser_process->browser_policy_connector(),
Wait, shouldn't you use &browser_policy_connector_ here as well?

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:245:
StrictMock<MockConfigurationPolicyObserver> observer;
Using a StrictMock to ensure that no other events occur? The googlemock guide
advises against that in favor of putting explicit EXPECT_CALL().Times(0) on the
functions, and I tend to agree, since StrictMock tends to overly restrict
acceptable behavior. Why would we want to make sure other notifications don't
happen?

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:258:
MockCloudPolicyCache cache2;
It's a bit weird to have the cache2 declaration here. Move all declarations up
to the top?

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:274: // required to fire
the update.
Is that intended? Either way, it probably doesn't matter for the problem we're
trying to solve.

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:291: // If a cache
refreshes more than once, the provider should still waits for
s/waits/wait/

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider.h:58: // call will be
visible on the next calls of OnUpdatePolicy on the observers,
s/calls/call/? I think grammar-wise it's correct the way you have it, but I
tripped over it twice now :)

[Joao da Silva, 2011-11-21 16:49:21]

Please have another look. Thanks for reviewing :-)

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:9: #include
"chrome/browser/browser_process.h"
On 2011/11/21 16:09:05, Mattias Nissler wrote:
> needed?

Not after fixing the g_browser_process below.

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:76:
g_browser_process->browser_policy_connector(),
On 2011/11/21 16:09:05, Mattias Nissler wrote:
> Wait, shouldn't you use &browser_policy_connector_ here as well?

Yep, sloppy me. Fixed.

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:245:
StrictMock<MockConfigurationPolicyObserver> observer;
On 2011/11/21 16:09:05, Mattias Nissler wrote:
> Using a StrictMock to ensure that no other events occur? The googlemock guide
> advises against that in favor of putting explicit EXPECT_CALL().Times(0) on
the
> functions, and I tend to agree, since StrictMock tends to overly restrict
> acceptable behavior. Why would we want to make sure other notifications don't
> happen?

Removed.

This was introduced when the test complained about some "uninteresting" calls
and I wanted to check where they came from. It turned out to be the caches going
out of scope, invoking OnCacheGoingAway and triggering 3 updates when the
function terminates.

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:258:
MockCloudPolicyCache cache2;
On 2011/11/21 16:09:05, Mattias Nissler wrote:
> It's a bit weird to have the cache2 declaration here. Move all declarations up
> to the top?

Done.

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:274: // required to fire
the update.
On 2011/11/21 16:09:05, Mattias Nissler wrote:
> Is that intended? Either way, it probably doesn't matter for the problem we're
> trying to solve.

It doesn't matter indeed, this comment is for clarification.

If a new cache comes along while we are waiting for an update, the update
shouldn't be fired until the pending caches are all updated too. Notice that
this only happens when there is a pending update, so it will be triggered
"soon".

If adding a new cache triggered an update it would lead to flakiness, since the
caller of RefreshPolicies expects all the existing caches to reload, but the
update fired by the addition might come before the reloading is done, and any
policy modifications are visible.

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:291: // If a cache
refreshes more than once, the provider should still waits for
On 2011/11/21 16:09:05, Mattias Nissler wrote:
> s/waits/wait/

Done.

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/8586030/diff/21001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider.h:58: // call will be
visible on the next calls of OnUpdatePolicy on the observers,
On 2011/11/21 16:09:05, Mattias Nissler wrote:
> s/calls/call/? I think grammar-wise it's correct the way you have it, but I
> tripped over it twice now :)

Done. The practice of german and the black horse are ruining my fragile english
grammar.

[Mattias Nissler (ping if slow), 2011-11-21 16:57:17]

LGTM

[commit-bot: I haz the power, 2011-11-21 17:38:55]

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

[commit-bot: I haz the power, 2011-11-21 19:26:16]

Change committed as 110977