Prevent multiple pending UpdatePolicy tasks.

content/renderer/scheduler/renderer_scheduler_impl.h

Index: content/renderer/scheduler/renderer_scheduler_impl.h
diff --git a/content/renderer/scheduler/renderer_scheduler_impl.h b/content/renderer/scheduler/renderer_scheduler_impl.h
index c908a974a0d8f21cf24f8cb34d36fc053a1d68c8..7fc33c2e647b50641e457132a6d013f2e320d3d8 100644
--- a/content/renderer/scheduler/renderer_scheduler_impl.h
+++ b/content/renderer/scheduler/renderer_scheduler_impl.h
@@ -53,6 +53,7 @@ class CONTENT_EXPORT RendererSchedulerImpl : public RendererScheduler {
 
  private:
   friend class RendererSchedulerImplTest;
+  friend class MockRendererSchedulerImpl;
 
   // Keep RendererSchedulerImpl::TaskQueueIdToString in sync with this enum.
   enum QueueId {
@@ -117,16 +118,31 @@ class CONTENT_EXPORT RendererSchedulerImpl : public RendererScheduler {
   // Returns the current scheduler policy. Must be called from the main thread.
   Policy SchedulerPolicy() const;
 
-  // Posts a call to UpdatePolicy on the control runner to be run after |delay|
-  void PostUpdatePolicyOnControlRunner(base::TimeDelta delay);
+  // Schedules an immediate PolicyUpdate, if there isn't one already pending and
+  // sets |policy_may_need_update_|. Note |incoming_signals_lock_| must be
+  // locked.
+  void ScheduleUrgentPolicyUpdate(const tracked_objects::Location& from_here);
+
+  // Schedules a delayed PolicyUpdate, if there isn't one already pending.
+  // Note |incoming_signals_lock_| must be locked.
+  void ScheduleDelayedPolicyUpdate(const tracked_objects::Location& from_here,
+                                   base::TimeDelta delay);
 
   // Update the policy if a new signal has arrived. Must be called from the main
   // thread.
   void MaybeUpdatePolicy();
 
-  // Updates the scheduler policy. Must be called from the main thread.
+  // Locks |incoming_signals_lock_| and calls UpdatePolicyLocked.
   void UpdatePolicy();

[Sami, 2015/03/11 16:58:25]

Maybe move this next to UpdatePolicyLocked()? The difference between the
functions is pretty obvious from the naming so we don't really need to explain
what this does IMO.

[alex clarke (OOO till 29th), 2015/03/12 13:41:32]

Done.

 
+  // Locks |incoming_signals_lock_|, clears |pending_delayed_policy_update_| and
+  // calls UpdatePolicyLocked.
+  void DelayedPolicyUpdateTask();

[Sami, 2015/03/11 16:58:25]

nit: Name this with a verb instead a noun to match the other functions here. The
comment might also be a little unnecessary if this is called something like
PerformDelayedPolicyUpdate()

Should we also allow this function only on the main thread, since we really only
expect it to be triggered from within the policy update (maybe we could assert
for that too)?

[alex clarke (OOO till 29th), 2015/03/12 13:41:31]

This function is gone.

+
+  // Updates the scheduler policy. Must be called from the main thread and
+  // |incoming_signals_lock_| must be locked.
+  virtual void UpdatePolicyLocked();
+
   // Helper for computing the new policy. |new_policy_duration| will be filled
   // with the amount of time after which the policy should be updated again. If
   // the duration is zero, a new policy update will not be scheduled. Must be
@@ -158,6 +174,7 @@ class CONTENT_EXPORT RendererSchedulerImpl : public RendererScheduler {
   scoped_refptr<SingleThreadIdleTaskRunner> idle_task_runner_;
 
   base::Closure update_policy_closure_;
+  base::Closure delayed_update_policy_closure_;

[rmcilroy, 2015/03/11 17:15:07]

I really think this should be a CancelableClosureHolder - the we wouldn't need
to worry about the pending_delayed_policy_update_ and wouldn't need to add the
DCHECK Sami is suggesting (since we would always be posting a update for the
soonest policy update).  Is there a reason we can't do this?

[Sami, 2015/03/11 17:18:47]

I think a good reason for avoiding cancellable closures is that they give you a
false sense of safety: the underlying task will still run even if it's
cancelled, adding invisible overhead. I think this is critical enough for
battery life that we should try an be explicit about it.

   CancelableClosureHolder end_idle_period_closure_;
 
   // Don't access current_policy_ directly, instead use SchedulerPolicy().
@@ -173,6 +190,7 @@ class CONTENT_EXPORT RendererSchedulerImpl : public RendererScheduler {
   blink::WebInputEvent::Type last_input_type_;
   InputStreamState input_stream_state_;
   PollableNeedsUpdateFlag policy_may_need_update_;
+  bool pending_delayed_policy_update_;
 
   scoped_refptr<cc::TestNowSource> time_source_;