Set the AudioManager factory before the browser main loop.

Set the AudioManager factory before the browser main loop.

Adds a function to set the AudioManagerFactory for the browser, and a
trivial implementation of this which does not set a factory, opting
instead to use the statically-linked factory method in Chrome.

Change-Id: I1a2df165fbd5c166a13b440876d031c937558cbb

BUG=None

Committed: https://crrev.com/830b0c5dda907f2560ab003e588172c43768293a
Cr-Commit-Position: refs/heads/master@{#327751}

[slan, 2015-04-30 15:47:19]

slan@chromium.org changed reviewers:
+ derekjchow@chromium.org, gunsch@chromium.org

[slan, 2015-04-30 15:47:19]

Chrome-branded builds will need to link in an implementation of
::chromecast::media::SetAudioManagerFactory() before they will build
successfully. This change will be pushed synchronously with any changes to
Chrome-branded builds to keep things working.

[gunsch, 2015-04-30 15:50:38]

https://codereview.chromium.org/1113053002/diff/1/chromecast/browser/cast_bro...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1113053002/diff/1/chromecast/browser/cast_bro...
chromecast/browser/cast_browser_main_parts.cc:148: void
SetPlatformAudioManagerFactory();
This interface probably lives better somewhere like CastContentBrowserClient,
such as:

::media::AudioManagerFactory*
CastContentBrowserClient::GetPlatformAudioManagerFactory()

[slan, 2015-04-30 17:20:57]

On 2015/04/30 15:50:38, gunsch wrote:
>
https://codereview.chromium.org/1113053002/diff/1/chromecast/browser/cast_bro...
> File chromecast/browser/cast_browser_main_parts.cc (right):
> 
>
https://codereview.chromium.org/1113053002/diff/1/chromecast/browser/cast_bro...
> chromecast/browser/cast_browser_main_parts.cc:148: void
> SetPlatformAudioManagerFactory();
> This interface probably lives better somewhere like CastContentBrowserClient,
> such as:
> 
> ::media::AudioManagerFactory*
> CastContentBrowserClient::GetPlatformAudioManagerFactory()

Done. There are two ways to do this: 1) Pass in the client to main_parts and
make the call in that class, or 2) Make the call before constructing main_parts
and pass in the instance.

I chose 2), but if 1) is superior, please let me know.

[gunsch, 2015-04-30 17:29:36]

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.h (right):

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.h:23: // AudioManagerFactory is a
platform factory used to create AudioManagers.
second line unnecessary, that can be gleaned from reading the
AudioManagerFactory interface

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_content_browser_client.h (right):

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_content_browser_client.h:118:
scoped_ptr<::media::AudioManagerFactory> PlatformGetAudioManagerFactory();
naming: since this is explicitly creating an instance and immediately
transferring ownership, prefer the "Create" verb over "Get". "Get" generally
implies the class retains ownership and only returns a reference.

[gunsch, 2015-04-30 17:29:46]

lgtm, the other comments are just nits.

[derekjchow1, 2015-04-30 17:33:15]

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.h (right):

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.h:13: #include
"media/audio/audio_manager_factory.h"
Don't include. Forward declare ::media::AudioManagerFactory.

[slan, 2015-04-30 17:52:04]

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.h (right):

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.h:13: #include
"media/audio/audio_manager_factory.h"
On 2015/04/30 17:33:15, derekjchow1 wrote:
> Don't include. Forward declare ::media::AudioManagerFactory.

Done.

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.h:23: // AudioManagerFactory is a
platform factory used to create AudioManagers.
On 2015/04/30 17:29:36, gunsch wrote:
> second line unnecessary, that can be gleaned from reading the
> AudioManagerFactory interface

Done.

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_content_browser_client.h (right):

https://codereview.chromium.org/1113053002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_content_browser_client.h:118:
scoped_ptr<::media::AudioManagerFactory> PlatformGetAudioManagerFactory();
On 2015/04/30 17:29:36, gunsch wrote:
> naming: since this is explicitly creating an instance and immediately
> transferring ownership, prefer the "Create" verb over "Get". "Get" generally
> implies the class retains ownership and only returns a reference.

Done.

[slan, 2015-04-30 17:53:35]

The CQ bit was checked by slan@chromium.org

[slan, 2015-04-30 17:53:35]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1113053002/#ps40001
(title: "Cleanup + naming")

[commit-bot: I haz the power, 2015-04-30 17:54:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1113053002/40001

[commit-bot: I haz the power, 2015-04-30 18:05:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-30 18:05:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[slan, 2015-04-30 18:23:14]

slan@chromium.org changed reviewers:
+ tommi@chromium.org

[slan, 2015-04-30 18:23:14]

Hey tommi@,

We depend on media/audio to set the AudioManager for the browser process. Please
take a look. Thank you.

[slan, 2015-04-30 18:29:13]

slan@chromium.org changed reviewers:
+ dalecurtis@chromium.org
- tommi@chromium.org

[slan, 2015-04-30 18:29:14]

tommi@,
dalecurtis@ originally reviewed the underlying code for this patch, so I will
have him sign off on it. I have pulled you off the reviewers list. Sorry about
the false alarm.


dalecurtis@,
Please take a look when you get a chance. Context CL is here:
https://codereview.chromium.org/1104583002. Thank you!

[DaleCurtis, 2015-04-30 18:37:20]

lgtm

[slan, 2015-04-30 18:45:14]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2015-04-30 18:46:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1113053002/40001

[commit-bot: I haz the power, 2015-04-30 18:51:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-30 18:52:16]

Patchset 3 (id:??) landed as
https://crrev.com/830b0c5dda907f2560ab003e588172c43768293a
Cr-Commit-Position: refs/heads/master@{#327751}