Readability review

net/base/prioritized_dispatcher.h

Index: net/base/prioritized_dispatcher.h
diff --git a/net/base/prioritized_dispatcher.h b/net/base/prioritized_dispatcher.h
index 7e59d4e67e1bf505ffe460b044ecc64ec942e060..1d594ce2cf1fcefc9d7f2a454cfb3b651d79630c 100644
--- a/net/base/prioritized_dispatcher.h
+++ b/net/base/prioritized_dispatcher.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef NET_BASE_PRIORITY_DISPATCH_H_
-#define NET_BASE_PRIORITY_DISPATCH_H_
+#ifndef NET_BASE_PRIORITIZED_DISPATCHER_H_
+#define NET_BASE_PRIORITIZED_DISPATCHER_H_
 #pragma once
 
 #include <vector>
@@ -18,10 +18,10 @@ namespace net {
 // jobs. It never revokes a job once started. The job must call OnJobFinished
 // once it finishes in order to dispatch further jobs.
 //
-// All operations are O(p) time for p priority levels. The class is fully
-// reentrant: it is safe to execute any method (incl. destructor) from within
-// Job callbacks. However, this class is NOT thread-safe, which is enforced
-// by the underlying non-thread-safe PriorityQueue.
+// This class is NOT thread-safe which is enforced by the underlying
+// non-thread-safe PriorityQueue. All operations are O(p) time for p priority
+// levels. It is safe to execute any method, including destructor, from within
+// Job::Start.
 //
 class NET_EXPORT_PRIVATE PrioritizedDispatcher {
  public:
@@ -29,10 +29,11 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
   typedef PriorityQueue<Job*>::Priority Priority;
 
   // Describes the limits for the number of jobs started by the dispatcher.
-  // For example, |total_jobs| = 30 and |reserved_slots| = { 5, 10, 5 }
+  // For example, |total_jobs| = 30 and |reserved_slots| = { 0, 5, 10, 5 }
   // allow for at most 30 running jobs in total. If there are already 24 jobs

[mmenke, 2012/06/01 17:45:34]

optional nit:  Suggest you add ", and no jobs at priority 0 will ever be
started", After "in total".

While higher priority -> more important may be intuitive, first item in a list
is the highest priority is also intuitive, so the comment draws a little more
attention to the fact that lowest priority is first in the list.

[szym, 2012/06/01 18:15:01]

Those are reserved slots. Only 20 slots are reserved, so there could be up to 10
jobs at priority 0. I added comment.

[mmenke, 2012/06/01 18:30:05]

Ah, right, I forgot about the 10.

-  // running, then there can be 6 more jobs started of which at most 1 can be
-  // at priority 1 or 2, but the rest have to be at 2.
+  // running, then only 6 more jobs can start. No jobs at priority 1 or below
+  // can start. After one more job starts, no jobs at priority 2 or below can
+  // start, since the remaining 5 slots are reserved for priority 3 or above.
   struct NET_EXPORT_PRIVATE Limits {
     Limits(Priority num_priorities, size_t total_jobs);
     ~Limits();
@@ -50,10 +51,10 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
   // deleted after it is dispatched or canceled, or the dispatcher is destroyed.
   class Job {
    public:
-    // Note: PriorityDispatch will never delete a Job.
+    // Note: PrioritizedDispatcher will never delete a Job.
     virtual ~Job() {}
-    // Called when the dispatcher starts the job. Must call OnJobFinished when
-    // done.
+    // Called when the dispatcher starts the job. Once the job finishes, it must
+    // call OnJobFinished.
     virtual void Start() = 0;
   };
 
@@ -62,7 +63,7 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
   typedef PriorityQueue<Job*>::Pointer Handle;
 
   // Creates a dispatcher enforcing |limits| on number of running jobs.
-  PrioritizedDispatcher(const Limits& limits);
+  explicit PrioritizedDispatcher(const Limits& limits);
 
   ~PrioritizedDispatcher();
 
@@ -72,7 +73,7 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
 
   // Adds |job| with |priority| to the dispatcher. If limits permit, |job| is
   // started immediately. Returns handle to the job or null-handle if the job is
-  // started.
+  // started. The dispatcher does not own |job|, but must be outlived by |job|.

[mmenke, 2012/06/01 17:45:34]

Shouldn't this be "but must outlive |job|"?

[szym, 2012/06/01 18:15:01]

The Job must outlive the dispatcher so that it can be manipulated by it. I
clarified the comment.

[mmenke, 2012/06/01 18:30:05]

But you say Jobs must call OnJobFinished after being started, and they're
generally deleted at some point after that, so they don't outlive the
dispatcher.

Maybe "|job| must outlive the dispatcher unless cancelled or started?"

[mmenke, 2012/06/01 18:32:49]

Err...Or evicted.

Ok...Maybe "|jobs| that are still queued cannot be deleted until the dispatcher
is destroyed or they are no longer queued?"

   Handle Add(Job* job, Priority priority);
 
   // Removes the job with |handle| from the queue. Invalidates |handle|.
@@ -111,5 +112,6 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
 
 }  // namespace net
 
-#endif  // NET_BASE_PRIORITY_DISPATCH_H_
+#endif  // NET_BASE_PRIORITIZED_DISPATCHER_H_
+
 

[mmenke, 2012/06/01 17:45:34]

nit:  Remove extra blank line (Goes for other files, too)

[szym, 2012/06/01 18:15:01]

Done.