Autoplay experiment metric fixes and additions.

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 f3c4ad3860e506678b34be4e784a8ada27ae72ed..0001c14212f8937f80fdc8a8c087918eb4de322f 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -347,6 +347,8 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_audioSourceNode(nullptr)
 #endif
     , m_autoplayHelper(*this)
+    , m_autoplayDeferredMetric(GesturelessPlaybackUnknownReason)
+    , m_recordedElement(false)
 {
 #if ENABLE(OILPAN)
     ThreadState::current()->registerPreFinalizer(this);
@@ -669,12 +671,9 @@ String HTMLMediaElement::canPlayType(const String& mimeType, const String& keySy
     return canPlay;
 }
 
-void HTMLMediaElement::recordMetricsIfPausing()
+void HTMLMediaElement::recordMetricsBeforePause()
 {
-    // If not playing, then nothing to record.
-    // TODO(liberato): test metrics.  this was m_paused.
-    if (m_paused)
-        return;
+    ASSERT(!m_paused);
 
     const bool bailout = isBailout();
 
@@ -696,15 +695,25 @@ void HTMLMediaElement::recordMetricsIfPausing()
     }
 }
 
+void HTMLMediaElement::recordMetricsAtPlaybackEnd()
+{
+    recordAutoplayMetric(AnyPlaybackComplete);
+    if (m_initialPlayWithoutUserGesture) {
+        m_initialPlayWithoutUserGesture = false;
+        recordAutoplayMetric(AutoplayComplete);
+    }
+}
+
 void HTMLMediaElement::load()
 {
     WTF_LOG(Media, "HTMLMediaElement::load(%p)", this);
 
-    recordMetricsIfPausing();
+    if (!m_paused)
+        recordMetricsBeforePause();
 
     if (UserGestureIndicator::processingUserGesture() && m_userGestureRequiredForPlay) {
         recordAutoplayMetric(AutoplayEnabledThroughLoad);
-        m_userGestureRequiredForPlay = false;
+        removeUserGestureRequirement(GesturelessPlaybackEnabledByLoad);
         // While usergesture-initiated load()s technically count as autoplayed,
         // they don't feel like such to the users and hence we don't want to
         // count them for the purposes of metrics.
@@ -962,6 +971,7 @@ void HTMLMediaElement::loadResource(const KURL& url, ContentType& contentType, c
     if (url.protocolIs(mediaSourceBlobProtocol)) {
         if (isMediaStreamURL(url.string())) {
             m_userGestureRequiredForPlay = false;

[philipj_slow, 2015/11/26 15:30:56]

Drop this line, it looks like it would make removeUserGestureRequirement() a
no-op.

[liberato (no reviews please), 2015/12/02 00:58:08]

Done.

+            removeUserGestureRequirement(GesturelessPlaybackEnabledByStream);
         } else {
             m_mediaSource = HTMLMediaSource::lookup(url.string());
 
@@ -1533,24 +1543,32 @@ void HTMLMediaElement::setReadyState(ReadyState state)
     bool isPotentiallyPlaying = potentiallyPlaying();
     if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA && tracksAreReady) {
         scheduleEvent(EventTypeNames::canplay);
-        if (isPotentiallyPlaying)
+        if (isPotentiallyPlaying) {
             scheduleEvent(EventTypeNames::playing);
+        }
         shouldUpdateDisplayState = true;
     }
 
     if (m_readyState == HAVE_ENOUGH_DATA && oldState < HAVE_ENOUGH_DATA && tracksAreReady) {
         if (oldState <= HAVE_CURRENT_DATA) {
             scheduleEvent(EventTypeNames::canplay);
-            if (isPotentiallyPlaying)
+            if (isPotentiallyPlaying) {
                 scheduleEvent(EventTypeNames::playing);
+            }
         }
 
         // Check for autoplay, and record metrics about it if needed.
-        if (shouldAutoplay(RecordMetricsBehavior::DoRecord)) {
+        if (shouldAutoplay(RecordMetricsBehavior::RecordOnSandboxFailure)) {
             // If the autoplay experiment says that it's okay to play now,
             // then don't require a user gesture.
             m_autoplayHelper.becameReadyToPlay();
 
+            // Record that we have encounted autoplay media.  We do this
+            // now, after the experiment has had a chance to override the
+            // user gesture requirement, since it assumes that the media
+            // will play if and only if a user gesture is not required.
+            autoplayMediaEncountered();
+
             if (!m_userGestureRequiredForPlay) {
                 m_paused = false;
                 invalidateCachedTime();
@@ -1870,12 +1888,13 @@ bool HTMLMediaElement::autoplay() const
 bool HTMLMediaElement::shouldAutoplay(const RecordMetricsBehavior recordMetrics)
 {
     if (m_autoplaying && m_paused && autoplay()) {
-        if (recordMetrics == RecordMetricsBehavior::DoRecord)
-            autoplayMediaEncountered();
-
         if (document().isSandboxed(SandboxAutomaticFeatures)) {
-            if (recordMetrics == RecordMetricsBehavior::DoRecord)
+            if (recordMetrics == RecordMetricsBehavior::RecordOnSandboxFailure) {
+                // We record autoplayMediaEncountered here because we know
+                // that the autoplay attempt will fail.
+                autoplayMediaEncountered();
                 recordAutoplayMetric(AutoplayDisabledBySandbox);
+            }
             return false;
         }
 
@@ -1964,6 +1983,10 @@ void HTMLMediaElement::play()
     } else if (m_userGestureRequiredForPlay) {
         if (m_autoplayMediaCounted)
             recordAutoplayMetric(AutoplayManualStart);
+        // Don't let future gestureless playbacks affect metrics.  This is
+        // also why we don't have to go through removeUserGestureRequirement.
+        m_autoplayMediaCounted = true;
+
         m_userGestureRequiredForPlay = false;

[philipj_slow, 2015/11/26 15:30:56]

Call recordAutoplayMetric(GesturelessPlaybackEnabledByPlay)? I don't think you
should need an unknown case, we know exactly what removes the gesture if we just
track it.

[liberato (no reviews please), 2015/12/02 00:58:09]

updated to removeUserGestureRequirement.  if gestureless playback starts later,
then we'll record why it was allowed.  i have to test this better tomorrow when
i'm in the office.

     }
 
@@ -1989,12 +2012,12 @@ void HTMLMediaElement::playInternal()
         invalidateCachedTime();
         scheduleEvent(EventTypeNames::play);
 
-        if (m_readyState <= HAVE_CURRENT_DATA)
+        if (m_readyState <= HAVE_CURRENT_DATA) {

[philipj_slow, 2015/11/26 15:30:56]

Drop the added braces here and elsewhere: git checkout -p origin/master

[liberato (no reviews please), 2015/12/02 00:58:08]

Done.

             scheduleEvent(EventTypeNames::waiting);
-        else if (m_readyState >= HAVE_FUTURE_DATA)
+        } else if (m_readyState >= HAVE_FUTURE_DATA) {
             scheduleEvent(EventTypeNames::playing);
+        }
     }
-    m_autoplaying = false;
 
     updatePlayState();
 }
@@ -2005,8 +2028,12 @@ void HTMLMediaElement::autoplayMediaEncountered()
         m_autoplayMediaCounted = true;
         recordAutoplayMetric(AutoplayMediaFound);
 
-        if (!m_userGestureRequiredForPlay)
+        // If no user gesture was required, then assume that playback will
+        // actually start.
+        if (!m_userGestureRequiredForPlay) {
             m_initialPlayWithoutUserGesture = true;
+            recordAutoplayMetric(m_autoplayDeferredMetric);

[philipj_slow, 2015/11/26 15:30:56]

I'm still finding this a bit hard to follow. In general, I would like to see
this:

1. All state is stored on AutoplayExperimentHelper, including the existing
m_initialPlayWithoutUserGesture and m_autoplayMediaCounted.
2. HTMLMediaElement calls methods to set relevant state, like
m_autoplayHelper.userGestureNotRequired(reason).
3. HTMLMediaElement calls the helper at points where it might want to record
metrics based on that state, like the existing
m_autoplayHelper.playMethodCalled() or
m_autoplayHelper.playerPlaying()/m_autoplayHelper.playerPaused() in
updatePlayState().

I think that would make the footprint in HTMLMediaElement a lot smaller, and
hopefully reduce the amount of if (m_this) { m_that = true } that's hard to
track, by just recording what events have occurred and laying the puzzle only
the place where the metrics are actually recorded.

If that makes it easier to write unit tests (for AutoplayExperimentHelper)
that'd be a bonus.

[liberato (no reviews please), 2015/12/02 00:58:08]

oh, i didn't realize that you were suggesting to refactor all of it earlier.
sure, that makes sense.

+        }
     }
 }
 
@@ -2031,7 +2058,7 @@ void HTMLMediaElement::pause()
     m_autoplaying = false;
 
     if (!m_paused) {
-        recordMetricsIfPausing();
+        recordMetricsBeforePause();
 
         m_paused = true;
         scheduleTimeupdateEvent(false);
@@ -2724,6 +2751,7 @@ void HTMLMediaElement::timeChanged()
             // forwards, and paused is false,
             if (!m_paused) {
                 // changes paused to true and fires a simple event named pause at the media element.
+                recordMetricsAtPlaybackEnd();
                 m_paused = true;
                 scheduleEvent(EventTypeNames::pause);
             }
@@ -2732,7 +2760,6 @@ void HTMLMediaElement::timeChanged()
                 m_sentEndEvent = true;
                 scheduleEvent(EventTypeNames::ended);
             }
-            recordMetricsIfPausing();
         }
     } else {
         m_sentEndEvent = false;
@@ -2939,17 +2966,19 @@ void HTMLMediaElement::updatePlayState()
             webMediaPlayer()->setRate(playbackRate());
             updateVolume();
             webMediaPlayer()->play();
+            recordAutoplayMetric(AnyPlaybackStarted);
         }
 
         if (mediaControls())
             mediaControls()->playbackStarted();
         startPlaybackProgressTimer();
         m_playing = true;
-        recordAutoplayMetric(AnyPlaybackStarted);
+        m_autoplaying = false;

[liberato (no reviews please), 2015/11/25 18:58:54]

i moved this from playInternal().  not sure that this is per-spec (it only
mentions removing autoplay on a play() call).  otherwise, though, letting a
video element with an autoplay attribute play to completion, then seeking to the
middle, starts playback immediately when updating the network state.  that seems
really weird.

[philipj_slow, 2015/11/26 15:30:56]

Nice catch! Fixed in https://github.com/whatwg/html/pull/362

Can you fix this in a separate CL with a test suitable for upstreaming to
web-platform-tests? hopefully there's some really short media resource you can
use, and if not you could always use two samples of silence PCM as a data: URL.

[liberato (no reviews please), 2015/12/02 00:58:09]

https://codereview.chromium.org/1482393003/

 
     } else { // Should not be playing right now
         if (isPlaying)
             webMediaPlayer()->pause();
+
         refreshCachedTime();
 
         m_playbackProgressTimer.stop();
@@ -3065,7 +3094,8 @@ void HTMLMediaElement::stop()
 {
     WTF_LOG(Media, "HTMLMediaElement::stop(%p)", this);
 
-    recordMetricsIfPausing();
+    if (!m_paused)
+        recordMetricsBeforePause();

[philipj_slow, 2015/11/26 15:30:56]

As I think I pointed out in an earlier review, this is ActiveDOMObject::stop()
and it's probably not a good idea to consider it like a user-initiated pause.
Maybe this will be reached if the tab was closed, but I don't think so.

[liberato (no reviews please), 2015/12/02 00:58:08]

Done.

 
     // Close the async event queue so that no events are enqueued by userCancelledLoad.
     cancelPendingEventsAndCallbacks();
@@ -3575,14 +3605,12 @@ bool HTMLMediaElement::isUserGestureRequiredForPlay() const
     return m_userGestureRequiredForPlay;
 }
 
-void HTMLMediaElement::removeUserGestureRequirement()
+void HTMLMediaElement::removeUserGestureRequirement(AutoplayMetrics deferredMetric)
 {
-    m_userGestureRequiredForPlay = false;
-}
-
-void HTMLMediaElement::setInitialPlayWithoutUserGestures(bool value)
-{
-    m_initialPlayWithoutUserGesture = value;
+    if (m_userGestureRequiredForPlay) {
+        m_userGestureRequiredForPlay = false;
+        m_autoplayDeferredMetric = deferredMetric;
+    }
 }
 
 void HTMLMediaElement::setNetworkState(NetworkState state)
@@ -3591,6 +3619,12 @@ void HTMLMediaElement::setNetworkState(NetworkState state)
         m_networkState = state;
         if (MediaControls* controls = mediaControls())
             controls->networkStateChanged();
+
+        if (m_networkState == NETWORK_LOADING && !m_recordedElement) {

[philipj_slow, 2015/11/26 15:30:56]

Why here instead of in HTMLMediaElement::loadResource at the beginning of the
resource fetch algorithm?

[liberato (no reviews please), 2015/12/02 00:58:09]

Done.

+            m_recordedElement = true;
+            recordAutoplayMetric(isHTMLVideoElement()
+                ? AnyVideoElement : AnyAudioElement);
+        }
     }
 }