Allow content embedders to create AudioManager.

Allow content embedders to create AudioManager.

This allows the content embedders to choose the implementation
and task runner for AudioManager instance. Moving the factory
function to ContentBrowserClient eliminates the need for
AudioManagerFactory, which this patch removes as well.

BUG=594234
Committed: https://crrev.com/f2f046b64c75438d36ac0b60a8bc6cc382f068a2
Cr-Commit-Position: refs/heads/master@{#389633}

[alokp, 2016-04-22 23:37:40]

alokp@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2016-04-22 23:45:06]

lg % removing hang monitor enable.

https://codereview.chromium.org/1908423006/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1908423006/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1520: if (use_hang_monitor)
This is disabled right now since we have enough crash dumps and it's in
microsoft's court now.

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:713: virtual
media::AudioManager* CreateAudioManager(
If caller owns, should return a scoped_ptr?

[alokp, 2016-04-22 23:54:21]

https://codereview.chromium.org/1908423006/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1908423006/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1520: if (use_hang_monitor)
On 2016/04/22 23:45:06, DaleCurtis wrote:
> This is disabled right now since we have enough crash dumps and it's in
> microsoft's court now.

On windows only or linux also?
Are you referring to EnableCrashKeyLoggingForAudioThreadHangs?
AFAICT this patch does not enable or disable hangl monitor, just moves it out of
AudioManager::Create.

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:713: virtual
media::AudioManager* CreateAudioManager(
On 2016/04/22 23:45:06, DaleCurtis wrote:
> If caller owns, should return a scoped_ptr?

Since we are using ScopedAudioManagerPtr, it would require including
media/audio/audio_manager.h unnecessarily in content/public. This seems to be a
common pattern in this file to avoid including stuff.

[DaleCurtis, 2016-04-22 23:56:29]

Ah right, lgtm then

[alokp, 2016-04-22 23:57:34]

alokp@chromium.org changed reviewers:
+ jam@chromium.org

[alokp, 2016-04-22 23:57:34]

jam: content/

[jam, 2016-04-25 15:40:47]

lgtm

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:711: // If this function returns
NULL, a default platform implementation will be
nit: nullptr

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:713: virtual
media::AudioManager* CreateAudioManager(
On 2016/04/22 23:54:21, alokp wrote:
> On 2016/04/22 23:45:06, DaleCurtis wrote:
> > If caller owns, should return a scoped_ptr?
> 
> Since we are using ScopedAudioManagerPtr, it would require including
> media/audio/audio_manager.h unnecessarily in content/public. This seems to be
a
> common pattern in this file to avoid including stuff.

it's definitely fine to include headers from "lower-level" modules like
net/media/cc etc in content/public. So feel free to include that to make
ownership clearer.

[alokp, 2016-04-25 16:59:53]

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:711: // If this function returns
NULL, a default platform implementation will be
On 2016/04/25 15:40:47, jam wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1908423006/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:713: virtual
media::AudioManager* CreateAudioManager(
On 2016/04/25 15:40:47, jam wrote:
> On 2016/04/22 23:54:21, alokp wrote:
> > On 2016/04/22 23:45:06, DaleCurtis wrote:
> > > If caller owns, should return a scoped_ptr?
> > 
> > Since we are using ScopedAudioManagerPtr, it would require including
> > media/audio/audio_manager.h unnecessarily in content/public. This seems to
be
> a
> > common pattern in this file to avoid including stuff.
> 
> it's definitely fine to include headers from "lower-level" modules like
> net/media/cc etc in content/public. So feel free to include that to make
> ownership clearer.

DONE. I had to update DEPS - PTAL.

[alokp, 2016-04-25 17:04:31]

alokp@chromium.org changed reviewers:
+ halliwell@chromium.org

[jam, 2016-04-25 18:21:25]

lgtm

[alokp, 2016-04-25 19:59:27]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-04-25 19:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1908423006/#ps20001
(title: "ScopedAudioManagerPtr instead of raw pointer")

[commit-bot: I haz the power, 2016-04-25 20:00:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1908423006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1908423006/20001

[commit-bot: I haz the power, 2016-04-26 00:26:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-26 00:29:10]

Description was changed from

==========
Allow content embedders to create AudioManager.

This allows the content embedders to choose the implementation
and task runner for AudioManager instance. Moving the factory
function to ContentBrowserClient eliminates the need for
AudioManagerFactory, which this patch removes as well.

BUG=594234
==========

to

==========
Allow content embedders to create AudioManager.

This allows the content embedders to choose the implementation
and task runner for AudioManager instance. Moving the factory
function to ContentBrowserClient eliminates the need for
AudioManagerFactory, which this patch removes as well.

BUG=594234

Committed: https://crrev.com/f2f046b64c75438d36ac0b60a8bc6cc382f068a2
Cr-Commit-Position: refs/heads/master@{#389633}
==========

[commit-bot: I haz the power, 2016-04-26 00:29:11]

Patchset 2 (id:??) landed as
https://crrev.com/f2f046b64c75438d36ac0b60a8bc6cc382f068a2
Cr-Commit-Position: refs/heads/master@{#389633}