Move the device enumerate/open/close work to device thread from IO thread

Pepper needs to repeatedly call enumeration API in order to get a synchronous up-to-date device list. This heavily loads the IO thread since the enumeration is done on IO thread.

This patch moves the device thread from VideoCaptureManager to MediaStreamManager, so that audio and video can share one device thread, and also allow running audio device API on this shared device thread.

BUG=132701, 130113
TEST=content_unittests

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

[no longer working on chromium, 2012-06-26 19:46:01]

Would you please review this CL?

Andrew, Magnus: would you please review the general code style and design?

John, could you please take a look at media_stream_manager.cc/.h to see if the
way of shutting down the device thread on FILE thread is correct?

Thanks,

BR,
-SX

[jam, 2012-06-26 22:32:01]

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:120: DCHECK(thread);
nit: unnecessary, if it's NULL the next line will crash which is just as useful
of a signal

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:152:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
nit: instead of the FILE thread, I would use the blocking pool thread (i.e.
using BrowserThread::PostBlockingPoolTask). Since if this thread can have a long
running task, you don't want to block the FILE thread for that long as well.

[scherkus (not reviewing), 2012-06-27 00:52:31]

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:111:
device_thread_.reset(new base::Thread("AudioInputDeviceManagerTestThread"));
given that you can inject message loops I'm curious whether you could (or want
to) use message_loop_ here and avoid the syncing between threads

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:120:
manager_->SetAudioManagerForTesting(audio_manager_.get());
can't we recreate a BrowserMainLoop for each test + use its audio_manager()?
what are we gaining by recreating our own manager?

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:209:
void SyncWithVideoCaptureManagerThread() {
do you really need to sync here? would the following be equivalent:

message_loop_->RunAllPending();
media_stream_manager_.reset();  // Teardown will finish pending tasks.

that way you don't need the test to peek into the manager's message loop

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:344:
video_capture_manager()->Error(session_id);
fix indent

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:639: return
device_thread_.get() ? device_thread_->message_loop_proxy() : NULL;
I don't think you need this method as in both call sites you have
EnsureDeviceThread() so I'd inline device_thread_->message_loop_proxy()

if there isn't a device thread you'll crash on the NULL deref immediately as
opposed to crashing sometime later because you passed in a NULL pointer into
objects

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.h:33: #include
"base/synchronization/waitable_event.h"
not seeing this used -- remove?

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager.cc:43: :
message_loop_(message_loop),
fix indent here

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/ren...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:96:
device_thread_->Stop();
nit: should we be stopping/resetting the threads in the reverse order? i.e., in
SetUp() you create the io_thread_ before device_thread_

alternatively you can remove the io_thread_.reset() call, similar to
video_capture_controller_unittest.cc

[no longer working on chromium, 2012-06-27 14:07:16]

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:111:
device_thread_.reset(new base::Thread("AudioInputDeviceManagerTestThread"));
On 2012/06/27 00:52:31, scherkus wrote:
> given that you can inject message loops I'm curious whether you could (or want
> to) use message_loop_ here and avoid the syncing between threads

True, I did not do this simply because I was not sure if it was a good way to do
the testing.

Thanks for pointing out, done now.

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:120:
manager_->SetAudioManagerForTesting(audio_manager_.get());
On 2012/06/27 00:52:31, scherkus wrote:
> can't we recreate a BrowserMainLoop for each test + use its audio_manager()?
> what are we gaining by recreating our own manager?

Good idea, done.

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:209:
void SyncWithVideoCaptureManagerThread() {
On 2012/06/27 00:52:31, scherkus wrote:
> do you really need to sync here? would the following be equivalent:
> 
> message_loop_->RunAllPending();
> media_stream_manager_.reset();  // Teardown will finish pending tasks.
> 
> that way you don't need the test to peek into the manager's message loop


I am not familiar with the code here, but the comment in TearDown() clearly
points out that we need to make sure the manager finishes all the task.
And we can't call media_stream_manager_.reset() since media_stream_manager is
owned by the profile and will be deleted when the profile goes away.
Magnus, do you know if message_loop_->RunAllPending() is enough here?

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:344:
video_capture_manager()->Error(session_id);
On 2012/06/27 00:52:31, scherkus wrote:
> fix indent

Done.

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:120: DCHECK(thread);
On 2012/06/26 22:32:01, John Abd-El-Malek wrote:
> nit: unnecessary, if it's NULL the next line will crash which is just as
useful
> of a signal

Done.

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:152:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
On 2012/06/26 22:32:01, John Abd-El-Malek wrote:
> nit: instead of the FILE thread, I would use the blocking pool thread (i.e.
> using BrowserThread::PostBlockingPoolTask). Since if this thread can have a
long
> running task, you don't want to block the FILE thread for that long as well.

Great idea. Thanks for pointing out.
Do you know if the PostBlockingPoolTask is running on one single thread?
I am thinking removing the device thread and using an existing thread instead,
can it be a good idea to handle all the tasks via
BrowserThread::PostBlockingPoolTask? One restriction as I know is that the video
open/start/stop/close need to run on a single thread.

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:639: return
device_thread_.get() ? device_thread_->message_loop_proxy() : NULL;
On 2012/06/27 00:52:31, scherkus wrote:
> I don't think you need this method as in both call sites you have
> EnsureDeviceThread() so I'd inline device_thread_->message_loop_proxy()
> 
> if there isn't a device thread you'll crash on the NULL deref immediately as
> opposed to crashing sometime later because you passed in a NULL pointer into
> objects 

It is mainly for
media_stream_dispatch_host_unittest.cc::SyncWithVideoCaptureManagerThread (looks
like I need to change the name though)

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:33: #include
"base/synchronization/waitable_event.h"
On 2012/06/27 00:52:31, scherkus wrote:
> not seeing this used -- remove?

Sorry, uncleaned code from debugging.

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager.cc:43: :
message_loop_(message_loop),
On 2012/06/27 00:52:31, scherkus wrote:
> fix indent here

Done.

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:96:
device_thread_->Stop();
On 2012/06/27 00:52:31, scherkus wrote:
> nit: should we be stopping/resetting the threads in the reverse order? i.e.,
in
> SetUp() you create the io_thread_ before device_thread_
> 
> alternatively you can remove the io_thread_.reset() call, similar to
> video_capture_controller_unittest.cc

I will remove the device_thread_ here and inject the message_loop to
VideoCaptureManager instead, exactly the same as how we do it in
AudioInputDeviceManager, so that we can skip the syncing of threads.

[jam, 2012-06-27 16:13:14]

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:152:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
On 2012/06/27 14:07:16, xians1 wrote:
> On 2012/06/26 22:32:01, John Abd-El-Malek wrote:
> > nit: instead of the FILE thread, I would use the blocking pool thread (i.e.
> > using BrowserThread::PostBlockingPoolTask). Since if this thread can have a
> long
> > running task, you don't want to block the FILE thread for that long as well.
> 
> Great idea. Thanks for pointing out.
> Do you know if the PostBlockingPoolTask is running on one single thread?
> I am thinking removing the device thread and using an existing thread instead,
> can it be a good idea to handle all the tasks via
> BrowserThread::PostBlockingPoolTask? One restriction as I know is that the
video
> open/start/stop/close need to run on a single thread.

it uses multiple threads. you can pass in a token with the PostTask call to make
all tasks with the same token run on the same thread. see
http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/sequenced_work...
for more information

[scherkus (not reviewing), 2012-06-28 01:24:20]

one suggestion for the GetMessageLoop() stuff

I'm removing myself as a reviewer for now as I'm heading on vacation -- overall
it looks OK so if someone else wants to give the final "ell gee tee emm" I'd be
OK w/ that

http://codereview.chromium.org/10662049/diff/15001/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10662049/diff/15001/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:636:
scoped_refptr<base::MessageLoopProxy> MediaStreamManager::GetMessageLoop() {
it still seems unfortunate to expose this for the sake of a unit test
force-quitting the thread

~MediaStreamManager() triggers thread shutdown -- can't we be using the dtor to
post the thread stop to BrowerThread then wait for that message loop to
complete?

[no longer working on chromium, 2012-06-28 16:53:22]

Andrew, done with removing the code which disposes the message loop for unit
test. Have a nice vocation.

John, I tried using the token, but it only guarantees the calls are executed in
sequence, but not exactly on the same thread. So I can't move to the thread pool
completely.

Hope it is fine to keep the thread now.

Magnus and John, do you mind taking another look?

BR,
/SX

[mflodman_chromium_OOO, 2012-06-29 14:46:04]

Nice CL!

Some comments.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.h:40:
AudioInputDeviceManager(scoped_refptr<base::MessageLoopProxy> message_loop);
Add explicit.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:107:
CommandLine command_line =  CommandLine(CommandLine::NO_PROGRAM);
nit: remove one extra space.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_host_unittest.cc:76:
MockVideoCaptureHost(content::ResourceContext* resource_context)
Explicit.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_host_unittest.cc:305:
EXPECT_TRUE(event.TimedWait(TestTimeouts::action_timeout()));
Not sure if there is a prefered Chromium way for this? What about using
message_loop_->Run() and let the WillOnce above post a QuitTask to
message_loop_? Would that work for this?

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_manager.cc:44:
listener_(NULL),
nit: indent this line and the two below.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_manager.cc:357: bool
VideoCaptureManager::IsOnCaptureDeviceThread() const {
Maybe change name of this function now when the thread is not named like this
any longer?

http://codereview.chromium.org/10662049/diff/28004/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_win.cc (right):

http://codereview.chromium.org/10662049/diff/28004/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.cc:180: // This API can only be
run on a COM thread.
'a COM thread.' -> 'a COM initialized thread.' ?

http://codereview.chromium.org/10662049/diff/28004/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_win.h (right):

http://codereview.chromium.org/10662049/diff/28004/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.h:29: // All the APIs in the
class can only be run on a COM thread.
'a COM thread.' -> 'a COM initialized thread.' ?

[no longer working on chromium, 2012-07-02 08:40:49]

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.h:40:
AudioInputDeviceManager(scoped_refptr<base::MessageLoopProxy> message_loop);
On 2012/06/29 14:46:04, mflodman wrote:
> Add explicit.

Done.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:107:
CommandLine command_line =  CommandLine(CommandLine::NO_PROGRAM);
On 2012/06/29 14:46:04, mflodman wrote:
> nit: remove one extra space.

Done.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_host_unittest.cc:76:
MockVideoCaptureHost(content::ResourceContext* resource_context)
On 2012/06/29 14:46:04, mflodman wrote:
> Explicit.

Done.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_host_unittest.cc:305:
EXPECT_TRUE(event.TimedWait(TestTimeouts::action_timeout()));
On 2012/06/29 14:46:04, mflodman wrote:
> Not sure if there is a prefered Chromium way for this? What about using
> message_loop_->Run() and let the WillOnce above post a QuitTask to
> message_loop_? Would that work for this? 

Done.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_manager.cc:44:
listener_(NULL),
On 2012/06/29 14:46:04, mflodman wrote:
> nit: indent this line and the two below.

Done.

http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_manager.cc:357: bool
VideoCaptureManager::IsOnCaptureDeviceThread() const {
On 2012/06/29 14:46:04, mflodman wrote:
> Maybe change name of this function now when the thread is not named like this
> any longer?

Done.

http://codereview.chromium.org/10662049/diff/28004/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_win.h (right):

http://codereview.chromium.org/10662049/diff/28004/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.h:29: // All the APIs in the
class can only be run on a COM thread.
On 2012/06/29 14:46:04, mflodman wrote:
> 'a COM thread.' -> 'a COM initialized thread.' ?

Done.

[no longer working on chromium, 2012-07-02 09:49:20]

Tommi, please take a quick look. Thanks.

BR,
/SX

[tommi (sloooow) - chröme, 2012-07-02 13:36:36]

I like simplifying the code but I'm not so sure about replacing it with
dependency on global objects or static getters since it makes writing tests much
harder.

I would like to do this in steps.  E.g. 

* Start by changing GetForResourceContext() so that it doesn't need an
audio_manager pointer but calls BrowserLoop::GetAudioManager().  This is a
static method that's not used in tests afaics and the AudioInputDeviceManager
can still be tested without dependency on BrowserLoop.

* Change MediaStreamManager to get a pointer to an AudioInputDeviceManager
instance instead of a pointer to AudioManager (which it only uses later to
construct AudioInputDeviceManager).  This has the extra benefit of allowing us
to test MediaStreamManager with a test implementation of
AudioInputDeviceManager.

* Move MediaStreamManager out of the ResourceContext as we did with
AudioManager.  As it is we're forced to create a new instance of
MediaStreamManager (+ related objects) per resource context.  I don't think this
is necessary as only a single MediaStreamManager should be enough.

- John, what do you think about moving the MediaStreamManager instance into
BrowserLoop, right next to AudioManager?  The two are related but perhaps there
is a more appropriate place to put it. 

Also, I think we should use the MTA and not STA since afaik this API supports
it. That's what we do in the audio code.

Some of this is repeated in my comments below, so sorry for the repetition.

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/audio_input_device_manager.cc:80: //
TODO(xians): Ensure AudioManager outlives MediaStreamManager.
Is the device thread guaranteed to have terminated before any of the "known"
Chrome thread such as the IO thread or the main browser loop?

If so, then you can safely call GetAudioManager() here without worrying if it's
valid or not since if the device thread is running, you're guaranteed that
BrowserLoop is valid.

If not, then I think we should fix that and here is not where we need to fix it.
 It should be a property of the device thread itself that it does never outlive
the browser loop and/or IO thread.

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/audio_input_device_manager.h:10: // device
thread, while start/stop are synchronous API on IO thread.
are synchronous on the IO thread.

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/audio_input_device_manager.h:41:
scoped_refptr<base::MessageLoopProxy> message_loop);
why scoped_refptr<> here?  instead just do:
explicit AudioInputDeviceManager(base::MessageLoopProxy* message_loop);

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/audio_input_device_manager.h:41:
scoped_refptr<base::MessageLoopProxy> message_loop);
add a comment here about what message loop this is and how it will be used. 
'message_loop' isn't very descriptive... perhaps device_loop_ or something like
that would be more descriptive?

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/audio_input_device_manager.h:72: // Helpers.
this comment doesn't really add anything so just remove it

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/audio_input_device_manager.h:86:
scoped_refptr<base::MessageLoopProxy> message_loop_;
device_loop_ or media_stream_loop_?

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:100:
// Returns true if machine has an audio input device.
I don't think that 'iff' was a typo if that's what you were fixing.  See for
more info:
http://en.wikipedia.org/wiki/If_and_only_if

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:106:
virtual void SetUp() {
OVERRIDE?

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
File content/browser/renderer_host/media/media_stream_dispatcher_host.h (left):

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/media_stream_dispatcher_host.h:33:
media::AudioManager* audio_manager);
I worry that we're going back to a singleton pattern.  Now we're adding a
dependency on global state of BrowserLoop to all classes that previously
supported injection of an audio manager interface.  Is there a good reason for
this?

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.h:58: explicit
DeviceThread(const char *name) : base::Thread(name) {}
const char* name

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.h:64:
CoInitialize(NULL);
use the ScopedCOMInitializer class and remove the #if's

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
content/browser/renderer_host/media/video_capture_manager.cc:42:
scoped_refptr<base::MessageLoopProxy> message_loop)
no need for scoped_refptr<>

https://chromiumcodereview.appspot.com/10662049/diff/21004/media/video/captur...
File media/video/capture/win/video_capture_device_win.cc (right):

https://chromiumcodereview.appspot.com/10662049/diff/21004/media/video/captur...
media/video/capture/win/video_capture_device_win.cc:184:
ScopedComPtr<ICreateDevEnum> dev_enum;
DCHECK(CalledOnValidThread()) first

https://chromiumcodereview.appspot.com/10662049/diff/21004/media/video/captur...
File media/video/capture/win/video_capture_device_win.h (right):

https://chromiumcodereview.appspot.com/10662049/diff/21004/media/video/captur...
media/video/capture/win/video_capture_device_win.h:30: class
VideoCaptureDeviceWin
inherit from base::NonThreadSafe and use CalledOnValidThread to ensure you're on
the COM initialized thread.

[mflodman_chromium_OOO, 2012-07-02 18:56:30]

On 2012/07/02 13:36:36, tommi wrote:
> I like simplifying the code but I'm not so sure about replacing it with
> dependency on global objects or static getters since it makes writing tests
much
> harder.
> 
> I would like to do this in steps.  E.g. 
> 
> * Start by changing GetForResourceContext() so that it doesn't need an
> audio_manager pointer but calls BrowserLoop::GetAudioManager().  This is a
> static method that's not used in tests afaics and the AudioInputDeviceManager
> can still be tested without dependency on BrowserLoop.
> 
> * Change MediaStreamManager to get a pointer to an AudioInputDeviceManager
> instance instead of a pointer to AudioManager (which it only uses later to
> construct AudioInputDeviceManager).  This has the extra benefit of allowing us
> to test MediaStreamManager with a test implementation of
> AudioInputDeviceManager.
> 
> * Move MediaStreamManager out of the ResourceContext as we did with
> AudioManager.  As it is we're forced to create a new instance of
> MediaStreamManager (+ related objects) per resource context.  I don't think
this
> is necessary as only a single MediaStreamManager should be enough.

Waiting for John's reply, but I think this make sense and have several benefits.
E.g. using the same video device by several profiles at the same time, which
doesn't work right now, and having same life time as AudioManager.

Refactoring the Media Stream code is on my todo list when I'm back from
vacation, so I can pitch in here when I'm back. If this hasn't been done by
then.


> - John, what do you think about moving the MediaStreamManager instance into
> BrowserLoop, right next to AudioManager?  The two are related but perhaps
there
> is a more appropriate place to put it. 
> 
> Also, I think we should use the MTA and not STA since afaik this API supports
> it. That's what we do in the audio code.
> 
> Some of this is repeated in my comments below, so sorry for the repetition.
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> File content/browser/renderer_host/media/audio_input_device_manager.cc
(right):
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/audio_input_device_manager.cc:80: //
> TODO(xians): Ensure AudioManager outlives MediaStreamManager.
> Is the device thread guaranteed to have terminated before any of the "known"
> Chrome thread such as the IO thread or the main browser loop?
> 
> If so, then you can safely call GetAudioManager() here without worrying if
it's
> valid or not since if the device thread is running, you're guaranteed that
> BrowserLoop is valid.
> 
> If not, then I think we should fix that and here is not where we need to fix
it.
>  It should be a property of the device thread itself that it does never
outlive
> the browser loop and/or IO thread.
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> File content/browser/renderer_host/media/audio_input_device_manager.h (right):
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/audio_input_device_manager.h:10: // device
> thread, while start/stop are synchronous API on IO thread.
> are synchronous on the IO thread.
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/audio_input_device_manager.h:41:
> scoped_refptr<base::MessageLoopProxy> message_loop);
> why scoped_refptr<> here?  instead just do:
> explicit AudioInputDeviceManager(base::MessageLoopProxy* message_loop);
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/audio_input_device_manager.h:41:
> scoped_refptr<base::MessageLoopProxy> message_loop);
> add a comment here about what message loop this is and how it will be used. 
> 'message_loop' isn't very descriptive... perhaps device_loop_ or something
like
> that would be more descriptive?
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/audio_input_device_manager.h:72: //
Helpers.
> this comment doesn't really add anything so just remove it
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/audio_input_device_manager.h:86:
> scoped_refptr<base::MessageLoopProxy> message_loop_;
> device_loop_ or media_stream_loop_?
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> File
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
>
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:100:
> // Returns true if machine has an audio input device.
> I don't think that 'iff' was a typo if that's what you were fixing.  See for
> more info:
> http://en.wikipedia.org/wiki/If_and_only_if
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
>
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:106:
> virtual void SetUp() {
> OVERRIDE?
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> File content/browser/renderer_host/media/media_stream_dispatcher_host.h
(left):
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/media_stream_dispatcher_host.h:33:
> media::AudioManager* audio_manager);
> I worry that we're going back to a singleton pattern.  Now we're adding a
> dependency on global state of BrowserLoop to all classes that previously
> supported injection of an audio manager interface.  Is there a good reason for
> this?
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> File content/browser/renderer_host/media/media_stream_manager.h (right):
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/media_stream_manager.h:58: explicit
> DeviceThread(const char *name) : base::Thread(name) {}
> const char* name
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/media_stream_manager.h:64:
> CoInitialize(NULL);
> use the ScopedCOMInitializer class and remove the #if's
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> File content/browser/renderer_host/media/video_capture_manager.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/content/browser/re...
> content/browser/renderer_host/media/video_capture_manager.cc:42:
> scoped_refptr<base::MessageLoopProxy> message_loop)
> no need for scoped_refptr<>
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/media/video/captur...
> File media/video/capture/win/video_capture_device_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/media/video/captur...
> media/video/capture/win/video_capture_device_win.cc:184:
> ScopedComPtr<ICreateDevEnum> dev_enum;
> DCHECK(CalledOnValidThread()) first
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/media/video/captur...
> File media/video/capture/win/video_capture_device_win.h (right):
> 
>
https://chromiumcodereview.appspot.com/10662049/diff/21004/media/video/captur...
> media/video/capture/win/video_capture_device_win.h:30: class
> VideoCaptureDeviceWin
> inherit from base::NonThreadSafe and use CalledOnValidThread to ensure you're
on
> the COM initialized thread.

[no longer working on chromium, 2012-07-04 12:35:25]

Tommi and John, this version move the MediaStreamManager from resource context
to BrowserMainloop.
One of the main benefit for doing this is: previously we have one
MediaStreamManager per profile, which means we may have multiple media stream
device threads if the user signs in with multiple profiles. By moving the
MediaStreamManager to BrowserMainloop, we will have only one media stream device
thread for the whole browser.

Please take another look, thanks.

BR,
/SX

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.cc:80: //
TODO(xians): Ensure AudioManager outlives MediaStreamManager.
On 2012/07/02 13:36:36, tommi wrote:
> Is the device thread guaranteed to have terminated before any of the "known"
> Chrome thread such as the IO thread or the main browser loop?
> 
> If so, then you can safely call GetAudioManager() here without worrying if
it's
> valid or not since if the device thread is running, you're guaranteed that
> BrowserLoop is valid.
> 
> If not, then I think we should fix that and here is not where we need to fix
it.
>  It should be a property of the device thread itself that it does never
outlive
> the browser loop and/or IO thread.

Done.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.h:10: // device
thread, while start/stop are synchronous API on IO thread.
On 2012/07/02 13:36:36, tommi wrote:
> are synchronous on the IO thread.

Done.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.h:41:
scoped_refptr<base::MessageLoopProxy> message_loop);
On 2012/07/02 13:36:36, tommi wrote:
> add a comment here about what message loop this is and how it will be used. 
> 'message_loop' isn't very descriptive... perhaps device_loop_ or something
like
> that would be more descriptive?

Done with changing the name to device_loop.

I have a comment above the declaration of device_loop_, I think it should be
enough. Hope it is fine to skip adding the comment here.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.h:41:
scoped_refptr<base::MessageLoopProxy> message_loop);
On 2012/07/02 13:36:36, tommi wrote:
> why scoped_refptr<> here?  instead just do:
> explicit AudioInputDeviceManager(base::MessageLoopProxy* message_loop);

Done.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.h:72: // Helpers.
On 2012/07/02 13:36:36, tommi wrote:
> this comment doesn't really add anything so just remove it

Done.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.h:86:
scoped_refptr<base::MessageLoopProxy> message_loop_;
On 2012/07/02 13:36:36, tommi wrote:
> device_loop_ or media_stream_loop_?

Done.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:100:
// Returns true if machine has an audio input device.
On 2012/07/02 13:36:36, tommi wrote:
> I don't think that 'iff' was a typo if that's what you were fixing.  See for
> more info:
> http://en.wikipedia.org/wiki/If_and_only_if

Good to know, thanks.
Done.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:106:
virtual void SetUp() {
On 2012/07/02 13:36:36, tommi wrote:
> OVERRIDE?

Done.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.h:58: explicit
DeviceThread(const char *name) : base::Thread(name) {}
On 2012/07/02 13:36:36, tommi wrote:
> const char* name

Done.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.h:64:
CoInitialize(NULL);
On 2012/07/02 13:36:36, tommi wrote:
> use the ScopedCOMInitializer class and remove the #if's

Done.

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/10662049/diff/21004/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_manager.cc:42:
scoped_refptr<base::MessageLoopProxy> message_loop)
On 2012/07/02 13:36:36, tommi wrote:
> no need for scoped_refptr<>

Done.

http://codereview.chromium.org/10662049/diff/21004/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_win.h (right):

http://codereview.chromium.org/10662049/diff/21004/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.h:30: class
VideoCaptureDeviceWin
On 2012/07/02 13:36:36, tommi wrote:
> inherit from base::NonThreadSafe and use CalledOnValidThread to ensure you're
on
> the COM initialized thread.

Done.

[tommi (sloooow) - chröme, 2012-07-04 13:46:48]

Excellent work Shijing. The classes are now much more easily testable as well as
being simpler to read and understand with less dependency on other
implementations.  LGTM (G==Great!).

http://codereview.chromium.org/10662049/diff/28030/content/browser/browser_ma...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/browser_ma...
content/browser/browser_main_loop.cc:487:
BrowserThread::DeleteOnIOThread::Destruct(media_stream_manager_.release());
maybe add a little comment here to explain why we need to do this.

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.cc:81: //
AudioManager is deleted after FILE thread stops, so the device thread
do we still use the FILE thread for something or do you mean the IO thread here?

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.h:48:
base::MessageLoopProxy* message_loop) OVERRIDE;
device_thread_loop?

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:183:
media_stream_manager_->UseFakeDevice();
I think this is a good example of how great this change is.  Now, since we
create the media stream manager in the test and inject it, we can (assuming we
don't really need this in production) get rid of all the "UseFakeDevice" hack
that's in the production code.  We could instead provide a special
implementation of the media stream manager that always uses the fake device when
the tests are run and not mess up the production code for it.
I don't think we should do that in this cl, just pointing out the possibilities
now. Very nice.

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.h:49: // Thread that
enters STA on windows, and is base::thread on linux and mac.
update comment s/STA/MTA and base::thread -> base::Thread

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_host_unittest.cc:224:
media_stream_manager_->UseFakeDevice();
Same thing here. (maybe adding a TODO in this cl would be a good idea?)

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_manager.cc:53:
base::MessageLoopProxy* message_loop) {
device_thread_loop or device_loop

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:66: class
MockFrameObserver: public media::VideoCaptureDevice::EventHandler {
add a space before :

http://codereview.chromium.org/10662049/diff/28030/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_win.h (right):

http://codereview.chromium.org/10662049/diff/28030/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.h:30: // All the APIs in the
class can only be run on a COM initialized thread.
s/APIs/methods

[no longer working on chromium, 2012-07-04 14:31:39]

Many thanks to Tommi's great help.

John, how do you think about the changes? Can I have your owner stamp to land
this?


BR,
/SX

http://codereview.chromium.org/10662049/diff/28030/content/browser/browser_ma...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/browser_ma...
content/browser/browser_main_loop.cc:487:
BrowserThread::DeleteOnIOThread::Destruct(media_stream_manager_.release());
On 2012/07/04 13:46:48, tommi wrote:
> maybe add a little comment here to explain why we need to do this.

Done.

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.cc:81: //
AudioManager is deleted after FILE thread stops, so the device thread
On 2012/07/04 13:46:48, tommi wrote:
> do we still use the FILE thread for something or do you mean the IO thread
here?

Wrong comment from some previous upload.

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_device_manager.h:48:
base::MessageLoopProxy* message_loop) OVERRIDE;
On 2012/07/04 13:46:48, tommi wrote:
> device_thread_loop?

Done.

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:183:
media_stream_manager_->UseFakeDevice();
On 2012/07/04 13:46:48, tommi wrote:
> I think this is a good example of how great this change is.  Now, since we
> create the media stream manager in the test and inject it, we can (assuming we
> don't really need this in production) get rid of all the "UseFakeDevice" hack
> that's in the production code.  We could instead provide a special
> implementation of the media stream manager that always uses the fake device
when
> the tests are run and not mess up the production code for it.
> I don't think we should do that in this cl, just pointing out the
possibilities
> now. Very nice.

True, thanks for all the good ideas. I will add a TODO to remind us of removing
the UseFakeDevice().

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.h:49: // Thread that
enters STA on windows, and is base::thread on linux and mac.
On 2012/07/04 13:46:48, tommi wrote:
> update comment s/STA/MTA and base::thread -> base::Thread

Done.

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_host_unittest.cc:224:
media_stream_manager_->UseFakeDevice();
On 2012/07/04 13:46:48, tommi wrote:
> Same thing here. (maybe adding a TODO in this cl would be a good idea?)

Done, I added a TODO in media_stream_manager.h, which should be enough.

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_manager.cc:53:
base::MessageLoopProxy* message_loop) {
On 2012/07/04 13:46:48, tommi wrote:
> device_thread_loop or device_loop

Done.

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

http://codereview.chromium.org/10662049/diff/28030/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:66: class
MockFrameObserver: public media::VideoCaptureDevice::EventHandler {
On 2012/07/04 13:46:48, tommi wrote:
> add a space before :

Done.

http://codereview.chromium.org/10662049/diff/28030/media/video/capture/win/vi...
File media/video/capture/win/video_capture_device_win.h (right):

http://codereview.chromium.org/10662049/diff/28030/media/video/capture/win/vi...
media/video/capture/win/video_capture_device_win.h:30: // All the APIs in the
class can only be run on a COM initialized thread.
On 2012/07/04 13:46:48, tommi wrote:
> s/APIs/methods

Done.

[no longer working on chromium, 2012-07-05 09:10:32]

Removed John from the reviewer list since he is on vocation.

sky: could you please help reviewing content/browser

Thanks,

BR,
SX

[no longer working on chromium, 2012-07-05 09:20:16]

avi or darin, can I have owner stamp from either of you for content/browser?

Thanks,

BR,
/SX

[jam, 2012-07-05 17:49:59]

content/browser lgtm (didn't look at the media subdirs)

[commit-bot: I haz the power, 2012-07-06 10:38:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/10662049/18013

[commit-bot: I haz the power, 2012-07-06 12:03:07]

Change committed as 145584