Use utilizeUserGesture() everywhere in HTMLMediaElement

Use utilizeUserGesture() everywhere in HTMLMediaElement

Drive-by: Make shouldAutoplay() const.

BUG=617598
R=avayvod@chromium.org,rbyers@chromium.org

[foolip, 2016-11-23 12:37:17]

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

[commit-bot: I haz the power, 2016-11-23 12:37: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-23 13:51:39]

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

[commit-bot: I haz the power, 2016-11-23 13:51:40]

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

[foolip, 2016-11-23 19:07:26]

Description was changed from

==========
Use UserGestureIndicator::utilizeUserGesture() for unmuting media

BUG=617598
R=avayvod@chromium.org,rbyers@chromium.org
==========

to

==========
Use utilizeUserGesture() everywhere in HTMLMediaElement

BUG=617598
R=avayvod@chromium.org,rbyers@chromium.org
==========

[foolip, 2016-11-23 19:21:30]

Description was changed from

==========
Use utilizeUserGesture() everywhere in HTMLMediaElement

BUG=617598
R=avayvod@chromium.org,rbyers@chromium.org
==========

to

==========
Use utilizeUserGesture() everywhere in HTMLMediaElement

Drive-by: Make shouldAutoplay() const.

BUG=617598
R=avayvod@chromium.org,rbyers@chromium.org
==========

[whywhat, 2016-11-23 20:43:49]

lgtm

[Rick Byers, 2016-11-25 15:51:59]

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

https://codereview.chromium.org/2529593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2150: if
(UserGestureIndicator::utilizeUserGesture()) {
This is just a semantics-preserving cleanup, right?

Like below, it's not entirely clear to me that it's what we want from a metrics
perspective since we may still play() just fine even when there is no user
gesture.

Maybe utilizeUserGesture should be renamed to something like requireUserGesture
or something to make it clear that it's intended to be a signal that a gesture
was definitely required for some functionality?

https://codereview.chromium.org/2529593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2353: if
(UserGestureIndicator::utilizeUserGesture())
It's not clear to me that utilizeUserGesture is appropriate here.  It means a
user gesture was really required for some functionality (and if we change it so
there is no longer a user gesture, we're going to break the page).  I don't know
this code at all, but it looks like setMuted still does useful things even when
there's no user gesture, right?

Eg. setMuted can apparently be used to mute a video without requiring a user
gesture.  So if we later made a change where muting a video was less likely to
have a user gesture, would we really want that recorded as a compat risk?

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

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

[foolip, 2016-11-28 15:13:06]

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

https://codereview.chromium.org/2529593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2150: if
(UserGestureIndicator::utilizeUserGesture()) {
On 2016/11/25 15:51:59, Rick Byers wrote:
> This is just a semantics-preserving cleanup, right?
> 
> Like below, it's not entirely clear to me that it's what we want from a
metrics
> perspective since we may still play() just fine even when there is no user
> gesture.
> 
> Maybe utilizeUserGesture should be renamed to something like
requireUserGesture
> or something to make it clear that it's intended to be a signal that a gesture
> was definitely required for some functionality?

This preserved behavior, yes.

If utilizeUserGesture is used to estimate risk of changing what is considered a
user gesture, then this could be rewritten to something like this:

if (isGestureNeededForPlayback()) {
  if (UserGestureIndicator::utilizeUserGesture()) {
    unlockUserGesture();
  } else {
    // error case
  }
}

Would that match the intended semantics of utilizeUserGesture()?

https://codereview.chromium.org/2529593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2353: if
(UserGestureIndicator::utilizeUserGesture())
On 2016/11/25 15:51:59, Rick Byers wrote:
> It's not clear to me that utilizeUserGesture is appropriate here.  It means a
> user gesture was really required for some functionality (and if we change it
so
> there is no longer a user gesture, we're going to break the page).  I don't
know
> this code at all, but it looks like setMuted still does useful things even
when
> there's no user gesture, right?
> 
> Eg. setMuted can apparently be used to mute a video without requiring a user
> gesture.  So if we later made a change where muting a video was less likely to
> have a user gesture, would we really want that recorded as a compat risk?

The documentation says "consider calling utilizeUserGesture instead if you know
for sure that the return value will have an effect".

I would take that as "use if the difference is web observable" and not "use if
the difference is web breaking".

This is web observable, but changing it is quite unlikely to break pages that
can already cope with play() sometimes failing.

[mlamouri (slow - plz ping), 2016-11-29 10:00:07]

foolip@, wouldn't this change break web pages creating multiple media elements
for the purpose of playlists?

[foolip, 2016-11-29 10:03:07]

On 2016/11/29 10:00:07, mlamouri (OOO until 29-11) wrote:
> foolip@, wouldn't this change break web pages creating multiple media elements
> for the purpose of playlists?

If so I've done something wrong, I intended for the change to not be observable
but simply use the utilizeUserGesture() and processingUserGesture() as intended.
I probably didn't get the right intention from the documentation, awaiting
rbyers@'s feedback on that :)

[mlamouri (slow - plz ping), 2016-11-29 10:10:56]

On 2016/11/29 at 10:03:07, foolip wrote:
> On 2016/11/29 10:00:07, mlamouri (OOO until 29-11) wrote:
> > foolip@, wouldn't this change break web pages creating multiple media
elements
> > for the purpose of playlists?
> 
> If so I've done something wrong, I intended for the change to not be
observable but simply use the utilizeUserGesture() and processingUserGesture()
as intended. I probably didn't get the right intention from the documentation,
awaiting rbyers@'s feedback on that :)

My bad, I confused `utilizeUserGesture` and `consumeUserGesture`. These names
are way too close to eachother to make sense to me :)

[foolip, 2016-11-29 10:29:56]

On 2016/11/29 10:10:56, mlamouri (OOO until 29-11) wrote:
> On 2016/11/29 at 10:03:07, foolip wrote:
> > On 2016/11/29 10:00:07, mlamouri (OOO until 29-11) wrote:
> > > foolip@, wouldn't this change break web pages creating multiple media
> elements
> > > for the purpose of playlists?
> > 
> > If so I've done something wrong, I intended for the change to not be
> observable but simply use the utilizeUserGesture() and processingUserGesture()
> as intended. I probably didn't get the right intention from the documentation,
> awaiting rbyers@'s feedback on that :)
> 
> My bad, I confused `utilizeUserGesture` and `consumeUserGesture`. These names
> are way too close to eachother to make sense to me :)

Yes. I thought the same thing, which is why when reviewing
https://codereview.chromium.org/2510353004 I went digging in history for how it
could make sense, and found that it's this new thing.

Not sure what better names would be, though.

[mlamouri (slow - plz ping), 2016-12-01 10:24:51]

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

https://codereview.chromium.org/2529593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2150: if
(UserGestureIndicator::utilizeUserGesture()) {
On 2016/11/28 at 15:13:05, foolip wrote:
> On 2016/11/25 15:51:59, Rick Byers wrote:
> > This is just a semantics-preserving cleanup, right?
> > 
> > Like below, it's not entirely clear to me that it's what we want from a
metrics
> > perspective since we may still play() just fine even when there is no user
> > gesture.
> > 
> > Maybe utilizeUserGesture should be renamed to something like
requireUserGesture
> > or something to make it clear that it's intended to be a signal that a
gesture
> > was definitely required for some functionality?
> 
> This preserved behavior, yes.
> 
> If utilizeUserGesture is used to estimate risk of changing what is considered
a user gesture, then this could be rewritten to something like this:
> 
> if (isGestureNeededForPlayback()) {
>   if (UserGestureIndicator::utilizeUserGesture()) {
>     unlockUserGesture();
>   } else {
>     // error case
>   }
> }
> 
> Would that match the intended semantics of utilizeUserGesture()?

I'm not Rick, but reading the header file, I would say yes. zqzhang@ is working
on a CL that removes the autoplay experiment object which should help making the
logic similar as you suggested.

https://codereview.chromium.org/2529593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2353: if
(UserGestureIndicator::utilizeUserGesture())
On 2016/11/28 at 15:13:05, foolip wrote:
> On 2016/11/25 15:51:59, Rick Byers wrote:
> > It's not clear to me that utilizeUserGesture is appropriate here.  It means
a
> > user gesture was really required for some functionality (and if we change it
so
> > there is no longer a user gesture, we're going to break the page).  I don't
know
> > this code at all, but it looks like setMuted still does useful things even
when
> > there's no user gesture, right?
> > 
> > Eg. setMuted can apparently be used to mute a video without requiring a user
> > gesture.  So if we later made a change where muting a video was less likely
to
> > have a user gesture, would we really want that recorded as a compat risk?
> 
> The documentation says "consider calling utilizeUserGesture instead if you
know for sure that the return value will have an effect".
> 
> I would take that as "use if the difference is web observable" and not "use if
the difference is web breaking".
> 
> This is web observable, but changing it is quite unlikely to break pages that
can already cope with play() sometimes failing.

That one is harder because we eagerly unlock but don't care much. You could
check if the element is locked then call utilize if it is? ie.: `if
(isLockedPendingUserGesture() && UserGestureIndicator::utiliseUserGesture())`

[Rick Byers, 2016-12-16 22:19:07]

On 2016/12/01 10:24:51, mlamouri wrote:
>
https://codereview.chromium.org/2529593002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/2529593002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2150: if
> (UserGestureIndicator::utilizeUserGesture()) {
> On 2016/11/28 at 15:13:05, foolip wrote:
> > On 2016/11/25 15:51:59, Rick Byers wrote:
> > > This is just a semantics-preserving cleanup, right?
> > > 
> > > Like below, it's not entirely clear to me that it's what we want from a
> metrics
> > > perspective since we may still play() just fine even when there is no user
> > > gesture.
> > > 
> > > Maybe utilizeUserGesture should be renamed to something like
> requireUserGesture
> > > or something to make it clear that it's intended to be a signal that a
> gesture
> > > was definitely required for some functionality?
> > 
> > This preserved behavior, yes.
> > 
> > If utilizeUserGesture is used to estimate risk of changing what is
considered
> a user gesture, then this could be rewritten to something like this:
> > 
> > if (isGestureNeededForPlayback()) {
> >   if (UserGestureIndicator::utilizeUserGesture()) {
> >     unlockUserGesture();
> >   } else {
> >     // error case
> >   }
> > }
> > 
> > Would that match the intended semantics of utilizeUserGesture()?
> 
> I'm not Rick, but reading the header file, I would say yes. zqzhang@ is
working
> on a CL that removes the autoplay experiment object which should help making
the
> logic similar as you suggested.

Sorry I missed this.

I designed the utilizeUserGesture stuff for simpler cases where things only work
when there is a user gesture in order to estimate the risk of eliminating user
gestures in some cases (eg. during a touch scroll).  The media cases seem more
complicated and I don't completely understand the model, but what you've got
above looks reasonable.

But my gesture changes are done so there's probably nobody actually generating
metrics from this right now so the details won't make much difference until the
next time someone wants to try eliminating a user gesture.  In general I err'd
on the side of reporting a user gesture as utilized only when I knew for sure
that the lack of gesture would break something so I could get a firm lower bound
on the risk.  Even then the metrics ended up being somewhat high yet my change
seems to going fine without reports of an issue (in 56 beta now - so probably
still TBD).  So perhaps the whole concept of trying to measure risk here was
flawed?  I'd be OK with removing the "utilize" infrastructure entirely - there's
definitely an argument that it's not worth the complexity.  Or we could wait
until 56 goes stable to have a better idea of how useful the metrics were in
practice.

>
https://codereview.chromium.org/2529593002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2353: if
> (UserGestureIndicator::utilizeUserGesture())
> On 2016/11/28 at 15:13:05, foolip wrote:
> > On 2016/11/25 15:51:59, Rick Byers wrote:
> > > It's not clear to me that utilizeUserGesture is appropriate here.  It
means
> a
> > > user gesture was really required for some functionality (and if we change
it
> so
> > > there is no longer a user gesture, we're going to break the page).  I
don't
> know
> > > this code at all, but it looks like setMuted still does useful things even
> when
> > > there's no user gesture, right?
> > > 
> > > Eg. setMuted can apparently be used to mute a video without requiring a
user
> > > gesture.  So if we later made a change where muting a video was less
likely
> to
> > > have a user gesture, would we really want that recorded as a compat risk?
> > 
> > The documentation says "consider calling utilizeUserGesture instead if you
> know for sure that the return value will have an effect".
> > 
> > I would take that as "use if the difference is web observable" and not "use
if
> the difference is web breaking".
> > 
> > This is web observable, but changing it is quite unlikely to break pages
that
> can already cope with play() sometimes failing.
> 
> That one is harder because we eagerly unlock but don't care much. You could
> check if the element is locked then call utilize if it is? ie.: `if
> (isLockedPendingUserGesture() && UserGestureIndicator::utiliseUserGesture())`

[foolip, 2016-12-16 23:21:45]

Thanks Rick, I'm just going to abandon this review then.