Allow metadata preload on cellular connections and disallow autoplay muted for low end devices.

Allow metadata preload on cellular connections and disallow autoplay muted for low end devices.

This allows metadata preload on cellular connections unless some exceptions
are met such as data saver is enabled or a low end device is being used.

In addition, low end devices are not allowed to autoplay in order to avoid
inconsistencies.

Intent to Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/_fxvn-ny2jA/KqlmhamVCAAJ

BUG=658313
Committed: https://crrev.com/ac3501ca20c96ad8bcde34bb044b36bc6ca6e685
Cr-Commit-Position: refs/heads/master@{#431037}

[mlamouri (slow - plz ping), 2016-11-02 12:13:49]

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

[mlamouri (slow - plz ping), 2016-11-02 12:13:50]

isherman@, PTAL at metrics change.

dalecurtis@, PTAL at HTMLMediaElement changes.

Thanks :)

[mlamouri (slow - plz ping), 2016-11-02 12:14:12]

mlamouri@chromium.org changed reviewers:
+ foolip@chromium.org

[mlamouri (slow - plz ping), 2016-11-02 12:14:12]

Actully, +foolip@ because core/ change.

[mlamouri (slow - plz ping), 2016-11-02 12:14:17]

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

[commit-bot: I haz the power, 2016-11-02 12:14:27]

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, 2016-11-02 13:38:23]

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

[commit-bot: I haz the power, 2016-11-02 13:38:24]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2016-11-02 19:23:55]

lgtm

[Ilya Sherman, 2016-11-02 20:23:47]

histograms.xml lgtm

[foolip, 2016-11-03 10:14:41]

lgtm % test nits

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html
(right):

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html:66: var
asyncTests = [];
asyncTests = tests.map(test => async_test('Testing autoplay behaviour for ' +
test.name));

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html:80:
internals.settings.setDataSaverEnabled(false);
If it matters that these are reset when the test is done, you can add a
t.add_cleanup in the test instead, so that it'll get done even if the tests fail
or time out.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html:107:
document.body.appendChild(media);
Does appending make a difference?

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/preload-conditions.html (right):

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:121:
assert_equals(media.getAttribute('preload'), null);
assert_false(media.hasAttribute('preload'))

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:124: [ 'none',
'metadata', 'auto' ].forEach(t.step_func(preload => {
It's harmless, but here you don't need step_func, because this callback is run
synchronously. You could use `for (const preload of [ 'none', 'metadata', 'auto'
])` to avoid the question entirely.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:125: var expected =
test.expectations.allowed.indexOf(preload) != -1
Can use .includes(preload)

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:136: var
expectedLoading = 4;
If it weren't for the global state in internals staying stable it'd be much
preferable to have separate tests for the below. But I don't have any good ideas
:/

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:137: [ '', 'none',
'metadata', 'auto' ].forEach(t.step_func(preload => {
Why the empty string here? Maybe fold it into the invalid values above and test
only 3 things here?

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:155: if
(media.readyState >= HTMLMediaElement.HAVE_METADATA) {
This should be impossible at this point. assert_equals(media.readyState,
HTMLMediaElement.HAVE_NOTHING) before the switch to make this clear.

[mlamouri (slow - plz ping), 2016-11-03 14:56:16]

Description was changed from

==========
Allow metadata preload on cellular connections and disallow autoplay muted for
low end devices.

This allows metadata preload on cellular connections unless some exceptions
are met such as data saver is enabled or a low end device is being used.

In addition, low end devices are not allowed to autoplay in order to avoid
inconsistencies.

BUG=658313
==========

to

==========
Allow metadata preload on cellular connections and disallow autoplay muted for
low end devices.

This allows metadata preload on cellular connections unless some exceptions
are met such as data saver is enabled or a low end device is being used.

In addition, low end devices are not allowed to autoplay in order to avoid
inconsistencies.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/_fxvn-ny2jA/KqlmhamV...

BUG=658313
==========

[mlamouri (slow - plz ping), 2016-11-03 20:02:32]

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html
(right):

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html:66: var
asyncTests = [];
On 2016/11/03 at 10:14:40, foolip wrote:
> asyncTests = tests.map(test => async_test('Testing autoplay behaviour for ' +
test.name));

Done.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html:80:
internals.settings.setDataSaverEnabled(false);
On 2016/11/03 at 10:14:40, foolip wrote:
> If it matters that these are reset when the test is done, you can add a
t.add_cleanup in the test instead, so that it'll get done even if the tests fail
or time out.

Done.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html:107:
document.body.appendChild(media);
On 2016/11/03 at 10:14:40, foolip wrote:
> Does appending make a difference?

No. But why not appending?

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/preload-conditions.html (right):

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:121:
assert_equals(media.getAttribute('preload'), null);
On 2016/11/03 at 10:14:40, foolip wrote:
> assert_false(media.hasAttribute('preload'))

Done.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:124: [ 'none',
'metadata', 'auto' ].forEach(t.step_func(preload => {
On 2016/11/03 at 10:14:41, foolip wrote:
> It's harmless, but here you don't need step_func, because this callback is run
synchronously. You could use `for (const preload of [ 'none', 'metadata', 'auto'
])` to avoid the question entirely.

Removed superfluous t.step_func.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:125: var expected =
test.expectations.allowed.indexOf(preload) != -1
On 2016/11/03 at 10:14:40, foolip wrote:
> Can use .includes(preload)

Youhou, didn't know this :)

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:136: var
expectedLoading = 4;
On 2016/11/03 at 10:14:40, foolip wrote:
> If it weren't for the global state in internals staying stable it'd be much
preferable to have separate tests for the below. But I don't have any good ideas
:/

We could have two tests but I don't think it would help much.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:137: [ '', 'none',
'metadata', 'auto' ].forEach(t.step_func(preload => {
On 2016/11/03 at 10:14:40, foolip wrote:
> Why the empty string here? Maybe fold it into the invalid values above and
test only 3 things here?

The empty string is to make sure that when we expose a "default" behaviour we
actually do the appropriate loading.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:155: if
(media.readyState >= HTMLMediaElement.HAVE_METADATA) {
On 2016/11/03 at 10:14:40, foolip wrote:
> This should be impossible at this point. assert_equals(media.readyState,
HTMLMediaElement.HAVE_NOTHING) before the switch to make this clear.

Done.

[mlamouri (slow - plz ping), 2016-11-03 20:03:47]

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

[commit-bot: I haz the power, 2016-11-03 20:04:23]

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

[foolip, 2016-11-03 20:25:57]

Still lgtm

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html
(right):

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/autoplay-muted-conditions.html:107:
document.body.appendChild(media);
On 2016/11/03 20:02:32, mlamouri wrote:
> On 2016/11/03 at 10:14:40, foolip wrote:
> > Does appending make a difference?
> 
> No. But why not appending?

It's fine if you like it better, but in general I try to remove anything that
isn't required to exercise the behavior under test.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/preload-conditions.html (right):

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:136: var
expectedLoading = 4;
On 2016/11/03 20:02:32, mlamouri wrote:
> On 2016/11/03 at 10:14:40, foolip wrote:
> > If it weren't for the global state in internals staying stable it'd be much
> preferable to have separate tests for the below. But I don't have any good
ideas
> :/
> 
> We could have two tests but I don't think it would help much.

Yeah, that wouldn't remove the extra book keeping.

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:137: [ '', 'none',
'metadata', 'auto' ].forEach(t.step_func(preload => {
On 2016/11/03 20:02:32, mlamouri wrote:
> On 2016/11/03 at 10:14:40, foolip wrote:
> > Why the empty string here? Maybe fold it into the invalid values above and
> test only 3 things here?
> 
> The empty string is to make sure that when we expose a "default" behaviour we
> actually do the appropriate loading.

OK, maybe add '' to the 'not allowed' values above to make it more obvious that
it maps to one of the other three states.

[commit-bot: I haz the power, 2016-11-03 21:57:59]

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

[commit-bot: I haz the power, 2016-11-03 21:58:00]

Dry run: 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), 2016-11-09 19:34:44]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-11-09 19:34:44]

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

[mlamouri (slow - plz ping), 2016-11-09 19:34:53]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-11-09 19:42:05]

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/preload-conditions.html (right):

https://codereview.chromium.org/2466273006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/preload-conditions.html:137: [ '', 'none',
'metadata', 'auto' ].forEach(t.step_func(preload => {
On 2016/11/03 at 20:25:57, foolip wrote:
> On 2016/11/03 20:02:32, mlamouri wrote:
> > On 2016/11/03 at 10:14:40, foolip wrote:
> > > Why the empty string here? Maybe fold it into the invalid values above and
> > test only 3 things here?
> > 
> > The empty string is to make sure that when we expose a "default" behaviour
we
> > actually do the appropriate loading.
> 
> OK, maybe add '' to the 'not allowed' values above to make it more obvious
that it maps to one of the other three states.

Done.

[mlamouri (slow - plz ping), 2016-11-09 19:43:14]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-11-09 19:43:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, dalecurtis@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2466273006/#ps40001
(title: "add not allowed value")

[commit-bot: I haz the power, 2016-11-09 19:43:35]

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-11-09 21:37:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-09 21:52:23]

Description was changed from

==========
Allow metadata preload on cellular connections and disallow autoplay muted for
low end devices.

This allows metadata preload on cellular connections unless some exceptions
are met such as data saver is enabled or a low end device is being used.

In addition, low end devices are not allowed to autoplay in order to avoid
inconsistencies.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/_fxvn-ny2jA/KqlmhamV...

BUG=658313
==========

to

==========
Allow metadata preload on cellular connections and disallow autoplay muted for
low end devices.

This allows metadata preload on cellular connections unless some exceptions
are met such as data saver is enabled or a low end device is being used.

In addition, low end devices are not allowed to autoplay in order to avoid
inconsistencies.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/_fxvn-ny2jA/KqlmhamV...

BUG=658313

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

[commit-bot: I haz the power, 2016-11-09 21:52:24]

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