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..f154555feebe18689406d960c549c12c0362f5a0 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_anyUserGestureEncountered(false)
+    , m_initialPlaybackRecorded(false)
 {
 #if ENABLE(OILPAN)
     ThreadState::current()->registerPreFinalizer(this);
@@ -360,6 +362,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
 
     setHasCustomStyleCallbacks();
     addElementToDocumentMap(this, &document);
+    recordAutoplayMetric(AnyMediaElement);

[philipj_slow, 2015/11/25 14:03:42]

It's likely that this and the AnyVideoElement will be poisoned entirely by
feature detection, i.e. elements created just to call canPlayType() and similar.

If you want an idea about media elements that could do something, I would
instead add the metric before setNetworkState(NETWORK_LOADING) in
HTMLMediaElement::loadResource, which is roughly the resource fetch algorithm in
spec terms. That's before preload=none has any effect.

With that change, you can also revert recordAutoplayMetric to being private, and
it will probably make sense to measure audio+video than media+video.

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

very good point, thanks.

 }
 
 HTMLMediaElement::~HTMLMediaElement()
@@ -672,17 +675,20 @@ String HTMLMediaElement::canPlayType(const String& mimeType, const String& keySy
 void HTMLMediaElement::recordMetricsIfPausing()

[philipj_slow, 2015/11/25 14:03:42]

Maybe rename to recordMetricsBeforePause() if that was always the intended
meaning? And then ASSERT(!m_paused) if that's actually true.

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

Done.

 {
     // If not playing, then nothing to record.
-    // TODO(liberato): test metrics.  this was m_paused.
     if (m_paused)
         return;
 
+    // Don't count seeking as a bailout.

[philipj_slow, 2015/11/25 14:03:42]

Why was this comment added? I see no changes related to seeking.

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

whoops, forgot to delete.

     const bool bailout = isBailout();
+    const bool ended = endedPlayback();

[philipj_slow, 2015/11/25 14:03:42]

Have you tested that this is always true when recordMetricsIfPausing() is called
from HTMLMediaElement::timeChanged()? HTMLMediaElement::endedPlayback() is
rather convoluted, after all.

I think this might all be clearer with a new recordMetricsAtPlaybackEnd() or
similar, since it can't be a bailout and it doesn't really make sense to
consider it a pause either.

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

it has worked in all of my tests.

however, i rather like the idea of separating them and avoiding it.

Done.

 
     // 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 (ended)
+        recordAutoplayMetric(AnyPlaybackComplete);
 
     // If this was a gestureless play, then record that separately.
     // These cover attr and play() gestureless starts.
@@ -693,6 +699,9 @@ void HTMLMediaElement::recordMetricsIfPausing()
 
         if (bailout)
             recordAutoplayMetric(AutoplayBailout);
+
+        if (ended)
+            recordAutoplayMetric(AutoplayComplete);
     }
 }
 
@@ -701,8 +710,10 @@ void HTMLMediaElement::load()
     WTF_LOG(Media, "HTMLMediaElement::load(%p)", this);
 
     recordMetricsIfPausing();
+    bool gesture = UserGestureIndicator::processingUserGesture();

[philipj_slow, 2015/11/25 14:03:42]

IIUC, this and a bunch of other changes are "will give us an idea about how
often a gesture in the inital load() call is responsible for a playback."

How about instead explicitly measuring this, by making all changes to
m_userGestureRequiredForPlay go through removeUserGestureRequirement() and
passing an enum for what it is that caused it?

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

i had done exactly that, but didn't like it.  i'll try again.

... hrm, for some reason it looks much nicer the second time around.  Done. 
Thanks.

+    m_anyUserGestureEncountered |= gesture;
 
-    if (UserGestureIndicator::processingUserGesture() && m_userGestureRequiredForPlay) {
+    if (gesture && m_userGestureRequiredForPlay) {
         recordAutoplayMetric(AutoplayEnabledThroughLoad);
         m_userGestureRequiredForPlay = false;
         // While usergesture-initiated load()s technically count as autoplayed,
@@ -1533,29 +1544,40 @@ 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);
+            recordAutoplayMetric(AnyPlaybackStarted);

[philipj_slow, 2015/11/25 14:03:41]

Rather than measuring the playing event, can you put this right after the
webMediaPlayer()->play() call instead? It *should* be equivalent, but in
https://github.com/whatwg/html/issues/309 we're considering changing when the
playing event is fired, and also the other measurements are right after the
play() call.

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

Done.

+        }
         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);
+                recordAutoplayMetric(AnyPlaybackStarted);
+            }
         }
 
         // 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 autoplay media was encountered, but wait until

[philipj_slow, 2015/11/25 14:03:41]

I don't quite understand this comment. Is it explaining why this code is here
instead of just inside HTMLMediaElement::shouldAutoplay() before the `return
true` statement?

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

yes.  i'll update the comment to make it clearer.

+            // after the autoplay experiment helper has had a chance to update
+            // the user gesture requirement.  This is because aME() tries to
+            // guess whether the play will succeed based on that.
+            autoplayMediaEncountered();
+
             if (!m_userGestureRequiredForPlay) {
                 m_paused = false;
                 invalidateCachedTime();
                 scheduleEvent(EventTypeNames::play);
                 scheduleEvent(EventTypeNames::playing);
+                recordAutoplayMetric(AnyPlaybackStarted);
             }
         }
 
@@ -1870,12 +1892,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;
         }
 
@@ -1952,7 +1975,10 @@ void HTMLMediaElement::play()
 
     m_autoplayHelper.playMethodCalled();
 
-    if (!UserGestureIndicator::processingUserGesture()) {
+    bool gesture = UserGestureIndicator::processingUserGesture();
+    m_anyUserGestureEncountered |= gesture;
+
+    if (!gesture) {
         autoplayMediaEncountered();
 
         if (m_userGestureRequiredForPlay) {
@@ -1989,10 +2015,12 @@ void HTMLMediaElement::playInternal()
         invalidateCachedTime();
         scheduleEvent(EventTypeNames::play);
 
-        if (m_readyState <= HAVE_CURRENT_DATA)
+        if (m_readyState <= HAVE_CURRENT_DATA) {
             scheduleEvent(EventTypeNames::waiting);
-        else if (m_readyState >= HAVE_FUTURE_DATA)
+        } else if (m_readyState >= HAVE_FUTURE_DATA) {
             scheduleEvent(EventTypeNames::playing);
+            recordAutoplayMetric(AnyPlaybackStarted);

[philipj_slow, 2015/11/25 14:03:42]

Can you put this right after the webMediaPlayer()->play() call instead? It
*should* be equivalent, but in https://github.com/whatwg/html/issues/309 we're
considering changing when the playing event is fired, and also the other
measurements are right after the play() call.

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

Done, and a third case as well.

+        }
     }
     m_autoplaying = false;
 
@@ -2005,6 +2033,8 @@ void HTMLMediaElement::autoplayMediaEncountered()
         m_autoplayMediaCounted = true;
         recordAutoplayMetric(AutoplayMediaFound);
 
+        // If no user gesture was required, then assume that playback will

[philipj_slow, 2015/11/25 14:03:42]

When you spell it out it's kind of weird. If you keep track of who cleared the
user gesture requirement, can't this be counted at webMediaPlayer()->play() call
site?

If so that should also make it possible to call autoplayMediaEncountered()
without worrying about whether the user gesture has been cleared yet, as is done
above.

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

i'm not sure that it gets any better.  wmp->play() might not be called (or might
be called for some other reason later), and we'd then have to worry about
whether it should record any metric at all.  it really wants to record it only
when we set m_autoplayMediaCounted here.

+        // actually start.
         if (!m_userGestureRequiredForPlay)
             m_initialPlayWithoutUserGesture = true;
     }
@@ -2724,6 +2754,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.
+                recordMetricsIfPausing();
                 m_paused = true;
                 scheduleEvent(EventTypeNames::pause);
             }
@@ -2732,7 +2763,6 @@ void HTMLMediaElement::timeChanged()
                 m_sentEndEvent = true;
                 scheduleEvent(EventTypeNames::ended);
             }
-            recordMetricsIfPausing();
         }
     } else {
         m_sentEndEvent = false;
@@ -2939,17 +2969,30 @@ void HTMLMediaElement::updatePlayState()
             webMediaPlayer()->setRate(playbackRate());
             updateVolume();
             webMediaPlayer()->play();
+
+            if (!m_initialPlaybackRecorded) {

[philipj_slow, 2015/11/25 14:03:42]

I believe this is also to "give us an idea about how often a gesture in the
inital load() call is responsible for a playback."

If you knew what it was that cleared the user gesture requirement to begin with,
would you still have any use for these four metrics? In particular the
During/NotDuring variants seem strange, since you can call pause() with a user
gesture to clear the requirement and play much later while the gesture is no
longer in effect, or even worse there could be another gesture in effect to
confuse the data.

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

the load call is part of it.

another part of the rationale behind these was to upper and lower bound the
cases you describe.  for example, the case with "no user gesture now or earlier"
is almost certainly "autoplay" from the user's point of view.

however, i'm removing these anyway.  after further thought, it's unclear that
it'll help.

+                bool gesture = UserGestureIndicator::processingUserGesture();
+                recordAutoplayMetric(gesture
+                    ? InitialPlayDuringUserGesture
+                    : InitialPlayNotDuringUserGesture);
+
+                m_anyUserGestureEncountered |= gesture;
+                recordAutoplayMetric(m_anyUserGestureEncountered
+                    ? InitialPlayWithUserGesture
+                    : InitialPlayWithoutUserGesture);
+                m_initialPlaybackRecorded = true;
+            }
         }
 
         if (mediaControls())
             mediaControls()->playbackStarted();
         startPlaybackProgressTimer();
         m_playing = true;
-        recordAutoplayMetric(AnyPlaybackStarted);
 
     } else { // Should not be playing right now
         if (isPlaying)
             webMediaPlayer()->pause();
+
         refreshCachedTime();
 
         m_playbackProgressTimer.stop();
@@ -3580,11 +3623,6 @@ void HTMLMediaElement::removeUserGestureRequirement()
     m_userGestureRequiredForPlay = false;
 }
 
-void HTMLMediaElement::setInitialPlayWithoutUserGestures(bool value)
-{
-    m_initialPlayWithoutUserGesture = value;
-}
-
 void HTMLMediaElement::setNetworkState(NetworkState state)
 {
     if (m_networkState != state) {