Refresh policies from DM server periodically

Refresh policies from DM server periodically

Also, this takes care of retrying when policy fetching fails. 


BUG=62487
TEST=unit tests: DeviceManagementPolicyProviderTest.*:DeviceTokenFetcherTest.*

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

[Jakob Kummerow (corp), 2010-11-22 13:06:35]

Please review.

[danno, 2010-11-22 13:47:49]

Here you go...

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_cache.cc (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_cache.cc:111: bool
new_policy_differs = !(value->Equals(policy_.get()));
const bool

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:117: << code << ")";
you may want to add a "retrying in x millis ...." to the log

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:138:
ScheduleRefreshTask(token_fetch_error_delay_ms_);
Perhaps a second, token-specific task to address my concern below?

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:173:
token_fetcher_->AddObserver(this);
What ensures that the observer gets removed? You probably need a registrar
object or the like.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:194: void
DeviceManagementPolicyProvider::RefreshTaskExecute() {
Combining both the token request and the policy request into one seems to not
appropriately split responsibilities, and in any case the name becomes
misleading. Refresh==get token if I don't have one and refresh policy. I think I
would like this better if this routine was a little less of a big hammer that
both faults in the token and sends policy requests, e.g. I see only the token
fetching code as an appropriate response to OnTokenError(), not policy fetching
code.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:195:
refresh_task_pending_ = false;
DCHECK(refresh_task_pending_) before?

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.h:62: // Give unit tests
the ability to override timeout settings.
All of these should definitely be private/protected if they are only for unit
testing.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.h:108: int64
policy_refresh_max_random_earlier_ms);
Why pass these in? You only pass in the values of the member anyway, so why
don't you just make it an instance method that works directly on the members?

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher_unittest.cc:144:
fetcher_->AddObserver(&observer);
Who removes the observer from the fetcher?

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher_unittest.cc:170:
fetcher_->AddObserver(&observer);
Who removes the observer from the fetcher?

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher_unittest.cc:173:
backend_->AllShouldFail();
indentation above should be 2

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/mock_devi...
File chrome/browser/policy/mock_device_management_backend.cc (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/mock_devi...
chrome/browser/policy/mock_device_management_backend.cc:32:
policy_works_later_(false),
do you need a separate boolean to track this? the count_ variables seem
sufficient when they are non-zero.

[Jakob Kummerow (corp), 2010-11-22 16:56:08]

Addressed your comments, please review again.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_cache.cc (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_cache.cc:111: bool
new_policy_differs = !(value->Equals(policy_.get()));
On 2010/11/22 13:47:50, danno wrote:
> const bool

Done.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:117: << code << ")";
On 2010/11/22 13:47:50, danno wrote:
> you may want to add a "retrying in x millis ...." to the log

Done.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:138:
ScheduleRefreshTask(token_fetch_error_delay_ms_);
On 2010/11/22 13:47:50, danno wrote:
> Perhaps a second, token-specific task to address my concern below?

See my answer below.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:173:
token_fetcher_->AddObserver(this);
On 2010/11/22 13:47:50, danno wrote:
> What ensures that the observer gets removed? You probably need a registrar
> object or the like.

Done. 
Added a registrar mechanism.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:194: void
DeviceManagementPolicyProvider::RefreshTaskExecute() {
On 2010/11/22 13:47:50, danno wrote:
> Combining both the token request and the policy request into one seems to not
> appropriately split responsibilities, and in any case the name becomes
> misleading. Refresh==get token if I don't have one and refresh policy. I think
I
> would like this better if this routine was a little less of a big hammer that
> both faults in the token and sends policy requests, e.g. I see only the token
> fetching code as an appropriate response to OnTokenError(), not policy
fetching
> code.

I don't see what's wrong with a task that assesses the overall situation and
then does whatever needs to be done. 
Also, splitting this into two separate tasks would mean to perform the same
functionality with almost twice as much code. 

Discussed offline: will keep it as it is.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:195:
refresh_task_pending_ = false;
On 2010/11/22 13:47:50, danno wrote:
> DCHECK(refresh_task_pending_) before?

Done.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.h:62: // Give unit tests
the ability to override timeout settings.
On 2010/11/22 13:47:50, danno wrote:
> All of these should definitely be private/protected if they are only for unit
> testing.

Done.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.h:108: int64
policy_refresh_max_random_earlier_ms);
On 2010/11/22 13:47:50, danno wrote:
> Why pass these in? You only pass in the values of the member anyway, so why
> don't you just make it an instance method that works directly on the members? 

Done.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher_unittest.cc:144:
fetcher_->AddObserver(&observer);
On 2010/11/22 13:47:50, danno wrote:
> Who removes the observer from the fetcher?

Done.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher_unittest.cc:170:
fetcher_->AddObserver(&observer);
On 2010/11/22 13:47:50, danno wrote:
> Who removes the observer from the fetcher?

Done.

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher_unittest.cc:173:
backend_->AllShouldFail();
On 2010/11/22 13:47:50, danno wrote:
> indentation above should be 2

Done.
(Of course -- copy-paste artifact.)

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/mock_devi...
File chrome/browser/policy/mock_device_management_backend.cc (right):

http://codereview.chromium.org/5219006/diff/1/chrome/browser/policy/mock_devi...
chrome/browser/policy/mock_device_management_backend.cc:32:
policy_works_later_(false),
On 2010/11/22 13:47:50, danno wrote:
> do you need a separate boolean to track this? the count_ variables seem
> sufficient when they are non-zero.

Done.

[danno, 2010-11-23 12:10:14]

getting much closer...

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:137: LOG(WARNING) <<
"This device is not managed.";
Change to a INFO, or better yet VLOG.

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.h:120:
scoped_ptr<DeviceTokenFetcher::ObserverRegistrar> registrar_;
Doesn't need to be a scoped_ptr. Just declare it as a member.

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:90: // This line is necessary to
work around Windows compiler weirdness.
Perhaps something more specific like: Without the following explicit type for
file_util::Delete, the Windows compiler is unable to properly match to the
correct NewRunnableFunction specialization and generates an error.

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:160:
registrar.reset(NULL);
Aternitavely, you can add a RemoveObserver method to the registrar, which you
can call here. That way the registrar doesn't have to be a scoped_ptr.

[Jakob Kummerow (corp), 2010-11-23 12:25:26]

Addressed your comments.

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.cc:137: LOG(WARNING) <<
"This device is not managed.";
On 2010/11/23 12:10:14, danno wrote:
> Change to a INFO, or better yet VLOG.

Done.

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_policy_provider.h:120:
scoped_ptr<DeviceTokenFetcher::ObserverRegistrar> registrar_;
On 2010/11/23 12:10:14, danno wrote:
> Doesn't need to be a scoped_ptr. Just declare it as a member.

Yes it does, because I pass in the token fetcher (which is a scoped_refptr and
is initialized pretty late) in the registrar's constructor. I find that approach
more elegant than having to specify the token fetcher on each AddObserver()
call, since the registrar is only responsible for one token fetcher anyway.

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:90: // This line is necessary to
work around Windows compiler weirdness.
On 2010/11/23 12:10:14, danno wrote:
> Perhaps something more specific like: Without the following explicit type for
> file_util::Delete, the Windows compiler is unable to properly match to the
> correct NewRunnableFunction specialization and generates an error.

Done.
I changed the whole workaround to a solution Mattias considers more elegant,
namely explicit instantiation of the NewRunnableFunction<> template.

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/5219006/diff/18001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:160:
registrar.reset(NULL);
On 2010/11/23 12:10:14, danno wrote:
> Aternitavely, you can add a RemoveObserver method to the registrar, which you
> can call here. That way the registrar doesn't have to be a scoped_ptr.

That's the alternative, yes. It would mean more code. Would you find that
better, or are you just providing ideas for discussion again? ;-)

[Jakob Kummerow (corp), 2010-11-23 12:51:36]

Here you go, addressed your offline comments: 

• Init() method for DeviceTokenFetcher::ObserverRegistrar, allowing it to be
used as a regular class member
• make use of DeviceTokenFetcher::ObserverRegistrar::RemoveAll() in unit test

[danno, 2010-11-23 12:54:00]

lgtm.