Make autoplay policies no longer platform dependant.

Make autoplay policies no longer platform dependant.

This is moving the two web preferences to be available on both platforms
and adds a central point to check for the current autoplay policy.

Next step will be to merge the two Blink Settings into one.

BUG=714676
Review-Url: https://codereview.chromium.org/2846623003
Cr-Commit-Position: refs/heads/master@{#470904}
Committed: https://chromium.googlesource.com/chromium/src/+/55a0543c377a69bbb9ab8c49629f037219289cd0

[mlamouri (slow - plz ping), 2017-04-27 14:01:15]

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

[nasko, 2017-04-27 16:52:50]

LGTM, but it will be nice to prefix the enum and member name with "media", so it
is a bit more explicit for folks not working directly on media.

https://codereview.chromium.org/2846623003/diff/1/content/public/common/commo...
File content/public/common/common_param_traits_macros.h (right):

https://codereview.chromium.org/2846623003/diff/1/content/public/common/commo...
content/public/common/common_param_traits_macros.h:255:
IPC_STRUCT_TRAITS_MEMBER(autoplay_policy)
nit: media_autoplay_policy?

https://codereview.chromium.org/2846623003/diff/1/content/public/common/web_p...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2846623003/diff/1/content/public/common/web_p...
content/public/common/web_preferences.h:71: enum class AutoplayPolicy {
nit: MediaAutoplayPolicy?

[DaleCurtis, 2017-04-27 18:59:39]

https://codereview.chromium.org/2846623003/diff/1/media/base/media_util.h
File media/base/media_util.h (right):

https://codereview.chromium.org/2846623003/diff/1/media/base/media_util.h#new...
media/base/media_util.h:30: // Based on a |command_line| and the current
platform, returns the effective
Hmm, I didn't even know we had a media_util file, this should probably be
deleted and these moved elsewhere. Can we keep this method in media_witches?

[DaleCurtis, 2017-04-27 18:59:47]

lgtm otherwise

[mlamouri (slow - plz ping), 2017-04-27 21:34:14]

https://codereview.chromium.org/2846623003/diff/1/content/public/common/commo...
File content/public/common/common_param_traits_macros.h (right):

https://codereview.chromium.org/2846623003/diff/1/content/public/common/commo...
content/public/common/common_param_traits_macros.h:255:
IPC_STRUCT_TRAITS_MEMBER(autoplay_policy)
On 2017/04/27 at 16:52:50, nasko wrote:
> nit: media_autoplay_policy?

nasko, to me, "autoplay" is for audio/video. What do you associate it with? In
my opinion, it would just make the name longer but I understand I might be
biased :)

https://codereview.chromium.org/2846623003/diff/1/media/base/media_util.h
File media/base/media_util.h (right):

https://codereview.chromium.org/2846623003/diff/1/media/base/media_util.h#new...
media/base/media_util.h:30: // Based on a |command_line| and the current
platform, returns the effective
On 2017/04/27 at 18:59:39, DaleCurtis wrote:
> Hmm, I didn't even know we had a media_util file, this should probably be
deleted and these moved elsewhere. Can we keep this method in media_witches?

I was considering having this in media_switches but happy to invoke some witches
:)

[nasko, 2017-04-27 21:42:56]

https://codereview.chromium.org/2846623003/diff/1/content/public/common/commo...
File content/public/common/common_param_traits_macros.h (right):

https://codereview.chromium.org/2846623003/diff/1/content/public/common/commo...
content/public/common/common_param_traits_macros.h:255:
IPC_STRUCT_TRAITS_MEMBER(autoplay_policy)
On 2017/04/27 21:34:14, mlamouri wrote:
> On 2017/04/27 at 16:52:50, nasko wrote:
> > nit: media_autoplay_policy?
> 
> nasko, to me, "autoplay" is for audio/video. What do you associate it with? In
> my opinion, it would just make the name longer but I understand I might be
> biased :)

Do you associate playback with media only or not? I see media_playback_* above,
so my brain automatically categorizes into the media world, so when seeing
autoplay, it expected seeing the prefix.

It is a nit for a reason, it is your call, I don't feel strongly.

[mlamouri (slow - plz ping), 2017-04-28 10:12:18]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-04-28 10:12:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2846623003/#ps20001
(title: "review comments")

[commit-bot: I haz the power, 2017-04-28 10:12: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, 2017-04-28 10:32:51]

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

[commit-bot: I haz the power, 2017-04-28 10:32:52]

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

[mlamouri (slow - plz ping), 2017-05-09 10:26:34]

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

[commit-bot: I haz the power, 2017-05-09 10:27:06]

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-05-09 10:51:54]

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

[commit-bot: I haz the power, 2017-05-09 10:51:54]

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

[mlamouri (slow - plz ping), 2017-05-09 12:23:02]

mlamouri@chromium.org changed reviewers:
+ bauerb@chromium.org, tobiasjs@chromium.org

[mlamouri (slow - plz ping), 2017-05-09 12:23:02]

+bauerb@ for chrome/browser/android/
+tobiasjs@ for android_webview/

[mlamouri (slow - plz ping), 2017-05-09 12:23:06]

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

[commit-bot: I haz the power, 2017-05-09 12:23:27]

Dry run: CQ is trying da patch.

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

[Bernhard Bauer, 2017-05-09 12:47:17]

LGTM with some nits:

https://codereview.chromium.org/2846623003/diff/60001/chrome/browser/android/...
File chrome/browser/android/voice_search_tab_helper.cc (right):

https://codereview.chromium.org/2846623003/diff/60001/chrome/browser/android/...
chrome/browser/android/voice_search_tab_helper.cc:42: bool
gesture_requiref_by_prefs =
Nit: |gesture_required_by_prefs|

https://codereview.chromium.org/2846623003/diff/60001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2846623003/diff/60001/content/public/common/w...
content/public/common/web_preferences.h:73: kCrossOriginUserGestureRequired,
Nit: "cross origin" here qualifies the requirement (a user gesture is required
for cross-origin autoplay) and not the user gesture (a cross-origin user gesture
is required), right? The way it currently reads (in particular following "no
user gesture required" is a bit confusing. Maybe
"kUserGestureRequiredForCrossOrigin"?

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

https://codereview.chromium.org/2846623003/diff/60001/media/base/media_switch...
media/base/media_switches.cc:259: if
(!command_line.HasSwitch(switches::kAutoplayPolicy)) {
Nit: I would probably flip this condition around, to keep the references to
kAutoplayPolicy closer together.

[Tobias Sargeant, 2017-05-09 13:53:55]

android_webview LGTM.

[commit-bot: I haz the power, 2017-05-09 14:40:53]

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

[commit-bot: I haz the power, 2017-05-09 14:40:53]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-05-09 17:23:49]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-05-09 17:23:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, nasko@chromium.org, bauerb@chromium.org,
tobiasjs@chromium.org
Link to the patchset: https://codereview.chromium.org/2846623003/#ps80001
(title: "bauerb review comments")

[commit-bot: I haz the power, 2017-05-09 17:24:28]

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-05-09 17:24:35]

https://codereview.chromium.org/2846623003/diff/60001/chrome/browser/android/...
File chrome/browser/android/voice_search_tab_helper.cc (right):

https://codereview.chromium.org/2846623003/diff/60001/chrome/browser/android/...
chrome/browser/android/voice_search_tab_helper.cc:42: bool
gesture_requiref_by_prefs =
On 2017/05/09 at 12:47:17, Bernhard Bauer wrote:
> Nit: |gesture_required_by_prefs|

Done.

https://codereview.chromium.org/2846623003/diff/60001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2846623003/diff/60001/content/public/common/w...
content/public/common/web_preferences.h:73: kCrossOriginUserGestureRequired,
On 2017/05/09 at 12:47:17, Bernhard Bauer wrote:
> Nit: "cross origin" here qualifies the requirement (a user gesture is required
for cross-origin autoplay) and not the user gesture (a cross-origin user gesture
is required), right? The way it currently reads (in particular following "no
user gesture required" is a bit confusing. Maybe
"kUserGestureRequiredForCrossOrigin"?

Fair enough. I've renamed the autoplay flag option as long as I was there.

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

https://codereview.chromium.org/2846623003/diff/60001/media/base/media_switch...
media/base/media_switches.cc:259: if
(!command_line.HasSwitch(switches::kAutoplayPolicy)) {
On 2017/05/09 at 12:47:17, Bernhard Bauer wrote:
> Nit: I would probably flip this condition around, to keep the references to
kAutoplayPolicy closer together.

Done.

[commit-bot: I haz the power, 2017-05-09 21:44:15]

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

[commit-bot: I haz the power, 2017-05-09 21:44:16]

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_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2017-05-10 13:16:44]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-05-10 13:17:21]

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-05-10 15:36:11]

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

[commit-bot: I haz the power, 2017-05-10 15:36:12]

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

[mlamouri (slow - plz ping), 2017-05-11 08:27:20]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-05-11 08:27:52]

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-05-11 10:34:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494491240319730,
"parent_rev": "481eb1f44f0db7dbfd321be80dccb3e0e8cbe314", "commit_rev":
"55a0543c377a69bbb9ab8c49629f037219289cd0"}

[commit-bot: I haz the power, 2017-05-11 10:34:30]

Description was changed from

==========
Make autoplay policies no longer platform dependant.

This is moving the two web preferences to be available on both platforms
and adds a central point to check for the current autoplay policy.

Next step will be to merge the two Blink Settings into one.

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

to

==========
Make autoplay policies no longer platform dependant.

This is moving the two web preferences to be available on both platforms
and adds a central point to check for the current autoplay policy.

Next step will be to merge the two Blink Settings into one.

BUG=714676

Review-Url: https://codereview.chromium.org/2846623003
Cr-Commit-Position: refs/heads/master@{#470904}
Committed:
https://chromium.googlesource.com/chromium/src/+/55a0543c377a69bbb9ab8c49629f...
==========

[commit-bot: I haz the power, 2017-05-11 10:34:33]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/55a0543c377a69bbb9ab8c49629f...