Use utilizeUserGesture() everywhere in HTMLMediaElement

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

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights
  * reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 2017 matching lines @@
   // 4.8.10.8 Playing the media resource
   // The ended attribute must return true if the media element has ended
   // playback and the direction of playback is forwards, and false otherwise.
   return endedPlayback() && getDirectionOfPlayback() == Forward;
 }
 
 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();
 }
 
 String HTMLMediaElement::preload() const {
   return preloadTypeToString(preloadType());
 }
 
 void HTMLMediaElement::setPreload(const AtomicString& preload) {
@@ skipping 91 matching lines @@
     resolver->reject(DOMException::create(code.get(), message));
     return promise;
   }
 
   return promise;
 }
 
 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.
       // Call playInternal to handle scheduling the promise resolution.
       if (!m_paused) {
         playInternal();
         return nullptr;
       }
 
       String message = ExceptionMessages::failedToExecute(
           "play", "HTMLMediaElement",
           "API can only be initiated by a user gesture.");
       document().addConsoleMessage(ConsoleMessage::create(
           JSMessageSource, WarningMessageLevel, message));
       return NotAllowedError;
     }
-  } else {
-    UserGestureIndicator::utilizeUserGesture();
-    unlockUserGesture();
   }
 
   if (m_error && m_error->code() == MediaError::kMediaErrSrcNotSupported)
     return NotSupportedError;
 
   playInternal();
 
   return nullptr;
 }
 
@@ skipping 164 matching lines @@
   BLINK_MEDIA_LOG << "setMuted(" << (void*)this << ", " << boolString(muted)
                   << ")";
 
   if (m_muted == muted)
     return;
 
   bool wasAutoplayingMuted = isAutoplayingMuted();
   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;
 
   scheduleEvent(EventTypeNames::volumechange);
 
   // If an element autoplayed while muted, it needs to be unlocked to unmute,
   // otherwise, it will be paused.
   if (wasAutoplayingMuted) {
     if (isGestureNeededForPlayback()) {
@@ skipping 1665 matching lines @@
 
 DEFINE_TRACE(HTMLMediaElement::AudioClientImpl) {
   visitor->trace(m_client);
 }
 
 DEFINE_TRACE(HTMLMediaElement::AudioSourceProviderImpl) {
   visitor->trace(m_client);
 }
 
 }  // namespace blink