Let Flash take one-shot focus in default MediaSession

Let Flash take one-shot focus in default MediaSession

Previously, the interaction between Flash and MediaSession is:

* When default MediaSession is enabled, Flash does not interact with
  MediaSession at all.
* When default MediaSession is enabled with Flash, Flash will join
  MediaSession. It takes audio focus and ducks when losing audio focus.

After this CL, the first behavior will be changed to:

* When default MediaSession is enabled, Flash takes audio focus once,
  but will not respond to audio focus changes.

BUG=619084
Committed: https://crrev.com/f9730d243f7a1f85ae6008cc4d6ceff367a16e66
Cr-Commit-Position: refs/heads/master@{#426770}

[Zhiqiang Zhang (Slow), 2016-10-20 18:42:39]

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

[commit-bot: I haz the power, 2016-10-20 18:43:26]

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

[Zhiqiang Zhang (Slow), 2016-10-20 18:44:50]

zqzhang@chromium.org changed reviewers:
+ dalecurtis@chromium.org, mlamouri@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-20 18:44:51]

PTAL. This CL is for the upcoming UX experiment.

[DaleCurtis, 2016-10-20 18:46:52]

Seems like this should be a base::Feature instead of switch, especially if
you're considering further experiments. 

Do I understand correctly that this would mean flash will never lose audio focus
once started?

[Zhiqiang Zhang (Slow), 2016-10-20 18:51:49]

On 2016/10/20 18:46:52, DaleCurtis wrote:
> Seems like this should be a base::Feature instead of switch, especially if
> you're considering further experiments. 

It was once using base::Feature, but we merged it as an argument of
kEnableMediaSession to allow us to have only one entry in chrome://flags, i.e.:
Disabled,
Enabled,
Enabled (with Flash)

We also tried using Feature variation but failed since it cannot be used from
content/.

> Do I understand correctly that this would mean flash will never lose audio
focus
> once started?

Yes. We have an implementation of ducking flash when losing audio focus but the
UX is crappy. So we are likely to go with the "one-shot" solution. Just like
WebRTC.
The UX will make decisions after they did experiments.

[DaleCurtis, 2016-10-20 18:56:23]

On 2016/10/20 at 18:51:49, zqzhang wrote:
> 
> We also tried using Feature variation but failed since it cannot be used from
content/.

Can you elaborate? If you define the base::Feature in media_switches.h why
wouldn't you be able to use it from content? Or even in content_switches?

> 
> > Do I understand correctly that this would mean flash will never lose audio
focus
> > once started?
> 
> Yes. We have an implementation of ducking flash when losing audio focus but
the UX is crappy. So we are likely to go with the "one-shot" solution. Just like
WebRTC.
> The UX will make decisions after they did experiments.

I think WebRTC is still ducked? At least it implements
OnVolumeMultiplierChanged.

[Zhiqiang Zhang (Slow), 2016-10-20 19:46:30]

On 2016/10/20 18:56:23, DaleCurtis wrote:
> On 2016/10/20 at 18:51:49, zqzhang wrote:
> > 
> > We also tried using Feature variation but failed since it cannot be used
from
> content/.
> 
> Can you elaborate? If you define the base::Feature in media_switches.h why
> wouldn't you be able to use it from content? Or even in content_switches?
> 

I mean we want to make "Flash joining session" as option of
kEnableDefaultMediaSession in chrome://flag, the reason is "Flash joining
session" depends on kEnableDefaultMediaSession.
However there's no easy way to do that if they are two separate flags/features.
The context is in a previous CL: https://codereview.chromium.org/2428873002/
After that CL, we have:
--enable-default-media-session => Enable MediaSession, and Flash doesn't join
MediaSession
--enable-default-media-session=with-flash => Enable MediaSession and Flash joins
MediaSession
(no arg) => Disable MediaSession

In that CL, asvitkine (from the UMA team) suggested me to make
kEnableDefaultMediaSession as a feature, and make "Flash joining session" as a
variation of the feature using the variation API (using
FEATURE_WITH_VARIATIONS_VALUE_TYPE, and go/variations-params API), but we
finally discovered it's not usable from content. So we end up with the solution
we currently have.

> > 
> > > Do I understand correctly that this would mean flash will never lose audio
> focus
> > > once started?
> > 
> > Yes. We have an implementation of ducking flash when losing audio focus but
> the UX is crappy. So we are likely to go with the "one-shot" solution. Just
like
> WebRTC.
> > The UX will make decisions after they did experiments.
> 
> I think WebRTC is still ducked? At least it implements
> OnVolumeMultiplierChanged.

Oh, yes. Maybe we should to disable the ducking. The problem is that if the
current active session is suspended, it does not give out audio focus, so users
may end up with having a ducked WebRTC and not knowing there's another suspended
session on top of it which causes WebRTC to duck.
Mounir discussed with the UX and decided to have "one-shot" focus for WebRTC and
Flash so the UX can experiment with.
BTW, I have another CL for WebRTC for you to review, maybe we should fix ducking
in that CL:
https://codereview.chromium.org/2428353005/

[DaleCurtis, 2016-10-20 21:06:03]

lgtm given my other concerns seem to have already been discussed.

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

https://codereview.chromium.org/2437623005/diff/1/content/browser/media/sessi...
content/browser/media/session/pepper_playback_observer.cc:20: bool IsDuckFlash()
{
ShouldDuckFlash?

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

https://codereview.chromium.org/2437623005/diff/1/content/browser/media/sessi...
content/browser/media/session/pepper_player_delegate.cc:19: bool IsDuckFlash() {
Ditto.

[Zhiqiang Zhang (Slow), 2016-10-21 10:04:21]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-21 10:04:21]

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/2437623005/#ps20001
(title: "IsDuckFlash -> ShouldDuckFlash")

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

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

[mlamouri (slow - plz ping), 2016-10-21 10:21:27]

lgtm

[commit-bot: I haz the power, 2016-10-21 11:38:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:28:50]

Description was changed from

==========
Let Flash take one-shot focus in default MediaSession

Previously, the interaction between Flash and MediaSession is:

* When default MediaSession is enabled, Flash does not interact with
  MediaSession at all.
* When default MediaSession is enabled with Flash, Flash will join
  MediaSession. It takes audio focus and ducks when losing audio focus.

After this CL, the first behavior will be changed to:

* When default MediaSession is enabled, Flash takes audio focus once,
  but will not respond to audio focus changes.

BUG=619084
==========

to

==========
Let Flash take one-shot focus in default MediaSession

Previously, the interaction between Flash and MediaSession is:

* When default MediaSession is enabled, Flash does not interact with
  MediaSession at all.
* When default MediaSession is enabled with Flash, Flash will join
  MediaSession. It takes audio focus and ducks when losing audio focus.

After this CL, the first behavior will be changed to:

* When default MediaSession is enabled, Flash takes audio focus once,
  but will not respond to audio focus changes.

BUG=619084

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

[commit-bot: I haz the power, 2016-10-21 13:28:51]

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