Pause autoplay muted video when unmuting if there's no user gesture

Pause autoplay muted video when unmuting if there's no user gesture

BUG=617598
TEST=https://avayvod.github.io/autoplay-test.html

Committed: https://crrev.com/73df13de2a2ba7d1d7e132a3934d4d39f1959b66
Cr-Commit-Position: refs/heads/master@{#399174}

[whywhat, 2016-06-08 12:45:56]

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

[whywhat, 2016-06-08 12:45:57]

PTaL

I feel like having isGestureNeeded... method un-const-ed and have a side effect
is not ideal but so far it was the best I could think of to avoid refactoring
the places it's called from and duplicating code.

[whywhat, 2016-06-08 12:46:18]

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

[commit-bot: I haz the power, 2016-06-08 12:46:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047333002/1

[whywhat, 2016-06-08 12:57:56]

Added layout test for pausing when unmute

[whywhat, 2016-06-08 13:01:20]

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

[commit-bot: I haz the power, 2016-06-08 13:02:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047333002/20001

[whywhat, 2016-06-08 15:16:58]

Added a test case for unmuting with gesture

[whywhat, 2016-06-08 15:17:35]

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

[commit-bot: I haz the power, 2016-06-08 15:18:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047333002/40001

[whywhat, 2016-06-08 15:19:12]

Fixed accidental ws change

[mlamouri (slow - plz ping), 2016-06-09 11:34:15]

What about having:
enum GestureRequirement {
  None,
  NoneBecauseMuted,
  Yes
}

GestureRequirement getGestureRequirement() const;

then code will do:
if (getGestureRequirement() != GestureRequirement::Yes) {
  if (getGestureRequirement() == GestureRequirement::NoneBecauseMuted) {
    m_autoplayingBecauseMUted = true;
  }
}

How silly does that sound?

+foolip@ in case he has opinions.

[foolip, 2016-06-09 11:41:42]

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

[foolip, 2016-06-09 11:41:43]

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

https://codereview.chromium.org/2047333002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2282: if (!muted &&
m_isAutoplayingMuted) {
As per https://github.com/whatwg/html/issues/976 I don't think should depend on
whether the reason that we're playing, but rather that when setting muted to
false and it was previously true and there is no user gesture, the internal
pause steps should be run.

That's easy to explain and avoids that extra state bit.

[mlamouri (slow - plz ping), 2016-06-09 13:17:13]

I'm really worried this would break way too many websites.

Actually, I wonder if we could avoid the extra state by simply checking if the
element is unlocked?

In ::setMuted(), we could change the mute state then run
isUserGestureRequired(). If a user gesture is required, we pause the playback.

WDYT?

[whywhat, 2016-06-09 13:51:01]

On 2016/06/09 at 13:17:13, mlamouri wrote:
> I'm really worried this would break way too many websites.
> 
> Actually, I wonder if we could avoid the extra state by simply checking if the
element is unlocked?
> 
> In ::setMuted(), we could change the mute state then run
isUserGestureRequired(). If a user gesture is required, we pause the playback.
> 
> WDYT?

It sounds like you and Philip suggest the same using slightly different wording?

[mlamouri (slow - plz ping), 2016-06-09 14:19:59]

On 2016/06/09 at 13:51:01, avayvod wrote:
> On 2016/06/09 at 13:17:13, mlamouri wrote:
> > I'm really worried this would break way too many websites.
> > 
> > Actually, I wonder if we could avoid the extra state by simply checking if
the element is unlocked?
> > 
> > In ::setMuted(), we could change the mute state then run
isUserGestureRequired(). If a user gesture is required, we pause the playback.
> > 
> > WDYT?
> 
> It sounds like you and Philip suggest the same using slightly different
wording?

Hmm, maybe I misunderstood Philip. What I assumed is that we would always
require a user gesture to unmute while what I mean is that we should only
require a gesture if non was ever provided. Maybe that was obvious for you two
and not for me :(

[whywhat, 2016-06-09 14:59:01]

Check isGestureNeeded when unmuting

[whywhat, 2016-06-09 15:00:22]

PTAL

[whywhat, 2016-06-09 15:00:35]

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

[commit-bot: I haz the power, 2016-06-09 15:01:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047333002/80001

[foolip, 2016-06-09 15:10:46]

On 2016/06/09 14:19:59, Mounir Lamouri (slow) wrote:
> On 2016/06/09 at 13:51:01, avayvod wrote:
> > On 2016/06/09 at 13:17:13, mlamouri wrote:
> > > I'm really worried this would break way too many websites.
> > > 
> > > Actually, I wonder if we could avoid the extra state by simply checking if
> the element is unlocked?
> > > 
> > > In ::setMuted(), we could change the mute state then run
> isUserGestureRequired(). If a user gesture is required, we pause the playback.
> > > 
> > > WDYT?
> > 
> > It sounds like you and Philip suggest the same using slightly different
> wording?
> 
> Hmm, maybe I misunderstood Philip. What I assumed is that we would always
> require a user gesture to unmute while what I mean is that we should only
> require a gesture if non was ever provided. Maybe that was obvious for you two
> and not for me :(

No, I really did mean always requiring a user gesture, otherwise we also have to
standardize where "requires user gesture" bit is stored. In Blink it's on
individual media elements, but WebKit has already switched to another system.

I asked about this in
https://github.com/whatwg/html/issues/976#issuecomment-223232285 to shake out
any assumptions around this, but communication is hard :)

I suggest that we discuss this in the coming spec PR so that we can also get Jer
to comment.

[foolip, 2016-06-09 15:31:32]

On 2016/06/09 15:10:46, Philip Jägenstedt wrote:
> On 2016/06/09 14:19:59, Mounir Lamouri (slow) wrote:
> > On 2016/06/09 at 13:51:01, avayvod wrote:
> > > On 2016/06/09 at 13:17:13, mlamouri wrote:
> > > > I'm really worried this would break way too many websites.
> > > > 
> > > > Actually, I wonder if we could avoid the extra state by simply checking
if
> > the element is unlocked?
> > > > 
> > > > In ::setMuted(), we could change the mute state then run
> > isUserGestureRequired(). If a user gesture is required, we pause the
playback.
> > > > 
> > > > WDYT?
> > > 
> > > It sounds like you and Philip suggest the same using slightly different
> > wording?
> > 
> > Hmm, maybe I misunderstood Philip. What I assumed is that we would always
> > require a user gesture to unmute while what I mean is that we should only
> > require a gesture if non was ever provided. Maybe that was obvious for you
two
> > and not for me :(
> 
> No, I really did mean always requiring a user gesture, otherwise we also have
to
> standardize where "requires user gesture" bit is stored. In Blink it's on
> individual media elements, but WebKit has already switched to another system.
> 
> I asked about this in
> https://github.com/whatwg/html/issues/976#issuecomment-223232285 to shake out
> any assumptions around this, but communication is hard :)
> 
> I suggest that we discuss this in the coming spec PR so that we can also get
Jer
> to comment.

So, I can also see the simplicity of using the exact same rules as for calls to
play(), having different rules isn't easy to explain either.

It is at least a theoretical problem that Chrome and Safari use different bits
of state here, but perhaps a better approach to that problem would be to follow
Safari and try to standardize if it seems like no variation is useful.

[whywhat, 2016-06-09 15:57:06]

On 2016/06/09 at 15:31:32, foolip wrote:
> On 2016/06/09 15:10:46, Philip Jägenstedt wrote:
> > On 2016/06/09 14:19:59, Mounir Lamouri (slow) wrote:
> > > On 2016/06/09 at 13:51:01, avayvod wrote:
> > > > On 2016/06/09 at 13:17:13, mlamouri wrote:
> > > > > I'm really worried this would break way too many websites.
> > > > > 
> > > > > Actually, I wonder if we could avoid the extra state by simply
checking if
> > > the element is unlocked?
> > > > > 
> > > > > In ::setMuted(), we could change the mute state then run
> > > isUserGestureRequired(). If a user gesture is required, we pause the
playback.
> > > > > 
> > > > > WDYT?
> > > > 
> > > > It sounds like you and Philip suggest the same using slightly different
> > > wording?
> > > 
> > > Hmm, maybe I misunderstood Philip. What I assumed is that we would always
> > > require a user gesture to unmute while what I mean is that we should only
> > > require a gesture if non was ever provided. Maybe that was obvious for you
two
> > > and not for me :(
> > 
> > No, I really did mean always requiring a user gesture, otherwise we also
have to
> > standardize where "requires user gesture" bit is stored. In Blink it's on
> > individual media elements, but WebKit has already switched to another
system.
> > 
> > I asked about this in
> > https://github.com/whatwg/html/issues/976#issuecomment-223232285 to shake
out
> > any assumptions around this, but communication is hard :)
> > 
> > I suggest that we discuss this in the coming spec PR so that we can also get
Jer
> > to comment.
> 
> So, I can also see the simplicity of using the exact same rules as for calls
to play(), having different rules isn't easy to explain either.
> 
> It is at least a theoretical problem that Chrome and Safari use different bits
of state here, but perhaps a better approach to that problem would be to follow
Safari and try to standardize if it seems like no variation is useful.

It seems like we would use the same rules as play for unmuting either way.
Changing the rule would change isGestureRequiredForPlayback() method but not the
logic introduced in setMuted(). I'll start the PR then.

[commit-bot: I haz the power, 2016-06-09 16:51:55]

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

[commit-bot: I haz the power, 2016-06-09 16:51:56]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-06-09 17:43:16]

lgtm with comments applied.

https://codereview.chromium.org/2047333002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/autoplay-muted.html (right):

https://codereview.chromium.org/2047333002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/autoplay-muted.html:37: {
style: { at end of previous line (here and other places in this file)

https://codereview.chromium.org/2047333002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/autoplay-muted.html:64: document.onclick =
t.step_func_done(function()
can you move this outside of the play() handler? just simulate the click in the
play handler.

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

https://codereview.chromium.org/2047333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2281: // Pause the
element when unmuting if there was no user gesture.
nit: s/if there was no user gesture./if the element is still locked./

[whywhat, 2016-06-10 10:52:38]

Addressed comments

[whywhat, 2016-06-10 10:52:49]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2016-06-10 10:52:49]

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/2047333002/#ps100001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-06-10 10:53:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047333002/100001

[commit-bot: I haz the power, 2016-06-10 14:14:44]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-10 14:16:06]

Description was changed from

==========
Pause autoplay muted video when unmuting if there's no user gesture

BUG=617598
TEST=https://avayvod.github.io/autoplay-test.html
==========

to

==========
Pause autoplay muted video when unmuting if there's no user gesture

BUG=617598
TEST=https://avayvod.github.io/autoplay-test.html

Committed: https://crrev.com/73df13de2a2ba7d1d7e132a3934d4d39f1959b66
Cr-Commit-Position: refs/heads/master@{#399174}
==========

[commit-bot: I haz the power, 2016-06-10 14:16:07]

Patchset 6 (id:??) landed as
https://crrev.com/73df13de2a2ba7d1d7e132a3934d4d39f1959b66
Cr-Commit-Position: refs/heads/master@{#399174}