Fixed cloud policy refresh API to return correct value.

Fixed cloud policy refresh API to return correct value.

Removed CloudPolicyRefreshScheduler::refresh_delay() because it returned
a value that was not the actual value used in practice.

Replaced with GetRefreshDelay() which returns the actual value that is used
by the refresh scheduler.

BUG=628991
TBR=xiyuan

Committed: https://crrev.com/de68c7646f3169421b1d2ae201ecdce0c459dc6f
Cr-Commit-Position: refs/heads/master@{#408430}

[Andrew T Wilson (Slow), 2016-07-19 14:05:43]

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

[commit-bot: I haz the power, 2016-07-19 14:06:06]

Dry run: 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-07-19 14:51:25]

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

[commit-bot: I haz the power, 2016-07-19 14:51:25]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2016-07-19 14:54:56]

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

[Andrew T Wilson (Slow), 2016-07-19 14:54:57]

PTAL

[Andrew T Wilson (Slow), 2016-07-19 14:54:57]

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

[Andrew T Wilson (Slow), 2016-07-19 14:54:57]

PTAL

[emaxx, 2016-07-20 12:45:55]

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

https://codereview.chromium.org/2159223002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:88:
int64_t CloudPolicyRefreshScheduler::GetRefreshDelay() const {
It looks a bit strange to me that this method behaves in a different way than
SetRefreshDelay (i.e. generally one may expect that getter and setter don't
contradict with each other).

So we could think about some better names for the getter / the setter / the
class data member.

But I'm wondering - isn't this a bug that the value set through this setter
(which currently comes from a policy) is actually ignored when the invalidations
are available? It sounds a bit odd, and I don't see any mentions of this
behavior in the policy docs.
If you agree that this should be fixed, then, IMO, the interface can be made
more straightforward: if the setter was called, then store the new value and
always use it; otherwise, inside the getter do the logic that chooses between
kDefaultRefreshDelayMs and kWithInvalidationsRefreshDelayMs.

[Andrew T Wilson (Slow), 2016-07-20 16:34:00]

On 2016/07/20 12:45:55, emaxx wrote:
>
https://codereview.chromium.org/2159223002/diff/20001/components/policy/core/...
> File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
> (right):
> 
>
https://codereview.chromium.org/2159223002/diff/20001/components/policy/core/...
> components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:88:
> int64_t CloudPolicyRefreshScheduler::GetRefreshDelay() const {
> It looks a bit strange to me that this method behaves in a different way than
> SetRefreshDelay (i.e. generally one may expect that getter and setter don't
> contradict with each other).
> 
> So we could think about some better names for the getter / the setter / the
> class data member.
> 
> But I'm wondering - isn't this a bug that the value set through this setter
> (which currently comes from a policy) is actually ignored when the
invalidations
> are available? It sounds a bit odd, and I don't see any mentions of this
> behavior in the policy docs.
> If you agree that this should be fixed, then, IMO, the interface can be made
> more straightforward: if the setter was called, then store the new value and
> always use it; otherwise, inside the getter do the logic that chooses between
> kDefaultRefreshDelayMs and kWithInvalidationsRefreshDelayMs.

I don't want to change the behavior of SetRefreshDelay() because it is WAI - the
person setting the policy doesn't know anything about whether invalidations are
available or not (depends on network config, etc) so we should not take the
policy as the final source of truth.

I will do the following:

1) Change Get/SetRefreshDelay() to SetDesiredRefreshDelay() and
GetActualRefreshDelay() to avoid confusion about Get/Set not returning the same
value.
2) Update policy docs to reflect the fact that the delay specified by policy may
be extended if invalidations are available on the device.

Sound reasonable?

[emaxx, 2016-07-20 16:36:09]

On 2016/07/20 16:34:00, Andrew T Wilson (Slow) wrote:
> On 2016/07/20 12:45:55, emaxx wrote:
> >
>
https://codereview.chromium.org/2159223002/diff/20001/components/policy/core/...
> > File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2159223002/diff/20001/components/policy/core/...
> > components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:88:
> > int64_t CloudPolicyRefreshScheduler::GetRefreshDelay() const {
> > It looks a bit strange to me that this method behaves in a different way
than
> > SetRefreshDelay (i.e. generally one may expect that getter and setter don't
> > contradict with each other).
> > 
> > So we could think about some better names for the getter / the setter / the
> > class data member.
> > 
> > But I'm wondering - isn't this a bug that the value set through this setter
> > (which currently comes from a policy) is actually ignored when the
> invalidations
> > are available? It sounds a bit odd, and I don't see any mentions of this
> > behavior in the policy docs.
> > If you agree that this should be fixed, then, IMO, the interface can be made
> > more straightforward: if the setter was called, then store the new value and
> > always use it; otherwise, inside the getter do the logic that chooses
between
> > kDefaultRefreshDelayMs and kWithInvalidationsRefreshDelayMs.
> 
> I don't want to change the behavior of SetRefreshDelay() because it is WAI -
the
> person setting the policy doesn't know anything about whether invalidations
are
> available or not (depends on network config, etc) so we should not take the
> policy as the final source of truth.
> 
> I will do the following:
> 
> 1) Change Get/SetRefreshDelay() to SetDesiredRefreshDelay() and
> GetActualRefreshDelay() to avoid confusion about Get/Set not returning the
same
> value.
> 2) Update policy docs to reflect the fact that the delay specified by policy
may
> be extended if invalidations are available on the device.
> 
> Sound reasonable?

OK.

[Andrew T Wilson (Slow), 2016-07-26 14:45:40]

emaxx: PTAL

[Andrew T Wilson (Slow), 2016-07-26 14:46:32]

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

[commit-bot: I haz the power, 2016-07-26 14:46:46]

Dry run: 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-07-26 15:15:36]

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

[commit-bot: I haz the power, 2016-07-26 15:15:40]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-07-26 16:25:01]

lgtm

[Andrew T Wilson (Slow), 2016-07-28 13:51:32]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2016-07-28 13:51:52]

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-07-28 13:57:14]

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

[commit-bot: I haz the power, 2016-07-28 13:57:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Andrew T Wilson (Slow), 2016-07-28 17:39:56]

Description was changed from

==========
Fixed cloud policy refresh API to return correct value.

Removed CloudPolicyRefreshScheduler::refresh_delay() because it returned
a value that was not the actual value used in practice.

Replaced with GetRefreshDelay() which returns the actual value that is used
by the refresh scheduler.

BUG=628991
==========

to

==========
Fixed cloud policy refresh API to return correct value.

Removed CloudPolicyRefreshScheduler::refresh_delay() because it returned
a value that was not the actual value used in practice.

Replaced with GetRefreshDelay() which returns the actual value that is used
by the refresh scheduler.

BUG=628991
TBR=xiyuan
==========

[Andrew T Wilson (Slow), 2016-07-28 17:39:56]

atwilson@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Andrew T Wilson (Slow), 2016-07-28 17:40:26]

Adding xiyuan as TBR due to change to policy_ui_handler.cc (renamed function
called from that file)

[Andrew T Wilson (Slow), 2016-07-28 17:40:34]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2016-07-28 17:40:50]

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-07-28 17:45:25]

Description was changed from

==========
Fixed cloud policy refresh API to return correct value.

Removed CloudPolicyRefreshScheduler::refresh_delay() because it returned
a value that was not the actual value used in practice.

Replaced with GetRefreshDelay() which returns the actual value that is used
by the refresh scheduler.

BUG=628991
TBR=xiyuan
==========

to

==========
Fixed cloud policy refresh API to return correct value.

Removed CloudPolicyRefreshScheduler::refresh_delay() because it returned
a value that was not the actual value used in practice.

Replaced with GetRefreshDelay() which returns the actual value that is used
by the refresh scheduler.

BUG=628991
TBR=xiyuan
==========

[commit-bot: I haz the power, 2016-07-28 17:45:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-28 17:47:03]

Description was changed from

==========
Fixed cloud policy refresh API to return correct value.

Removed CloudPolicyRefreshScheduler::refresh_delay() because it returned
a value that was not the actual value used in practice.

Replaced with GetRefreshDelay() which returns the actual value that is used
by the refresh scheduler.

BUG=628991
TBR=xiyuan
==========

to

==========
Fixed cloud policy refresh API to return correct value.

Removed CloudPolicyRefreshScheduler::refresh_delay() because it returned
a value that was not the actual value used in practice.

Replaced with GetRefreshDelay() which returns the actual value that is used
by the refresh scheduler.

BUG=628991
TBR=xiyuan

Committed: https://crrev.com/de68c7646f3169421b1d2ae201ecdce0c459dc6f
Cr-Commit-Position: refs/heads/master@{#408430}
==========

[commit-bot: I haz the power, 2016-07-28 17:47:04]

Patchset 3 (id:??) landed as
https://crrev.com/de68c7646f3169421b1d2ae201ecdce0c459dc6f
Cr-Commit-Position: refs/heads/master@{#408430}