Remove GetAudioManager and GetMediaStreamManager from ResourceContext. The reason is the content mo…

Remove GetAudioManager and GetMediaStreamManager from ResourceContext. The reason is the content module should create all the concrete objects that it knows about, instead of depending on every embedder to do so. ResourceContext is then just the interfaces that it implements and which it provides on the IO thread.

MediaStreamManager now has its own static getter like other internal content classes on ResourceContext or BrowserContext.

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

[scherkus (not reviewing), 2012-02-21 19:12:44]

+tommi who landed the initial AudioManager de-singleton-ification
http://src.chromium.org/viewvc/chrome?view=rev&revision=114084

Is AudioManager leaked at shutdown now? This change seems conflict with trying
to avoid singletons. Perhaps there's some middle ground somewhere.

I'm not sure why AudioManager still stores itself in g_audio_manager -- I don't
see a reason why multiple AudioManagers could be created if need be.

Adding tommi for MediaStreamManager stuff and some ideas on AudioManager

[jam, 2012-02-21 20:29:33]

(my reply isn't making it here for some reason, so pasting on rietveld)

I didn't convert AudioManager to a singleton. There was just a global pointer to
it in both media/audio/audio_manager.cc and chrome/browser_process_impl.cc. So
what I did is make that pointer accessible for one place, since we don't want
chrome owning it. The ownership is still the same as before, which is per
process. I moved the ownership from BrowserProcessImpl (in chrome) to
BrowserMainLoop (in content)

[tommi (sloooow) - chröme, 2012-02-21 21:04:07]

https://chromiumcodereview.appspot.com/9433006/diff/4018/media/audio/audio_ma...
File media/audio/audio_manager.cc (right):

https://chromiumcodereview.appspot.com/9433006/diff/4018/media/audio/audio_ma...
media/audio/audio_manager.cc:37: return g_audio_manager;
See the comment above for g_audio_manager:
" // Used only to make sure we never create more than one instance."

I intend to remove this variable, but if we do this, we start to actually rely
on it again which would be a step backwards.

Fixing this for content_client has actually been on the TODO list though (so
thanks for doing this!) - but what I was planning on doing was to add instead an
audio_manager() property to ContentClient and access it from there.  WDYT?

[jam, 2012-02-21 21:09:47]

thanks for the review!

https://chromiumcodereview.appspot.com/9433006/diff/4018/media/audio/audio_ma...
File media/audio/audio_manager.cc (right):

https://chromiumcodereview.appspot.com/9433006/diff/4018/media/audio/audio_ma...
media/audio/audio_manager.cc:37: return g_audio_manager;
On 2012/02/21 21:04:08, tommi wrote:
> See the comment above for g_audio_manager:
> " // Used only to make sure we never create more than one instance."
> 
> I intend to remove this variable, but if we do this, we start to actually rely
> on it again which would be a step backwards.

got it. I didn't know you were planning on removing it, and so I didn't want to
duplicate this global in both media and content.

I'll remove it from media and move it to content.

> 
> Fixing this for content_client has actually been on the TODO list though (so
> thanks for doing this!) - but what I was planning on doing was to add instead
an
> audio_manager() property to ContentClient and access it from there.  WDYT?

Do you mean ContentBrowserClient (ContentClient is an interface that embedders
implement in each process, while CBC is one that's implemented in the browser
process)? If so, then that would take us back to the embedder owning and
creating this object, which is what this change is trying to avoid. I'll look
for a good place to store this in content (usually we store it with the class,
but in this case the class is outside of content)

[tommi (sloooow) - chröme, 2012-02-21 21:15:22]

https://chromiumcodereview.appspot.com/9433006/diff/4018/media/audio/audio_ma...
File media/audio/audio_manager.cc (right):

https://chromiumcodereview.appspot.com/9433006/diff/4018/media/audio/audio_ma...
media/audio/audio_manager.cc:37: return g_audio_manager;
On 2012/02/21 21:09:47, John Abd-El-Malek wrote:
> On 2012/02/21 21:04:08, tommi wrote:
> > See the comment above for g_audio_manager:
> > " // Used only to make sure we never create more than one instance."
> > 
> > I intend to remove this variable, but if we do this, we start to actually
rely
> > on it again which would be a step backwards.
> 
> got it. I didn't know you were planning on removing it, and so I didn't want
to
> duplicate this global in both media and content.
> 
> I'll remove it from media and move it to content.
> 
> > 
> > Fixing this for content_client has actually been on the TODO list though (so
> > thanks for doing this!) - but what I was planning on doing was to add
instead
> an
> > audio_manager() property to ContentClient and access it from there.  WDYT?
> 
> Do you mean ContentBrowserClient (ContentClient is an interface that embedders
> implement in each process, while CBC is one that's implemented in the browser
> process)?

Yes, ContentBrowserClient.

> If so, then that would take us back to the embedder owning and
> creating this object, which is what this change is trying to avoid. I'll look
> for a good place to store this in content (usually we store it with the class,
> but in this case the class is outside of content)

I see what you mean.  Sounds like a plan.

[vrk (LEFT CHROMIUM), 2012-02-22 19:07:02]

drive-by: Thanks for getting this sorted out! I will update my CL 9316077
accordingly when I get back to it (which will be soon I hope! still fighting
other fires currently :) ).

[jam, 2012-02-23 00:42:34]

Tommi/Andrew: please take another look. I've removed the static getter on
AudioManager and plumbed AudioManager through to different classes instead of
ResourceContext.

I've avoided making the embedder layer (chrome) know about AudioManager, since
it can just be an implementation detail of content.

For followup changes I'll add proper interfaces around SpeechInputManager and
SpeechRecognizer.

[jam, 2012-02-23 00:43:00]

On 2012/02/22 19:07:02, Victoria Kirst wrote:
> drive-by: Thanks for getting this sorted out! I will update my CL 9316077
> accordingly when I get back to it (which will be soon I hope! still fighting
> other fires currently :) ).

ah, I forgot about your change. Hopefully it makes it easier and doesn't cause
you conflicts!

[tommi (sloooow) - chröme, 2012-02-23 07:50:59]

awesome.  no concerns, just a couple of questions and nits.

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

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:24: static const
char* kMediaStreamManagerKeyName = "content_media_stream_manager";
char[] instead of char*

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
content/browser/renderer_host/media/video_capture_host_unittest.cc:73: explicit
MockVideoCaptureHost(content::ResourceContext* resource_context,
no need for explicit anymore

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:468:
content::BrowserMainLoop::GetAudioManager()));
nit: cache this in a local variable on the stack instead of multiple call sites.

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/spe...
File content/browser/speech/speech_input_manager.h (right):

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/spe...
content/browser/speech/speech_input_manager.h:56: // These are wrappers around
AudioManager.
much nicer!

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/spe...
File content/browser/speech/speech_recognizer.cc (right):

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/spe...
content/browser/speech/speech_recognizer.cc:119: audio_manager_ ? audio_manager_
: BrowserMainLoop::GetAudioManager(),
when will audio_manager_ not be the same as GetAudioManager?
Should we perhaps initialize audio_manager_ to GetAudioManager() in the
constructor and DCHECK there if it is NULL?

(update: please read the next comment as well)

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/spe...
content/browser/speech/speech_recognizer.cc:325: audio_manager_ = audio_manager;
ah, I see...
Instead of adding a 'ForTesting' method, can we use an inherited class in the
unit test that sets the audio_manager_ in its constructor?

In order to assign to the audio_manager_ you could expose a protected
set_audio_manager() and avoid friending.

[jam, 2012-02-23 08:05:08]

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

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:24: static const
char* kMediaStreamManagerKeyName = "content_media_stream_manager";
On 2012/02/23 07:50:59, tommi wrote:
> char[] instead of char*

what's the reason for this? i see both styles in the code, and the chrome style
guide gives this as an example:
http://www.chromium.org/developers/coding-style#Static_Variables

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
File content/browser/renderer_host/media/video_capture_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
content/browser/renderer_host/media/video_capture_host_unittest.cc:73: explicit
MockVideoCaptureHost(content::ResourceContext* resource_context,
On 2012/02/23 07:50:59, tommi wrote:
> no need for explicit anymore

Done.

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:468:
content::BrowserMainLoop::GetAudioManager()));
On 2012/02/23 07:50:59, tommi wrote:
> nit: cache this in a local variable on the stack instead of multiple call
sites.

Done.

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/spe...
File content/browser/speech/speech_recognizer.cc (right):

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/spe...
content/browser/speech/speech_recognizer.cc:325: audio_manager_ = audio_manager;
On 2012/02/23 07:50:59, tommi wrote:
> ah, I see...
> Instead of adding a 'ForTesting' method, can we use an inherited class in the
> unit test that sets the audio_manager_ in its constructor?

I tend to avoid inheritance unless necessary, which is why I added a setter.
this is pretty common in the code. since the setter is only used by test code, I
prefer making that explicit with the postfix.

> 
> In order to assign to the audio_manager_ you could expose a protected
> set_audio_manager() and avoid friending.

[tommi (sloooow) - chröme, 2012-02-23 08:20:29]

lgtm.  thanks for doing this.

Regarding the "ForTest" method, that's fine with me.  I've seen both approaches
in Chrome and have a slight personal preference for not adding test types into
production code.  Not big enough to make a fuzz though.

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

https://chromiumcodereview.appspot.com/9433006/diff/15001/content/browser/ren...
content/browser/renderer_host/media/media_stream_manager.cc:24: static const
char* kMediaStreamManagerKeyName = "content_media_stream_manager";
On 2012/02/23 08:05:09, John Abd-El-Malek wrote:
> On 2012/02/23 07:50:59, tommi wrote:
> > char[] instead of char*
> 
> what's the reason for this? i see both styles in the code, and the chrome
style
> guide gives this as an example:
> http://www.chromium.org/developers/coding-style#Static_Variables

The practical difference is that in this example:

const char* kFoo = "12345";

The compiler makes two allocations: 4 bytes for the kFoo pointer and 6 for the
"12345" string (including \0).  sizeof(kFoo) is 4.

However, for in this case:

const char kFoo[] = "12345";

There's no pointer that the compiler has to allocate, only the string.
Furthermore sizeof(kFoo) is 6, so we can write code like this:

for (int i = 0; i < arraysize(kFoo); ++i)
  OnChar(kFoo[i]);