Fix behavior of cloud policy refreshes scheduled for soon

Fix behavior of cloud policy refreshes scheduled for soon

The CloudPolicyRefreshScheduler class was implemented in such a way
that refreshes scheduled for soon using the RefreshSoon method could
be actually overridden by subsequent changes resulting in re-scheduling
of the refresh.

As an example of case which would result in ignoring a soon refresh is the OnStoreLoaded observer called when there's a non-empty response map in the policy client. This observer in this case would update the last_refresh_ field when it's empty, breaking some assumptions in the existing code.

This CL introduces a different implementation for the RefreshSoon method which ensures that the scheduled soon refresh cannot be overridden by subsequent refresh updates.

BUG=644304
TEST=unit test

Committed: https://crrev.com/6660a9d0880ad8cc3847fba07d98ef1f05d3869b
Cr-Commit-Position: refs/heads/master@{#429632}

[emaxx, 2016-11-02 19:45:06]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:45:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2016-11-02 20:13:43]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 20:13:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2016-11-02 20:21:32]

Description was changed from

==========
Fix behavior of cloud policy refreshes scheduled for soon

The |CloudPolicyRefreshScheduler| class was implemented in such a way
that refreshes scheduled for soon using the |RefreshSoon| method could
be actually overridden by subsequent changes resulting in re-scheduling
of the refresh.

BUG=644304
==========

to

==========
Fix behavior of cloud policy refreshes scheduled for soon

The CloudPolicyRefreshScheduler class was implemented in such a way
that refreshes scheduled for soon using the RefreshSoon method could
be actually overridden by subsequent changes resulting in re-scheduling
of the refresh.

As an example of case which would result in ignoring a soon refresh is the
OnStoreLoaded observer called when there's a non-empty response map in the
policy client. This observer in this case would update the last_refresh_ field
when it's empty, breaking some assumptions in the existing code.

This CL introduces a different implementation for the RefreshSoon method which
ensures that the scheduled soon refresh cannot be overridden by subsequent
refresh updates.

BUG=644304
==========

[emaxx, 2016-11-02 20:24:09]

emaxx@chromium.org changed reviewers:
+ atwilson@chromium.org, mnissler@chromium.org

[emaxx, 2016-11-02 20:24:09]

Drew, PTAL.

Mattias: FYI, but happy to receive any feedback from you too.

[commit-bot: I haz the power, 2016-11-02 21:13:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 21:13:26]

Dry run: This issue passed the CQ dry run.

[Mattias Nissler (ping if slow), 2016-11-03 09:47:34]

FWIW, looks good to me.

[Andrew T Wilson (Slow), 2016-11-03 15:02:56]

This is a bit wonky - it feels like what we're doing is *sometimes* ignoring
RefreshAfter() calls.

So if I call RefreshSoon() then RefreshAfter(1000), I'll get a single refresh.

But if I call RefreshAfter(0) then RefreshAfter(1000), I'll get two refreshes
despite the fact that RefreshAfter(0) is functionally equivalent to
RefreshSoon().

Is that really what we want?

https://codereview.chromium.org/2472593003/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h
(right):

https://codereview.chromium.org/2472593003/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h:60: //
Requests a policy refresh to be performed soon.
BTW, this API documentation is confusing. We should say what "soon" means.

https://codereview.chromium.org/2472593003/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h:106: //
Schedules a policy refresh to happen approximately not later than after
This is confusing. What does approximately mean here? Can we just say "no later
than |delta_ms| msecs after |last_refresh_|"?

[emaxx, 2016-11-03 15:33:38]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 15:34:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2016-11-03 15:35:56]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 15:36:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2016-11-03 16:13:10]

PTAL.


On 2016/11/03 15:02:56, Andrew T Wilson (Slow) wrote:
> This is a bit wonky - it feels like what we're doing is *sometimes* ignoring
> RefreshAfter() calls.

Agreed, it'd better to decide this in the caller code rather than inside
RefreshAfter. I've updated the CL.



> So if I call RefreshSoon() then RefreshAfter(1000), I'll get a single refresh.
> 
> But if I call RefreshAfter(0) then RefreshAfter(1000), I'll get two refreshes
> despite the fact that RefreshAfter(0) is functionally equivalent to
> RefreshSoon().

Note that there's always a single refresh, as the previously scheduled job is
always canceled.

https://codereview.chromium.org/2472593003/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h
(right):

https://codereview.chromium.org/2472593003/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h:60: //
Requests a policy refresh to be performed soon.
On 2016/11/03 15:02:56, Andrew T Wilson (Slow) wrote:
> BTW, this API documentation is confusing. We should say what "soon" means.

OK, I've updated it.
Does it sound better to you?

https://codereview.chromium.org/2472593003/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h:106: //
Schedules a policy refresh to happen approximately not later than after
On 2016/11/03 15:02:56, Andrew T Wilson (Slow) wrote:
> This is confusing. What does approximately mean here? Can we just say "no
later
> than |delta_ms| msecs after |last_refresh_|"?

Done.

[emaxx, 2016-11-03 16:14:04]

Description was changed from

==========
Fix behavior of cloud policy refreshes scheduled for soon

The CloudPolicyRefreshScheduler class was implemented in such a way
that refreshes scheduled for soon using the RefreshSoon method could
be actually overridden by subsequent changes resulting in re-scheduling
of the refresh.

As an example of case which would result in ignoring a soon refresh is the
OnStoreLoaded observer called when there's a non-empty response map in the
policy client. This observer in this case would update the last_refresh_ field
when it's empty, breaking some assumptions in the existing code.

This CL introduces a different implementation for the RefreshSoon method which
ensures that the scheduled soon refresh cannot be overridden by subsequent
refresh updates.

BUG=644304
==========

to

==========
Fix behavior of cloud policy refreshes scheduled for soon

The CloudPolicyRefreshScheduler class was implemented in such a way
that refreshes scheduled for soon using the RefreshSoon method could
be actually overridden by subsequent changes resulting in re-scheduling
of the refresh.

As an example of case which would result in ignoring a soon refresh is the
OnStoreLoaded observer called when there's a non-empty response map in the
policy client. This observer in this case would update the last_refresh_ field
when it's empty, breaking some assumptions in the existing code.

This CL introduces a different implementation for the RefreshSoon method which
ensures that the scheduled soon refresh cannot be overridden by subsequent
refresh updates.

BUG=644304
TEST=unit test
==========

[commit-bot: I haz the power, 2016-11-03 16:28:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 16:28:02]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2016-11-03 17:23:44]

lgtm

[emaxx, 2016-11-03 17:28:34]

The CQ bit was checked by emaxx@chromium.org

[commit-bot: I haz the power, 2016-11-03 17:29:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 17:35:01]

Description was changed from

==========
Fix behavior of cloud policy refreshes scheduled for soon

The CloudPolicyRefreshScheduler class was implemented in such a way
that refreshes scheduled for soon using the RefreshSoon method could
be actually overridden by subsequent changes resulting in re-scheduling
of the refresh.

As an example of case which would result in ignoring a soon refresh is the
OnStoreLoaded observer called when there's a non-empty response map in the
policy client. This observer in this case would update the last_refresh_ field
when it's empty, breaking some assumptions in the existing code.

This CL introduces a different implementation for the RefreshSoon method which
ensures that the scheduled soon refresh cannot be overridden by subsequent
refresh updates.

BUG=644304
TEST=unit test
==========

to

==========
Fix behavior of cloud policy refreshes scheduled for soon

The CloudPolicyRefreshScheduler class was implemented in such a way
that refreshes scheduled for soon using the RefreshSoon method could
be actually overridden by subsequent changes resulting in re-scheduling
of the refresh.

As an example of case which would result in ignoring a soon refresh is the
OnStoreLoaded observer called when there's a non-empty response map in the
policy client. This observer in this case would update the last_refresh_ field
when it's empty, breaking some assumptions in the existing code.

This CL introduces a different implementation for the RefreshSoon method which
ensures that the scheduled soon refresh cannot be overridden by subsequent
refresh updates.

BUG=644304
TEST=unit test
==========

[commit-bot: I haz the power, 2016-11-03 17:35:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-03 17:48:12]

Description was changed from

==========
Fix behavior of cloud policy refreshes scheduled for soon

The CloudPolicyRefreshScheduler class was implemented in such a way
that refreshes scheduled for soon using the RefreshSoon method could
be actually overridden by subsequent changes resulting in re-scheduling
of the refresh.

As an example of case which would result in ignoring a soon refresh is the
OnStoreLoaded observer called when there's a non-empty response map in the
policy client. This observer in this case would update the last_refresh_ field
when it's empty, breaking some assumptions in the existing code.

This CL introduces a different implementation for the RefreshSoon method which
ensures that the scheduled soon refresh cannot be overridden by subsequent
refresh updates.

BUG=644304
TEST=unit test
==========

to

==========
Fix behavior of cloud policy refreshes scheduled for soon

The CloudPolicyRefreshScheduler class was implemented in such a way
that refreshes scheduled for soon using the RefreshSoon method could
be actually overridden by subsequent changes resulting in re-scheduling
of the refresh.

As an example of case which would result in ignoring a soon refresh is the
OnStoreLoaded observer called when there's a non-empty response map in the
policy client. This observer in this case would update the last_refresh_ field
when it's empty, breaking some assumptions in the existing code.

This CL introduces a different implementation for the RefreshSoon method which
ensures that the scheduled soon refresh cannot be overridden by subsequent
refresh updates.

BUG=644304
TEST=unit test

Committed: https://crrev.com/6660a9d0880ad8cc3847fba07d98ef1f05d3869b
Cr-Commit-Position: refs/heads/master@{#429632}
==========

[commit-bot: I haz the power, 2016-11-03 17:48:13]

Patchset 4 (id:??) landed as
https://crrev.com/6660a9d0880ad8cc3847fba07d98ef1f05d3869b
Cr-Commit-Position: refs/heads/master@{#429632}