Use utilizeUserGesture() everywhere in HTMLMediaElement

third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
index 26a0c879ca69182999a93fcefe9c9d1258c13e6b..5c322c5665496a0f19ab9ec89bd5852851f5dab9 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -2035,7 +2035,7 @@ bool HTMLMediaElement::autoplay() const {
   return fastHasAttribute(autoplayAttr);
 }
 
-bool HTMLMediaElement::shouldAutoplay() {
+bool HTMLMediaElement::shouldAutoplay() const {
   if (document().isSandboxed(SandboxAutomaticFeatures))
     return false;
   return m_autoplaying && m_paused && autoplay();
@@ -2147,7 +2147,9 @@ ScriptPromise HTMLMediaElement::playForBindings(ScriptState* scriptState) {
 Nullable<ExceptionCode> HTMLMediaElement::play() {
   BLINK_MEDIA_LOG << "play(" << (void*)this << ")";
 
-  if (!UserGestureIndicator::processingUserGesture()) {
+  if (UserGestureIndicator::utilizeUserGesture()) {

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

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?

[foolip, 2016/11/28 15:13:05]

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()?

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

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.

+    unlockUserGesture();
+  } else {
     m_autoplayUmaHelper->onAutoplayInitiated(AutoplaySource::Method);
     if (isGestureNeededForPlayback()) {
       // If we're already playing, then this play would do nothing anyway.
@@ -2164,9 +2166,6 @@ Nullable<ExceptionCode> HTMLMediaElement::play() {
           JSMessageSource, WarningMessageLevel, message));
       return NotAllowedError;
     }
-  } else {
-    UserGestureIndicator::utilizeUserGesture();
-    unlockUserGesture();
   }
 
   if (m_error && m_error->code() == MediaError::kMediaErrSrcNotSupported)
@@ -2351,7 +2350,7 @@ void HTMLMediaElement::setMuted(bool muted) {
   bool wasPendingAutoplayMuted = m_autoplayVisibilityObserver && paused() &&
                                  m_muted && isLockedPendingUserGesture();
 
-  if (UserGestureIndicator::processingUserGesture())
+  if (UserGestureIndicator::utilizeUserGesture())

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

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?

[foolip, 2016/11/28 15:13:05]

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/12/01 10:24:50]

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())`

     unlockUserGesture();
 
   m_muted = muted;