Create autoplay policy flag and merge cross-origin autoplay blocking into it.

Create autoplay policy flag and merge cross-origin autoplay blocking into it.

Also allows to use this flag to turn off autoplay restrictions on
Android. Follow-ups will merge
kDisableGestureRequirementsForMediaPlayback into this new flag.

BUG=714676
Review-Url: https://codereview.chromium.org/2837023002
Cr-Commit-Position: refs/heads/master@{#467142}
Committed: https://chromium.googlesource.com/chromium/src/+/1f264da2cfe8d40c211f07ce3960898e5c998773

[mlamouri (slow - plz ping), 2017-04-24 17:17:09]

mlamouri@chromium.org changed reviewers:
+ asvitkine@chromium.org, dalecurtis@chromium.org, nasko@chromium.org

[mlamouri (slow - plz ping), 2017-04-24 17:17:10]

Hi, PTAL:

asvitkine:
  tools/metrics/histograms/histograms.xml
dalecurtis:
  media/base/media_switches.cc
  media/base/media_switches.h
nasko:
  content/browser/renderer_host/render_process_host_impl.cc
  content/browser/renderer_host/render_view_host_impl.cc
  content/child/runtime_features.cc
  content/public/common/content_features.cc
  content/public/common/content_features.h

[whywhat, 2017-04-24 18:06:23]

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

[whywhat, 2017-04-24 18:06:24]

drive-by

https://codereview.chromium.org/2837023002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2837023002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:445:
command_line.GetSwitchValueASCII(switches::kAutoplayPolicy) ==
shouldn't this be != ?
or rather == kAutoplayPolicyGestureRequired?

https://codereview.chromium.org/2837023002/diff/1/content/child/runtime_featu...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2837023002/diff/1/content/child/runtime_featu...
content/child/runtime_features.cc:383: #if !defined(OS_ANDROID)
nit: does it have to be #if-ed?

[mlamouri (slow - plz ping), 2017-04-24 18:41:24]

https://codereview.chromium.org/2837023002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2837023002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:445:
command_line.GetSwitchValueASCII(switches::kAutoplayPolicy) ==
On 2017/04/24 at 18:06:24, whywhat wrote:
> shouldn't this be != ?
> or rather == kAutoplayPolicyGestureRequired?

Oups, yes :)

https://codereview.chromium.org/2837023002/diff/1/content/child/runtime_featu...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2837023002/diff/1/content/child/runtime_featu...
content/child/runtime_features.cc:383: #if !defined(OS_ANDROID)
On 2017/04/24 at 18:06:24, whywhat wrote:
> nit: does it have to be #if-ed?

I want to avoid a super long CL so I'm keeping the same assumptions as the
current code for now.

[Alexei Svitkine (slow), 2017-04-24 18:45:07]

lgtm

https://codereview.chromium.org/2837023002/diff/1/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2837023002/diff/1/media/base/media_switches.c...
media/base/media_switches.cc:143: // Autoplay policy to require a user gesture
in ordor to play for cross origin
order

[nasko, 2017-04-24 18:48:47]

content/ rubberstamp LGTM, once whywhat's comments are addressed.

[mlamouri (slow - plz ping), 2017-04-24 18:50:00]

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

[commit-bot: I haz the power, 2017-04-24 18:51:00]

Dry run: CQ is trying da patch.

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

[mlamouri (slow - plz ping), 2017-04-24 18:56:49]

On 2017/04/24 at 18:48:47, nasko wrote:
> content/ rubberstamp LGTM, once whywhat's comments are addressed.

They are :)

[DaleCurtis, 2017-04-24 18:57:41]

As discussed via e-mail I think this is not quite correct, "no restrictions"
actually means "foreground only today", so we should clean up the use case you
found in the Chrome content renderer client with this too.

[commit-bot: I haz the power, 2017-04-24 19:15:58]

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

[commit-bot: I haz the power, 2017-04-24 19:15:59]

Dry run: 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...)

[mlamouri (slow - plz ping), 2017-04-24 19:16:49]

On 2017/04/24 at 18:57:41, dalecurtis wrote:
> As discussed via e-mail I think this is not quite correct, "no restrictions"
actually means "foreground only today", so we should clean up the use case you
found in the Chrome content renderer client with this too.

The same reason why I don't have the Web APK whitelist as an autoplay policy, I
think that the "must be in the foreground to play the first time" is a sub-type
that we can leave on the side for now. It will make the logic more complex and
it wouldn't be sent to Blink (one end goal is to give an autoplay policy to
Blink instead of many flags). One issue with the foreground feature is that it
would apply to various autoplay and will make the number of policy explodes
(x-origin gesture required w/ or wo/ foreground restrictions for example).

[DaleCurtis, 2017-04-24 19:26:38]

On 2017/04/24 at 19:16:49, mlamouri wrote:
> On 2017/04/24 at 18:57:41, dalecurtis wrote:
> > As discussed via e-mail I think this is not quite correct, "no restrictions"
actually means "foreground only today", so we should clean up the use case you
found in the Chrome content renderer client with this too.
> 
> The same reason why I don't have the Web APK whitelist as an autoplay policy,
I think that the "must be in the foreground to play the first time" is a
sub-type that we can leave on the side for now. It will make the logic more
complex and it wouldn't be sent to Blink (one end goal is to give an autoplay
policy to Blink instead of many flags). One issue with the foreground feature is
that it would apply to various autoplay and will make the number of policy
explodes (x-origin gesture required w/ or wo/ foreground restrictions for
example).

I'm fine with leaving it alone for now, but I think this feature could easily
live in blink. I don't think policy explosion is a valid excuse though for not
handling it unless we're willing to make the statement that we don't think this
should be configurable. Which is certainly something we could decide, however
"no restrictions" is misleading.

[mlamouri (slow - plz ping), 2017-04-24 19:54:53]

On 2017/04/24 at 19:26:38, dalecurtis wrote:
> On 2017/04/24 at 19:16:49, mlamouri wrote:
> > On 2017/04/24 at 18:57:41, dalecurtis wrote:
> > > As discussed via e-mail I think this is not quite correct, "no
restrictions" actually means "foreground only today", so we should clean up the
use case you found in the Chrome content renderer client with this too.
> > 
> > The same reason why I don't have the Web APK whitelist as an autoplay
policy, I think that the "must be in the foreground to play the first time" is a
sub-type that we can leave on the side for now. It will make the logic more
complex and it wouldn't be sent to Blink (one end goal is to give an autoplay
policy to Blink instead of many flags). One issue with the foreground feature is
that it would apply to various autoplay and will make the number of policy
explodes (x-origin gesture required w/ or wo/ foreground restrictions for
example).
> 
> I'm fine with leaving it alone for now, but I think this feature could easily
live in blink. I don't think policy explosion is a valid excuse though for not
handling it unless we're willing to make the statement that we don't think this
should be configurable. Which is certainly something we could decide, however
"no restrictions" is misleading.

"No user gesture required" instead of "No restrictions" maybe?

[DaleCurtis, 2017-04-24 20:30:24]

On 2017/04/24 at 19:54:53, mlamouri wrote:
> On 2017/04/24 at 19:26:38, dalecurtis wrote:
> > On 2017/04/24 at 19:16:49, mlamouri wrote:
> > > On 2017/04/24 at 18:57:41, dalecurtis wrote:
> > > > As discussed via e-mail I think this is not quite correct, "no
restrictions" actually means "foreground only today", so we should clean up the
use case you found in the Chrome content renderer client with this too.
> > > 
> > > The same reason why I don't have the Web APK whitelist as an autoplay
policy, I think that the "must be in the foreground to play the first time" is a
sub-type that we can leave on the side for now. It will make the logic more
complex and it wouldn't be sent to Blink (one end goal is to give an autoplay
policy to Blink instead of many flags). One issue with the foreground feature is
that it would apply to various autoplay and will make the number of policy
explodes (x-origin gesture required w/ or wo/ foreground restrictions for
example).
> > 
> > I'm fine with leaving it alone for now, but I think this feature could
easily live in blink. I don't think policy explosion is a valid excuse though
for not handling it unless we're willing to make the statement that we don't
think this should be configurable. Which is certainly something we could decide,
however "no restrictions" is misleading.
> 
> "No user gesture required" instead of "No restrictions" maybe?

It fits our existing language, so that's fine with me -- but it isn't much of an
improvement since foreground -> background is gesture from the user's
perspective. Longer term some maybe some escalating settings like:
- "No restrictions"
- "Foreground only autoplay until first playback." [current desktop default]
- "User gesture required for playback."
- "Cross origin autoplay requires user gesture." [android default]
- "Autoplay disabled."

Seems like it might make sense. It depends on where we want to go in the future.

[mlamouri (slow - plz ping), 2017-04-24 21:05:35]

On 2017/04/24 at 20:30:24, dalecurtis wrote:
> On 2017/04/24 at 19:54:53, mlamouri wrote:
> > On 2017/04/24 at 19:26:38, dalecurtis wrote:
> > > On 2017/04/24 at 19:16:49, mlamouri wrote:
> > > > On 2017/04/24 at 18:57:41, dalecurtis wrote:
> > > > > As discussed via e-mail I think this is not quite correct, "no
restrictions" actually means "foreground only today", so we should clean up the
use case you found in the Chrome content renderer client with this too.
> > > > 
> > > > The same reason why I don't have the Web APK whitelist as an autoplay
policy, I think that the "must be in the foreground to play the first time" is a
sub-type that we can leave on the side for now. It will make the logic more
complex and it wouldn't be sent to Blink (one end goal is to give an autoplay
policy to Blink instead of many flags). One issue with the foreground feature is
that it would apply to various autoplay and will make the number of policy
explodes (x-origin gesture required w/ or wo/ foreground restrictions for
example).
> > > 
> > > I'm fine with leaving it alone for now, but I think this feature could
easily live in blink. I don't think policy explosion is a valid excuse though
for not handling it unless we're willing to make the statement that we don't
think this should be configurable. Which is certainly something we could decide,
however "no restrictions" is misleading.
> > 
> > "No user gesture required" instead of "No restrictions" maybe?
> 
> It fits our existing language, so that's fine with me -- but it isn't much of
an improvement since foreground -> background is gesture from the user's
perspective. Longer term some maybe some escalating settings like:
> - "No restrictions"
> - "Foreground only autoplay until first playback." [current desktop default]
> - "User gesture required for playback."
> - "Cross origin autoplay requires user gesture." [android default]
> - "Autoplay disabled."
> 
> Seems like it might make sense. It depends on where we want to go in the
future.

The goal is to have a new autoplay policy unified for mobile and desktop. Chrome
policy has always been that flags shouldn't be used as options but as ways to
test things so IMO we should the disabling foreground restriction as a command
line flag not exposed in about_flags.cc which can be turned on for tests.
Though, maybe this should be discussed in a bug :)

I will update the CL tomorrow.

[DaleCurtis, 2017-04-24 21:09:46]

On 2017/04/24 at 21:05:35, mlamouri wrote:
> On 2017/04/24 at 20:30:24, dalecurtis wrote:
> > On 2017/04/24 at 19:54:53, mlamouri wrote:
> > > On 2017/04/24 at 19:26:38, dalecurtis wrote:
> > > > On 2017/04/24 at 19:16:49, mlamouri wrote:
> > > > > On 2017/04/24 at 18:57:41, dalecurtis wrote:
> > > > > > As discussed via e-mail I think this is not quite correct, "no
restrictions" actually means "foreground only today", so we should clean up the
use case you found in the Chrome content renderer client with this too.
> > > > > 
> > > > > The same reason why I don't have the Web APK whitelist as an autoplay
policy, I think that the "must be in the foreground to play the first time" is a
sub-type that we can leave on the side for now. It will make the logic more
complex and it wouldn't be sent to Blink (one end goal is to give an autoplay
policy to Blink instead of many flags). One issue with the foreground feature is
that it would apply to various autoplay and will make the number of policy
explodes (x-origin gesture required w/ or wo/ foreground restrictions for
example).
> > > > 
> > > > I'm fine with leaving it alone for now, but I think this feature could
easily live in blink. I don't think policy explosion is a valid excuse though
for not handling it unless we're willing to make the statement that we don't
think this should be configurable. Which is certainly something we could decide,
however "no restrictions" is misleading.
> > > 
> > > "No user gesture required" instead of "No restrictions" maybe?
> > 
> > It fits our existing language, so that's fine with me -- but it isn't much
of an improvement since foreground -> background is gesture from the user's
perspective. Longer term some maybe some escalating settings like:
> > - "No restrictions"
> > - "Foreground only autoplay until first playback." [current desktop default]
> > - "User gesture required for playback."
> > - "Cross origin autoplay requires user gesture." [android default]
> > - "Autoplay disabled."
> > 
> > Seems like it might make sense. It depends on where we want to go in the
future.
> 
> The goal is to have a new autoplay policy unified for mobile and desktop.
Chrome policy has always been that flags shouldn't be used as options but as
ways to test things so IMO we should the disabling foreground restriction as a
command line flag not exposed in about_flags.cc which can be turned on for
tests. Though, maybe this should be discussed in a bug :)
> 
> I will update the CL tomorrow.

I was under the impression you're formatting these for an eventual UI setting;
is that not the case?

[mlamouri (slow - plz ping), 2017-04-25 13:56:55]

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

[commit-bot: I haz the power, 2017-04-25 13:57:01]

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, 2017-04-25 15:24:44]

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

[commit-bot: I haz the power, 2017-04-25 15:24:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mlamouri (slow - plz ping), 2017-04-25 16:43:18]

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

[commit-bot: I haz the power, 2017-04-25 16:43:37]

Dry run: CQ is trying da patch.

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

[mlamouri (slow - plz ping), 2017-04-25 17:17:11]

On 2017/04/24 at 21:09:46, dalecurtis wrote:
> I was under the impression you're formatting these for an eventual UI setting;
is that not the case?

It's not. I'm merging those because today we have two autoplay policies per
platform and we are going to introduce another one. I don't want to have
multiple booleans that consumers will ultimately have to deal with. If we have a
flag with multiple values, we can guarantee that the consumers will know which
autoplay policy is set. Ultimately, if the new policy sticks, the others options
will be removed. FWIW, this is what happened with autoplay muted (it's only a
Blink RuntimeEnabledFeature now that users can't control).

I've applied all the comments. PTAL

[commit-bot: I haz the power, 2017-04-25 18:57:19]

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

[commit-bot: I haz the power, 2017-04-25 18:57:20]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2017-04-25 19:07:43]

Thanks for the explanation! Sorry for my confusion. lgtm

[whywhat, 2017-04-25 22:20:07]

lgtm

[mlamouri (slow - plz ping), 2017-04-25 22:22:25]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-04-25 22:22:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2837023002/#ps20002
(title: "fix tests")

[commit-bot: I haz the power, 2017-04-25 22:22: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, 2017-04-25 22:34:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20002, "attempt_start_ts": 1493158945879630,
"parent_rev": "901f389a22a01c1c2bbee6b665685a8db123e70e", "commit_rev":
"1f264da2cfe8d40c211f07ce3960898e5c998773"}

[commit-bot: I haz the power, 2017-04-25 22:35:05]

Description was changed from

==========
Create autoplay policy flag and merge cross-origin autoplay blocking into it.

Also allows to use this flag to turn off autoplay restrictions on
Android. Follow-ups will merge
kDisableGestureRequirementsForMediaPlayback into this new flag.

BUG=714676
==========

to

==========
Create autoplay policy flag and merge cross-origin autoplay blocking into it.

Also allows to use this flag to turn off autoplay restrictions on
Android. Follow-ups will merge
kDisableGestureRequirementsForMediaPlayback into this new flag.

BUG=714676

Review-Url: https://codereview.chromium.org/2837023002
Cr-Commit-Position: refs/heads/master@{#467142}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f264da2cfe8d40c211f07ce3960...
==========

[commit-bot: I haz the power, 2017-04-25 22:35:07]

Committed patchset #4 (id:20002) as
https://chromium.googlesource.com/chromium/src/+/1f264da2cfe8d40c211f07ce3960...