Implement Audio Focus Manager for desktop.

Implement Audio Focus Manager for desktop.

This CL implements a real Audio Focus Manager internal to Chrome.
The intent is to use it on Chrome desktop, when there is no system
audio focus manager.

The Audio Focus Manager handles ducking and audio focus gain. At
the moment, the rules are that only one entry can have audio focus
and any transient will duck the audio focus entry.

BUG=609503
Committed: https://crrev.com/ce42802dbc1e900b9d0689508b2f6284ee617a55
Cr-Commit-Position: refs/heads/master@{#406843}

[Zhiqiang Zhang (Slow), 2016-05-10 18:48:11]

Description was changed from

==========
Implement Audio Focus Manager for desktop.

Still WIP. Be gentle :)

BUG=<TODO>
==========

to

==========
Implement Audio Focus Manager for desktop.

Still WIP. Be gentle :)

BUG=<TODO>
==========

[Zhiqiang Zhang (Slow), 2016-05-11 15:57:31]

zqzhang@chromium.org changed reviewers:
+ zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-05-11 15:57:32]

https://codereview.chromium.org/1971443002/diff/1/content/browser/media/sessi...
File content/browser/media/session/audio_focus_manager.cc (right):

https://codereview.chromium.org/1971443002/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_manager.cc:22:
audio_focus_manager_(audio_focus_manager) {
You forgot type_(type) here :)

https://codereview.chromium.org/1971443002/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_manager.cc:119:
MediaSession::Get(*focus_stack_.begin())->Resume(MediaSession::SuspendType::SYSTEM);
On Android, we don't resume other sessions when abandoning audio focus of type
GAIN. Actually, I think the stack should be cleared for requesting focus of GAIN
type.

[Zhiqiang Zhang (Slow), 2016-05-20 14:14:44]

https://codereview.chromium.org/1971443002/diff/1/content/browser/media/sessi...
File content/browser/media/session/audio_focus_manager.cc (right):

https://codereview.chromium.org/1971443002/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_manager.cc:119:
MediaSession::Get(*focus_stack_.begin())->Resume(MediaSession::SuspendType::SYSTEM);
On 2016/05/11 15:57:32, Zhiqiang Zhang wrote:
> On Android, we don't resume other sessions when abandoning audio focus of type
> GAIN. Actually, I think the stack should be cleared for requesting focus of
GAIN
> type.

Sorry, no need to clear the stack here.
Our current behavior is that MediaSession abandons the audio focus when losing
focus.

[mlamouri (slow - plz ping), 2016-07-07 14:51:22]

Description was changed from

==========
Implement Audio Focus Manager for desktop.

Still WIP. Be gentle :)

BUG=<TODO>
==========

to

==========
Implement Audio Focus Manager for desktop.

This CL implements a real Audio Focus Manager internal to Chrome.
The intent is to use it on Chrome desktop, when there is no system
audio focus manager.

The Audio Focus Manager handles ducking and audio focus gain. At
the moment, the rules are that only one entry can have audio focus
and any transient will duck the audio focus entry.

BUG=609503
==========

[mlamouri (slow - plz ping), 2016-07-07 14:51:52]

Zhiqiang, this is ready to be reaviewed. PTAL :)

[Zhiqiang Zhang (Slow), 2016-07-07 16:04:25]

Some initial comments.
I haven't looked much into the tests yet because I need to confirm the logic.

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager.cc (right):

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:26:
AudioFocusManager::AudioFocusType AudioFocusManager::AudioFocusEntry::type()
nit: Maybe add line break here instead of between `type()` and `const`?

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:33: // |this| will be
destroyed now.
Wow, this is dangerous. Just make sure no one will interact with |this| after
this point. (I don't have any in mind yet)

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:46:
MaybeRemoveFocusEntry(web_contents);
Is it possible that a MediaSession requesting transient audio focus when it is
already in the manager? Maybe you should use DCHECK?

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:83: MaybeStartDucking();
So the logic here is that transient sessions won't duck/pause each other, and if
any transient session is active, we duck the content sessions, even if it's
newly added?

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager.h (right):

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.h:21: enum class
AudioFocusType {
Maybe we can merge this with MediaSession::Type? I can do it in my CL
(https://codereview.chromium.org/1996043002/), depending on which lands first.

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.h:40: class AudioFocusEntry :
public WebContentsObserver {
As I use WebContentsObserver in the Pepper CL, one of the reviewers suggest me
to avoid inheriting from WebContentsObserver if possible, since it will add
overhead to all WebContents events.

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.h:88:
std::unique_ptr<AudioFocusEntry> focus_entry_;
Maybe we should keep all MediaSession entries? Pepper instances in a suspended
MediaSession needs a signal to unduck.

[mlamouri (slow - plz ping), 2016-07-08 13:20:47]

Thanks for the comments. Answered the questions below.

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager.cc (right):

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:26:
AudioFocusManager::AudioFocusType AudioFocusManager::AudioFocusEntry::type()
On 2016/07/07 at 16:04:25, Zhiqiang Zhang wrote:
> nit: Maybe add line break here instead of between `type()` and `const`?

I agree that it's not nice. Though, I used `git cl format`. Changed.

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:33: // |this| will be
destroyed now.
On 2016/07/07 at 16:04:25, Zhiqiang Zhang wrote:
> Wow, this is dangerous. Just make sure no one will interact with |this| after
this point. (I don't have any in mind yet)

No one can. It's a fairly common pattern :)

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:46:
MaybeRemoveFocusEntry(web_contents);
On 2016/07/07 at 16:04:25, Zhiqiang Zhang wrote:
> Is it possible that a MediaSession requesting transient audio focus when it is
already in the manager? Maybe you should use DCHECK?

I want this to be fairly flexible and to not assume much behaviour from
MediaSession so we can change the MediaSession code without having to touch
this.

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:83: MaybeStartDucking();
On 2016/07/07 at 16:04:25, Zhiqiang Zhang wrote:
> So the logic here is that transient sessions won't duck/pause each other, and
if any transient session is active, we duck the content sessions, even if it's
newly added?

Correct. See tests :)

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager.h (right):

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.h:21: enum class
AudioFocusType {
On 2016/07/07 at 16:04:25, Zhiqiang Zhang wrote:
> Maybe we can merge this with MediaSession::Type? I can do it in my CL
(https://codereview.chromium.org/1996043002/), depending on which lands first.

I don't think it would make sense for now. In general, I would prefer to have
AudioFocusManager to be fairly independent from MediaSession. Maybe MediaSession
will not try to get AudioFocus in some cases for example.

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.h:40: class AudioFocusEntry :
public WebContentsObserver {
On 2016/07/07 at 16:04:25, Zhiqiang Zhang wrote:
> As I use WebContentsObserver in the Pepper CL, one of the reviewers suggest me
to avoid inheriting from WebContentsObserver if possible, since it will add
overhead to all WebContents events.

This is on-demand so it's probably not that bad. Though, I'm open to alternative
ideas. Were you thinking of something in particular?

https://codereview.chromium.org/1971443002/diff/60001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.h:88:
std::unique_ptr<AudioFocusEntry> focus_entry_;
On 2016/07/07 at 16:04:25, Zhiqiang Zhang wrote:
> Maybe we should keep all MediaSession entries? Pepper instances in a suspended
MediaSession needs a signal to unduck.

Let's deal with Flash in a follow-up :) We need to find out how to implement the
different possible solutions too :)

[Zhiqiang Zhang (Slow), 2016-07-08 15:26:26]

lgtm /w nits.

https://codereview.chromium.org/1971443002/diff/80001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager.cc (right):

https://codereview.chromium.org/1971443002/diff/80001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:86: void
AudioFocusManager::OnWebContentsDestroyed(WebContents* web_contents) {
Nit: maybe reuse AbandonAudioFocus()?

https://codereview.chromium.org/1971443002/diff/80001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager_unittest.cc (right):

https://codereview.chromium.org/1971443002/diff/80001/content/browser/media/s...
content/browser/media/session/audio_focus_manager_unittest.cc:11: #include
"testing/gmock/include/gmock/gmock.h"
"testing/gtest/include/gtest/gtest.h" should be enough? Mocking is not used in
these tests.

[mlamouri (slow - plz ping), 2016-07-08 17:41:35]

https://codereview.chromium.org/1971443002/diff/80001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager.cc (right):

https://codereview.chromium.org/1971443002/diff/80001/content/browser/media/s...
content/browser/media/session/audio_focus_manager.cc:86: void
AudioFocusManager::OnWebContentsDestroyed(WebContents* web_contents) {
On 2016/07/08 at 15:26:26, Zhiqiang Zhang wrote:
> Nit: maybe reuse AbandonAudioFocus()?

Done. I've added AbandonAudioFocusInternal() that takes a WebContents so we
don't do WebContents->MediaSession->WebContonts.

https://codereview.chromium.org/1971443002/diff/80001/content/browser/media/s...
File content/browser/media/session/audio_focus_manager_unittest.cc (right):

https://codereview.chromium.org/1971443002/diff/80001/content/browser/media/s...
content/browser/media/session/audio_focus_manager_unittest.cc:11: #include
"testing/gmock/include/gmock/gmock.h"
On 2016/07/08 at 15:26:26, Zhiqiang Zhang wrote:
> "testing/gtest/include/gtest/gtest.h" should be enough? Mocking is not used in
these tests.

Done.

[mlamouri (slow - plz ping), 2016-07-08 17:41:40]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-07-08 17:41:41]

The patchset sent to the CQ was uploaded after l-g-t-m from zqzhang@chromium.org
Link to the patchset: https://codereview.chromium.org/1971443002/#ps100001
(title: "review comments")

[commit-bot: I haz the power, 2016-07-08 17:42:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 18:40:11]

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

[commit-bot: I haz the power, 2016-07-08 18:40:12]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2016-07-19 17:14:12]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 17:14:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 18:41:36]

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

[commit-bot: I haz the power, 2016-07-19 18:41:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2016-07-21 13:41:36]

The unit tests were leaking because the RenderProcessHost was doing things that
were not expected from the other mocked classes (TestWebContents and
TestBrowserContext). Using MockRenderProcessHostFactory solved the issue.

[mlamouri (slow - plz ping), 2016-07-21 13:41:39]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-07-21 13:41:41]

The patchset sent to the CQ was uploaded after l-g-t-m from zqzhang@chromium.org
Link to the patchset: https://codereview.chromium.org/1971443002/#ps140001
(title: "fix leak in unit tests")

[commit-bot: I haz the power, 2016-07-21 13:41:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 14:22:10]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-21 14:24:03]

Description was changed from

==========
Implement Audio Focus Manager for desktop.

This CL implements a real Audio Focus Manager internal to Chrome.
The intent is to use it on Chrome desktop, when there is no system
audio focus manager.

The Audio Focus Manager handles ducking and audio focus gain. At
the moment, the rules are that only one entry can have audio focus
and any transient will duck the audio focus entry.

BUG=609503
==========

to

==========
Implement Audio Focus Manager for desktop.

This CL implements a real Audio Focus Manager internal to Chrome.
The intent is to use it on Chrome desktop, when there is no system
audio focus manager.

The Audio Focus Manager handles ducking and audio focus gain. At
the moment, the rules are that only one entry can have audio focus
and any transient will duck the audio focus entry.

BUG=609503

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

[commit-bot: I haz the power, 2016-07-21 14:24:04]

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