Add options for audio/video autoplay to chrome://settings/content.

Add options for audio/video autoplay to chrome://settings/content.

** WORK IN PROGRESS **

For almost five years users have been requesting autoplay settings. With
the recent addition of deferred playback in background tabs, we have a
further handful of unhappy users who want their autoplay back.  It's high
time we had some settings for this!

This adds the following form to the Content Settings section:
* Allow autoplay of all audio or video content.
* Allow autoplay of only audio or video content detected as important.
* Do not allow autoplay of any audio or video content.
[ Manage Exceptions ]

The second option is today's default behavior (only visible content will
autoplay). The last option is what Android does, where a user gesture is
required to initiate playback.

BUG=50132, 544723
TEST=...tbd...

[DaleCurtis, 2015-10-23 01:00:42]

dalecurtis@chromium.org changed reviewers:
+ jochen@chromium.org, philipj@opera.com

[DaleCurtis, 2015-10-23 01:00:43]

Hi jochen, philipj. This CL is not ready for a full review. While it does
accomplish everything I need, I've never added an chrome://setting before so I'm
sure I've done something terribly wrong.

I'd really appreciate it if you could take a high level look and evaluate the
sanity of this change. Please point me at any relevant documentation or examples
which might demonstrate any errors I'm making. I wasn't able to find anything
obvious unfortunately :/

Thanks in advance!

[philipj_slow, 2015-10-23 09:06:14]

philipj@opera.com changed reviewers:
+ liberato@chromium.org

[philipj_slow, 2015-10-23 09:06:14]

Adding liberato@ who is handling the autoplay experiment. Yhe outcome of that
could be that autoplay is simply allowed with no user setting in certain
situations.

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:359: if
(!document.frame()->loader().allowMediaAutoplay(false))
Can document().frame() be null? It's usually null checked in this file.

[jochen (gone - plz use gerrit), 2015-10-23 12:09:34]

jochen@chromium.org changed reviewers:
+ markusheintz@chromium.org, raymes@chromium.org

[jochen (gone - plz use gerrit), 2015-10-23 12:09:34]

+markusheintz/raymes for content settings

+felt fyi

[philipj_slow, 2015-10-23 12:17:30]

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

https://codereview.chromium.org/1414853003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:14600: +        Allow autoplay of only audio
or video content detected as important (recommended)
The description sounds like there's some non-trivial heuristic going on, but as
implemented it only checks if the tab is visible, I think. Is it described like
this because you plan to change this? It doesn't matter much in this CL, but I'm
rather worried about ever making some non-trivial heuristic the default, as that
seems rather hopeless to get interoperably implemented, and it's hard to explain
to web developers.

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:642: if
(!observer->allowMediaAutoplay(is_hidden)) {
What's happening here? On the face of things it seems possible to have autoplay
handling almost entirely within HTMLMediaElement, so that WebMediaPlayer then
simply does as it is told. Given that there's something called DeferMediaLoad I
can see that this already isn't the case.

liberato@, did you know about these bits, and is there any chance that the
autoplay experiment now thinks that we're trying to play in situations where
this code is actually deferring it further?

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:362: (setting ==
CONTENT_SETTING_DETECT_IMPORTANT_CONTENT && !hidden);
Oh... so "important content" actually means anything in an open tab?

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebContentSettingsClient.h (right):

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebContentSettingsClient.h:37: virtual bool
allowMediaAutoplay(bool hidden) { return true; }
AFAICT, the hidden bool comes only from HTMLMediaElement, and it is always
false. What is it actually intended to do? I suspect that it would be cleaner to
expose all three states to HTMLMediaElement and decide there whether or not to
autoplay. The code as it is de-facto only has two states of course, so unless
something is added to the CL you might want to simplify and add the extra state
only when it's properly supported.

[philipj_slow, 2015-10-23 12:30:22]

High level, I see two related issues:

1. What is the default autoplay behavior going to be? This is what the autoplay
experiment is designed to help answer.

2. If that default is too restrictive, should we have a setting to allow
autoplay?

My hope is that we will change the default to tie autoplay to the page
visibility API, so that essentially playback can start as long as you're in the
foreground.

If that happens, I don't think a setting to always allow autoplay would be in
high demand.

If the default does not change, I'm not sure what to do. Web developers would
still have to assume that autoplay basically never works on mobile, so if they
still try to do it, it seems almost as likely to be a mistake as intentional.

From the initial report in issue 50132 it sounds like what's requested is
actually the ability to prevent autoplay on desktop, not the ability to allow it
on mobile. Which is the main concern here?

[DaleCurtis, 2015-10-23 18:30:43]

On 2015/10/23 12:30:22, philipj wrote:
> High level, I see two related issues:
> 
> 1. What is the default autoplay behavior going to be? This is what the
autoplay
> experiment is designed to help answer.

At present, default autoplay on desktop is autoplay is always allowed in tabs
that started playing audio/video while the user was interacting with them. Those
tabs are allowed to continue autoplaying audio or video indefinitely to support
playlist type applications. The android situation is still under experiment, but
today means a user gesture is required. 
> 
> 2. If that default is too restrictive, should we have a setting to allow
> autoplay?

What do you mean? The options list above is "always autoplay", "autoplay only
visible content", or "no autoplay / user gesture required"

> 
> My hope is that we will change the default to tie autoplay to the page
> visibility API, so that essentially playback can start as long as you're in
the
> foreground.
> 
> If that happens, I don't think a setting to always allow autoplay would be in
> high demand.

Per the bugs and discussions with ConOps folk, it's a contentious and long
requested issue, so I think there's high enough demand to warrant a settings
page. I think depending on the outcome of the Autoplay experiments, it will be
even more in demand on mobile.

> 
> If the default does not change, I'm not sure what to do. Web developers would
> still have to assume that autoplay basically never works on mobile, so if they
> still try to do it, it seems almost as likely to be a mistake as intentional.
> 
> From the initial report in issue 50132 it sounds like what's requested is
> actually the ability to prevent autoplay on desktop, not the ability to allow
it
> on mobile. Which is the main concern here?

This change is primarily targeted at desktop users right now, since mobile is
no-autoplay currently. I do want to use the space created by this change to
expose similar settings on Android though. I.e. with these chrome://settings
available we could remove the chrome://flags entry for autoplay (but not the
switch for tests) and move the default value into the settings page. Making it
more discoverable to users if they are unhappy with the outcome of the
experiments.

[DaleCurtis, 2015-10-23 19:21:27]

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

https://codereview.chromium.org/1414853003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:14600: +        Allow autoplay of only audio
or video content detected as important (recommended)
On 2015/10/23 12:17:30, philipj wrote:
> The description sounds like there's some non-trivial heuristic going on, but
as
> implemented it only checks if the tab is visible, I think. Is it described
like
> this because you plan to change this? It doesn't matter much in this CL, but
I'm
> rather worried about ever making some non-trivial heuristic the default, as
that
> seems rather hopeless to get interoperably implemented, and it's hard to
explain
> to web developers.

There are discussions to see if we can reuse some of the heuristics from the
plugin-power-saver for selectively allowing autoplay and then there's the
experiments work you mention. So I chose "detected as important" here to fit
into the existing landscape of CONTENT_SETTINGS_TYPE values, but also in the
case we want to expand the scope beyond what we use today on desktop and
Android.

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:642: if
(!observer->allowMediaAutoplay(is_hidden)) {
On 2015/10/23 12:17:30, philipj wrote:
> What's happening here? On the face of things it seems possible to have
autoplay
> handling almost entirely within HTMLMediaElement, so that WebMediaPlayer then
> simply does as it is told. Given that there's something called DeferMediaLoad
I
> can see that this already isn't the case.
> 
> liberato@, did you know about these bits, and is there any chance that the
> autoplay experiment now thinks that we're trying to play in situations where
> this code is actually deferring it further?

Chrome has long had a load deferral mechanism for prerender; I reused it to
avoid video playback during session restore and accidental background playback
in tabs the user has never seen before. There's no "open link in background tab"
on Android and Android session restore doesn't reload killed pages until they're
used for the first time AFAIK, so this shouldn't be an issue there.

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:362: (setting ==
CONTENT_SETTING_DETECT_IMPORTANT_CONTENT && !hidden);
On 2015/10/23 12:17:30, philipj wrote:
> Oh... so "important content" actually means anything in an open tab?

Currently and as planned yes. There are discussion to see if the plugin power
saver heuristics could be used: 

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebContentSettingsClient.h (right):

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebContentSettingsClient.h:37: virtual bool
allowMediaAutoplay(bool hidden) { return true; }
On 2015/10/23 12:17:30, philipj wrote:
> AFAICT, the hidden bool comes only from HTMLMediaElement, and it is always
> false. What is it actually intended to do? I suspect that it would be cleaner
to
> expose all three states to HTMLMediaElement and decide there whether or not to
> autoplay. The code as it is de-facto only has two states of course, so unless
> something is added to the CL you might want to simplify and add the extra
state
> only when it's properly supported.

This is called by the Chrome content client with the actual hidden state, but
the FrameLoader APIs could have the hidden state removed.

[philipj_slow, 2015-10-27 16:06:59]

OK, so this is intended primarily as a way for desktop users to inhibit
autoplay, I think that makes sense. Sorry I didn't get around to a proper
follow-up today.

One quick thing I'd like to ask is if we need both the existing setting and a
content setting? It sure would be nice if, from the perspective of
HTMLMediaElement at least, there was just once setting, and it has as few states
as possible. Note that the autoplay experiment also implements some of the
visibility rules.

[DaleCurtis, 2015-10-27 17:14:25]

On 2015/10/27 16:06:59, philipj wrote:
> OK, so this is intended primarily as a way for desktop users to inhibit
> autoplay, I think that makes sense. Sorry I didn't get around to a proper
> follow-up today.

No worries, this has a long way to go before it's ready, so no rush -- we still
need UI+UX review, internationalization, etc, etc.

> 
> One quick thing I'd like to ask is if we need both the existing setting and a
> content setting? It sure would be nice if, from the perspective of
> HTMLMediaElement at least, there was just once setting, and it has as few
states
> as possible. Note that the autoplay experiment also implements some of the
> visibility rules.

Which is the "existing setting"? Presumably "content setting" refers to the
3-option radio+exceptions I'm proposing here. If you mean the entry in
chrome://flags for the user gesture requirement, I definitely want to remove the
UI bit -- we can't remove the command line switch since that will still be
needed by our test infrastructure (unless there's some command line flag to
force content settings I don't know about).

[DaleCurtis, 2015-10-27 23:26:40]

raymes, markusheintz: Any advice on chrome://settings/content changes? (See
first post in this thread).

[raymes, 2015-10-27 23:29:40]

Sorry I've been slow on this, but hope to take a closer look today. Thanks!

On Wed, 28 Oct 2015 at 10:26 <dalecurtis@chromium.org> wrote:

> raymes, markusheintz: Any advice on chrome://settings/content changes? (See
> first post in this thread).
>
> https://codereview.chromium.org/1414853003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[raymes, 2015-10-27 23:29:41]

Sorry I've been slow on this, but hope to take a closer look today. Thanks!

On Wed, 28 Oct 2015 at 10:26 <dalecurtis@chromium.org> wrote:

> raymes, markusheintz: Any advice on chrome://settings/content changes? (See
> first post in this thread).
>
> https://codereview.chromium.org/1414853003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[raymes, 2015-10-27 23:34:51]

raymes@chromium.org changed reviewers:
+ msramek@chromium.org
- markusheintz@chromium.org

[raymes, 2015-10-27 23:34:52]

This looks good to me overall. 

+msramek who is more familiar with content settings UI though (and cc bauerb)

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_registry.cc
(right):

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:233:
WebsiteSettingsInfo::SYNCABLE, WhitelistedSchemes(),
Do we really want this setting sync'd across devices?

[DaleCurtis, 2015-10-27 23:38:31]

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_registry.cc
(right):

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:233:
WebsiteSettingsInfo::SYNCABLE, WhitelistedSchemes(),
On 2015/10/27 23:34:51, raymes wrote:
> Do we really want this setting sync'd across devices?

Hmm, possibly not since Android would default to CONTENT_SETTING_BLOCK while
desktop would default to CONTENT_SETTING_DETECT_IMPORTANT_CONTENT. If
CONTENT_SETTING_DEFAULT is  what is actually sync'd then that's okay since we
can just control the default at runtime.

This is a good question for the eventual UX/UI review.

[felt, 2015-10-28 07:52:44]

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

[felt, 2015-10-28 07:52:45]

drive-by comments

https://codereview.chromium.org/1414853003/diff/1/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1414853003/diff/1/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources.grd:37: <!-- TODO(dalecurtis): Add images for
allowed auotplay. -->
nit: s/auotplay/autoplay
(same below too)

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_registry.cc
(right):

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:233:
WebsiteSettingsInfo::SYNCABLE, WhitelistedSchemes(),
On 2015/10/27 23:38:31, DaleCurtis wrote:
> On 2015/10/27 23:34:51, raymes wrote:
> > Do we really want this setting sync'd across devices?
> 
> Hmm, possibly not since Android would default to CONTENT_SETTING_BLOCK while
> desktop would default to CONTENT_SETTING_DETECT_IMPORTANT_CONTENT. If
> CONTENT_SETTING_DEFAULT is  what is actually sync'd then that's okay since we
> can just control the default at runtime.
> 
> This is a good question for the eventual UX/UI review.

My $0.02: the fact that there are different defaults implies that we think
people will want different things across device types. So syncing it seems odd.

[philipj_slow, 2015-10-28 10:19:36]

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:357: if
(document.settings() && document.settings()->mediaPlaybackRequiresUserGesture())
This is the "existing setting" I was referring to.

[philipj_slow, 2015-10-28 10:30:41]

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

https://codereview.chromium.org/1414853003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:14600: +        Allow autoplay of only audio
or video content detected as important (recommended)
On 2015/10/23 19:21:27, DaleCurtis wrote:
> On 2015/10/23 12:17:30, philipj wrote:
> > The description sounds like there's some non-trivial heuristic going on, but
> as
> > implemented it only checks if the tab is visible, I think. Is it described
> like
> > this because you plan to change this? It doesn't matter much in this CL, but
> I'm
> > rather worried about ever making some non-trivial heuristic the default, as
> that
> > seems rather hopeless to get interoperably implemented, and it's hard to
> explain
> > to web developers.
> 
> There are discussions to see if we can reuse some of the heuristics from the
> plugin-power-saver for selectively allowing autoplay and then there's the
> experiments work you mention. So I chose "detected as important" here to fit
> into the existing landscape of CONTENT_SETTINGS_TYPE values, but also in the
> case we want to expand the scope beyond what we use today on desktop and
> Android.

Perhaps this is a long way from shipping, but if it does initially ship with
only the requirement that media element's document is visible, then describing
it as "allow autoplay in foreground" or similar to the users seems to be more
understandable. However, this is ultimately a question for UI/UX. I would prefer
it to be accurate internally, however and doing a search/replace if it changes.

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:642: if
(!observer->allowMediaAutoplay(is_hidden)) {
On 2015/10/23 19:21:27, DaleCurtis wrote:
> On 2015/10/23 12:17:30, philipj wrote:
> > What's happening here? On the face of things it seems possible to have
> autoplay
> > handling almost entirely within HTMLMediaElement, so that WebMediaPlayer
then
> > simply does as it is told. Given that there's something called
DeferMediaLoad
> I
> > can see that this already isn't the case.
> > 
> > liberato@, did you know about these bits, and is there any chance that the
> > autoplay experiment now thinks that we're trying to play in situations where
> > this code is actually deferring it further?
> 
> Chrome has long had a load deferral mechanism for prerender; I reused it to
> avoid video playback during session restore and accidental background playback
> in tabs the user has never seen before. There's no "open link in background
tab"
> on Android and Android session restore doesn't reload killed pages until
they're
> used for the first time AFAIK, so this shouldn't be an issue there.

So is this entirely related to prerender and session restore, and has nothing to
do with implementing the preload policy for a media element in a "normal" active
document? If so should that actually change, shouldn't those loads continue to
be deferred? I think I still must be missing a piece of the puzzle :)

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebContentSettingsClient.h (right):

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebContentSettingsClient.h:37: virtual bool
allowMediaAutoplay(bool hidden) { return true; }
On 2015/10/23 19:21:27, DaleCurtis wrote:
> On 2015/10/23 12:17:30, philipj wrote:
> > AFAICT, the hidden bool comes only from HTMLMediaElement, and it is always
> > false. What is it actually intended to do? I suspect that it would be
cleaner
> to
> > expose all three states to HTMLMediaElement and decide there whether or not
to
> > autoplay. The code as it is de-facto only has two states of course, so
unless
> > something is added to the CL you might want to simplify and add the extra
> state
> > only when it's properly supported.
> 
> This is called by the Chrome content client with the actual hidden state, but
> the FrameLoader APIs could have the hidden state removed. 

Could we instead use mediaElement.document().hidden()? For web-observable
behavior it's really nice if the behavior can be expressed in terms of other
web-exposed APIs, so that web developers can see the same state and understand
what is happening.

[msramek, 2015-10-28 10:42:50]

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

https://codereview.chromium.org/1414853003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:14597: +        Allow autoplay of all audio
or video content
The wording should be "Allow all sites to autoplay...", "Ask if a site...", "Do
not allow any site..." as elsewhere. (I've now noticed that there are already
some inconsistencies, and some are my fault :/, but we probably shouldn't add
more)

https://codereview.chromium.org/1414853003/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings.html (right):

https://codereview.chromium.org/1414853003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.html:617: <!-- Media autoplay
settings -->
I'd move this up, just below mic and camera, since it thematically belongs
there.

https://codereview.chromium.org/1414853003/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings_exceptions_area.html
(right):

https://codereview.chromium.org/1414853003/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings_exceptions_area.html:155: <div
contentType="media-autoplay">
For consistency with the other comment - please move this up under mic and
camera.

Also, is there a reason to omit the incognito box (mode="otr")?

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_registry.cc
(right):

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:233:
WebsiteSettingsInfo::SYNCABLE, WhitelistedSchemes(),
On 2015/10/28 07:52:45, felt (slow through Nov 3) wrote:
> On 2015/10/27 23:38:31, DaleCurtis wrote:
> > On 2015/10/27 23:34:51, raymes wrote:
> > > Do we really want this setting sync'd across devices?
> > 
> > Hmm, possibly not since Android would default to CONTENT_SETTING_BLOCK while
> > desktop would default to CONTENT_SETTING_DETECT_IMPORTANT_CONTENT. If
> > CONTENT_SETTING_DEFAULT is  what is actually sync'd then that's okay since
we
> > can just control the default at runtime.
> > 
> > This is a good question for the eventual UX/UI review.
> 
> My $0.02: the fact that there are different defaults implies that we think
> people will want different things across device types. So syncing it seems
odd.

Note that content settings are currently not synced to Android, so technically
it's not a problem. But I agree with the sentiment.

Another argument for NOT syncing is consistency with existing media-related
content settings (mic & camera).

[Bernhard Bauer, 2015-10-28 12:52:57]

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

[Bernhard Bauer, 2015-10-28 12:52:58]

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
File components/content_settings/core/browser/content_settings_registry.cc
(right):

https://codereview.chromium.org/1414853003/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_registry.cc:233:
WebsiteSettingsInfo::SYNCABLE, WhitelistedSchemes(),
On 2015/10/28 10:42:50, msramek wrote:
> On 2015/10/28 07:52:45, felt (slow through Nov 3) wrote:
> > On 2015/10/27 23:38:31, DaleCurtis wrote:
> > > On 2015/10/27 23:34:51, raymes wrote:
> > > > Do we really want this setting sync'd across devices?
> > > 
> > > Hmm, possibly not since Android would default to CONTENT_SETTING_BLOCK
while
> > > desktop would default to CONTENT_SETTING_DETECT_IMPORTANT_CONTENT. If
> > > CONTENT_SETTING_DEFAULT is  what is actually sync'd then that's okay since
> we
> > > can just control the default at runtime.
> > > 
> > > This is a good question for the eventual UX/UI review.
> > 
> > My $0.02: the fact that there are different defaults implies that we think
> > people will want different things across device types. So syncing it seems
> odd.
> 
> Note that content settings are currently not synced to Android, so technically
> it's not a problem. But I agree with the sentiment.

It kind of sounds to me like what we want here is actually syncing, but with
separate settings for Android vs. desktop. There is no reason why I should have
to configure my settings again on an additional desktop or Android device.

> Another argument for NOT syncing is consistency with existing media-related
> content settings (mic & camera).

[DaleCurtis, 2015-10-30 19:25:55]

dalecurtis@chromium.org changed reviewers:
+ renganathan@chromium.org

[DaleCurtis, 2015-10-30 19:25:56]

+pangu

Thanks for the feedback, since this is roughly the right approach pangu@ is
putting together a doc which we can use for the finer points of discussion with
UX/UI.

We'll post that doc to the bug when it's ready and ensure it's chromium.org
viewable for external comments.

Once discussion settles I'll revamp this to match our decisions.

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1414853003/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:642: if
(!observer->allowMediaAutoplay(is_hidden)) {
On 2015/10/28 10:30:40, philipj wrote:
> On 2015/10/23 19:21:27, DaleCurtis wrote:
> > On 2015/10/23 12:17:30, philipj wrote:
> > > What's happening here? On the face of things it seems possible to have
> > autoplay
> > > handling almost entirely within HTMLMediaElement, so that WebMediaPlayer
> then
> > > simply does as it is told. Given that there's something called
> DeferMediaLoad
> > I
> > > can see that this already isn't the case.
> > > 
> > > liberato@, did you know about these bits, and is there any chance that the
> > > autoplay experiment now thinks that we're trying to play in situations
where
> > > this code is actually deferring it further?
> > 
> > Chrome has long had a load deferral mechanism for prerender; I reused it to
> > avoid video playback during session restore and accidental background
playback
> > in tabs the user has never seen before. There's no "open link in background
> tab"
> > on Android and Android session restore doesn't reload killed pages until
> they're
> > used for the first time AFAIK, so this shouldn't be an issue there.
> 
> So is this entirely related to prerender and session restore, and has nothing
to
> do with implementing the preload policy for a media element in a "normal"
active
> document? If so should that actually change, shouldn't those loads continue to
> be deferred? I think I still must be missing a piece of the puzzle :)

Ah, you're not missing anything, this does in fact break the session restore and
prerender cases :) At the very least prerender should continue to be blocked on
the visibility check regardless of the content setting. Session restore should
probably be separated out as well, but we'll vet with UX first. It would need
some new bit plumbed from the browser process too to indicate "process launched
for session restore."

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:357: if
(document.settings() && document.settings()->mediaPlaybackRequiresUserGesture())
On 2015/10/28 10:19:35, philipj wrote:
> This is the "existing setting" I was referring to.

Ah gotcha, yeah this should be removable once we have the content setting.

[philipj_slow, 2015-11-02 13:15:45]

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1414853003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:357: if
(document.settings() && document.settings()->mediaPlaybackRequiresUserGesture())
On 2015/10/30 19:25:56, DaleCurtis wrote:
> On 2015/10/28 10:19:35, philipj wrote:
> > This is the "existing setting" I was referring to.
> 
> Ah gotcha, yeah this should be removable once we have the content setting.

If it isn't too much trouble, could you do it in the same CL or at least have
the follow-up ready for review? Otherwise it's harder to be confident that the
content setting is a full replacement.