Record the offscreen playing duration of autoplaying muted videos

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

Index: third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp
diff --git a/third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp b/third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp
index bc6fe41590ece6524fdd9c2b7c6f5c45bd8c768c..b39bb65d50224c625fbc15804005ff6e7cf7b5c8 100644
--- a/third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp
+++ b/third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp
@@ -10,16 +10,14 @@
 #include "core/frame/LocalDOMWindow.h"
 #include "core/html/HTMLMediaElement.h"
 #include "platform/Histogram.h"
+#include "wtf/CurrentTime.h"
 
 namespace blink {
 
 namespace {
 
-void recordVideoAutoplayMutedPlayMethodBecomesVisibleUma(bool visible)
-{
-    DEFINE_STATIC_LOCAL(BooleanHistogram, histogram, ("Media.Video.Autoplay.Muted.PlayMethod.BecomesVisible"));
-    histogram.count(visible);
-}
+static const double maxUmaDurationMS = 1e6;
+static const double umaBucketCount = 20;

[mlamouri (slow - plz ping), 2016/08/18 14:15:14]

nit: no need for static if in anonymous namespace. You can also move this inside
the function given that it's only used there. Might be easier for readability.

[Zhiqiang Zhang (Slow), 2016/08/18 15:11:02]

Done.

 
 } // namespace
 
@@ -32,7 +30,11 @@ AutoplayUmaHelper::AutoplayUmaHelper(HTMLMediaElement* element)
     : EventListener(CPPEventListenerType)
     , m_source(AutoplaySource::NumberOfSources)
     , m_element(element)
-    , m_videoMutedPlayMethodVisibilityObserver(nullptr) { }
+    , m_mutedVideoPlayMethodVisibilityObserver(nullptr)
+    , m_mutedVideoAutoplayOffscreenStartTimeMS(0)
+    , m_mutedVideoAutoplayOffscreenDurationMS(0)
+    , m_isVisible(false)
+    , m_mutedVideoOffscreenDurationVisibilityObserver(nullptr) { }
 
 AutoplayUmaHelper::~AutoplayUmaHelper() = default;
 
@@ -47,6 +49,11 @@ void AutoplayUmaHelper::onAutoplayInitiated(AutoplaySource source)
     DEFINE_STATIC_LOCAL(EnumerationHistogram, mutedVideoHistogram, ("Media.Video.Autoplay.Muted", static_cast<int>(AutoplaySource::NumberOfSources)));
     DEFINE_STATIC_LOCAL(EnumerationHistogram, audioHistogram, ("Media.Audio.Autoplay", static_cast<int>(AutoplaySource::NumberOfSources)));
 
+    // Autoplay already initiated
+    // TODO(zqzhang): how about having autoplay attribute and calling `play()` in the script?
+    if (m_source != AutoplaySource::NumberOfSources)
+        return;
+
     m_source = source;
 
     if (m_element->isHTMLVideoElement()) {
@@ -57,8 +64,7 @@ void AutoplayUmaHelper::onAutoplayInitiated(AutoplaySource source)
         audioHistogram.count(static_cast<int>(m_source));
     }
 
-    if (m_source == AutoplaySource::Method && m_element->isHTMLVideoElement() && m_element->muted())
-        m_element->addEventListener(EventTypeNames::playing, this, false);
+    m_element->addEventListener(EventTypeNames::playing, this, false);
 }
 
 void AutoplayUmaHelper::recordAutoplayUnmuteStatus(AutoplayUnmuteActionStatus status)
@@ -70,7 +76,7 @@ void AutoplayUmaHelper::recordAutoplayUnmuteStatus(AutoplayUnmuteActionStatus st
 
 void AutoplayUmaHelper::didMoveToNewDocument(Document& oldDocument)
 {
-    if (!m_videoMutedPlayMethodVisibilityObserver)
+    if (!shouldListenToUnloadEvent())
         return;
 
     if (oldDocument.domWindow())
@@ -79,55 +85,136 @@ void AutoplayUmaHelper::didMoveToNewDocument(Document& oldDocument)
         m_element->document().domWindow()->addEventListener(EventTypeNames::unload, this, false);
 }
 
-void AutoplayUmaHelper::onVisibilityChangedForVideoMutedPlayMethod(bool isVisible)
+void AutoplayUmaHelper::onVisibilityChangedForMutedVideoPlayMethodBecomeVisible(bool isVisible)
 {
-    if (!isVisible || !m_videoMutedPlayMethodVisibilityObserver)
+    if (!isVisible || !m_mutedVideoPlayMethodVisibilityObserver)
         return;
 
-    recordVideoAutoplayMutedPlayMethodBecomesVisibleUma(true);
-    m_videoMutedPlayMethodVisibilityObserver->stop();
-    m_videoMutedPlayMethodVisibilityObserver = nullptr;
-    if (m_element && m_element->document().domWindow())
-        m_element->document().domWindow()->removeEventListener(EventTypeNames::unload, this, false);
+    maybeStopRecordingMutedVideoPlayMethodBecomeVisible(true);
+}
+
+void AutoplayUmaHelper::onVisibilityChangedForMutedVideoOffscreenDuration(bool isVisible)
+{
+    if (isVisible == m_isVisible)
+        return;
+
+    if (isVisible)
+        m_mutedVideoAutoplayOffscreenDurationMS += monotonicallyIncreasingTimeMS() - m_mutedVideoAutoplayOffscreenStartTimeMS;
+    else
+        m_mutedVideoAutoplayOffscreenStartTimeMS = monotonicallyIncreasingTimeMS();
+
+    m_isVisible = isVisible;
 }
 
 void AutoplayUmaHelper::handleEvent(ExecutionContext* executionContext, Event* event)
 {
     if (event->type() == EventTypeNames::playing)
         handlePlayingEvent();
+    else if (event->type() == EventTypeNames::pause)
+        handlePauseEvent();
     else if (event->type() == EventTypeNames::unload)
         handleUnloadEvent();
     else
         NOTREACHED();
 }
 
+void AutoplayUmaHelper::handlePlayingEvent()
+{
+    maybeStartRecordingMutedVideoPlayMethodBecomeVisible();
+    maybeStartRecordingMutedVideoOffscreenDuration();
+
+    m_element->removeEventListener(EventTypeNames::playing, this, false);
+}
+
+void AutoplayUmaHelper::handlePauseEvent()
+{
+    if (!m_isVisible)
+        m_mutedVideoAutoplayOffscreenDurationMS += monotonicallyIncreasingTimeMS() - m_mutedVideoAutoplayOffscreenStartTimeMS;

[Zhiqiang Zhang (Slow), 2016/08/18 10:52:43]

This is new. Accumulating the offscreen time when pause() is called when video
is offscreen.

BTW, I'm not sure if pause event is fired when the frame is unloaded, in that
case, we are still recording a finite value. Is this OK?

[mlamouri (slow - plz ping), 2016/08/18 14:15:14]

Could you increment on unload if !m_IsVisible? Maybe you would need to reset
m_mutedVideoAutoplayOffscreenStartTimeMS to 0 here and check the value in the
unload event?

[Zhiqiang Zhang (Slow), 2016/08/18 15:11:02]

Done, moved the final incrementing logic in maybeStopRecording...()

+
+    maybeStopRecordingMutedVideoOffscreenDuration(false);
+}
+
 void AutoplayUmaHelper::handleUnloadEvent()
 {
-    if (m_videoMutedPlayMethodVisibilityObserver) {
-        recordVideoAutoplayMutedPlayMethodBecomesVisibleUma(false);
-        m_videoMutedPlayMethodVisibilityObserver->stop();
-        m_videoMutedPlayMethodVisibilityObserver = nullptr;
-        m_element->document().domWindow()->removeEventListener(EventTypeNames::unload, this, false);
-    }
+    maybeStopRecordingMutedVideoPlayMethodBecomeVisible(false);
+    maybeStopRecordingMutedVideoOffscreenDuration(true);

[mlamouri (slow - plz ping), 2016/08/18 14:15:14]

Instead of passing isInfinite=true here, could you compute how long it was
offscreen? We have m_isVisible, that could help, right?

[Zhiqiang Zhang (Slow), 2016/08/18 15:11:02]

Done, moved the final incrementing logic in maybeStopRecording...()

 }
 
-void AutoplayUmaHelper::handlePlayingEvent()
+void AutoplayUmaHelper::maybeStartRecordingMutedVideoPlayMethodBecomeVisible()
+{
+    if (m_source != AutoplaySource::Method || !m_element->isHTMLVideoElement() || !m_element->muted())
+        return;
+
+    m_mutedVideoPlayMethodVisibilityObserver = new ElementVisibilityObserver(m_element, WTF::bind(&AutoplayUmaHelper::onVisibilityChangedForMutedVideoPlayMethodBecomeVisible, wrapWeakPersistent(this)));
+    m_mutedVideoPlayMethodVisibilityObserver->start();
+    m_element->document().domWindow()->addEventListener(EventTypeNames::unload, this, false);
+}
+
+void AutoplayUmaHelper::maybeStopRecordingMutedVideoPlayMethodBecomeVisible(bool visible)
 {
-    if (m_source == AutoplaySource::Method && m_element->isHTMLVideoElement() && m_element->muted()) {
-        if (!m_videoMutedPlayMethodVisibilityObserver) {
-            m_videoMutedPlayMethodVisibilityObserver = new ElementVisibilityObserver(m_element, WTF::bind(&AutoplayUmaHelper::onVisibilityChangedForVideoMutedPlayMethod, wrapWeakPersistent(this)));
-            m_videoMutedPlayMethodVisibilityObserver->start();
-            m_element->document().domWindow()->addEventListener(EventTypeNames::unload, this, false);
-        }
+    DEFINE_STATIC_LOCAL(BooleanHistogram, histogram, ("Media.Video.Autoplay.Muted.PlayMethod.BecomesVisible"));
+
+    if (!m_mutedVideoPlayMethodVisibilityObserver)

[mlamouri (slow - plz ping), 2016/08/18 14:15:14]

nit: to make sure the metric is properly recorded, maybe you could wrap the
observer stop around this check but not the rest of the method?

[Zhiqiang Zhang (Slow), 2016/08/18 15:11:02]

Won't fix as we discussed offline.

+        return;
+
+    histogram.count(visible);
+    m_mutedVideoPlayMethodVisibilityObserver->stop();
+    m_mutedVideoPlayMethodVisibilityObserver = nullptr;
+    maybeUnregisterUnloadListener();
+}
+
+void AutoplayUmaHelper::maybeStartRecordingMutedVideoOffscreenDuration()
+{
+    if (!m_element->isHTMLVideoElement() || !m_element->muted())
+        return;
+
+    // Start recording muted video playing offscreen duration.
+    m_mutedVideoAutoplayOffscreenStartTimeMS = monotonicallyIncreasingTimeMS();
+    m_isVisible = false;
+    m_mutedVideoOffscreenDurationVisibilityObserver = new ElementVisibilityObserver(m_element, WTF::bind(&AutoplayUmaHelper::onVisibilityChangedForMutedVideoOffscreenDuration, wrapWeakPersistent(this)));
+    m_mutedVideoOffscreenDurationVisibilityObserver->start();
+    m_element->addEventListener(EventTypeNames::pause, this, false);
+    m_element->document().domWindow()->addEventListener(EventTypeNames::unload, this, false);
+}
+
+void AutoplayUmaHelper::maybeStopRecordingMutedVideoOffscreenDuration(bool isInfinite)
+{
+    if (!m_mutedVideoOffscreenDurationVisibilityObserver)
+        return;

[mlamouri (slow - plz ping), 2016/08/18 14:15:14]

ditto, see above

[Zhiqiang Zhang (Slow), 2016/08/18 15:11:01]

ditto.

+
+    double timeDeltaMS = isInfinite ? std::numeric_limits<int32_t>::max() : m_mutedVideoAutoplayOffscreenDurationMS;

[mlamouri (slow - plz ping), 2016/08/18 14:15:14]

I'm really confused in how you use this. m_mutedVideoAutoplay... is a double.
Can you save state in there? It can be set to max/infinity, etc.

[Zhiqiang Zhang (Slow), 2016/08/18 15:11:02]

Only using m_mutedVideoAutoplayOffscreenDurationMS now, removed infinity.

+
+    if (m_source == AutoplaySource::Attribute) {
+        DEFINE_STATIC_LOCAL(CustomCountHistogram, durationHistogram, ("Media.Video.Autoplay.Muted.Attribute.OffscreenDuration", 1, maxUmaDurationMS, umaBucketCount));
+        durationHistogram.count(timeDeltaMS);
+    } else {
+        DEFINE_STATIC_LOCAL(CustomCountHistogram, durationHistogram, ("Media.Video.Autoplay.Muted.PlayMethod.OffscreenDuration", 1, maxUmaDurationMS, umaBucketCount));
+        durationHistogram.count(timeDeltaMS);
     }
-    m_element->removeEventListener(EventTypeNames::playing, this, false);
+    m_mutedVideoOffscreenDurationVisibilityObserver->stop();
+    m_mutedVideoOffscreenDurationVisibilityObserver = nullptr;
+    m_mutedVideoAutoplayOffscreenDurationMS = 0;
+    m_element->removeEventListener(EventTypeNames::pause, this, false);
+    maybeUnregisterUnloadListener();
+}
+
+void AutoplayUmaHelper::maybeUnregisterUnloadListener()
+{
+    if (!shouldListenToUnloadEvent() && m_element && m_element->document().domWindow())
+        m_element->document().domWindow()->removeEventListener(EventTypeNames::unload, this, false);
+}
+
+bool AutoplayUmaHelper::shouldListenToUnloadEvent() const
+{
+    return m_mutedVideoPlayMethodVisibilityObserver || m_mutedVideoOffscreenDurationVisibilityObserver;
 }
 
 DEFINE_TRACE(AutoplayUmaHelper)
 {
     EventListener::trace(visitor);
     visitor->trace(m_element);
-    visitor->trace(m_videoMutedPlayMethodVisibilityObserver);
+    visitor->trace(m_mutedVideoPlayMethodVisibilityObserver);
+    visitor->trace(m_mutedVideoOffscreenDurationVisibilityObserver);
 }
 
 } // namespace blink