Autoplay experiment metric fixes and additions.

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
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 329 matching lines @@
     , m_initialPlayWithoutUserGesture(false)
     , m_autoplayMediaCounted(false)
     , m_inOverlayFullscreenVideo(false)
     , m_audioTracks(AudioTrackList::create(*this))
     , m_videoTracks(VideoTrackList::create(*this))
     , m_textTracks(nullptr)
 #if ENABLE(WEB_AUDIO)
     , m_audioSourceNode(nullptr)
 #endif
     , m_autoplayHelper(*this)
+    , m_anyUserGestureEncountered(false)
+    , m_initialPlaybackRecorded(false)
 {
 #if ENABLE(OILPAN)
     ThreadState::current()->registerPreFinalizer(this);
 #endif
     ASSERT(RuntimeEnabledFeatures::mediaEnabled());
 
     WTF_LOG(Media, "HTMLMediaElement::HTMLMediaElement(%p)", this);
 
     if (document.settings() && document.settings()->mediaPlaybackRequiresUserGesture())
         m_userGestureRequiredForPlay = true;
 
     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()
 {
     WTF_LOG(Media, "HTMLMediaElement::~HTMLMediaElement(%p)", this);
 
 #if !ENABLE(OILPAN)
     // HTMLMediaElement and m_asyncEventQueue always become unreachable
     // together. So HTMLMediaElement and m_asyncEventQueue are destructed in
     // the same GC. We don't need to close it explicitly in Oilpan.
@@ skipping 289 matching lines @@
     case WebMimeRegistry::IsSupported:
         canPlay = "probably";
         break;
     }
 
     WTF_LOG(Media, "HTMLMediaElement::canPlayType(%p, %s, %s) -> %s", this, mimeType.utf8().data(), keySystem.utf8().data(), canPlay.utf8().data());
 
     return canPlay;
 }
 
 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.
     if (m_initialPlayWithoutUserGesture) {
         m_initialPlayWithoutUserGesture = false;
 
         recordAutoplayMetric(AutoplayPaused);
 
         if (bailout)
             recordAutoplayMetric(AutoplayBailout);
+
+        if (ended)
+            recordAutoplayMetric(AutoplayComplete);
     }
 }
 
 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,
         // 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;
     }
 
     prepareForLoad();
     loadInternal();
@@ skipping 810 matching lines @@
     if (m_readyState >= HAVE_CURRENT_DATA && oldState < HAVE_CURRENT_DATA && !m_haveFiredLoadedData) {
         m_haveFiredLoadedData = true;
         shouldUpdateDisplayState = true;
         scheduleEvent(EventTypeNames::loadeddata);
         setShouldDelayLoadEvent(false);
     }
 
     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);
             }
         }
 
         scheduleEvent(EventTypeNames::canplaythrough);
 
         shouldUpdateDisplayState = true;
     }
 
     if (shouldUpdateDisplayState) {
         updateDisplayState();
@@ skipping 294 matching lines @@
 }
 
 bool HTMLMediaElement::autoplay() const
 {
     return fastHasAttribute(autoplayAttr);
 }
 
 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;
         }
 
         return true;
     }
 
     return false;
 }
 
 String HTMLMediaElement::preload() const
@@ skipping 56 matching lines @@
 {
     return autoplay() ? WebMediaPlayer::PreloadAuto : preloadType();
 }
 
 void HTMLMediaElement::play()
 {
     WTF_LOG(Media, "HTMLMediaElement::play(%p)", this);
 
     m_autoplayHelper.playMethodCalled();
 
-    if (!UserGestureIndicator::processingUserGesture()) {
+    bool gesture = UserGestureIndicator::processingUserGesture();
+    m_anyUserGestureEncountered |= gesture;
+
+    if (!gesture) {
         autoplayMediaEncountered();
 
         if (m_userGestureRequiredForPlay) {
             recordAutoplayMetric(PlayMethodFailed);
             String message = ExceptionMessages::failedToExecute("play", "HTMLMediaElement", "API can only be initiated by a user gesture.");
             document().addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, message));
             return;
         }
     } else if (m_userGestureRequiredForPlay) {
         if (m_autoplayMediaCounted)
@@ skipping 16 matching lines @@
     // is ignored to seek back to start in case loop was set after playback
     // ended. See http://crbug.com/364442
     if (endedPlayback(LoopCondition::Ignored))
         seek(0);
 
     if (m_paused) {
         m_paused = false;
         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;
 
     updatePlayState();
 }
 
 void HTMLMediaElement::autoplayMediaEncountered()
 {
     if (!m_autoplayMediaCounted) {
         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;
     }
 }
 
 bool HTMLMediaElement::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 = currentTime();
@@ skipping 699 matching lines @@
         // If the media element has a loop attribute specified
         if (loop()) {
             m_sentEndEvent = false;
             //  then seek to the earliest possible position of the media resource and abort these steps.
             seek(0);
         } else {
             // If the media element has still ended playback, and the direction of playback is still
             // 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);
             }
             // Queue a task to fire a simple event named ended at the media element.
             if (!m_sentEndEvent) {
                 m_sentEndEvent = true;
                 scheduleEvent(EventTypeNames::ended);
             }
-            recordMetricsIfPausing();
         }
     } else {
         m_sentEndEvent = false;
     }
 
     updatePlayState();
 }
 
 void HTMLMediaElement::durationChanged()
 {
@@ skipping 186 matching lines @@
     if (shouldBePlaying) {
         setDisplayMode(Video);
         invalidateCachedTime();
 
         if (!isPlaying) {
             // Set rate, muted before calling play in case they were set before the media engine was setup.
             // The media engine should just stash the rate and muted values since it isn't already playing.
             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();
         m_playing = false;
         double time = currentTime();
         if (time > m_lastSeekTime)
             addPlayedRange(m_lastSeekTime, time);
 
         if (couldPlayIfEnoughData())
             prepareToPlay();
@@ skipping 610 matching lines @@
 bool HTMLMediaElement::isUserGestureRequiredForPlay() const
 {
     return m_userGestureRequiredForPlay;
 }
 
 void HTMLMediaElement::removeUserGestureRequirement()
 {
     m_userGestureRequiredForPlay = false;
 }
 
-void HTMLMediaElement::setInitialPlayWithoutUserGestures(bool value)
-{
-    m_initialPlayWithoutUserGesture = value;
-}
-
 void HTMLMediaElement::setNetworkState(NetworkState state)
 {
     if (m_networkState != state) {
         m_networkState = state;
         if (MediaControls* controls = mediaControls())
             controls->networkStateChanged();
     }
 }
 
 void HTMLMediaElement::notifyPositionMayHaveChanged(const IntRect& visibleRect)
@@ skipping 74 matching lines @@
     visitor->trace(m_client);
 }
 
 DEFINE_TRACE(HTMLMediaElement::AudioSourceProviderImpl)
 {
     visitor->trace(m_client);
 }
 #endif
 
 }