Autoplay experiment metric fixes and additions.

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

Index: third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp
diff --git a/third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp b/third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp
index 65b7483b6f53aaf0b62550e01989c179d34ca972..081c7b3bc37c58baa7410239d5bca160e5e552d0 100644
--- a/third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp
+++ b/third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp
@@ -5,7 +5,6 @@
 #include "core/html/AutoplayExperimentHelper.h"
 
 #include "core/dom/Document.h"
-#include "core/frame/FrameView.h"
 #include "core/frame/Settings.h"
 #include "core/html/HTMLMediaElement.h"
 #include "core/layout/LayoutBox.h"
@@ -24,22 +23,25 @@ using namespace HTMLNames;
 // Seconds to wait after a video has stopped moving before playing it.
 static const double kViewportTimerPollDelay = 0.5;
 
-AutoplayExperimentHelper::AutoplayExperimentHelper(HTMLMediaElement& element)
-    : m_element(&element)
+AutoplayExperimentHelper::AutoplayExperimentHelper(Client& client)
+    : m_client(client)
     , m_mode(Mode::ExperimentOff)
     , m_playPending(false)
     , m_registeredWithLayoutObject(false)
     , m_wasInViewport(false)
+    , m_autoplayMediaCounted(false)
+    , m_playbackStartedBefore(false)
+    , m_initialPlayWithoutUserGesture(false)
+    , m_recordedElement(false)
     , m_lastLocationUpdateTime(-std::numeric_limits<double>::infinity())
     , m_viewportTimer(this, &AutoplayExperimentHelper::viewportTimerFired)
+    , m_autoplayDeferredMetric(GesturelessPlaybackUnknownReason)
 {
-    if (document().settings()) {
-        m_mode = fromString(document().settings()->autoplayExperimentMode());
+    m_mode = fromString(this->client().autoplayExperimentMode());
 
-        if (m_mode != Mode::ExperimentOff) {
-            WTF_LOG(Media, "HTMLMediaElement: autoplay experiment set to %d",
-                m_mode);
-        }
+    if (m_mode != Mode::ExperimentOff) {
+        WTF_LOG(Media, "HTMLMediaElement: autoplay experiment set to %d",
+            m_mode);
     }
 }
 
@@ -52,10 +54,13 @@ void AutoplayExperimentHelper::becameReadyToPlay()
 {
     // Assuming that we're eligible to override the user gesture requirement,
     // either play if we meet the visibility checks, or install a listener
-    // to wait for them to pass.
+    // to wait for them to pass.  We do not actually start playback; our
+    // caller must do that.
+    autoplayMediaEncountered();
+
     if (isEligible()) {
         if (meetsVisibilityRequirements())
-            prepareToPlay(GesturelessPlaybackStartedByAutoplayFlagImmediately);
+            prepareToAutoplay(GesturelessPlaybackStartedByAutoplayFlagImmediately);
         else
             registerForPositionUpdatesIfNeeded();
     }
@@ -69,6 +74,7 @@ void AutoplayExperimentHelper::playMethodCalled()
     m_playPending = true;
 
     if (!UserGestureIndicator::processingUserGesture()) {
+        autoplayMediaEncountered();
 
         if (isEligible()) {
             // Remember that userGestureRequiredForPlay is required for
@@ -76,15 +82,20 @@ void AutoplayExperimentHelper::playMethodCalled()
             // If we are able to override the gesture requirement now, then
             // do so.  Otherwise, install an event listener if we need one.
             if (meetsVisibilityRequirements()) {
-                // Override the gesture and play.
-                prepareToPlay(GesturelessPlaybackStartedByPlayMethodImmediately);
+                // Override the gesture and assume that play() will succeed.
+                prepareToAutoplay(GesturelessPlaybackStartedByPlayMethodImmediately);
             } else {
                 // Wait for viewport visibility.
                 registerForPositionUpdatesIfNeeded();
             }
         }
+    } else if (isUserGestureRequiredForPlay()) {
+        if (m_autoplayMediaCounted)
+            recordAutoplayMetric(AutoplayManualStart);
+        // Don't let future gestureless playbacks affect metrics.

[philipj_slow, 2016/01/12 15:12:16]

This doesn't seem true, AutoplayManualStart could still be counted again above,
right? Simply calling play() twice inside a touch event handler should cause
this.

[liberato (no reviews please), 2016/01/29 08:25:24]

good catch.  i think this was guarded before, and was unintentionally deleted.

+        m_autoplayMediaCounted = true;
+        m_playbackStartedBefore = true;

[philipj_slow, 2016/01/12 15:12:17]

Shouldn't m_playbackStartedBefore be set only in playbackStarted()?

[liberato (no reviews please), 2016/01/29 08:25:24]

i didn't just in case it doesn't get that far.  i'd rather be sure that it
records the metrics only once.  i'll rename it to m_playbackStartMetricRecorded.
 then it's clear.

[philipj_slow, 2016/02/02 08:08:15]

Acknowledged.

 
-    } else if (element().isUserGestureRequiredForPlay()) {
         unregisterForPositionUpdatesIfNeeded();
     }
 }
@@ -94,6 +105,25 @@ void AutoplayExperimentHelper::pauseMethodCalled()
     // Don't try to autoplay, if we would have.
     m_playPending = false;
     unregisterForPositionUpdatesIfNeeded();
+
+    if (!client().paused())
+        recordMetricsBeforePause();
+}
+
+void AutoplayExperimentHelper::loadMethodCalled()
+{
+    if (!client().paused())
+        recordMetricsBeforePause();

[philipj_slow, 2016/01/12 15:12:17]

I wouldn't say that load() amounts to pausing. Something that calls load()
usually isn't exposed to the user as a button or anything, so I don't think this
will say anything about user behavior.

[liberato (no reviews please), 2016/01/29 08:25:24]

true, but not recording anything doesn't seem right either.  unless you've got
strong objections, i'd prefer to leave it this way to prevent us from missing a
bailout.

i didn't see any instances of this case actually happening, so hopefully it's
not common.

[philipj_slow, 2016/02/02 08:08:15]

If it were common it would taint the metric for pause, it seems, but I suppose
that's an acceptable risk.

+
+    if (UserGestureIndicator::processingUserGesture() && isUserGestureRequiredForPlay()) {
+        recordAutoplayMetric(AutoplayEnabledThroughLoad);

[philipj_slow, 2016/01/12 15:12:17]

Should AutoplayEnabledThroughLoad simply be removed? Looks like it will be
exactly the same as GesturelessPlaybackEnabledByLoad.

[liberato (no reviews please), 2016/01/29 08:25:24]

the former is recorded when the override happens, while the latter is recorded
only if it tries to autoplay.

[philipj_slow, 2016/02/02 08:08:14]

Oh, different function call there :)

+        removeUserGestureRequirement(GesturelessPlaybackEnabledByLoad);
+        // While usergesture-initiated load()s technically count as autoplayed,

[philipj_slow, 2016/01/12 15:12:17]

Why don't these plays feel like autoplay? And isn't measuring
GesturelessPlaybackEnabledByLoad enough to get an idea of how common this case
is?

If further measuring really must be inhibited, an explicit way of doing it (like
checking m_autoplayDeferredMetric == GesturelessPlaybackEnabledByLoad) seems
better than pretending to have counted and played.

[liberato (no reviews please), 2016/01/29 08:25:24]

now that we're recording "GesturelessPlaybackEnabledByLoad" only when the thing
actually starts playing, we probably don't need to treat it specially anymore.

+        // they don't feel like such to the users and hence we don't want to
+        // count them for the purposes of metrics.
+        m_autoplayMediaCounted = true;
+        m_playbackStartedBefore = true;
+    }
 }
 
 void AutoplayExperimentHelper::mutedChanged()
@@ -102,6 +132,13 @@ void AutoplayExperimentHelper::mutedChanged()
     // quit listening for events.  If we are eligible, and if we should be
     // playing, then start playing.  In other words, start playing if
     // we just needed 'mute' to autoplay.
+
+    // Make sure that autoplay was actually deferred.  If, for example, the
+    // autoplay attribute is set after the media is ready to play, then it
+    // would normally have no effect.  We don't want to start playing.
+    if (!m_autoplayMediaCounted)
+        return;
+
     if (!isEligible()) {
         unregisterForPositionUpdatesIfNeeded();
     } else {
@@ -120,10 +157,7 @@ void AutoplayExperimentHelper::registerForPositionUpdatesIfNeeded()
             return;
     }
 
-    if (LayoutObject* layoutObject = element().layoutObject()) {
-        LayoutMedia* layoutMedia = toLayoutMedia(layoutObject);
-        layoutMedia->setRequestPositionUpdates(true);
-    }
+    client().setRequestPositionUpdates(true);
 
     // Set this unconditionally, in case we have no layout object yet.
     m_registeredWithLayoutObject = true;
@@ -131,12 +165,8 @@ void AutoplayExperimentHelper::registerForPositionUpdatesIfNeeded()
 
 void AutoplayExperimentHelper::unregisterForPositionUpdatesIfNeeded()
 {
-    if (m_registeredWithLayoutObject) {
-        if (LayoutObject* obj = element().layoutObject()) {
-            LayoutMedia* layoutMedia = toLayoutMedia(obj);
-            layoutMedia->setRequestPositionUpdates(false);
-        }
-    }
+    if (m_registeredWithLayoutObject)
+        client().setRequestPositionUpdates(false);
 
     // Clear this unconditionally so that we don't re-register if we didn't
     // have a LayoutObject now, but get one later.
@@ -151,13 +181,15 @@ void AutoplayExperimentHelper::positionChanged(const IntRect& visibleRect)
     // Also note that we are called quite often, including when the
     // page becomes visible.  That's why we don't bother to register
     // for page visibility changes explicitly.
+    if (visibleRect.isEmpty())
+        return;
 
     m_lastVisibleRect = visibleRect;
 
-    if (!element().layoutObject())
+    IntRect currentLocation = client().absoluteBoundingBoxRect();
+    if (currentLocation.isEmpty())
         return;
 
-    IntRect currentLocation = element().layoutObject()->absoluteBoundingBoxRect();
     bool inViewport = meetsVisibilityRequirements();
 
     if (m_lastLocation != currentLocation) {
@@ -175,18 +207,12 @@ void AutoplayExperimentHelper::positionChanged(const IntRect& visibleRect)
 
 void AutoplayExperimentHelper::updatePositionNotificationRegistration()
 {
-    if (m_registeredWithLayoutObject) {
-        LayoutMedia* layoutMedia = toLayoutMedia(element().layoutObject());
-        layoutMedia->setRequestPositionUpdates(true);
-    }
+    if (m_registeredWithLayoutObject)
+        client().setRequestPositionUpdates(true);
 }
 
 void AutoplayExperimentHelper::triggerAutoplayViewportCheckForTesting()
 {
-    FrameView* view = document().view();
-    if (view)
-        positionChanged(view->rootFrameToContents(view->computeVisibleArea()));
-
     // Make sure that the last update appears to be sufficiently far in the
     // past to appear that scrolling has stopped by now in viewportTimerFired.
     m_lastLocationUpdateTime = monotonicallyIncreasingTime() - kViewportTimerPollDelay - 1;
@@ -214,7 +240,7 @@ void AutoplayExperimentHelper::viewportTimerFired(Timer<AutoplayExperimentHelper
 bool AutoplayExperimentHelper::meetsVisibilityRequirements() const
 {
     if (enabled(IfPageVisible)
-        && element().document().pageVisibilityState() != PageVisibilityStateVisible)
+        && client().pageVisibilityState() != PageVisibilityStateVisible)
         return false;
 
     if (!enabled(IfViewport))
@@ -223,12 +249,10 @@ bool AutoplayExperimentHelper::meetsVisibilityRequirements() const
     if (m_lastVisibleRect.isEmpty())
         return false;
 
-    LayoutObject* layoutObject = element().layoutObject();
-    if (!layoutObject)
+    IntRect currentLocation = client().absoluteBoundingBoxRect();
+    if (currentLocation.isEmpty())
         return false;
 
-    IntRect currentLocation = layoutObject->absoluteBoundingBoxRect();
-
     // If element completely fills the screen, then truncate it to exactly
     // match the screen.  Any element that is wider just has to cover.
     if (currentLocation.x() <= m_lastVisibleRect.x()
@@ -254,10 +278,19 @@ bool AutoplayExperimentHelper::maybeStartPlaying()
     }
 
     // Start playing!
-    prepareToPlay(element().shouldAutoplay()
+    prepareToAutoplay(client().shouldAutoplay()
         ? GesturelessPlaybackStartedByAutoplayFlagAfterScroll
         : GesturelessPlaybackStartedByPlayMethodAfterScroll);
-    element().playInternal();
+
+    // Record that this played without a user gesture.
+    // This should rarely actually do anything.  Usually, playMethodCalled()
+    // and becameReadyToPlay will handle it, but toggling muted state can,
+    // in some cases, also trigger autoplay if the autoplay attribute is set
+    // after the media is ready to play.
+    // TODO(liberato): remove this.

[philipj_slow, 2016/01/12 15:12:17]

Remove it how?

[liberato (no reviews please), 2016/01/29 08:25:24]

i was planning to change how 'is muted' works, so that there's not a loophole. 
however, after i came back to planet earth, i realized that this would happen
about zero times.  i'll remove the comment.

[philipj_slow, 2016/02/02 08:08:14]

Welcome back :)

+    autoplayMediaEncountered();
+
+    client().playInternal();
 
     return true;
 }
@@ -271,31 +304,31 @@ bool AutoplayExperimentHelper::isEligible() const
     // This is what prevents us from starting playback more than once.
     // Since this flag is never set to true once it's cleared, it will block
     // the autoplay experiment forever.
-    if (!element().isUserGestureRequiredForPlay())
+    if (!isUserGestureRequiredForPlay())
         return false;
 
     // Make sure that this is an element of the right type.
-    if (!enabled(ForVideo) && isHTMLVideoElement(element()))
+    if (!enabled(ForVideo) && client().isHTMLVideoElement())
         return false;
 
-    if (!enabled(ForAudio) && isHTMLAudioElement(element()))
+    if (!enabled(ForAudio) && client().isHTMLAudioElement())
         return false;
 
     // If nobody has requested playback, either by the autoplay attribute or
     // a play() call, then do nothing.
 
-    if (!m_playPending && !element().shouldAutoplay())
+    if (!m_playPending && !client().shouldAutoplay())
         return false;
 
     // Note that the viewport test always returns false on desktop, which is
     // why video-autoplay-experiment.html doesn't check -ifmobile .
     if (enabled(IfMobile)
-        && !document().viewportDescription().isLegacyViewportType())
+        && !client().isLegacyViewportType())
         return false;
 
     // If we require muted media and this is muted, then it is eligible.
     if (enabled(IfMuted))
-        return element().muted();
+        return client().muted();
 
     // Element is eligible for gesture override, maybe muted.
     return true;
@@ -308,38 +341,39 @@ void AutoplayExperimentHelper::muteIfNeeded()
         // If we are actually changing the muted state, then this will call
         // mutedChanged().  If isEligible(), then mutedChanged() will try
         // to start playback, which we should not do here.
-        element().setMuted(true);
+        client().setMuted(true);
     }
 }
 
-void AutoplayExperimentHelper::prepareToPlay(AutoplayMetrics metric)
+void AutoplayExperimentHelper::removeUserGestureRequirement(AutoplayMetrics metric)
 {
-    element().recordAutoplayMetric(metric);
+    if (client().isUserGestureRequiredForPlay()) {
+        m_autoplayDeferredMetric = metric;
+        client().removeUserGestureRequirement();
+    }
+}
 
+void AutoplayExperimentHelper::prepareToAutoplay(AutoplayMetrics metric)
+{
     // This also causes !isEligible, so that we don't allow autoplay more than
     // once.  Be sure to do this before muteIfNeeded().
-    element().removeUserGestureRequirement();
+    // Also note that, at this point, we know that we're goint to start
+    // playback.  However, we still don't record the metric here.  Instead,
+    // we let autoplayMediaEncountered() do that later.

[philipj_slow, 2016/01/12 15:12:17]

It's actually recorded in playbackStarted(), right?

Also, this comment would perhaps be better placed inside
removeUserGestureRequirement().

[liberato (no reviews please), 2016/01/29 08:25:24]

it should say playbackStarted.  however, i don't see how
removeUserGestureRequirement is the right place for the comment.  that does not
know whether playback is going to start or not.

[philipj_slow, 2016/02/02 08:08:14]

Acknowledged.

+    removeUserGestureRequirement(metric);
+
+    // Don't bother to call autoplayMediaEncountered, since whoever initiates
+    // playback has do it anyway, in case we don't allow autoplay.
 
     unregisterForPositionUpdatesIfNeeded();
     muteIfNeeded();
 
-    // Record that this autoplayed without a user gesture.  This is normally
-    // set when we discover an autoplay attribute, but we include all cases
-    // where playback started without a user gesture, e.g., play().
-    element().setInitialPlayWithoutUserGestures(true);
-
     // Do not actually start playback here.
 }
 
-Document& AutoplayExperimentHelper::document() const
+AutoplayExperimentHelper::Client& AutoplayExperimentHelper::client() const
 {
-    return element().document();
-}
-
-HTMLMediaElement& AutoplayExperimentHelper::element() const
-{
-    ASSERT(m_element);
-    return *m_element;
+    return m_client;
 }
 
 AutoplayExperimentHelper::Mode AutoplayExperimentHelper::fromString(const String& mode)
@@ -363,4 +397,97 @@ AutoplayExperimentHelper::Mode AutoplayExperimentHelper::fromString(const String
     return value;
 }
 
+void AutoplayExperimentHelper::autoplayMediaEncountered()
+{
+    if (!m_autoplayMediaCounted) {

[philipj_slow, 2016/01/12 15:12:17]

Maybe rename to m_autoplayMediaEncountered? There isn't much counting going on.

[liberato (no reviews please), 2016/01/29 08:25:24]

Done.

+        m_autoplayMediaCounted = true;
+        recordAutoplayMetric(AutoplayMediaFound);
+    }
+}
+
+bool AutoplayExperimentHelper::isUserGestureRequiredForPlay() const
+{
+    return client().isUserGestureRequiredForPlay();
+}
+
+void AutoplayExperimentHelper::recordMetricsBeforePause()
+{
+    ASSERT(!client().paused());
+
+    const bool bailout = isBailout();
+
+    // Record that play was paused.  We don't care if it was autoplay,
+    // play(), or the user manually started it.
+    recordAutoplayMetric(AnyPlaybackPaused);
+    if (bailout)
+        recordAutoplayMetric(AnyPlaybackBailout);
+
+    // If this was a gestureless play, then record that separately.
+    // These cover attr and play() gestureless starts.
+    if (m_initialPlayWithoutUserGesture) {
+        m_initialPlayWithoutUserGesture = false;

[philipj_slow, 2016/01/12 15:12:17]

Why is m_initialPlayWithoutUserGesture cleared here? It can't stop being true :)

[liberato (no reviews please), 2016/01/29 08:25:24]

good point.  it's really more like "need to record stop/pause metrics for
autoplayed media".  i'll rename.

+
+        recordAutoplayMetric(AutoplayPaused);
+
+        if (bailout)
+            recordAutoplayMetric(AutoplayBailout);
+    }
+}
+
+void AutoplayExperimentHelper::playbackStarted()
+{
+    recordAutoplayMetric(AnyPlaybackStarted);
+
+    if (!m_playbackStartedBefore) {
+        m_playbackStartedBefore = true;
+
+        // If this is a gestureless start, record why it was allowed.
+        if (!UserGestureIndicator::processingUserGesture()) {
+            m_initialPlayWithoutUserGesture = true;
+            recordAutoplayMetric(m_autoplayDeferredMetric);

[philipj_slow, 2016/01/12 15:12:16]

assert something about m_autoplayDeferredMetric?

[liberato (no reviews please), 2016/01/29 08:25:24]

do you mean for the default (GesturelessPlaybackNotOverridden)?  if so, then i
think that it's not unexpected if run on desktop or with the user gesture
requirement turned off in about://flags.

[philipj_slow, 2016/02/02 08:08:14]

Right, that other platform :)

+        }
+    }
+}
+
+void AutoplayExperimentHelper::playbackEnded()
+{
+    recordAutoplayMetric(AnyPlaybackComplete);
+    if (m_initialPlayWithoutUserGesture) {
+        m_initialPlayWithoutUserGesture = false;

[philipj_slow, 2016/01/12 15:12:16]

And why here? Since it's still true that the initial play was without a user
gesture, I assume that resetting this is in order to cause some other effect
later, but what?

[liberato (no reviews please), 2016/01/29 08:25:24]

it prevents pause/end metrics from being recorded again for autoplay.  i've
renamed it to make this more clear.

[philipj_slow, 2016/02/02 08:08:15]

Acknowledged.

+        recordAutoplayMetric(AutoplayComplete);
+    }
+}
+
+void AutoplayExperimentHelper::recordAutoplayMetric(AutoplayMetrics metric)
+{
+    client().recordAutoplayMetric(metric);
+}
+
+bool AutoplayExperimentHelper::isBailout() const
+{
+    // We count the user as having bailed-out on the video if they watched
+    // less than one minute and less than 50% of it.
+    const double playedTime = client().currentTime();
+    const double progress = playedTime / client().duration();
+    return (playedTime < 60) && (progress < 0.5);
+}
+
+void AutoplayExperimentHelper::recordSandboxFailure()
+{
+    // We record autoplayMediaEncountered here because we know
+    // that the autoplay attempt will fail.
+    autoplayMediaEncountered();
+    recordAutoplayMetric(AutoplayDisabledBySandbox);
+}
+
+void AutoplayExperimentHelper::loadingStarted()
+{
+    if (m_recordedElement)
+        return;
+
+    m_recordedElement = true;
+    recordAutoplayMetric(client().isHTMLVideoElement()
+        ? AnyVideoElement
+        : AnyAudioElement);
+}
 }