Readability review for gfeher

chrome/browser/policy/delayed_work_scheduler.h

 // Copyright (c) 2011 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 CHROME_BROWSER_POLICY_DELAYED_WORK_SCHEDULER_H_
 #define CHROME_BROWSER_POLICY_DELAYED_WORK_SCHEDULER_H_
 #pragma once
 
 #include "base/callback.h"
 #include "base/timer.h"
 
 namespace policy {
 
 // A mockable class for scheduling and cancelling a delayed task.
 // This is only a thin wrapper around base::OneShotTimer.
 class DelayedWorkScheduler {
  public:
   DelayedWorkScheduler();
   virtual ~DelayedWorkScheduler();
 
   // Cancels the delayed work task.
   virtual void CancelDelayedWork();
 
   // Posts a new delayed task.
   virtual void PostDelayedWork(const base::Closure& callback, int64 delay);
 
  private:
   base::OneShotTimer<DelayedWorkScheduler> timer_;
   base::Closure callback_;

[zunger, 2011/07/19 19:41:46]

I'm confused by this. Callbacks are designed to be heap-allocated objects, and
non-reusable ones are self-deleting. Do you really mean to have a Closure
instead of a Closure* here, and pass a const Closure& to PostDelayedWork?! This
seems highly nonstandard.

[gfeher, 2011/07/20 16:36:30]

Callbacks in the Chromium codebase seem to work differently. Please see:
http://src.chromium.org/viewvc/chrome/trunk/src/base/callback.h?view=markup

In short, the comment in their header says that they should be stored by copy,
and the copies take care of ref-counting the shared data they store.

 
   void DoDelayedWork();
 
   DISALLOW_COPY_AND_ASSIGN(DelayedWorkScheduler);
 };
 
 }  // namespace policy
 
 #endif  // CHROME_BROWSER_POLICY_DELAYED_WORK_SCHEDULER_H_
+