Added RefreshPolicies to PolicyService.

Added RefreshPolicies to PolicyService.

Changed users of BrowserPolicyConnector::RefreshPolicies to use the new interface.

BUG=108999
TEST=unit_tests green, pyauto tests green, all works as before


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

[Joao da Silva, 2012-04-26 10:46:20]

Please review, thanks!

[Mattias Nissler (ping if slow), 2012-04-26 11:08:52]

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/mock_con...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/mock_con...
chrome/browser/policy/mock_configuration_policy_provider.h:31: void
SetNotifyOnRefresh(bool notify_on_refresh);
How about making RefreshPolicies() a proper mock instead?

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/policy_s...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/policy_s...
chrome/browser/policy/policy_service_impl.cc:176: if (refresh_pending)
This results in all updates being blocked until the last provider answers. This
can potentially put us into big trouble. I think we should not abort here, but
update the effective polices and send OnPolicyUpdate events. Just don't invoke
the callbacks.

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/policy_s...
chrome/browser/policy/policy_service_impl.cc:215: // on FILE, which is initiated
from IO.
This doesn't belong here. Policies might trigger various asynchronous events
(e.g. extension installation), and this is not the right place to wait for all
these effects to take place.

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

http://codereview.chromium.org/10236003/diff/1/chrome/browser/ui/webui/policy...
chrome/browser/ui/webui/policy_ui.cc:130: true));
Hm, this code still follows the model that the first SendDataToUI call indicates
the refresh is complete. I'd rather see the OnPolicyUpdated path to be separate
from the RefreshPolicies callback (so you see policy updates coming in as they
appear) and just send a signal to re-enable the refresh button once this
callback fires.

[Joao da Silva, 2012-04-26 14:43:57]

PTAL

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/mock_con...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/mock_con...
chrome/browser/policy/mock_configuration_policy_provider.h:31: void
SetNotifyOnRefresh(bool notify_on_refresh);
On 2012/04/26 11:08:52, Mattias Nissler wrote:
> How about making RefreshPolicies() a proper mock instead?

This is following the style of IsInitializationComplete, which also has a method
to configure it. To make this one a mock method I'd make all the others mock
methods too, which I just did :-)

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/policy_s...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/policy_s...
chrome/browser/policy/policy_service_impl.cc:176: if (refresh_pending)
On 2012/04/26 11:08:52, Mattias Nissler wrote:
> This results in all updates being blocked until the last provider answers.
This
> can potentially put us into big trouble. I think we should not abort here, but
> update the effective polices and send OnPolicyUpdate events. Just don't invoke
> the callbacks.

Done.

http://codereview.chromium.org/10236003/diff/1/chrome/browser/policy/policy_s...
chrome/browser/policy/policy_service_impl.cc:215: // on FILE, which is initiated
from IO.
On 2012/04/26 11:08:52, Mattias Nissler wrote:
> This doesn't belong here. Policies might trigger various asynchronous events
> (e.g. extension installation), and this is not the right place to wait for all
> these effects to take place.

Right; this was introduced to fix automation tests, so I've moved the code
there.

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

http://codereview.chromium.org/10236003/diff/1/chrome/browser/ui/webui/policy...
chrome/browser/ui/webui/policy_ui.cc:130: true));
On 2012/04/26 11:08:52, Mattias Nissler wrote:
> Hm, this code still follows the model that the first SendDataToUI call
indicates
> the refresh is complete. I'd rather see the OnPolicyUpdated path to be
separate
> from the RefreshPolicies callback (so you see policy updates coming in as they
> appear) and just send a signal to re-enable the refresh button once this
> callback fires.

Added a new field to flag this.

[Mattias Nissler (ping if slow), 2012-04-27 09:03:27]

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:970: void
ResetServiceAndStore() {
Since this is not declared virtual in ConfigurationPolicyPrefStoreTest, the
constructor will actually run
ConfigurationPolicyPrefStoreTest::ResetServiceAndStore(). I guess that's
intentional, but then you should use different names in the first place.

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:1008:
ResetServiceAndStore();
Why is this necessary?

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:95:
(*it)->provider->RefreshPolicies();
What if providers_ is empty?

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_stub.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_stub.cc:35:
MessageLoop::current()->PostTask(FROM_HERE, callback);
Any reason to not directly invoke the callback? That may happen if the providers
call back immediately in the full implementation as well.

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/ui/webui/po...
File chrome/browser/ui/webui/policy_ui.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/ui/webui/po...
chrome/browser/ui/webui/policy_ui.cc:134: bool is_refresh_done) {
I feel it's odd to multiplex all the different signals we want to send through a
single javascript function call. Why don't we have separate functions for the
different notifications we send?

[Joao da Silva, 2012-04-27 11:24:12]

Please have another look!

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:970: void
ResetServiceAndStore() {
On 2012/04/27 09:03:27, Mattias Nissler wrote:
> Since this is not declared virtual in ConfigurationPolicyPrefStoreTest, the
> constructor will actually run
> ConfigurationPolicyPrefStoreTest::ResetServiceAndStore(). I guess that's
> intentional, but then you should use different names in the first place.

It's intentional; using different names to avoid confusion.

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:1008:
ResetServiceAndStore();
On 2012/04/27 09:03:27, Mattias Nissler wrote:
> Why is this necessary?

Because store_, as created from SetUp, is already initialized (because the
provider_ is also initialized there).

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:95:
(*it)->provider->RefreshPolicies();
On 2012/04/27 09:03:27, Mattias Nissler wrote:
> What if providers_ is empty?

Nice catch :-) Fixed.

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_stub.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_stub.cc:35:
MessageLoop::current()->PostTask(FROM_HERE, callback);
On 2012/04/27 09:03:27, Mattias Nissler wrote:
> Any reason to not directly invoke the callback? That may happen if the
providers
> call back immediately in the full implementation as well.

In this case it doesn't matter. I was concerned with callbacks that call
RefreshPolicies again during their execution; in the main impl it's not a
problem because the vectors are swapped. I only see this potentially happening
in tests.

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/ui/webui/po...
File chrome/browser/ui/webui/policy_ui.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/ui/webui/po...
chrome/browser/ui/webui/policy_ui.cc:134: bool is_refresh_done) {
On 2012/04/27 09:03:27, Mattias Nissler wrote:
> I feel it's odd to multiplex all the different signals we want to send through
a
> single javascript function call. Why don't we have separate functions for the
> different notifications we send?

I've removed the booleans from SendDataToUI. The UI now determines whether
'returnData' comes from the first load or an update, and a separate call signals
that a refresh is done.

[Mattias Nissler (ping if slow), 2012-04-27 12:52:57]

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:1008:
ResetServiceAndStore();
On 2012/04/27 11:24:12, Joao da Silva wrote:
> On 2012/04/27 09:03:27, Mattias Nissler wrote:
> > Why is this necessary?
> 
> Because store_, as created from SetUp, is already initialized (because the
> provider_ is also initialized there).

Can't we just initialize with IsInitializationComplete() returning false, and
then, as the last step in SetUp switch it to true (so the other tests don't
fail). Then, in this test you only need put another override to false. Would
that work?

http://codereview.chromium.org/10236003/diff/19001/chrome/browser/resources/p...
File chrome/browser/resources/policy.js (right):

http://codereview.chromium.org/10236003/diff/19001/chrome/browser/resources/p...
chrome/browser/resources/policy.js:232: policy.updatePolicyVisibility();
Any reason why we need the isFirstLoad distinction here? Can't we just run the
else branch always?

[Joao da Silva, 2012-04-27 13:26:43]

Testing this over X forwarding over VPN is great fun :-) PTAL

http://codereview.chromium.org/10236003/diff/19001/chrome/browser/resources/p...
File chrome/browser/resources/policy.js (right):

http://codereview.chromium.org/10236003/diff/19001/chrome/browser/resources/p...
chrome/browser/resources/policy.js:232: policy.updatePolicyVisibility();
On 2012/04/27 12:52:58, Mattias Nissler wrote:
> Any reason why we need the isFirstLoad distinction here? Can't we just run the
> else branch always?

I didn't look at first. This was the original code, from
http://codereview.chromium.org/7828042/diff/1/chrome/browser/resources/policy....
collapseExpandedCells() and updatePolicyVisibility() can both handle an empty
template, and I've verified that it works, so I've removed this flag too.

[Mattias Nissler (ping if slow), 2012-04-27 13:34:20]

On 2012/04/27 13:26:43, Joao da Silva wrote:
> Testing this over X forwarding over VPN is great fun :-) PTAL
> 
>
http://codereview.chromium.org/10236003/diff/19001/chrome/browser/resources/p...
> File chrome/browser/resources/policy.js (right):
> 
>
http://codereview.chromium.org/10236003/diff/19001/chrome/browser/resources/p...
> chrome/browser/resources/policy.js:232: policy.updatePolicyVisibility();
> On 2012/04/27 12:52:58, Mattias Nissler wrote:
> > Any reason why we need the isFirstLoad distinction here? Can't we just run
the
> > else branch always?
> 
> I didn't look at first. This was the original code, from
>
http://codereview.chromium.org/7828042/diff/1/chrome/browser/resources/policy....
> collapseExpandedCells() and updatePolicyVisibility() can both handle an empty
> template, and I've verified that it works, so I've removed this flag too.

Right, what about the other question regarding the test?

[Joao da Silva, 2012-04-27 13:46:29]

Somehow missed that comment; it has been addressed in the latest patch set.

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10236003/diff/14001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:1008:
ResetServiceAndStore();
On 2012/04/27 12:52:58, Mattias Nissler wrote:
> On 2012/04/27 11:24:12, Joao da Silva wrote:
> > On 2012/04/27 09:03:27, Mattias Nissler wrote:
> > > Why is this necessary?
> > 
> > Because store_, as created from SetUp, is already initialized (because the
> > provider_ is also initialized there).
> 
> Can't we just initialize with IsInitializationComplete() returning false, and
> then, as the last step in SetUp switch it to true (so the other tests don't
> fail). Then, in this test you only need put another override to false. Would
> that work?

I've realized that the store can be uninitialized on all the other tests, so
configuring IsInitializationComplete() to return false at SetUp actually works.

[Mattias Nissler (ping if slow), 2012-04-27 14:10:23]

LGTM

[commit-bot: I haz the power, 2012-04-27 14:11:25]

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

[commit-bot: I haz the power, 2012-04-27 14:11:42]

Presubmit check for 10236003-25002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui
    chrome/browser/resources

Presubmit checks took 2.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Joao da Silva, 2012-04-27 14:19:29]

@James: please review ui/webui and resources/. Thanks!

[James Hawkins, 2012-04-27 17:13:08]

LGTM with optional nit.

http://codereview.chromium.org/10236003/diff/25002/chrome/browser/ui/webui/po...
File chrome/browser/ui/webui/policy_ui.h (right):

http://codereview.chromium.org/10236003/diff/25002/chrome/browser/ui/webui/po...
chrome/browser/ui/webui/policy_ui.h:72: base::WeakPtrFactory<PolicyUIHandler>
weak_ptr_factory_;
Optional nit: Precedence says this should be 'weak_factory_'.

[Joao da Silva, 2012-04-27 17:44:06]

Thanks for the review!

http://codereview.chromium.org/10236003/diff/25002/chrome/browser/ui/webui/po...
File chrome/browser/ui/webui/policy_ui.h (right):

http://codereview.chromium.org/10236003/diff/25002/chrome/browser/ui/webui/po...
chrome/browser/ui/webui/policy_ui.h:72: base::WeakPtrFactory<PolicyUIHandler>
weak_ptr_factory_;
On 2012/04/27 17:13:08, James Hawkins wrote:
> Optional nit: Precedence says this should be 'weak_factory_'.

Done. Out of curiosity, the tools say:

# cd src/chrome
# git grep weak_factory_ | cut -f1 -d: | uniq | wc -l
140
# git grep weak_ptr_factory_ | cut -f1 -d: | uniq | wc -l
135

[commit-bot: I haz the power, 2012-04-27 17:46:05]

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

[James Hawkins, 2012-04-27 18:07:24]

http://codereview.chromium.org/10236003/diff/25002/chrome/browser/ui/webui/po...
File chrome/browser/ui/webui/policy_ui.h (right):

http://codereview.chromium.org/10236003/diff/25002/chrome/browser/ui/webui/po...
chrome/browser/ui/webui/policy_ui.h:72: base::WeakPtrFactory<PolicyUIHandler>
weak_ptr_factory_;
On 2012/04/27 17:44:06, Joao da Silva wrote:
> On 2012/04/27 17:13:08, James Hawkins wrote:
> > Optional nit: Precedence says this should be 'weak_factory_'.
> 
> Done. Out of curiosity, the tools say:
> 
> # cd src/chrome
> # git grep weak_factory_ | cut -f1 -d: | uniq | wc -l
> 140
> # git grep weak_ptr_factory_ | cut -f1 -d: | uniq | wc -l
> 135

We did this grep earlier this week, but on the whole code base.  It was
something like 1200 : 700.

[commit-bot: I haz the power, 2012-04-27 20:02:57]

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

[commit-bot: I haz the power, 2012-05-02 08:30:09]

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

[commit-bot: I haz the power, 2012-05-02 10:04:16]

Change committed as 134886