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

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

The biggest improvement here is that early termination of the callback series is accomplished by deleting the object. 

BUG=138583
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149951

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

PTAL both of you

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h
File media/base/callback_util.h (right):

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:5: #ifndef MEDIA_BASE_CALLBACK_UTIL_H_
I'll rename this file to callback_series.h in a future PS if we're happy with
the approach + name of the class

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:43: 
if we want I can add a running() helper that can be used for DCHECK purposes

http://codereview.chromium.org/10830146/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10830146/diff/1/media/base/pipeline.cc#newcode461
media/base/pipeline.cc:461: &RunClosureFunc, base::Bind(&AudioRenderer::Pause,
audio_renderer_)));
I thought of having CallbackSeries::Run() that accepts a queue of ClosureFuncs,
but after this CL I want to combine pause/flush/seek and that requires having a
queue of the same func type, so I'm fine with the RunClosureFunc business

http://codereview.chromium.org/10830146/diff/1/media/base/pipeline.cc#newcode...
media/base/pipeline.cc:1194: pending_callbacks_.reset();
AFAIK this is the fix -- previously we may have had queued operations that still
ran while we were going through the teardown path

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

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h
File media/base/callback_util.h (right):

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:22: typedef base::Callback<void(const
base::Closure&)> ClosureFunc;
That's a terrible name :)

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:27: void RunClosureFunc(const ClosureFunc& closure,
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.

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:30: class CallbackSeries {
inherit from NonThreadSafe and DCHECK BelongsToCurrentThread liberally?

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:30: class CallbackSeries {
SerialCallbackRunner?

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:38: // Deleting the object will terminate any pending
closures, including
s/pending/unstarted/

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:38: // Deleting the object will terminate any pending
closures, including
s/closures/callbacks/

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:38: // Deleting the object will terminate any pending
closures, including
s/terminate/prevent execution of/

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:41: scoped_ptr<std::queue<PipelineStatusCBFunc> >
status_cbs,
This seems unnecessarily complex.
Why can this not PipelineStatusCB's instead of PipelineStatusCBFunc's?

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:43: 
On 2012/08/02 22:17:04, scherkus wrote:
> if we want I can add a running() helper that can be used for DCHECK purposes

No, that's usually a code smell.

http://codereview.chromium.org/10830146/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10830146/diff/1/media/base/pipeline.cc#newcode459
media/base/pipeline.cc:459: if (audio_renderer_)
Not reviewing this carefully yet b/c I have hopes for simplification of the API.
But if the verbosity remains, you need to add a bunch of {}'s to conditional
bodies that are no longer single-lines.

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

thanks for the review -- PTAL

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h
File media/base/callback_util.h (right):

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:27: void RunClosureFunc(const ClosureFunc& closure,
On 2012/08/03 04:49:57, Ami Fischman wrote:
> 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.

Done.

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:30: class CallbackSeries {
On 2012/08/03 04:49:57, Ami Fischman wrote:
> SerialCallbackRunner?

Done.

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:30: class CallbackSeries {
On 2012/08/03 04:49:57, Ami Fischman wrote:
> inherit from NonThreadSafe and DCHECK BelongsToCurrentThread liberally?

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

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

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:38: // Deleting the object will terminate any pending
closures, including
On 2012/08/03 04:49:57, Ami Fischman wrote:
> s/pending/unstarted/

Done.

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:38: // Deleting the object will terminate any pending
closures, including
On 2012/08/03 04:49:57, Ami Fischman wrote:
> s/closures/callbacks/

Done.

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:38: // Deleting the object will terminate any pending
closures, including
On 2012/08/03 04:49:57, Ami Fischman wrote:
> s/terminate/prevent execution of/

Done.

http://codereview.chromium.org/10830146/diff/1/media/base/callback_util.h#new...
media/base/callback_util.h:41: scoped_ptr<std::queue<PipelineStatusCBFunc> >
status_cbs,
On 2012/08/03 04:49:57, Ami Fischman wrote:
> This seems unnecessarily complex.
> Why can this not PipelineStatusCB's instead of PipelineStatusCBFunc's?

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)

http://codereview.chromium.org/10830146/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10830146/diff/1/media/base/pipeline.cc#newcode459
media/base/pipeline.cc:459: if (audio_renderer_)
On 2012/08/03 04:49:57, Ami Fischman wrote:
> Not reviewing this carefully yet b/c I have hopes for simplification of the
API.
> But if the verbosity remains, you need to add a bunch of {}'s to conditional
> bodies that are no longer single-lines.

Done.

[Ami GONE FROM CHROMIUM, 2012-08-03 19:29:01]

LGTM % nits (and file rename)

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.cc
File media/base/callback_util.cc (right):

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.cc#ne...
media/base/callback_util.cc:44: callback_series->RunNextInSeries(PIPELINE_OK);
Post this instead of running inline per acolwell@'s observation.

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.h
File media/base/callback_util.h (right):

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.h#new...
media/base/callback_util.h:22: // Why the odd looking typedef?
SerialCallbackRunner requires callbacks to
This comment doesn't explain *why* SCR would want such a thing.
It might be clearer to name this something closer to its use than its type; WDYT
of BoundPipelineStatusCB?

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.h#new...
media/base/callback_util.h:24: typedef base::Callback<void(const
PipelineStatusCB&)> PipelineStatusCBFunc;
This typedef belongs at the top of the public: section of SCR.

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.h#new...
media/base/callback_util.h:28: // Executes callbacks in |status_cbs| in series,
executing |done_cb| when
s/callbacks in/bound functions from/ ?

http://codereview.chromium.org/10830146/diff/7/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10830146/diff/7/media/base/pipeline.cc#newcode30
media/base/pipeline.cc:30: // Used to convert a callback to a function accepting
a Closure into a callback
s/callback to a /bound/ twice

[acolwell GONE FROM CHROMIUM, 2012-08-03 20:15:18]

LGTM

[scherkus (not reviewing), 2012-08-03 20:19:04]

can you two take another quick look at SerialCallbackRunner::Queue?

if we're 100% happy with things I'll do the file rename on the next PS + hit the
cq button

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.cc
File media/base/callback_util.cc (right):

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.cc#ne...
media/base/callback_util.cc:44: callback_series->RunNextInSeries(PIPELINE_OK);
On 2012/08/03 19:29:01, Ami Fischman wrote:
> Post this instead of running inline per acolwell@'s observation.

done in ctor as it has access to message_loop_ and weak_this_

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.h
File media/base/callback_util.h (right):

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.h#new...
media/base/callback_util.h:22: // Why the odd looking typedef?
SerialCallbackRunner requires callbacks to
On 2012/08/03 19:29:01, Ami Fischman wrote:
> This comment doesn't explain *why* SCR would want such a thing.
> It might be clearer to name this something closer to its use than its type;
WDYT
> of BoundPipelineStatusCB?

Done although I'm sad at losing our funk

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.h#new...
media/base/callback_util.h:24: typedef base::Callback<void(const
PipelineStatusCB&)> PipelineStatusCBFunc;
On 2012/08/03 19:29:01, Ami Fischman wrote:
> This typedef belongs at the top of the public: section of SCR.

Done.

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.h#new...
media/base/callback_util.h:24: typedef base::Callback<void(const
PipelineStatusCB&)> PipelineStatusCBFunc;
On 2012/08/03 19:29:01, Ami Fischman wrote:
> This typedef belongs at the top of the public: section of SCR.

Done.

http://codereview.chromium.org/10830146/diff/7/media/base/callback_util.h#new...
media/base/callback_util.h:28: // Executes callbacks in |status_cbs| in series,
executing |done_cb| when
On 2012/08/03 19:29:01, Ami Fischman wrote:
> s/callbacks in/bound functions from/ ?

Done.

http://codereview.chromium.org/10830146/diff/7/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10830146/diff/7/media/base/pipeline.cc#newcode30
media/base/pipeline.cc:30: // Used to convert a callback to a function accepting
a Closure into a callback
On 2012/08/03 19:29:01, Ami Fischman wrote:
> s/callback to a /bound/ twice

Done.

http://codereview.chromium.org/10830146/diff/12002/media/base/callback_util.h
File media/base/callback_util.h (right):

http://codereview.chromium.org/10830146/diff/12002/media/base/callback_util.h...
media/base/callback_util.h:31: class Queue {
PTAL here + usage in pipeline.cc

[Ami GONE FROM CHROMIUM, 2012-08-03 20:31:43]

http://codereview.chromium.org/10830146/diff/12002/media/base/callback_util.cc
File media/base/callback_util.cc (right):

http://codereview.chromium.org/10830146/diff/12002/media/base/callback_util.c...
media/base/callback_util.cc:17: void RunBoundClosure(
static

http://codereview.chromium.org/10830146/diff/12002/media/base/callback_util.h
File media/base/callback_util.h (right):

http://codereview.chromium.org/10830146/diff/12002/media/base/callback_util.h...
media/base/callback_util.h:57: scoped_ptr<Queue> bound_fns, const
PipelineStatusCB& done_cb);
copy ctor bound_fns per offline convo

http://codereview.chromium.org/10830146/diff/12002/media/base/callback_util.h...
media/base/callback_util.h:70: scoped_ptr<Queue> bound_fns_;
can drop the scoped_ptr here too if pass-by-copy.

[scherkus (not reviewing), 2012-08-03 20:44:09]

PTAL one mo' time

http://codereview.chromium.org/10830146/diff/14001/media/base/serial_runner.h
File media/base/serial_runner.h (right):

http://codereview.chromium.org/10830146/diff/14001/media/base/serial_runner.h...
media/base/serial_runner.h:40: friend class SerialRunner;
turns out this _isn't_ java!

http://codereview.chromium.org/10830146/diff/14001/media/base/serial_runner.h...
media/base/serial_runner.h:46: };
FYI no more DISALLOW_XXX() -- I don't think I need anything else here, right?

[Ami GONE FROM CHROMIUM, 2012-08-03 20:48:23]

LGTM
ship it!

http://codereview.chromium.org/10830146/diff/14001/media/base/serial_runner.h
File media/base/serial_runner.h (right):

http://codereview.chromium.org/10830146/diff/14001/media/base/serial_runner.h...
media/base/serial_runner.h:43: bool empty() { return bound_fns_.empty(); }
out-of-line?

[acolwell GONE FROM CHROMIUM, 2012-08-03 21:03:20]

LGTM