Don't remove the gesture requirement in the autoplay experiment.

Don't remove the gesture requirement in the autoplay experiment.

Instead of permanently unlocking the media element when the autoplay
experiment permits autoplay, leave the element locked and override
the gesture requirement for the particular play request.

This also turns off deferred playback due to mute changes.  Instead,
checks for muted media are applied only when the playback is
attempted.  Only visibility requirements are deferred.

BUG=608341
Committed: https://crrev.com/1301c3d6b3ea7b0ace8849580eaecd311c83b7bb
Cr-Commit-Position: refs/heads/master@{#394558}

[liberato (no reviews please), 2016-05-04 16:54:22]

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

[liberato (no reviews please), 2016-05-04 16:54:22]

this was more involved than i was expecting.  please pay particular attention to
HTMLMediaElement::play to see if i'm handling the promise correctly.

thanks
-fl

[mlamouri (slow - plz ping), 2016-05-05 13:45:40]

I had a quick look but a lot of things to digest here. Will looked again later.
You might want to have a look at the failing DCHECK :)

https://codereview.chromium.org/1949633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1949633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2052: if
(m_autoplayHelper->isPlaybackDeferred())
I'm a bit confused. What does that mean? When will we hit this case?

https://codereview.chromium.org/1949633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3668: void
HTMLMediaElement::removeUserGestureRequirement()
Should this be renamed?

[liberato (no reviews please), 2016-05-05 17:48:57]

https://codereview.chromium.org/1949633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1949633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2052: if
(m_autoplayHelper->isPlaybackDeferred())
On 2016/05/05 13:45:40, Mounir Lamouri wrote:
> I'm a bit confused. What does that mean? When will we hit this case?

if the element needs a user gesture, and is off-screen, and we require
visibility to override, and play() is called, then we'll hit this case.  we hold
the promise until it scrolls on-screen.

what is the desired behavior?

https://codereview.chromium.org/1949633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3668: void
HTMLMediaElement::removeUserGestureRequirement()
On 2016/05/05 13:45:40, Mounir Lamouri wrote:
> Should this be renamed?

good point, unlockWithUserGesture().

[mlamouri (slow - plz ping), 2016-05-06 15:05:47]

It seems overly complicated :( Is there a way we can reduce the complexity of
the CL? I would gladly l-g-t-m if you have no idea how.

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:352: if
(client().isLockedPendingUserGesture()) {
if (!client().isLockedPendingUserGesture())
    return;

[...]

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:361: if
(client().isLockedPendingUserGesture())
ditto

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:447: if
(!UserGestureIndicator::processingUserGesture()) {
Slightly out of topic but if we don't preload, wouldn't playbackStarted() be
called when we have enough data to play?

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:146: void
playMethodCalled(bool playing);
Could you add a `virtual bool paused() const = 0;` to the Client interface
instead? I think it would be better than passing the Client's state as a
function paramater.

[liberato (no reviews please), 2016-05-17 21:24:33]

i can't come up with anything that's much simpler.  the key problem is that the
original code relied on the locked / unlocked state of the element too much. 
now, all that has to be kept inside AutoplayExperimentHelper.

thanks
-fl

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:352: if
(client().isLockedPendingUserGesture()) {
On 2016/05/06 15:05:47, Mounir Lamouri wrote:
> if (!client().isLockedPendingUserGesture())
>     return;
> 
> [...]

Done.

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:361: if
(client().isLockedPendingUserGesture())
On 2016/05/06 15:05:47, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:447: if
(!UserGestureIndicator::processingUserGesture()) {
On 2016/05/06 15:05:47, Mounir Lamouri wrote:
> Slightly out of topic but if we don't preload, wouldn't playbackStarted() be
> called when we have enough data to play?

wow, good catch.  i'll cache this on play() and other places for the next time
playbackStarted() is called.

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1949633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:146: void
playMethodCalled(bool playing);
On 2016/05/06 15:05:47, Mounir Lamouri wrote:
> Could you add a `virtual bool paused() const = 0;` to the Client interface
> instead? I think it would be better than passing the Client's state as a
> function paramater.

Done.

[mlamouri (slow - plz ping), 2016-05-18 10:41:10]

rs-lgtm to unblock then. I will have a look after it lands and see if I can come
up with some refactoring.

[liberato (no reviews please), 2016-05-18 16:33:00]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2016-05-18 16:33:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1949633002/#ps140001
(title: "fixed test expectations.")

[commit-bot: I haz the power, 2016-05-18 16:33:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1949633002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1949633002/140001

[commit-bot: I haz the power, 2016-05-18 16:37:47]

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

[commit-bot: I haz the power, 2016-05-18 16:37:49]

Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[liberato (no reviews please), 2016-05-18 20:56:13]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2016-05-18 20:56:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1949633002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1949633002/140001

[commit-bot: I haz the power, 2016-05-18 21:44:07]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-18 21:45:29]

Description was changed from

==========
Don't remove the gesture requirement in the autoplay experiment.

Instead of permanently unlocking the media element when the autoplay
experiment permits autoplay, leave the element locked and override
the gesture requirement for the particular play request.

This also turns off deferred playback due to mute changes.  Instead,
checks for muted media are applied only when the playback is
attempted.  Only visibility requirements are deferred.

BUG=608341
==========

to

==========
Don't remove the gesture requirement in the autoplay experiment.

Instead of permanently unlocking the media element when the autoplay
experiment permits autoplay, leave the element locked and override
the gesture requirement for the particular play request.

This also turns off deferred playback due to mute changes.  Instead,
checks for muted media are applied only when the playback is
attempted.  Only visibility requirements are deferred.

BUG=608341

Committed: https://crrev.com/1301c3d6b3ea7b0ace8849580eaecd311c83b7bb
Cr-Commit-Position: refs/heads/master@{#394558}
==========

[commit-bot: I haz the power, 2016-05-18 21:45:31]

Patchset 8 (id:??) landed as
https://crrev.com/1301c3d6b3ea7b0ace8849580eaecd311c83b7bb
Cr-Commit-Position: refs/heads/master@{#394558}