Removing singleton property of MediaStreamManager and creating thread first when needed.

Removing singleton property of MediaStreamManager and creating thread first when needed.


BUG=92125
TEST=

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

[scherkus (not reviewing), 2011-08-16 19:24:09]

drive by but agree w/ jam creation looks good!

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:29:
class MockMediaStreamResourceContext : public ResourceContext {
you might be able to use MockResourceContext

content/browser/mock_resource_context.h

[willchan no longer on Chromium, 2011-08-17 19:55:19]

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.cc:365: delete
device_settings_;
What do you think about using scoped_ptr instead? It becomes auto-deleting and
self-documenting that MediaStreamManager owns these members then.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.h:43: : public
base::RefCounted<media_stream::MediaStreamManager>,
Why is this refcounted? Who owns this object? It's in the ProfileIOData which
suggests that ProfileIOData owns it. If so, please remove the refcounting. If it
truly has shared ownership, please document the ownership here.

Note that if you're using refcounting due to NewRunnableMethod() needing it to
be refcounted, then you should either use ScopedRunnableMethodFactory,
base::WeakPtrFactory, or base::Bind()+base::Unretained() to eliminate the need
for the refcounting.

My inkling is that the ownership of this object is well-defined. It should die
when the ProfileIOData dies (which is right after the Profile dies). And nothing
should be posting to it afterwards, and if anything is doing so, it's a bug. If
this is correct, then please don't use refcounting. Use the solutions I
recommended.

http://codereview.chromium.org/7649016/diff/1/content/browser/resource_contex...
File content/browser/resource_context.cc (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/resource_contex...
content/browser/resource_context.cc:149: media_stream::MediaStreamManager*
ResourceContext::media_stream_manager()
This formatting is less common. Try putting the return value on its own line.
See lines 162-163.

[wjia(left Chromium), 2011-08-17 21:13:54]

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/b...
File content/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/b...
content/browser/renderer_host/browser_render_process_host.cc:363: new
VideoCaptureHost(&browser_context()->GetResourceContext()));
do you need content::ResourceContext or just media_stream::MediaStreamManager
here? It looks that VideoCaputreHost only needs
media_stream::MediaStreamManager, and it will simplify the code in video capture
host.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/b...
content/browser/renderer_host/browser_render_process_host.cc:377:
&browser_context()->GetResourceContext(), id()));
ditto.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.h:47: typedef
MediaStreamManager* (AccessorMethod)();
is this still in use?

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/video_capture_controller.h:41: const
content::ResourceContext* resource_context);
media_stream::MediaStreamManager is good enough here.

[jam, 2011-08-18 00:55:19]

non media content files lgtm

[mflodman_chromium_OOO, 2011-08-19 11:07:09]

New patch based on review, main changes:

- Replaced scoped_refptr with scoped_ptr in ProfileIOData.
- Changed so a MediaStreamManager instance is handed to
MediaStreamDispatcherHost and VideoCaptureHost instead of ResourceContext.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/b...
File content/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/b...
content/browser/renderer_host/browser_render_process_host.cc:363: new
VideoCaptureHost(&browser_context()->GetResourceContext()));
On 2011/08/17 21:13:54, wjia wrote:
> do you need content::ResourceContext or just media_stream::MediaStreamManager
> here? It looks that VideoCaputreHost only needs
> media_stream::MediaStreamManager, and it will simplify the code in video
capture
> host.

Done.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/b...
content/browser/renderer_host/browser_render_process_host.cc:377:
&browser_context()->GetResourceContext(), id()));
On 2011/08/17 21:13:54, wjia wrote:
> ditto.

Done.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:29:
class MockMediaStreamResourceContext : public ResourceContext {
On 2011/08/16 19:24:09, scherkus wrote:
> you might be able to use MockResourceContext
> 
> content/browser/mock_resource_context.h

This part was removed based on review feedback from wjia.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.cc:365: delete
device_settings_;
On 2011/08/17 19:55:19, willchan wrote:
> What do you think about using scoped_ptr instead? It becomes auto-deleting and
> self-documenting that MediaStreamManager owns these members then.

Done.
I've received that comment from scherkus as well, changed.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.h:43: : public
base::RefCounted<media_stream::MediaStreamManager>,
On 2011/08/17 19:55:19, willchan wrote:
> Why is this refcounted? Who owns this object? It's in the ProfileIOData which
> suggests that ProfileIOData owns it. If so, please remove the refcounting. If
it
> truly has shared ownership, please document the ownership here.
> 
> Note that if you're using refcounting due to NewRunnableMethod() needing it to
> be refcounted, then you should either use ScopedRunnableMethodFactory,
> base::WeakPtrFactory, or base::Bind()+base::Unretained() to eliminate the need
> for the refcounting.
> 
> My inkling is that the ownership of this object is well-defined. It should die
> when the ProfileIOData dies (which is right after the Profile dies). And
nothing
> should be posting to it afterwards, and if anything is doing so, it's a bug.
If
> this is correct, then please don't use refcounting. Use the solutions I
> recommended.

Done, removed ref counted and replaced scoped_refptr with scoped_ptr in
ProfileIOData.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_manager.h:47: typedef
MediaStreamManager* (AccessorMethod)();
On 2011/08/17 21:13:54, wjia wrote:
> is this still in use?

Removed, done.

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/video_capture_controller.h (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/video_capture_controller.h:41: const
content::ResourceContext* resource_context);
On 2011/08/17 21:13:54, wjia wrote:
> media_stream::MediaStreamManager is good enough here.

Done.

http://codereview.chromium.org/7649016/diff/1/content/browser/resource_contex...
File content/browser/resource_context.cc (right):

http://codereview.chromium.org/7649016/diff/1/content/browser/resource_contex...
content/browser/resource_context.cc:149: media_stream::MediaStreamManager*
ResourceContext::media_stream_manager()
On 2011/08/17 19:55:19, willchan wrote:
> This formatting is less common. Try putting the return value on its own line.
> See lines 162-163.

Done.

[mflodman_chromium_OOO, 2011-08-19 12:31:51]

I was too quick on uploading the previous patch and reverted the part changing
that VideoCaptureHost and MediaStreamDispatcherHost only gets a
MediaStreamManager* instead of content::ResourceContext*. Sorry about that...

The reason:
Patch set 2 called content::ResourceContext::media_stream_manager() from
BrowserRenderProcessHost::CreateMessageFilters, which isn't done on the IO
thread.

Wei,
Any ideas/suggestions?

[wjia(left Chromium), 2011-08-19 18:09:54]

It appears that ResourceContext can be accessed only on IO thread. So it's good
to keep using content::ResourceContext* when VideoCaptureHost and
MediaStreamDispatcherHost are created in
BrowserRenderProcessHost::CreateMessageFilters.

But it's better to use either MediaStreamManager* or VideoCaptureManager* in
VideoCaptureController constructor, since one function is the callback from
another thread.

http://codereview.chromium.org/7649016/diff/17002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/7649016/diff/17002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:162:
Error(params_.session_id);
resource_context_ can be accessed only on IO thread.
VideoCaptureController::OnError is called from device. That's on device thread.
It's good to pass in VideoCaptureManager* when VideoCaptureController is
created.

[mflodman_chromium_OOO, 2011-08-21 19:15:24]

Changed so VideoCaptureController ctor gets VideoCaptureManager* instead of
ResourceContext*.

http://codereview.chromium.org/7649016/diff/17002/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

http://codereview.chromium.org/7649016/diff/17002/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_controller.cc:162:
Error(params_.session_id);
On 2011/08/19 18:09:54, wjia wrote:
> resource_context_ can be accessed only on IO thread.
> VideoCaptureController::OnError is called from device. That's on device
thread.
> It's good to pass in VideoCaptureManager* when VideoCaptureController is
> created.

Done.

[wjia(left Chromium), 2011-08-21 22:41:00]

lgtm.

[scherkus (not reviewing), 2011-08-23 19:33:26]

I've got a question about the Mock*ResourceContext classes but rest is LGTM

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

http://codereview.chromium.org/7649016/diff/20001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:29:
class MockMediaStreamResourceContext : public ResourceContext {
in patchset #1 you said this "This part was removed based on review feedback
from wjia."

but it's still here? I'm a bit confused.

http://codereview.chromium.org/7649016/diff/20001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

http://codereview.chromium.org/7649016/diff/20001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host_unittest.cc:73: class
MockVideoCaptureResourceContext : public ResourceContext {
ditto here -- this can likely be deduped in some way

[mflodman_chromium_OOO, 2011-08-24 14:27:24]

Changed unit tests based on feedback by scherkus.

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

http://codereview.chromium.org/7649016/diff/20001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:29:
class MockMediaStreamResourceContext : public ResourceContext {
On 2011/08/23 19:33:26, scherkus wrote:
> in patchset #1 you said this "This part was removed based on review feedback
> from wjia."
> 
> but it's still here? I'm a bit confused.

It was removed and then re-added due to threading issues in that upload. I've
changed to use MockResourceContext now.

http://codereview.chromium.org/7649016/diff/20001/content/browser/renderer_ho...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

http://codereview.chromium.org/7649016/diff/20001/content/browser/renderer_ho...
content/browser/renderer_host/media/video_capture_host_unittest.cc:73: class
MockVideoCaptureResourceContext : public ResourceContext {
On 2011/08/23 19:33:26, scherkus wrote:
> ditto here -- this can likely be deduped in some way

Done.

[scherkus (not reviewing), 2011-08-24 14:43:40]

thanks!

[Elliot Glaysher, 2011-08-25 19:22:59]

I am going to cautiously lgtm the profile_io_data change.

willchan wrote that and would be a better reviewer, but he's out and your change
to those files looks simple enough... (though those are famous last words)

[mflodman_chromium_OOO, 2011-08-25 19:34:56]

Thanks!

I'll commit the CL, but of course listen to feedback from willchan and make
changes if he has any more comments when he is back.


On Thu, Aug 25, 2011 at 9:22 PM, <erg@chromium.org> wrote:

> I am going to cautiously lgtm the profile_io_data change.
>
> willchan wrote that and would be a better reviewer, but he's out and your
> change
> to those files looks simple enough... (though those are famous last words)
>
>
>
http://codereview.chromium.**org/7649016/<http://codereview.chromium.org/7649...
>

[willchan no longer on Chromium, 2011-08-26 17:10:13]

LGTM!

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

http://codereview.chromium.org/7649016/diff/25002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:9: #include
"base/lazy_instance.h"
Delete

http://codereview.chromium.org/7649016/diff/25002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.cc:49: :
device_settings_(new MediaStreamDeviceSettings(this)),
Don't you need ALLOW_THIS_IN_INITIALIZER_LIST and compiler_specific.h?

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

http://codereview.chromium.org/7649016/diff/25002/content/browser/renderer_ho...
content/browser/renderer_host/media/media_stream_manager.h:109:
MediaStreamProvider* GetDeviceManager(MediaStreamType stream_type);
My personal style pref is to keep this const and make this logically const by
using mutable.