Refactor the windows policy provider to use AsynchronousPolicyProvider.

Refactor the windows policy provider to use AsynchronousPolicyProvider.

BUG=66453, 65094
TEST=*Policy.*

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

[danno, 2010-12-21 16:12:23]

please review.

[Mattias Nissler (ping if slow), 2010-12-21 16:42:24]

A few initial comments, will take a closer look tomorrow.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.cc (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:32: BrowserThread::FILE,
FROM_HERE,
UI!

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:76: void ReloadFromTask();
I'm not so sure whether we really want all this reload stuff in the base class.
The two implementations are actually different: FileBased does periodic checks
unconditionally, while Windows only does that as a fallback in case the watchers
fail.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider_unittest.cc (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:47: 10);
Linebreak after = and all arguments on the same line?

[Jakob Kummerow, 2010-12-21 16:56:10]

Found a few nits ;-)

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.cc (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:27: // file thread. Posting
to a to the file directly before the file thread is
nit: "Posting a task to the file thread before the file thread is..."

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:113: // referer that keeps
us alive, so we should not Cancel() it.
nit: referrer

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_loader.h (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.h:40: // Creates the file path
watcher and configures it watch |config_file_path_|.
nit: missing "to"

[Mattias Nissler (ping if slow), 2010-12-22 08:45:53]

A couple more nits, looks pretty good in general.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:81: // Finishes loader
finitialization after the threading system has been fully
s/finitialization/initialization/

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:105: // The interval that a
policy reload will be triggered as a fallback.
s/that/at which/?

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_loader_win.cc (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_loader_win.cc:53:
user_policy_changed_event_.handle(), this)) {
Maybe collapse the ifs into a single one? (also below)

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_loader_win.cc:69: }
Can drop the curlies here.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_loader_win.h (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_loader_win.h:50: bool
machine_policy_watcher_failed_;
DISALLOW_COPY_AND_ASSIGN

[danno, 2010-12-22 11:02:17]

addressed feedback, please take another look.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.cc (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:27: // file thread. Posting
to a to the file directly before the file thread is
On 2010/12/21 16:56:10, jkummerow wrote:
> nit: "Posting a task to the file thread before the file thread is..."

Done.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:32: BrowserThread::FILE,
FROM_HERE,
Done, hadn't merged with my fix yet.
On 2010/12/21 16:42:24, Mattias Nissler wrote:
> UI!

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.cc:113: // referer that keeps
us alive, so we should not Cancel() it.
On 2010/12/21 16:56:10, jkummerow wrote:
> nit: referrer

Done.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:76: void ReloadFromTask();
Right, the reasons are different, but the underlying logic is virtually
identical. I originally had these separately, and the code was so similar I
merged them.

On 2010/12/21 16:42:24, Mattias Nissler wrote:
> I'm not so sure whether we really want all this reload stuff in the base
class.
> The two implementations are actually different: FileBased does periodic checks
> unconditionally, while Windows only does that as a fallback in case the
watchers
> fail.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:81: // Finishes loader
finitialization after the threading system has been fully
On 2010/12/22 08:45:53, Mattias Nissler wrote:
> s/finitialization/initialization/

Done.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_loader.h:105: // The interval that a
policy reload will be triggered as a fallback.
On 2010/12/22 08:45:53, Mattias Nissler wrote:
> s/that/at which/?

Done.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider_unittest.cc (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:47: 10);
On 2010/12/21 16:42:24, Mattias Nissler wrote:
> Linebreak after = and all arguments on the same line?

Done.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_loader_win.cc (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_loader_win.cc:53:
user_policy_changed_event_.handle(), this)) {
On 2010/12/22 08:45:53, Mattias Nissler wrote:
> Maybe collapse the ifs into a single one? (also below)

Done.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_loader_win.cc:69: }
On 2010/12/22 08:45:53, Mattias Nissler wrote:
> Can drop the curlies here.

Done.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_loader_win.h (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_loader_win.h:50: bool
machine_policy_watcher_failed_;
On 2010/12/22 08:45:53, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_loader.h (right):

http://codereview.chromium.org/6091002/diff/15001/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_loader.h:40: // Creates the file path
watcher and configures it watch |config_file_path_|.
On 2010/12/21 16:56:10, jkummerow wrote:
> nit: missing "to"

Done.

[Mattias Nissler (ping if slow), 2010-12-22 11:09:31]

lgtm pending trybot happiness and the missing changes.

[danno, 2010-12-22 16:43:59]

take a quick look at the windows tests, I had to improve them to get them to
work.

[Mattias Nissler (ping if slow), 2010-12-22 16:47:30]

still lgtm.