Replace RunInSeries() and RunInParallel() with CallbackSeries helper class.

media/base/callback_util.h

 // Copyright (c) 2012 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 MEDIA_BASE_CALLBACK_UTIL_H_

[scherkus (not reviewing), 2012/08/02 22:17:04]

I'll rename this file to callback_series.h in a future PS if we're happy with
the approach + name of the class

 #define MEDIA_BASE_CALLBACK_UTIL_H_
 
 #include <queue>
 
 #include "base/callback.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "media/base/pipeline_status.h"
 
+namespace base {
+class MessageLoopProxy;
+}
+
 namespace media {
 
 typedef base::Callback<void(const base::Closure&)> ClosureFunc;

[Ami GONE FROM CHROMIUM, 2012/08/03 04:49:57]

That's a terrible name :)

 typedef base::Callback<void(const PipelineStatusCB&)> PipelineStatusCBFunc;
 
-// Executes the closures in FIFO order, executing |done_cb| when the last
-// closure has completed running.
-//
-// All closures (including |done_cb|) are executed on same thread as the
-// calling thread.
-void RunInSeries(scoped_ptr<std::queue<ClosureFunc> > closures,
-                 const base::Closure& done_cb);
+// Used to convert a ClosureFunc into a PipelineStatusCBFunc. Since closures
+// do not report a status, |status_cb| is executed with PIPELINE_OK.
+void RunClosureFunc(const ClosureFunc& closure,

[Ami GONE FROM CHROMIUM, 2012/08/03 04:49:57]

Since this is used in only one file and this is changing from *_util.{h,cc} to a
specific purpose (yay), this function should move to that file (pipeline.cc).
Ditto for the typedef.

[scherkus (not reviewing), 2012/08/03 18:08:50]

Done.

+                    const PipelineStatusCB& status_cb);
 
-// Executes the closures in FIFO order, executing |done_cb| when the last
-// closure has completed running, reporting the final status code.
-//
-// Closures will stop being executed if a previous closure in the series
-// returned an error status and |done_cb| will be executed prematurely.
-//
-// All closures (including |done_cb|) are executed on same thread as the
-// calling thread.
-void RunInSeriesWithStatus(
-    scoped_ptr<std::queue<PipelineStatusCBFunc> > status_cbs,
-    const PipelineStatusCB& done_cb);
+class CallbackSeries {

[Ami GONE FROM CHROMIUM, 2012/08/03 04:49:57]

inherit from NonThreadSafe and DCHECK BelongsToCurrentThread liberally?

[Ami GONE FROM CHROMIUM, 2012/08/03 04:49:57]

SerialCallbackRunner?

[scherkus (not reviewing), 2012/08/03 18:08:50]

Done.

[scherkus (not reviewing), 2012/08/03 18:08:50]

isn't that accomplished via message_loop_->BelongsToCurrentThread() in
RunNextInSeries?

The only other place I could sprinkle a DCHECK() is the dtor

+ public:
+  // Executes closures in |status_cbs| in series, executing |done_cb| when
+  // finished.
+  //
+  // All closures are executed on the thread that Run() is called on, including
+  // |done_cb|.
+  //
+  // Deleting the object will terminate any pending closures, including

[Ami GONE FROM CHROMIUM, 2012/08/03 04:49:57]

s/pending/unstarted/

[Ami GONE FROM CHROMIUM, 2012/08/03 04:49:57]

s/closures/callbacks/

[Ami GONE FROM CHROMIUM, 2012/08/03 04:49:57]

s/terminate/prevent execution of/

[scherkus (not reviewing), 2012/08/03 18:08:50]

Done.

[scherkus (not reviewing), 2012/08/03 18:08:50]

Done.

[scherkus (not reviewing), 2012/08/03 18:08:50]

Done.

+  // |done_cb|.
+  static scoped_ptr<CallbackSeries> Run(
+      scoped_ptr<std::queue<PipelineStatusCBFunc> > status_cbs,

[Ami GONE FROM CHROMIUM, 2012/08/03 04:49:57]

This seems unnecessarily complex.
Why can this not PipelineStatusCB's instead of PipelineStatusCBFunc's?

[scherkus (not reviewing), 2012/08/03 18:08:50]

PipelineStatusCBs are void(PipelineStatus)

We want a void function that accepts a PipelineStatusCB as an argument, not a
PipelineStatus (i.e., the function pointer to a Seek(PipelineStatusCB) function,
not the PipelineStatusCB itself)

+      const PipelineStatusCB& done_cb);
 

[scherkus (not reviewing), 2012/08/02 22:17:04]

if we want I can add a running() helper that can be used for DCHECK purposes

[Ami GONE FROM CHROMIUM, 2012/08/03 04:49:57]

No, that's usually a code smell.

-// Executes the closures in parallel, executing |done_cb| when all closures have
-// completed running.
-//
-// No attempt is made to parallelize execution of the closures. In other words,
-// this method will run all closures in FIFO order if said closures execute
-// synchronously on the same call stack.
-//
-// All closures (including |done_cb|) are executed on same thread as the
-// calling thread.
-void RunInParallel(scoped_ptr<std::queue<ClosureFunc> > closures,
-                   const base::Closure& done_cb);
+ private:
+  friend class scoped_ptr<CallbackSeries>;
+
+  CallbackSeries(
+      scoped_ptr<std::queue<PipelineStatusCBFunc> > status_cbs,
+      const PipelineStatusCB& done_cb);
+  ~CallbackSeries();
+
+  void RunNextInSeries(PipelineStatus last_status);
+
+  base::WeakPtrFactory<CallbackSeries> weak_this_;
+  scoped_refptr<base::MessageLoopProxy> message_loop_;
+  scoped_ptr<std::queue<PipelineStatusCBFunc> > status_cbs_;
+  PipelineStatusCB done_cb_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackSeries);
+};
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_CALLBACK_UTIL_H_