Readability review for gfeher

chrome/browser/policy/logging_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_LOGGING_WORK_SCHEDULER_H_
 #define CHROME_BROWSER_POLICY_LOGGING_WORK_SCHEDULER_H_
 #pragma once
 
 #include <queue>
 #include <vector>
@@ skipping 16 matching lines @@
  private:
   DISALLOW_COPY_AND_ASSIGN(DummyWorkScheduler);
 };
 
 // Helper class for LoggingWorkScheduler. It essentially emulates a real
 // message loop. All the submitted tasks are run with zero delay, but the
 // order in which they would run with delays is preserved.
 // All the task posting requests of the schedulers will be channeled through
 // a common instance of EventLogger. This makes sure, that this instance can
 // keep track of time in the simulation and record logged events with correct
 // timestamps.

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

Again, what's the thread safety? And is this class meant to be part of the API
of this file? Seeing a bunch of helper classes before I know what this file is
event meant to do is not particularly helpful.

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

This is part of the public API. I've added some explanations at the beginning of
this file.

 class EventLogger {
  public:
   EventLogger();
   virtual ~EventLogger();

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

Is this meant to be subclassed?

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

No. Shall I remove virtual then?

[gfeher, 2011/08/04 10:26:08]

Done.

 
   // Post a task to be executed |delay| milliseconds from now. The task can be
   // cancelled later by calling Reset() on the callback.
   void PostDelayedWork(linked_ptr<base::Closure> callback, int64 delay);
 
  // Register a new event that happened now according to the internal clock.
   void RegisterEvent();
 
   // Swap out the internal list of events.
   void Swap(std::vector<int64>* events);
 
   // Counts the events in a sorted integer array that are >= |start| but
   // < |start| + |length|.
   static int CountEvents(const std::vector<int64>& events,

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

google3 code is using namespace std. (Not sure if Chromium is as well)

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

This is interesting because the C++ style guide seem to be forbidding "using
namespace":
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

But anyways, I've grepped the Chromium source tree for "using namespace std" and
there were only a very few matches, all from third-party libraries.

                          int64 start, int64 length);
 
  private:
   class Task {

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

Just declare Task here, and give the full class declaration inside the .cc file.

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

Done.

    public:
     Task();
     Task(int64 trigger_time,
          int64 secondary_key,
          linked_ptr<base::Closure> callback);
     ~Task();
 
     // Returns true if |this| should be executed before |rhs|.
     // Used for sorting by the priority queue.
     bool operator< (const Task& rhs) const;
@@ skipping 38 matching lines @@
   int64 task_counter_;
 
   DISALLOW_COPY_AND_ASSIGN(EventLogger);
 };
 
 // Issues delayed tasks with zero effective delay, but posts them through
 // an EventLogger, to make it possible to log events and reconstruct their
 // execution time.
 class LoggingWorkScheduler : public DelayedWorkScheduler {
  public:
   explicit LoggingWorkScheduler(EventLogger* logger);

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

Does the scheduler take ownership of the logger? Not clear. Ditto the thread
safety.

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

Done. (I've only talked about thread safety at the beginning of this file.)

   virtual ~LoggingWorkScheduler();
 
   virtual void PostDelayedWork(const base::Closure& callback, int64 delay);
   virtual void CancelDelayedWork();
 
  private:
   EventLogger* logger_;
   linked_ptr<base::Closure> callback_;
 
   DISALLOW_COPY_AND_ASSIGN(LoggingWorkScheduler);
 };
 
 }  // namespace policy
 
 #endif  // CHROME_BROWSER_POLICY_LOGGING_WORK_SCHEDULER_H_
+