Add about_flag for experimenting MediaSession on Desktop

Add a flag to chrome://flags for experimenting with MediaSession on desktop

This CL adds a flag for experimenting with MediaSession on desktop to chrome://flags. kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make chrome://flags simpler.

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

[Zhiqiang Zhang (Slow), 2016-10-18 12:22:47]

Description was changed from

==========
Add about_flags for experimenting MediaSession on Desktop

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

to

==========
Add about_flag for experimenting MediaSession on Desktop

This CL adds an about_flag for experimenting MediaSession on
Desktop, kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make the about_flag simpler.

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

[Zhiqiang Zhang (Slow), 2016-10-18 12:25:43]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org, isherman@chromium.org, mlamouri@chromium.org,
xhwang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-18 12:25:43]

+avayvod/mlamouri for general review
+xhwang for RS media/
+isherman for histograms.xml

[Alexei Svitkine (slow), 2016-10-18 14:00:05]

What's the reason for switching to a flag from a base::Feature?

A base::Feature can already support having an entry on about:flags as well
as be turned on via the command line --enable-feature=name syntax.

On Oct 18, 2016 8:25 AM, <zqzhang@chromium.org> wrote:

> Reviewers: whywhat, mlamouri, xhwang, Ilya Sherman
> CL: https://codereview.chromium.org/2428873002/
>
> Message:
> +avayvod/mlamouri for general review
> +xhwang for RS media/
> +isherman for histograms.xml
>
> Description:
> Add about_flag for experimenting MediaSession on Desktop
>
> This CL adds an about_flag for experimenting MediaSession on
> Desktop, kFlashJoinsMediaSession is merged into
> kEnableDefaultMediaSession to make the about_flag simpler.
>
> BUG=619084
>
> Affected files (+52, -4 lines):
> M chrome/app/generated_resources.grd
> M chrome/browser/about_flags.cc
> M content/browser/media/session/pepper_playback_observer.cc
> M media/base/media_switches.h
> M media/base/media_switches.cc
> M tools/metrics/histograms/histograms.xml
>
>
> Index: chrome/app/generated_resources.grd
> diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_
> resources.grd
> index ce3ce3c822f9dc7ea9eabf0cc35690c5a2d3a9e4..
> b2d8f0b3bbbad3a5990f46373c605fd7e7a368b1 100644
> --- a/chrome/app/generated_resources.grd
> +++ b/chrome/app/generated_resources.grd
> @@ -15507,6 +15507,25 @@ read aloud to screenreader users to announce that
> a completion is available.">
> <message name="IDS_FLAGS_EXPENSIVE_BACKGROUND_TIMER_THROTTLING_DESCRIPTION"
> desc="Description for the flag to enable expensive background timer
> throttling">
> Enables intervention to limit CPU usage of background timers to 1%.
> </message>
> +
> + <!-- Default MediaSession flag -->
> + <if expr="not is_android">
> + <message name="IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_NAME" desc="Name
> for the flag to enable default MediaSession on desktop">
> + Enable <ph name="FEATURE_NAME">default MediaSession</ph>
> + </message>
> + <message name="IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_DESCRIPTION"
> desc="Desciption for the flag to enable default MediaSession on desktop">
> + Enables the default MediaSession for managing audio focus across tabs.
> + </message>
> + <message name="IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_DISABLED"
> desc="Option for disabling the default MediaSession">
> + Disabled
> + </message>
> + <message name="IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_ENABLED"
> desc="Option for enabling the default MediaSession">
> + Enabled
> + </message>
> + <message name="IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_ENABLED_WITH_FLASH"
> desc="Option for enabling the default MediaSession with Flash">
> + Enabled (with Flash)
> + </message>
> + </if>
> </messages>
> </release>
> </grit>
> Index: chrome/browser/about_flags.cc
> diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
> index b2ee923f355a8a37c065e1621ecd82fe6492aa9f..
> 514097b2c721b487527eb49cce4f1adeb72b9215 100644
> --- a/chrome/browser/about_flags.cc
> +++ b/chrome/browser/about_flags.cc
> @@ -600,6 +600,17 @@ const FeatureEntry::Choice
> kSecurityChipAnimationChoices[] = {
> switches::kSecurityChipAnimationAll},
> };
>
> +const FeatureEntry::Choice kEnableDefaultMediaSessionChoices[] = {
> + {IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_DISABLED, "", ""},
> + {IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_ENABLED,
> + switches::kEnableDefaultMediaSession, ""},
> +#if defined(ENABLE_PLUGINS)
> + {IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_ENABLED_WITH_FLASH,
> + switches::kEnableDefaultMediaSession,
> + switches::kEnableDefaultMediaSessionWithFlash},
> +#endif // defined(ENABLE_PLUGINS)
> +};
> +
> // RECORDING USER METRICS FOR FLAGS:
> // ------------------------------------------------------------
> -----------------
> // The first line of the entry is the internal name.
> @@ -2029,12 +2040,16 @@ const FeatureEntry kFeatureEntries[] = {
> IDS_FLAGS_EXPENSIVE_BACKGROUND_TIMER_THROTTLING_NAME,
> IDS_FLAGS_EXPENSIVE_BACKGROUND_TIMER_THROTTLING_DESCRIPTION, kOsAll,
> FEATURE_VALUE_TYPE(features::kExpensiveBackgroundTimerThrottling)},
> - {"security-chip", IDS_FLAGS_SECURITY_CHIP_NAME,
> + {"security-chip", IDS_FLAGS_SECURITY_CHIP_NAME,
> IDS_FLAGS_SECURITY_CHIP_DESCRIPTION, kOsDesktop,
> MULTI_VALUE_TYPE(kSecurityChipChoices)},
> {"security-chip-animation", IDS_FLAGS_SECURITY_CHIP_ANIMATION_NAME,
> IDS_FLAGS_SECURITY_CHIP_ANIMATION_DESCRIPTION, kOsDesktop,
> MULTI_VALUE_TYPE(kSecurityChipAnimationChoices)},
> + {"enable-default-media-session",
> + IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_NAME,
> + IDS_FLAGS_ENABLE_DEFAULT_MEDIA_SESSION_DESCRIPTION, kOsDesktop,
> + MULTI_VALUE_TYPE(kEnableDefaultMediaSessionChoices)},
> // NOTE: Adding new command-line switches requires adding corresponding
> // entries to enum "LoginCustomFlags" in histograms.xml. See note in
> // histograms.xml and don't forget to run AboutFlagsHistogramTest unit
> test.
> Index: content/browser/media/session/pepper_playback_observer.cc
> diff --git a/content/browser/media/session/pepper_playback_observer.cc
> b/content/browser/media/session/pepper_playback_observer.cc
> index 8132f3ea3803e4c22fb3816756ce0f541dcc5d31..
> c0cc7e75a9be8548f67f9a080ed0098a97658494 100644
> --- a/content/browser/media/session/pepper_playback_observer.cc
> +++ b/content/browser/media/session/pepper_playback_observer.cc
> @@ -46,8 +46,11 @@ void PepperPlaybackObserver::PepperInstanceDeleted(int32_t
> pp_instance) {
> void PepperPlaybackObserver::PepperStartsPlayback(int32_t pp_instance) {
> players_played_sound_map_[pp_instance] = true;
>
> - if (!base::FeatureList::IsEnabled(media::kFlashJoinsMediaSession))
> + if (base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
> + switches::kEnableDefaultMediaSession) !=
> + switches::kEnableDefaultMediaSessionWithFlash) {
> return;
> + }
>
> if (players_map_.count(pp_instance))
> return;
> Index: media/base/media_switches.cc
> diff --git a/media/base/media_switches.cc b/media/base/media_switches.cc
> index f9ade75b07fe10ad0a4d0f10a925af69dc8e2892..
> e43b97ad27faf73dc440e94e56096b5c41fa443b 100644
> --- a/media/base/media_switches.cc
> +++ b/media/base/media_switches.cc
> @@ -71,7 +71,12 @@ const char kUseCras[] = "use-cras";
> // each other. This is different from the Media Session API as it is
> enabling a
> // default behaviour for the browser.
> const char kEnableDefaultMediaSession[] = "enable-default-media-session";
> -#endif
> +
> +#if defined(ENABLE_PLUGINS)
> +const char kEnableDefaultMediaSessionWithFlash[] = "with-flash";
> +#endif // defined(ENABLE_PLUGINS)
> +
> +#endif // !defined(OS_ANDROID)
>
> // Use fake device for Media Stream to replace actual camera and
> microphone.
> const char kUseFakeDeviceForMediaStream[] = "use-fake-device-for-media-
> stream";
> Index: media/base/media_switches.h
> diff --git a/media/base/media_switches.h b/media/base/media_switches.h
> index b1ed8408301b2c182525387b7e42f72b56e8b2e2..
> 0b617bb32d3d0421a139ee904a35ffbbac27b308 100644
> --- a/media/base/media_switches.h
> +++ b/media/base/media_switches.h
> @@ -45,7 +45,12 @@ MEDIA_EXPORT extern const char kUseCras[];
>
> #if !defined(OS_ANDROID)
> MEDIA_EXPORT extern const char kEnableDefaultMediaSession[];
> -#endif
> +
> +#if defined(ENABLE_PLUGINS)
> +MEDIA_EXPORT extern const char kEnableDefaultMediaSessionWithFlash[];
> +#endif // defined(ENABLE_PLUGINS)
> +
> +#endif // !defined(OS_ANDROID)
>
> MEDIA_EXPORT extern const char kUseFakeDeviceForMediaStream[];
> MEDIA_EXPORT extern const char kUseFileForFakeVideoCapture[];
> Index: tools/metrics/histograms/histograms.xml
> diff --git a/tools/metrics/histograms/histograms.xml
> b/tools/metrics/histograms/histograms.xml
> index be127aa45d4b1d8470bd64b90fcb5a2f8d6520ad..
> 6de5c513bb952827ada7d0b563c044db440a2b70 100644
> --- a/tools/metrics/histograms/histograms.xml
> +++ b/tools/metrics/histograms/histograms.xml
> @@ -89306,6 +89306,7 @@ value.
> <int value="-52483823" label="disable-new-video-renderer"/>
> <int value="-52241456" label="enable-single-click-autofill"/>
> <int value="-48920737" label="enable-smooth-scrolling"/>
> + <int value="-45532639" label="enable-default-media-session"/>
> <int value="-45074716" label="SystemDownloadManager:disabled"/>
> <int value="-23090520" label="disable-search-button-in-omnibox"/>
> <int value="-22544408" label="enable-video-player-chromecast-support"/>
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

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

On 2016/10/18 14:00:05, Alexei Svitkine (slow) wrote:
> What's the reason for switching to a flag from a base::Feature?
> 
> A base::Feature can already support having an entry on about:flags as well
> as be turned on via the command line --enable-feature=name syntax.
> 

Yes, that flag was --flash-joins-media-session, and we switched it into
base::Feature in a previous CL.
We are merging the flag as an argument value of --enable-default-media-session
to make the about flags can have three values:
"Disable", "Enable", "Enable (with Flash)". Otherwise, we need two about flags
and one of them depends on the other.

I tried FEATURE_WITH_VARIATIONS_VALUE_TYPE but it seems does fit into our case
easily. base::Feature is only binary (enabled/disabled), right?
Does it make sense to you?

[Alexei Svitkine (slow), 2016-10-18 14:17:33]

FEATURE_WITH_VARIATIONS_VALUE_TYPE might be appropriate if your plan is for
people to use this mainly from about:flags (and not so much the
command-line).

The idea is you have to top-level enable/disable switch and then use
go/variations-params API to control other variants of the feature (e.g.
whether to enable flash).

On Tue, Oct 18, 2016 at 10:11 AM, <zqzhang@chromium.org> wrote:

> On 2016/10/18 14:00:05, Alexei Svitkine (slow) wrote:
> > What's the reason for switching to a flag from a base::Feature?
> >
> > A base::Feature can already support having an entry on about:flags as
> well
> > as be turned on via the command line --enable-feature=name syntax.
> >
>
> Yes, that flag was --flash-joins-media-session, and we switched it into
> base::Feature in a previous CL.
> We are merging the flag as an argument value of
> --enable-default-media-session
> to make the about flags can have three values:
> "Disable", "Enable", "Enable (with Flash)". Otherwise, we need two about
> flags
> and one of them depends on the other.
>
> I tried FEATURE_WITH_VARIATIONS_VALUE_TYPE but it seems does fit into our
> case
> easily. base::Feature is only binary (enabled/disabled), right?
> Does it make sense to you?
>
> https://codereview.chromium.org/2428873002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Zhiqiang Zhang (Slow), 2016-10-18 14:30:48]

On 2016/10/18 14:17:33, Alexei Svitkine (slow) wrote:
> FEATURE_WITH_VARIATIONS_VALUE_TYPE might be appropriate if your plan is for
> people to use this mainly from about:flags (and not so much the
> command-line).
> 
> The idea is you have to top-level enable/disable switch and then use
> go/variations-params API to control other variants of the feature (e.g.
> whether to enable flash).
> 
> On Tue, Oct 18, 2016 at 10:11 AM, <mailto:zqzhang@chromium.org> wrote:
> 
> > On 2016/10/18 14:00:05, Alexei Svitkine (slow) wrote:
> > > What's the reason for switching to a flag from a base::Feature?
> > >
> > > A base::Feature can already support having an entry on about:flags as
> > well
> > > as be turned on via the command line --enable-feature=name syntax.
> > >
> >
> > Yes, that flag was --flash-joins-media-session, and we switched it into
> > base::Feature in a previous CL.
> > We are merging the flag as an argument value of
> > --enable-default-media-session
> > to make the about flags can have three values:
> > "Disable", "Enable", "Enable (with Flash)". Otherwise, we need two about
> > flags
> > and one of them depends on the other.
> >
> > I tried FEATURE_WITH_VARIATIONS_VALUE_TYPE but it seems does fit into our
> > case
> > easily. base::Feature is only binary (enabled/disabled), right?
> > Does it make sense to you?
> >
> > https://codereview.chromium.org/2428873002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

OK, I'll try it. Thanks!

[Zhiqiang Zhang (Slow), 2016-10-18 15:56:40]

On 2016/10/18 14:17:33, Alexei Svitkine (slow) wrote:
> FEATURE_WITH_VARIATIONS_VALUE_TYPE might be appropriate if your plan is for
> people to use this mainly from about:flags (and not so much the
> command-line).
> 
> The idea is you have to top-level enable/disable switch and then use
> go/variations-params API to control other variants of the feature (e.g.
> whether to enable flash).
> 
> On Tue, Oct 18, 2016 at 10:11 AM, <mailto:zqzhang@chromium.org> wrote:
> 
> > On 2016/10/18 14:00:05, Alexei Svitkine (slow) wrote:
> > > What's the reason for switching to a flag from a base::Feature?
> > >
> > > A base::Feature can already support having an entry on about:flags as
> > well
> > > as be turned on via the command line --enable-feature=name syntax.
> > >
> >
> > Yes, that flag was --flash-joins-media-session, and we switched it into
> > base::Feature in a previous CL.
> > We are merging the flag as an argument value of
> > --enable-default-media-session
> > to make the about flags can have three values:
> > "Disable", "Enable", "Enable (with Flash)". Otherwise, we need two about
> > flags
> > and one of them depends on the other.
> >
> > I tried FEATURE_WITH_VARIATIONS_VALUE_TYPE but it seems does fit into our
> > case
> > easily. base::Feature is only binary (enabled/disabled), right?
> > Does it make sense to you?
> >
> > https://codereview.chromium.org/2428873002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hi Alexei, I'm trying to use the variations API in content/browser. Since there
are no prior cases, I need to add variations dependency in content/browser. Is
it fine?

[whywhat, 2016-10-18 15:57:36]

Description was changed from

==========
Add about_flag for experimenting MediaSession on Desktop

This CL adds an about_flag for experimenting MediaSession on
Desktop, kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make the about_flag simpler.

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

to

==========
Add a flag to chrome://flags for experimenting with MediaSession on desktop

This CL adds a flag for experimenting with MediaSession on desktop to
chrome://flags. kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make chrome://flags simpler.

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

[Zhiqiang Zhang (Slow), 2016-10-18 16:00:07]

Description was changed from

==========
Add a flag to chrome://flags for experimenting with MediaSession on desktop

This CL adds a flag for experimenting with MediaSession on desktop to
chrome://flags. kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make chrome://flags simpler.

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

to

==========
Add a flag to chrome://flags for experimenting with MediaSession on desktop

This CL adds a flag for experimenting with MediaSession on desktop to
chrome://flags. kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make chrome://flags simpler.

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

[Alexei Svitkine (slow), 2016-10-18 16:19:37]

Ah, I didn't realise you needed it from content/.

In this case, it's probably simpler to use the flags approach as you had
originally, until we make variations params more widely accessible. (I
think there's been resistance from content/ owners in the past from adding
that dependency.)

On Tue, Oct 18, 2016 at 11:56 AM, <zqzhang@chromium.org> wrote:

> On 2016/10/18 14:17:33, Alexei Svitkine (slow) wrote:
> > FEATURE_WITH_VARIATIONS_VALUE_TYPE might be appropriate if your plan is
> for
> > people to use this mainly from about:flags (and not so much the
> > command-line).
> >
> > The idea is you have to top-level enable/disable switch and then use
> > go/variations-params <https://goto.google.com/variations-params> API to
> control other variants of the feature (e.g.
> > whether to enable flash).
> >
> > On Tue, Oct 18, 2016 at 10:11 AM, <mailto:zqzhang@chromium.org> wrote:
> >
> > > On 2016/10/18 14:00:05, Alexei Svitkine (slow) wrote:
> > > > What's the reason for switching to a flag from a base::Feature?
> > > >
> > > > A base::Feature can already support having an entry on about:flags as
> > > well
> > > > as be turned on via the command line --enable-feature=name syntax.
> > > >
> > >
> > > Yes, that flag was --flash-joins-media-session, and we switched it into
> > > base::Feature in a previous CL.
> > > We are merging the flag as an argument value of
> > > --enable-default-media-session
> > > to make the about flags can have three values:
> > > "Disable", "Enable", "Enable (with Flash)". Otherwise, we need two
> about
> > > flags
> > > and one of them depends on the other.
> > >
> > > I tried FEATURE_WITH_VARIATIONS_VALUE_TYPE but it seems does fit into
> our
> > > case
> > > easily. base::Feature is only binary (enabled/disabled), right?
> > > Does it make sense to you?
> > >
> > > https://codereview.chromium.org/2428873002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> Hi Alexei, I'm trying to use the variations API in content/browser. Since
> there
> are no prior cases, I need to add variations dependency in
> content/browser. Is
> it fine?
>
> https://codereview.chromium.org/2428873002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2016-10-18 16:31:32]

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

https://codereview.chromium.org/2428873002/diff/1/content/browser/media/sessi...
content/browser/media/session/pepper_playback_observer.cc:49: if
(!base::FeatureList::IsEnabled(media::kFlashJoinsMediaSession))
If you do end up going with the flag, rather than the feature, please also
remove the feature.

[Zhiqiang Zhang (Slow), 2016-10-18 16:46:26]

PTAL

Since feature variation currently cannot be used from content/, I'm going back
to the switches solution.

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

https://codereview.chromium.org/2428873002/diff/1/content/browser/media/sessi...
content/browser/media/session/pepper_playback_observer.cc:49: if
(!base::FeatureList::IsEnabled(media::kFlashJoinsMediaSession))
On 2016/10/18 16:31:32, Alexei Svitkine (slow) wrote:
> If you do end up going with the flag, rather than the feature, please also
> remove the feature.

Done.

[Alexei Svitkine (slow), 2016-10-18 16:51:38]

lgtm

https://codereview.chromium.org/2428873002/diff/20001/media/base/media_switch...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2428873002/diff/20001/media/base/media_switch...
media/base/media_switches.cc:76: const char
kEnableDefaultMediaSessionWithFlash[] = "with-flash";
Add a comment mentioning that this is the value for the specific flag.

[mlamouri (slow - plz ping), 2016-10-18 16:59:37]

lgtm

[whywhat, 2016-10-18 17:23:57]

lgtm modulo nits & pending agreement with Alexei on flag vs feature.

https://codereview.chromium.org/2428873002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2428873002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:15511: +    <!-- Default MediaSession flag
-->
I think from the people who will read this, including translators and users,
only you, me and Mounir will understand what default MediaSession means :)

I think using the text you wrote already instead "managing audio focus across
tabs" would be nicer.

E.g. "Name for the flag to enable managing audio focus across tabs", "Enables
managing audio focus across tabs." etc.
Maybe enable-audio-focus-manager would be a better name for the flag itself.

[xhwang, 2016-10-18 17:26:59]

Looking good. I just have a question.

What's the roll out plan here? Do you plan to run any experiments in user
groups? Or do you expect normal users to change the flags in about://flags?

https://codereview.chromium.org/2428873002/diff/20001/content/browser/media/s...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://codereview.chromium.org/2428873002/diff/20001/content/browser/media/s...
content/browser/media/session/pepper_playback_observer.cc:46: void
PepperPlaybackObserver::PepperStartsPlayback(int32_t pp_instance) {
OOC, what is defined as a "pepper playback"? pepper flash, pepper cdm, and/or
pepper audio?

https://codereview.chromium.org/2428873002/diff/20001/media/base/media_switch...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2428873002/diff/20001/media/base/media_switch...
media/base/media_switches.cc:73: const char kEnableDefaultMediaSession[] =
"enable-default-media-session";
Please add more comments about what the allowed values are.

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

On 2016/10/18 17:26:59, xhwang wrote:
> What's the roll out plan here? Do you plan to run any experiments in user
> groups? Or do you expect normal users to change the flags in about://flags?

We are adding this flag so that UX people can experiment with the feature
(command line flags is hard to use).
The feature is still experimental until we got more feedback from UX.

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

https://chromiumcodereview.appspot.com/2428873002/diff/1/chrome/app/generated...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/2428873002/diff/1/chrome/app/generated...
chrome/app/generated_resources.grd:15511: +    <!-- Default MediaSession flag
-->
On 2016/10/18 17:23:57, whywhat wrote:
> I think from the people who will read this, including translators and users,
> only you, me and Mounir will understand what default MediaSession means :)
> 
> I think using the text you wrote already instead "managing audio focus across
> tabs" would be nicer.
> 
> E.g. "Name for the flag to enable managing audio focus across tabs", "Enables
> managing audio focus across tabs." etc.
> Maybe enable-audio-focus-manager would be a better name for the flag itself.

Updated the flag name. I think it's fine for the comment since it's the internal
name.

https://chromiumcodereview.appspot.com/2428873002/diff/20001/content/browser/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://chromiumcodereview.appspot.com/2428873002/diff/20001/content/browser/...
content/browser/media/session/pepper_playback_observer.cc:46: void
PepperPlaybackObserver::PepperStartsPlayback(int32_t pp_instance) {
On 2016/10/18 17:26:59, xhwang wrote:
> OOC, what is defined as a "pepper playback"? pepper flash, pepper cdm, and/or
> pepper audio?

It's a pepper plugin instance producing audio.

https://chromiumcodereview.appspot.com/2428873002/diff/20001/media/base/media...
File media/base/media_switches.cc (right):

https://chromiumcodereview.appspot.com/2428873002/diff/20001/media/base/media...
media/base/media_switches.cc:73: const char kEnableDefaultMediaSession[] =
"enable-default-media-session";
On 2016/10/18 17:26:59, xhwang wrote:
> Please add more comments about what the allowed values are.

Done.

https://chromiumcodereview.appspot.com/2428873002/diff/20001/media/base/media...
media/base/media_switches.cc:76: const char
kEnableDefaultMediaSessionWithFlash[] = "with-flash";
On 2016/10/18 16:51:38, Alexei Svitkine (slow) wrote:
> Add a comment mentioning that this is the value for the specific flag.

Done.

[Zhiqiang Zhang (Slow), 2016-10-18 18:55:02]

Hmm, codereview seems down, and I cannot upload the new patch :(

[Zhiqiang Zhang (Slow), 2016-10-18 20:07:15]

On 2016/10/18 18:55:02, Zhiqiang Zhang wrote:
> Hmm, codereview seems down, and I cannot upload the new patch :(

Uploaded :)

[xhwang, 2016-10-18 20:25:08]

lgtm % nit

https://chromiumcodereview.appspot.com/2428873002/diff/20001/content/browser/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://chromiumcodereview.appspot.com/2428873002/diff/20001/content/browser/...
content/browser/media/session/pepper_playback_observer.cc:46: void
PepperPlaybackObserver::PepperStartsPlayback(int32_t pp_instance) {
On 2016/10/18 18:51:54, Zhiqiang Zhang wrote:
> On 2016/10/18 17:26:59, xhwang wrote:
> > OOC, what is defined as a "pepper playback"? pepper flash, pepper cdm,
and/or
> > pepper audio?
> 
> It's a pepper plugin instance producing audio.

Could you please add a comment somewhere? IMHO the current name is a bit
confusing.

[Zhiqiang Zhang (Slow), 2016-10-19 10:22:05]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-19 10:22:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, asvitkine@chromium.org, mlamouri@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2428873002/#ps60001
(title: "commenting on "pepper playback"")

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

https://chromiumcodereview.appspot.com/2428873002/diff/20001/content/browser/...
File content/browser/media/session/pepper_playback_observer.cc (right):

https://chromiumcodereview.appspot.com/2428873002/diff/20001/content/browser/...
content/browser/media/session/pepper_playback_observer.cc:46: void
PepperPlaybackObserver::PepperStartsPlayback(int32_t pp_instance) {
On 2016/10/18 20:25:08, xhwang wrote:
> On 2016/10/18 18:51:54, Zhiqiang Zhang wrote:
> > On 2016/10/18 17:26:59, xhwang wrote:
> > > OOC, what is defined as a "pepper playback"? pepper flash, pepper cdm,
> and/or
> > > pepper audio?
> > 
> > It's a pepper plugin instance producing audio.
> 
> Could you please add a comment somewhere? IMHO the current name is a bit
> confusing.

Done.

[commit-bot: I haz the power, 2016-10-19 10:22:55]

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-10-19 11:21:03]

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

[commit-bot: I haz the power, 2016-10-19 11:21:05]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Zhiqiang Zhang (Slow), 2016-10-19 12:23:05]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-19 12:23:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, xhwang@chromium.org, asvitkine@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2428873002/#ps80001
(title: "fixed build")

[commit-bot: I haz the power, 2016-10-19 12:23:28]

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-10-19 13:11:15]

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

[commit-bot: I haz the power, 2016-10-19 13:11:17]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Zhiqiang Zhang (Slow), 2016-10-19 14:55:51]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-19 14:55:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, xhwang@chromium.org, asvitkine@chromium.org,
mlamouri@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2428873002/#ps100001 (title: "fixed
cast_shell build")

[commit-bot: I haz the power, 2016-10-19 14:56:07]

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-10-19 16:29:24]

Description was changed from

==========
Add a flag to chrome://flags for experimenting with MediaSession on desktop

This CL adds a flag for experimenting with MediaSession on desktop to
chrome://flags. kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make chrome://flags simpler.

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

to

==========
Add a flag to chrome://flags for experimenting with MediaSession on desktop

This CL adds a flag for experimenting with MediaSession on desktop to
chrome://flags. kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make chrome://flags simpler.

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

[commit-bot: I haz the power, 2016-10-19 16:29:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-21 13:09:17]

Description was changed from

==========
Add a flag to chrome://flags for experimenting with MediaSession on desktop

This CL adds a flag for experimenting with MediaSession on desktop to
chrome://flags. kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make chrome://flags simpler.

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

to

==========
Add a flag to chrome://flags for experimenting with MediaSession on desktop

This CL adds a flag for experimenting with MediaSession on desktop to
chrome://flags. kFlashJoinsMediaSession is merged into
kEnableDefaultMediaSession to make chrome://flags simpler.

BUG=619084

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

[commit-bot: I haz the power, 2016-10-21 13:09:19]

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