Extract AD policy scheduler into separate class

components/policy/core/common/policy_scheduler.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_POLICY_CORE_COMMON_POLICY_SCHEDULER_H_
+#define COMPONENTS_POLICY_CORE_COMMON_POLICY_SCHEDULER_H_
+
+#include <memory>
+
+#include "base/callback.h"
+#include "base/cancelable_callback.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
+#include "components/policy/policy_export.h"
+
+namespace policy {
+
+// Scheduler for driving repeated asynchronous tasks such as e.g. policy
+// fetches. Subsequent tasks are guaranteed not to overlap. Tasks are posted to
+// the current thread and therefore must not block (suitable e.g. for
+// asynchronous D-Bus calls).
+// Tasks scheduling begins immediately after instantiation of the class. Upon
+// destruction, scheduled but not yet started tasks are cancelled. The result of
+// started but not finished tasks is NOT reported.
+class POLICY_EXPORT PolicyScheduler {
+ public:
+  // Callback for the task to report success or failure.
+  using TaskCallback = base::OnceCallback<void(bool success)>;
+
+  // Task to be performed at regular intervals. The task takes a |callback| to
+  // return success or failure.
+  using Task = base::RepeatingCallback<void(TaskCallback callback)>;
+
+  // Callback for PolicyScheduler to report success or failure of the tasks.
+  using SchedulerCallback = base::RepeatingCallback<void(bool success)>;
+
+  // Defines the |task| to be run every |interval| and the |callback| for the
+  // scheduler to report the result. In case a task runs longer than the
+  // |interval| the next task is backed off the completion time of the previous
+  // task by |backoff_interval|. (Intervals are computed as the time difference
+  // between the end of the previous and the start of the subsequent task.)
+  PolicyScheduler(Task task,

[pmarko, 2017/06/19 10:14:37]

Although it means repeating the comment on the class, consider mentioning that
the constructor automatically schedules a task to run ASAP (so the caller don't
think they should still call ScheduleTask() and also it might be surprising to
some that |interval| is not used for the first task).

[Thiemo Nagel, 2017/06/19 22:29:51]

Done.

+                  SchedulerCallback callback,
+                  base::TimeDelta interval,
+                  base::TimeDelta backoff_interval);
+  ~PolicyScheduler();
+
+  // Schedules a task to run after |delay|. Deletes any previously scheduled but
+  // not yet started task.
+  void ScheduleTask(base::TimeDelta delay);

[pmarko, 2017/06/19 10:14:37]

Do we need the delay parameter in the public API? I.e., is it a usecase to plan
a refresh after a specified delay from outside? If not, we could call this
ScheduleTaskNow and drop the parameter.
If this is only the testing, I think the test calling
ScheduleTask(TimeDelta::Max()) could use the regular SchuedleNextTask-logic with
interval==TimeDelta::Max to achieve the same behavior.

Also worth mentioning in a comment what happens if a task is currently running
(i.e. backoff logic also applies here).

[Thiemo Nagel, 2017/06/19 22:29:51]

Good point. Right now, I'm not aware of a use case other than testing.
Imho it's cleaner if tests adhere to the public interface. Thus I'd tend towards
keeping it as it is (least bad solution), but maybe you have another idea? 
Actually, it doesn't. I've clarified this in the comment.

[Thiemo Nagel, 2017/06/19 22:50:04]

Nah, you're right, it does.

And now I'm thinking, whether I should add a second "backoff" parameter to 
ScheduleTask().  The only use case so far is when a user clicks the "update
policy" button.  Ideally we'd like to have 0 timeout and 0 backoff for that
whereas backoff for the scheduled policy fetch (90 minutes interval) could seem
reasonable at maybe 10 minutes.  (Though I hope it is never hit in practice.)

[pmarko, 2017/06/20 20:01:05]

Re: Omitting the parameter completely vs leaving it in for testing:
I think the Reschedule test would be fine without calling this. It passes
TimeDelta::Max() as interval to the ctor, so after the first task is executed,
the second task should be automatically scheduled at TimeDelta::Max() delay. I
believe we can just skip that call in the test.

Re: Second back-off parameter:
Hrm, simply setting backoff=0 would mean wasting CPU cycles unnecessarily
though, wouldn't it? (while the not-yet-finished task is still running)

I'm not sure about the purpose of backoff-process in general as it is used now -
currently 1s gets passed in, which would functionally be almost equivalent to
starting the next task immediately after the previous one finished if it took
longer than interval. How do we want this to behave?

[Thiemo Nagel, 2017/06/21 07:05:31]

Good point.  Done.
It would cause tasks to run back-to-back in case they overlap.  But maybe that's
actually the best thing to happen -- especially in case the "refresh policy"
button is pressed by the user, the refresh should happen asap.
Now that you ask this: I think the intention was to prevent very tight loops,
but I don't think this can happen realistically.  It seems to make more sense to
remove it.

[pmarko, 2017/06/21 08:04:54]

Yeah, agree that it shouldn't be make a big difference if it's 1s or 0.

+
+ private:
+  // Automatically schedules next task based on |interval_| or
+  // |backoff_interval_|. Deletes any previously scheduled tasks.
+  void ScheduleNextTask();
+
+  // Actually executes the scheduled task.
+  void RunScheduledTask();
+
+  // Reports back the |result| of the previous task and schedules the next one.
+  void OnTaskDone(bool result);
+
+  Task task_;
+  SchedulerCallback callback_;
+  const base::TimeDelta interval_;
+  const base::TimeDelta backoff_interval_;
+
+  // Whether a task is in progress.
+  bool task_in_progress_ = false;
+
+  // Whether there had been an overlap of tasks and thus the next task needs to
+  // be scheduled at |backoff_interval_|.
+  bool backoff_ = false;
+
+  base::TimeTicks last_task_;

[pmarko, 2017/06/19 10:14:37]

Please add a comment that this captures the _end_ time of the last task.

[Thiemo Nagel, 2017/06/19 22:29:51]

Done.

+  const base::TimeTicks startup_ = base::TimeTicks::Now();
+  std::unique_ptr<base::CancelableClosure> job_;
+
+  // Must be last member.
+  base::WeakPtrFactory<PolicyScheduler> weak_ptr_factory_{this};
+
+  DISALLOW_COPY_AND_ASSIGN(PolicyScheduler);
+};
+
+}  // namespace policy
+
+#endif  // COMPONENTS_POLICY_CORE_COMMON_POLICY_SCHEDULER_H_