Move media library AutoTaskRunner to base and rename ScopedTaskRunner.

Move media library AutoTaskRunner to base and rename ScopedTaskRunner.

This is needed to avoid faux dependencies on media/ creeping in to remoting/ code, and creating linker issues.

BUG=
TEST=Everything works as before.  Shared component builds certainly don't break.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86971 (Reverted - broke shared builds)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87068

[Wez, 2011-05-26 02:42:16]

This CL moves ::media::AutoTaskRunner to be ::ScopedTaskRunner, on the basis
that it's a useful class whose functionality will likely be replicated against
the new callback/task system once that is more settled-in.

Having AutoTaskRunner in media creates problems in that it forces code using it
to depend on media, which leads to linker issues because of the way media loads
codecs.

The CL also removes an unused class from media/base/callback.h.

[awong, 2011-05-26 02:51:38]

LGTM

I found this a useful class, and had been meaning to migrate it up to base for a
long time now.

Brett, if you're okay with moving it up there, the rest of the changes look good
to me.

Note that moving to the new base::Callback doesn't obsolete this.  The main
purpose of this was/is to force execution of a callback on leaving a scope. 
That it handles the memory management is more of an adornment.

-Albert

http://codereview.chromium.org/7062013/diff/1/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode551
base/task.h:551: class ScopedTaskRunner {
Should this be thrown into the namespace base?  I realize the rest of the file
is in the global namespace, but might as well not introduce more...

http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode554
base/task.h:554: explicit ScopedTaskRunner(Task* task) : task_(task) {}
Since we're out-of-lining the destructor, might as well out-of-line all of
these.

Also, I think clang may yell at you.

[scherkus (not reviewing), 2011-05-26 02:57:16]

driveby!

http://codereview.chromium.org/7062013/diff/1/base/task.cc
File base/task.cc (right):

http://codereview.chromium.org/7062013/diff/1/base/task.cc#newcode1
base/task.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved.
copyright year for this + other files (or use commit queue)

http://codereview.chromium.org/7062013/diff/1/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode563
base/task.h:563: DISALLOW_COPY_AND_ASSIGN(ScopedTaskRunner);
micro-nit: DISALLOW_IMPLICIT_CONSTRUCTORS

[Wez, 2011-05-26 06:05:47]

On 2011/05/26 02:51:38, awong wrote:
> http://codereview.chromium.org/7062013/diff/1/base/task.h
> File base/task.h (right):
> 
> http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode551
> base/task.h:551: class ScopedTaskRunner {
> Should this be thrown into the namespace base?  I realize the rest of the file
> is in the global namespace, but might as well not introduce more...

I went for consistency over structuredness.  It seems wrong, somehow, to put a
destined-for-obsoletion class in base, rather than leaving it to languish with
its equally doomed global-namespace peers.

> http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode554
> base/task.h:554: explicit ScopedTaskRunner(Task* task) : task_(task) {}
> Since we're out-of-lining the destructor, might as well out-of-line all of
> these.
> 
> Also, I think clang may yell at you.

That would be bad.  Consider it done (tomorrow).

(I'd normally have both constructor and destructor in-line, on the basis that
this class is really just shorthand for remembering to run the Task before
exiting the current scope.  The ctor & dtor function calls may be practically
free, but in-lining their implementations gives the compiler the same
optimisation possibilities as if you'd written the calling code without this
helper class.)

[Wez, 2011-05-26 06:06:28]

On 2011/05/26 02:57:16, scherkus wrote:
> driveby!
> 
> http://codereview.chromium.org/7062013/diff/1/base/task.cc
> File base/task.cc (right):
> 
> http://codereview.chromium.org/7062013/diff/1/base/task.cc#newcode1
> base/task.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights
reserved.
> copyright year for this + other files (or use commit queue)
> 
> http://codereview.chromium.org/7062013/diff/1/base/task.h
> File base/task.h (right):
> 
> http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode563
> base/task.h:563: DISALLOW_COPY_AND_ASSIGN(ScopedTaskRunner);
> micro-nit: DISALLOW_IMPLICIT_CONSTRUCTORS

Good [micro] nit.  Thanks!

[brettw, 2011-05-26 17:17:22]

http://codereview.chromium.org/7062013/diff/1/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode551
base/task.h:551: class ScopedTaskRunner {
I agree that using the base namespace is a good idea.

http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode558
base/task.h:558: Task* release() { Task* tmp = task_; task_ = NULL; return tmp;
}
I think out-of-lining is also good for this (should probably be called "Release"
then). Having the 3 statements on the same line makes this hard to read anyway.

http://codereview.chromium.org/7062013/diff/1/remoting/client/rectangle_updat...
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/7062013/diff/1/remoting/client/rectangle_updat...
remoting/client/rectangle_update_decoder.cc:17: using
::remoting::protocol::ChannelConfig;
Are the leading colons here necessary? Usually we don't use them in cases like
this.

[dmac, 2011-05-26 17:55:42]

Drive by

http://codereview.chromium.org/7062013/diff/1/media/filters/decoder_base.h
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/7062013/diff/1/media/filters/decoder_base.h#ne...
media/filters/decoder_base.h:105: done_cb->Run();
Maybe I'm being slow this morning, but why is this not a ScopedTaskRunnner now?

[Wez, 2011-05-26 18:48:21]

I've addressed everyone's comments, I think, and added a few unit tests.

http://codereview.chromium.org/7062013/diff/1/base/task.cc
File base/task.cc (right):

http://codereview.chromium.org/7062013/diff/1/base/task.cc#newcode1
base/task.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved.
On 2011/05/26 02:57:16, scherkus wrote:
> copyright year for this + other files (or use commit queue)

Done.

http://codereview.chromium.org/7062013/diff/1/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode551
base/task.h:551: class ScopedTaskRunner {
On 2011/05/26 17:17:22, brettw wrote:
> I agree that using the base namespace is a good idea.

Done.

http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode554
base/task.h:554: explicit ScopedTaskRunner(Task* task) : task_(task) {}
On 2011/05/26 02:51:38, awong wrote:
> Since we're out-of-lining the destructor, might as well out-of-line all of
> these.
> 
> Also, I think clang may yell at you.

Done.

http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode558
base/task.h:558: Task* release() { Task* tmp = task_; task_ = NULL; return tmp;
}
On 2011/05/26 17:17:22, brettw wrote:
> I think out-of-lining is also good for this (should probably be called
"Release"
> then). Having the 3 statements on the same line makes this hard to read
anyway.

Done.

http://codereview.chromium.org/7062013/diff/1/base/task.h#newcode563
base/task.h:563: DISALLOW_COPY_AND_ASSIGN(ScopedTaskRunner);
On 2011/05/26 02:57:16, scherkus wrote:
> micro-nit: DISALLOW_IMPLICIT_CONSTRUCTORS

Done.

http://codereview.chromium.org/7062013/diff/1/media/filters/decoder_base.h
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/7062013/diff/1/media/filters/decoder_base.h#ne...
media/filters/decoder_base.h:105: done_cb->Run();
On 2011/05/26 17:55:42, dmac wrote:
> Maybe I'm being slow this morning, but why is this not a ScopedTaskRunnner
now?

Reverted.

I was thinking of this as an unnecessary dependency; there's only one code-path
so |done_cb->Run(); delete done_cb;| seemed cleaner.  I'm not entirely sure why
this method has a default implementation at all, given that its peers don't.

http://codereview.chromium.org/7062013/diff/1/remoting/client/rectangle_updat...
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/7062013/diff/1/remoting/client/rectangle_updat...
remoting/client/rectangle_update_decoder.cc:17: using
::remoting::protocol::ChannelConfig;
On 2011/05/26 17:17:22, brettw wrote:
> Are the leading colons here necessary? Usually we don't use them in cases like
> this.

Reverted.

The C++ Style Guide implied it was preferred that they be there, but perhaps I
mis-read it.

[awong, 2011-05-26 19:01:01]

LGTM w/ style nits.

Thanks for making all these changes!  Looks good from my side.

http://codereview.chromium.org/7062013/diff/5002/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7062013/diff/5002/base/task.h#newcode567
base/task.h:567: }
style-OCD nit:

End namespaces with a comment.

}  // namespace base

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc
File base/task_unittest.cc (right):

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode55
base/task_unittest.cc:55: : run_count_(run_count), was_deleted_(was_deleted) {}
Since you had to break for the initializer list, the {} shoudl be on 2 lines.

DoneTask(int* run_count, bool* was_deleted)
    : run_count_(run_count), was_deleted_(was_deleted) {
}

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode56
base/task_unittest.cc:56: ~DoneTask() {
virtual

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode74
base/task_unittest.cc:74: ASSERT_EQ(1, run_count);
Use EXPECT_* not ASSERT_* here and in the other new tests. These expectations
seem to still be valid even if the previous failed, so EXPECT is more normal.

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode75
base/task_unittest.cc:75: ASSERT_EQ(true, was_deleted);
_TRUE, not _EQ

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode84
base/task_unittest.cc:84: ASSERT_EQ(true, was_deleted);
ASSERT_TRUE

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode96
base/task_unittest.cc:96: ASSERT_TRUE(NULL != done_task);
ASSERT_EQ?

[Wez, 2011-05-26 20:53:46]

http://codereview.chromium.org/7062013/diff/5002/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7062013/diff/5002/base/task.h#newcode567
base/task.h:567: }
On 2011/05/26 19:01:01, awong wrote:
> style-OCD nit:
> 
> End namespaces with a comment.
> 
> }  // namespace base

Done.

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc
File base/task_unittest.cc (right):

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode55
base/task_unittest.cc:55: : run_count_(run_count), was_deleted_(was_deleted) {}
On 2011/05/26 19:01:01, awong wrote:
> Since you had to break for the initializer list, the {} shoudl be on 2 lines.
> 
> DoneTask(int* run_count, bool* was_deleted)
>     : run_count_(run_count), was_deleted_(was_deleted) {
> }

Done.

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode56
base/task_unittest.cc:56: ~DoneTask() {
On 2011/05/26 19:01:01, awong wrote:
> virtual

Done.

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode74
base/task_unittest.cc:74: ASSERT_EQ(1, run_count);
On 2011/05/26 19:01:01, awong wrote:
> Use EXPECT_* not ASSERT_* here and in the other new tests. These expectations
> seem to still be valid even if the previous failed, so EXPECT is more normal.

Done.

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode75
base/task_unittest.cc:75: ASSERT_EQ(true, was_deleted);
On 2011/05/26 19:01:01, awong wrote:
> _TRUE, not _EQ

Done.

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode84
base/task_unittest.cc:84: ASSERT_EQ(true, was_deleted);
On 2011/05/26 19:01:01, awong wrote:
> ASSERT_TRUE

Done.

http://codereview.chromium.org/7062013/diff/5002/base/task_unittest.cc#newcode96
base/task_unittest.cc:96: ASSERT_TRUE(NULL != done_task);
On 2011/05/26 19:01:01, awong wrote:
> ASSERT_EQ?

Would need to be ASSERT_NE(), which gets confused about the type of NULL. :(

[Wez, 2011-05-26 20:55:00]

Would an OWNER of base like to take look, please?

[brettw, 2011-05-26 20:59:04]

LGTM with style nit.

http://codereview.chromium.org/7062013/diff/5003/base/task.cc
File base/task.cc (right):

http://codereview.chromium.org/7062013/diff/5003/base/task.cc#newcode37
base/task.cc:37: } // namespace base
Two spaces before end-of-line comments.

http://codereview.chromium.org/7062013/diff/5003/base/task.h
File base/task.h (right):

http://codereview.chromium.org/7062013/diff/5003/base/task.h#newcode567
base/task.h:567: } // namespace base
Two spaces before end-of-line comments.

[commit-bot: I haz the power, 2011-05-26 21:11:54]

Try job failure for 7062013-1025 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-05-26 23:59:46]

Try job failure for 7062013-15 on linux_clang:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-05-27 01:54:22]

Try job failure for 7062013-15 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-05-27 04:17:25]

Change committed as 86971

[commit-bot: I haz the power, 2011-05-27 19:35:13]

Change committed as 87068