Refactored the CloudPolicyProvider so that it becomes initialized once and stays initialized.

Refactored the CloudPolicyProvider so that it becomes initialized once and stays initialized.

Also added device policy command line flags to InProcessBrowserTest, so that browser_tests run with this provider on ChromeOS.

BUG=120967
TEST=Everything works as before


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

[Joao da Silva, 2012-03-29 21:21:34]

Refactor of the CloudPolicyProvider, as discussed today. Please review, thanks!

[Mattias Nissler (ping if slow), 2012-03-30 08:48:46]

I have looked at the provider parts. What I didn't check is the
DevicePolicyCache test changes yet. I don't understand what they are good for
and why they are needed. Please provide some background on that before I look
into them.

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
(right):

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:138:
.WillRepeatedly(Return(false));
Why this?

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_browsertest.cc (right):

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_browsertest.cc:38:
.WillRepeatedly(Return(false));
and this? It seems like the more reasonable choice would be to return true.

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

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:30: void
CloudPolicyProvider::SetUserPolicyCache(CloudPolicyCacheBase* cache) {
Any chance somebody might call SetUserPolicyCache(NULL)?

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:38: void
CloudPolicyProvider::SetDevicePolicyCache(CloudPolicyCacheBase* cache) {
same here

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

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:20: // A policy provider that
merges the policies contained in its policy caches.
This sentence suggests that every policy provide has policy caches, which isn't
quite correct.

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

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:55: device_policy_cache_
= NULL;
Wait, so you're running this test with actual user and device cache instances?
Why not just a mock?

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider.cc (right):

http://codereview.chromium.org/9911029/diff/7001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.cc:104: (!existing ||
current_priority.has_higher_priority_than(*existing))) {
good catch.

[Joao da Silva, 2012-03-30 10:09:23]

PTAL

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/chrom...
File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/chrom...
chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:138:
.WillRepeatedly(Return(false));
On 2012/03/30 08:48:46, Mattias Nissler wrote:
> Why this?

Because the --device-management-url and --enable-device-policy flags were added
to InProcessBrowserTests. Initialization of device policy uses
CrosLibrary::Get(), but since it has been overridden with this mock, the mock
must be updated to expect this call.

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/chrom...
chrome/browser/chromeos/login/login_browsertest.cc:38:
.WillRepeatedly(Return(false));
On 2012/03/30 08:48:46, Mattias Nissler wrote:
> and this? It seems like the more reasonable choice would be to return true.

Same reason. Returning false is more straightforward, since returning true will
require a couple more expected calls; but since device policy doesn't matter for
these tests, returning false suffices.

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/polic...
File chrome/browser/policy/cloud_policy_provider.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/polic...
chrome/browser/policy/cloud_policy_provider.cc:30: void
CloudPolicyProvider::SetUserPolicyCache(CloudPolicyCacheBase* cache) {
On 2012/03/30 08:48:46, Mattias Nissler wrote:
> Any chance somebody might call SetUserPolicyCache(NULL)?

That'd be incorrect, and crash on AddObserver(). I've added a comment in the
header.

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/polic...
chrome/browser/policy/cloud_policy_provider.cc:38: void
CloudPolicyProvider::SetDevicePolicyCache(CloudPolicyCacheBase* cache) {
On 2012/03/30 08:48:46, Mattias Nissler wrote:
> same here

Same here.

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/polic...
File chrome/browser/policy/cloud_policy_provider.h (right):

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/polic...
chrome/browser/policy/cloud_policy_provider.h:20: // A policy provider that
merges the policies contained in its policy caches.
On 2012/03/30 08:48:46, Mattias Nissler wrote:
> This sentence suggests that every policy provide has policy caches, which
isn't
> quite correct.

Done.

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/polic...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/polic...
chrome/browser/policy/cloud_policy_provider_unittest.cc:55: device_policy_cache_
= NULL;
On 2012/03/30 08:48:46, Mattias Nissler wrote:
> Wait, so you're running this test with actual user and device cache instances?
> Why not just a mock?

This was done when the cloud provider was owning the caches. I've simplified
this test using a mock cache again.

[Mattias Nissler (ping if slow), 2012-03-30 11:46:35]

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/7001/chrome/browser/chrom...
chrome/browser/chromeos/login/login_browsertest.cc:38:
.WillRepeatedly(Return(false));
On 2012/03/30 10:09:23, Joao da Silva wrote:
> On 2012/03/30 08:48:46, Mattias Nissler wrote:
> > and this? It seems like the more reasonable choice would be to return true.
> 
> Same reason. Returning false is more straightforward, since returning true
will
> require a couple more expected calls; but since device policy doesn't matter
for
> these tests, returning false suffices.

Hm. In that case, why do we enable the flags for all tests? Is there away to
just enable them for the tests that actually need them?

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_provider.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider.cc:31: DCHECK(caches_[CACHE_USER] ==
NULL);
DCHECK_NE or just DCHECK(cache_[CACHE_USER])

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:10: #include
"base/time.h"
needed?

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:63:
file_thread_(content::BrowserThread::FILE, &message_loop_) {}
Does this test actually need the loop?

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:65: void ResetProvider()
{
This is only ever called on test startup. Merge into constructor and maybe make
some things simple members?

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:116:
scoped_ptr<MockCloudPolicyCache> user_policy_cache_;
Any reason the caches need to be wrapped in scoped_ptrs and cannot just be
regular members?

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:155:
user_policy_cache_->SetReady();
So this is just to notify the observers? If so, please u se NotifyObservers().

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:107:
file_thread_(BrowserThread::FILE, &loop_),
why?

[Joao da Silva, 2012-03-30 16:36:58]

PTYAL :-)

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_provider.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider.cc:31: DCHECK(caches_[CACHE_USER] ==
NULL);
On 2012/03/30 11:46:35, Mattias Nissler wrote:
> DCHECK_NE or just DCHECK(cache_[CACHE_USER])

Done.

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:10: #include
"base/time.h"
On 2012/03/30 11:46:35, Mattias Nissler wrote:
> needed?

No, cleaned up.

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:63:
file_thread_(content::BrowserThread::FILE, &message_loop_) {}
On 2012/03/30 11:46:35, Mattias Nissler wrote:
> Does this test actually need the loop?

No, cleaned up.

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:65: void ResetProvider()
{
On 2012/03/30 11:46:35, Mattias Nissler wrote:
> This is only ever called on test startup. Merge into constructor and maybe
make
> some things simple members?

Done.

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:116:
scoped_ptr<MockCloudPolicyCache> user_policy_cache_;
On 2012/03/30 11:46:35, Mattias Nissler wrote:
> Any reason the caches need to be wrapped in scoped_ptrs and cannot just be
> regular members?

Done.

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_provider_unittest.cc:155:
user_policy_cache_->SetReady();
On 2012/03/30 11:46:35, Mattias Nissler wrote:
> So this is just to notify the observers? If so, please u se NotifyObservers().

Done.

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

https://chromiumcodereview.appspot.com/9911029/diff/18002/chrome/browser/poli...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:107:
file_thread_(BrowserThread::FILE, &loop_),
On 2012/03/30 11:46:35, Mattias Nissler wrote:
> why?

Because otherwise the UserPolicyCache DCHECKs on Load(), since it can't post to
FILE. That didn't happen before I introduced the DCHECK, but the cache wasn't
loading either.

[Mattias Nissler (ping if slow), 2012-03-30 16:55:04]

LGTM!

[commit-bot: I haz the power, 2012-03-30 16:56:11]

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

[commit-bot: I haz the power, 2012-03-30 21:04:31]

Try job failure for 9911029-14008 (retry) (retry) on win_rel for step
"browser_tests" (clobber build).
It's a second try, previously, steps "ui_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-31 10:29:34]

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

[commit-bot: I haz the power, 2012-03-31 13:51:06]

Try job failure for 9911029-14008 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "ui_tests, browser_tests,
installer_util_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-31 14:42:08]

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

[commit-bot: I haz the power, 2012-03-31 14:42:11]

Can't apply patch for file chrome/browser/policy/browser_policy_connector.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/policy/browser_policy_connector.cc
Hunk #2 FAILED at 110.
Hunk #3 succeeded at 319 (offset 2 lines).
Hunk #4 succeeded at 454 (offset 2 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/policy/browser_policy_connector.cc.rej

[commit-bot: I haz the power, 2012-03-31 18:41:11]

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

[commit-bot: I haz the power, 2012-03-31 22:02:26]

Change committed as 130067