Include viewport visibility checks for autoplay experiment.

Source/core/html/AutoplayExperimentHelper.cpp

Index: Source/core/html/AutoplayExperimentHelper.cpp
diff --git a/Source/core/html/AutoplayExperimentHelper.cpp b/Source/core/html/AutoplayExperimentHelper.cpp
index 3aa5811dc2c6c232404f8dea9fbf040b989ce8f1..ceef2425d6161e4bd21602a5d0b266b5f9431ca4 100644
--- a/Source/core/html/AutoplayExperimentHelper.cpp
+++ b/Source/core/html/AutoplayExperimentHelper.cpp
@@ -21,10 +21,18 @@ namespace blink {
 
 using namespace HTMLNames;
 
+// How long do we wait after a scroll event before deciding that no more
+// scroll events are going to arrive?
+static const double viewportTimerPollDelay = 0.5;
+
 AutoplayExperimentHelper::AutoplayExperimentHelper(HTMLMediaElement& element)
     : m_element(element)
     , m_mode(AutoplayExperimentConfig::Mode::Off)
     , m_playPending(false)
+    , m_registeredWithView(false)
+    , m_wasInViewport(false)
+    , m_lastLocationUpdateTime(-std::numeric_limits<double>::infinity())
+    , m_viewportTimer(this, &AutoplayExperimentHelper::viewportTimerFired)
 {
     if (document().settings()) {
         m_mode = AutoplayExperimentConfig::fromString(document().settings()->autoplayExperimentMode());
@@ -38,14 +46,19 @@ AutoplayExperimentHelper::AutoplayExperimentHelper(HTMLMediaElement& element)
 
 AutoplayExperimentHelper::~AutoplayExperimentHelper()
 {
+    unregisterForPositionUpdatesIfNeeded();
 }
 
 void AutoplayExperimentHelper::becameReadyToPlay()
 {
     // Assuming that we're eligible to override the user gesture requirement,
-    // then play.
+    // either play if we meet the visibility checks, or install a listener
+    // to wait for them to pass.
     if (isEligible()) {
-        prepareToPlay(GesturelessPlaybackStartedByAutoplayFlagImmediately);
+        if (meetsVisibilityRequirements())
+            prepareToPlay(GesturelessPlaybackStartedByAutoplayFlagImmediately);
+        else
+            registerForPositionUpdatesIfNeeded();
     }
 }
 
@@ -61,11 +74,19 @@ void AutoplayExperimentHelper::playMethodCalled()
         if (isEligible()) {
             // Remember that userGestureRequiredForPlay is required for
             // us to be eligible for the experiment.
-            // We are able to override the gesture requirement now, so
-            // do so.
-            prepareToPlay(GesturelessPlaybackStartedByPlayMethodImmediately);
+            // 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);
+            } else {
+                // Wait for viewport visibility.

[philipj_slow, 2015/09/21 15:02:04]

I guess it actually waits for either page visiblity or element visibility
depending on settings?

Also, I guess you need to also register for changes in page visibility? This
might be hard to write a test for, unless page visibility works in iframes.

[liberato (no reviews please), 2015/09/23 06:14:57]

it doesn't register for page visibility because the callback from FrameView
covers it.  we just register for that always.

[philipj_slow, 2015/09/25 15:32:27]

Acknowledged.

+                registerForPositionUpdatesIfNeeded();
+            }
         }
 
+    } else if (m_element.isUserGestureRequiredForPlay()) {
+        unregisterForPositionUpdatesIfNeeded();
     }
 }
 
@@ -73,18 +94,149 @@ void AutoplayExperimentHelper::pauseMethodCalled()
 {
     // Don't try to autoplay, if we would have.
     m_playPending = false;
+    unregisterForPositionUpdatesIfNeeded();
 }
 
 void AutoplayExperimentHelper::mutedChanged()
 {
-    // In other words, start playing if we just needed 'mute' to autoplay.
+    // If we are no longer eligible for the autoplay experiment, then also
+    // 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.
+    if (!isEligible()) {
+        unregisterForPositionUpdatesIfNeeded();
+    } else {
+        // Try to play.  If we can't, then install a listener.
+        if (!maybeStartPlaying())
+            registerForPositionUpdatesIfNeeded();
+    }
+}
+
+void AutoplayExperimentHelper::registerForPositionUpdatesIfNeeded()
+{
+    // If we don't require that the player is in the viewport, then we don't
+    // need the listener.
+    if (!enabled(AutoplayExperimentConfig::Mode::IfViewport)
+        && !enabled(AutoplayExperimentConfig::Mode::IfPageVisible))
+        return;
+
+    LayoutObject* layoutObject = m_element.layoutObject();
+    if (layoutObject && layoutObject->isMedia()) {
+        LayoutMedia* layoutMedia = static_cast<LayoutMedia*>(layoutObject);
+        layoutMedia->setRequestPositionUpdates(true);
+        m_registeredWithView = true;

[philipj_slow, 2015/09/21 15:02:05]

Here we're registering with the layout object, so should m_registeredWithView be
m_registeredWithLayoutObject?

[liberato (no reviews please), 2015/09/23 06:14:56]

Done.

+    }
+}
+
+void AutoplayExperimentHelper::unregisterForPositionUpdatesIfNeeded()
+{
+    if (m_registeredWithView) {
+        LayoutObject* obj = m_element.layoutObject();
+        if (obj && obj->isMedia()) {
+            LayoutMedia* layoutMedia = (LayoutMedia*)obj;

[philipj_slow, 2015/09/21 15:02:04]

static_cast

[liberato (no reviews please), 2015/09/23 06:14:56]

Done.

+            layoutMedia->setRequestPositionUpdates(false);
+            m_registeredWithView = false;
+        }
+    }
+}
+
+void AutoplayExperimentHelper::positionChanged()
+{
+    // Something, maybe position, has changed.  If applicable, start a
+    // timer to look for the end of a scroll operation.
+    // Don't do much work here.
+    // 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.
+
+    LocationState curLocation(m_element);
+    const bool inViewport = meetsVisibilityRequirements(curLocation);
+
+    // If the location has changed, then record the time of the last change.
+    if (m_lastLocation != curLocation) {
+        m_lastLocationUpdateTime = monotonicallyIncreasingTime();
+        m_lastLocation = curLocation;
+    }
+
+    // If we have transitioned to be visible, then start the timer.
+    if (inViewport && !m_wasInViewport) {
+        // We have transitioned from not visible to visible.  Reset the timer
+        // to check if we should start autoplay.  We do this to avoid
+        // resetting the timer too often in this callback.
+        m_viewportTimer.startOneShot(viewportTimerPollDelay, FROM_HERE);
+    }
+    m_wasInViewport = inViewport;
+}
+
+void AutoplayExperimentHelper::triggerAutoplayViewportCheck()
+{
+    // This method is for testing.
+
+    // If we're registered to receive updates on position, pretend that we did
+    // receive one.
+    if (m_registeredWithView) {
+        positionChanged();
+    }
+
+    // Make sure that the timer is actually active.  If not, then
+    // positionChanged wasn't called yet.
+    if (!m_viewportTimer.isActive()) {
+        return;

[philipj_slow, 2015/09/21 15:02:04]

Is this code path reached and if so when *is* positionChanged called? If it
isn't reached, then ASSERT

[liberato (no reviews please), 2015/09/23 06:14:56]

actually, the more i think about it, we don't want to check at all in case the
timer did fire.

[philipj_slow, 2015/09/25 15:32:27]

I see this block was removed.

+    }
+
+    // Make sure that the last update appears to be sufficiently far in the
+    // past to allow the test to succeed.

[philipj_slow, 2015/09/21 15:02:04]

I think "the test" here refers to an if-statement in viewportTimerFired(), I
tried parsing it as the test that calls triggerAutoplayViewportCheck() and that
didn't make as much sense. I guess say that it has to appear as if scrolling has
stopped?

[liberato (no reviews please), 2015/09/23 06:14:56]

Done.

+    m_lastLocationUpdateTime = monotonicallyIncreasingTime() - viewportTimerPollDelay - 1;
+    viewportTimerFired(nullptr);
+}
+
+void AutoplayExperimentHelper::viewportTimerFired(Timer<AutoplayExperimentHelper>*)
+{
+    double now = monotonicallyIncreasingTime();
+    double delta = now - m_lastLocationUpdateTime;
+    if (delta < viewportTimerPollDelay) {
+        // If we are not visible, then skip the timer.  It will be started
+        // again if we become visible again.
+        if (m_wasInViewport)
+            m_viewportTimer.startOneShot(viewportTimerPollDelay - delta, FROM_HERE);
+
+        return;
+    }
+
+    // Sufficient time has passed since the last scroll that we'll
+    // treat it as the end of scroll.  Autoplay if we should.
     maybeStartPlaying();
 }
 
+bool AutoplayExperimentHelper::meetsVisibilityRequirements(const LocationState& location) const
+{
+    if (enabled(AutoplayExperimentConfig::Mode::IfPageVisible)
+        && !location.isPageVisible())
+        return false;
+
+    return !enabled(AutoplayExperimentConfig::Mode::IfViewport)
+        || location.isInViewport();
+}
+
+bool AutoplayExperimentHelper::meetsVisibilityRequirements() const
+{
+    // We could check for eligibility here, but we skip it.  Some of our
+    // callers need to do it separately, and we don't want to check more

[philipj_slow, 2015/09/21 15:02:05]

Are there some callers that don't check it, and if so doesn't it matter to them?
If all call paths have the check, then ASSERT something.

[liberato (no reviews please), 2015/09/23 06:14:56]

mVR() is called quite often during visibility checks.  they don't care about the
rest of it.

both mVR and isEligible are well-defined in the absence of the other, so i'm not
sure on what one should assert.

[philipj_slow, 2015/09/25 15:32:27]

OK, I read "We could check for eligibility here, but we skip it" as "`if
(!isEligible()) return false` would make no difference" and so thought perhaps
there was invariant here.

+    // than we need to.
+
+    // If visibility isn't required, then it's visible enough.
+    if (!enabled(AutoplayExperimentConfig::Mode::IfViewport)
+        && !enabled(AutoplayExperimentConfig::Mode::IfPageVisible))
+        return true;
+
+    LocationState location(m_element);
+    return meetsVisibilityRequirements(location);
+}
+
 bool AutoplayExperimentHelper::maybeStartPlaying()
 {
     // See if we're allowed to autoplay now.
-    if (!isEligible()) {
+    if (!isEligible() || !meetsVisibilityRequirements()) {
         return false;
     }
 
@@ -123,23 +275,17 @@ bool AutoplayExperimentHelper::isEligible() const
     if (!m_playPending && !m_element.shouldAutoplay())
         return false;
 
-    // If the video is already playing, then do nothing.  Note that there
-    // is not a path where a user gesture is required but the video is
-    // playing.  However, we check for completeness.
-    if (!m_element.paused())
-        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(AutoplayExperimentConfig::Mode::IfMobile)
         && !document().viewportDescription().isLegacyViewportType())
         return false;
 
-    // If media is muted, then autoplay when it comes into view.
+    // If we require muted media and this is muted, then it is eligible.
     if (enabled(AutoplayExperimentConfig::Mode::IfMuted))
         return m_element.muted();
 
-    // Autoplay when it comes into view (if needed), maybe muted.
+    // Element is eligible for gesture override, maybe muted.
     return true;
 }
 
@@ -162,6 +308,7 @@ void AutoplayExperimentHelper::prepareToPlay(AutoplayMetrics metric)
     // once.  Be sure to do this before muteIfNeeded().
     m_element.removeUserGestureRequirement();
 
+    unregisterForPositionUpdatesIfNeeded();
     muteIfNeeded();
 
     // Record that this autoplayed without a user gesture.  This is normally
@@ -177,4 +324,87 @@ Document& AutoplayExperimentHelper::document() const
     return m_element.document();
 }
 
+AutoplayExperimentHelper::LocationState::LocationState(Element& element)
+    : m_valid(false)
+{
+    const LocalDOMWindow* domWindow = element.document().domWindow();
+    if (!domWindow)
+        return;
+
+    // Get the page visibility.
+    Frame* frame = domWindow->frame();
+    if (!frame)
+        return;
+
+    Page* page = frame->page();
+    if (!page)
+        return;
+
+    if (!element.layoutObject())

[philipj_slow, 2015/09/21 15:02:04]

Should this updateLayoutIgnorePendingStylesheets(), or is the idea that the
resize callbacks will invalidate this when the layout has happened so that it
doesn't matter?

[liberato (no reviews please), 2015/09/23 06:14:56]

i don't think that we need to update the layout here.  during scroll checking,
the layout is valid so it doesn't matter.

i've certainly had bad luck with causing a layout previously.  :)

if we get here and it isn't available, then we'll just register for a callback
and get notification when it's available.

[philipj_slow, 2015/09/25 15:32:27]

Acknowledged.

+        return;
+
+    const LayoutBox* elementBox = element.layoutObject()->enclosingBox();
+    if (!elementBox)
+        return;
+
+    float zoom = elementBox->style()->effectiveZoom();
+    IntRect us(elementBox->offsetLeft().toInt()
+        , elementBox->offsetTop().toInt()
+        , elementBox->clientWidth().toInt()
+        , elementBox->clientHeight().toInt());
+    IntRect screen(domWindow->scrollX()*zoom, domWindow->scrollY()*zoom, domWindow->innerWidth()*zoom, domWindow->innerHeight()*zoom);
+
+    m_visibilityState = page->visibilityState();
+    m_element = us;
+    m_screen = screen;
+    m_valid = true;
+}
+
+bool AutoplayExperimentHelper::LocationState::isInViewport() const
+{
+    // Check if we're in the viewport.  If the element is bigger than the
+    // viewport, then be happy if it covers it.

[philipj_slow, 2015/09/21 15:02:04]

This all seems pretty elaborate, why not just return element.intersects(screen)?

[liberato (no reviews please), 2015/09/23 06:14:56]

"contained in" just seemed nicer than "intersects with" when playing with it on
a device.

[philipj_slow, 2015/09/25 15:32:27]

It seems like it could be a bit annoying in cases where the video is relatively
big, but OK, if you've tested it :) As you know I'm not too keen element
visibility checks, but let's see what the experiment reveals.

+    if (!m_valid)
+        return false;
+
+    IntRect element = m_element;
+    // If element completely fills the screen, then truncate it to exactly
+    // match the screen.  Any element that is wider just has to cover.
+    if (element.x() <= m_screen.x()
+        && element.x() + element.width() >= m_screen.x() + m_screen.width()) {
+        element.setX(m_screen.x());
+        element.setWidth(m_screen.width());
+    }
+
+    if (element.y() <= m_screen.y()
+        && element.y() + element.height() >= m_screen.y() + m_screen.height()) {
+        element.setY(m_screen.y());
+        element.setHeight(m_screen.height());
+    }
+
+    // See if elemet is contained by the screen.

[philipj_slow, 2015/09/21 15:02:04]

Typo if this line doesn't go *poof*

[liberato (no reviews please), 2015/09/23 06:14:56]

poof.

+    return m_screen.contains(element);
+}
+
+bool AutoplayExperimentHelper::LocationState::isPageVisible() const
+{
+    // Check if we're in the viewport.  If the element is bigger than the

[philipj_slow, 2015/09/21 15:02:05]

This comment is copied from the above and isn't accurate here.

[liberato (no reviews please), 2015/09/23 06:14:56]

it doesn't add much over the single line of code, so i removed it.

+    // viewport, then be happy if it covers it.
+    return m_valid && m_visibilityState == PageVisibilityStateVisible;
+}
+
+bool AutoplayExperimentHelper::LocationState::operator==(const LocationState& them) const
+{
+    // If either state is not valid, then they are not equal.
+    return m_valid && them.valid()
+        && m_visibilityState == them.visibilityState()
+        && m_screen == them.screen()
+        && m_element == them.element();
+}
+
+bool AutoplayExperimentHelper::LocationState::operator!=(const LocationState& them) const
+{
+    return !((*this) == them);

[philipj_slow, 2015/09/21 15:02:04]

Don't need () around *this I think.

[liberato (no reviews please), 2015/09/23 06:14:56]

Done.

+}
+
 }