Include viewport visibility checks for autoplay experiment.

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 3aa5811dc2c6c232404f8dea9fbf040b989ce8f1..9a0b6d8ffbbf3b519cdc527f097d91b3afda9c37 100644
--- a/third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp
+++ b/third_party/WebKit/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?

[ojan, 2015/10/01 06:14:26]

Nit: This shouldn't really be a question. :) Also, it't not just scrolling that
we handle anymore. How about something like:
// Seconds to wait after a video has stopped moving before playing it.

[liberato (no reviews please), 2015/10/01 22:49:01]

Done.

+static const double viewportTimerPollDelay = 0.5;

[ojan, 2015/10/01 06:14:26]

We typically prefix static variables with a "k".

[esprehn, 2015/10/01 07:04:14]

that is constants, s_ or c prefixes are also okay

[ojan, 2015/10/01 18:00:19]

This is a constant, no?

[esprehn, 2015/10/01 18:22:42]

Yeah, sorry I wasn't clear. I meant to say for statics we use k, s_ or c as the
prefix. k and c are for constants, s_ is for mutable statics, although I think
there's some sloppy s_ stuff that's actually constants.

[liberato (no reviews please), 2015/10/01 22:49:01]

Done.

[liberato (no reviews please), 2015/10/01 22:49:02]

Acknowledged.

+
 AutoplayExperimentHelper::AutoplayExperimentHelper(HTMLMediaElement& element)
     : m_element(element)
     , m_mode(AutoplayExperimentConfig::Mode::Off)
     , m_playPending(false)
+    , m_registeredWithLayoutObject(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.
+                registerForPositionUpdatesIfNeeded();
+            }
         }
 
+    } else if (m_element.isUserGestureRequiredForPlay()) {
+        unregisterForPositionUpdatesIfNeeded();
     }
 }
 
@@ -73,18 +94,143 @@ 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))

[esprehn, 2015/10/01 07:04:13]

nit: I would do these as two separate if statements, then you don't need the
funny wrapping.

[liberato (no reviews please), 2015/10/01 22:49:01]

it doesn't seem particularly unusual to me, but done.

+        return;
+
+    LayoutObject* layoutObject = m_element.layoutObject();
+    if (layoutObject && layoutObject->isMedia()) {

[ojan, 2015/10/01 06:14:26]

This should always be a LayoutMedia. Also, we avoid using static_cast for these.
Instead, use toLayoutMedia, which will ASSERT_WITH_SECURITY_IMPLICATION that the
type is correct (those asserts are run on ClusterFuzz and debug builds). FYI,
this method is defined by the following line in LayoutMedia.h:
DEFINE_LAYOUT_OBJECT_TYPE_CASTS(LayoutMedia, isMedia());
 so change this code to:

if (LayoutObject* obj = m_element.layoutObject()) {
  LayoutMedia* layoutMedia = toLayoutMedia(obj);
  ...
}

[liberato (no reviews please), 2015/10/01 22:49:01]

Done.

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

[ojan, 2015/10/01 06:14:26]

ditto

[liberato (no reviews please), 2015/10/01 22:49:02]

Done.

+            LayoutMedia* layoutMedia = static_cast<LayoutMedia*>(obj);
+            layoutMedia->setRequestPositionUpdates(false);

[esprehn, 2015/10/01 07:04:14]

is the requestPositionUpdates flag the same as the m_registeredWithLayoutObject
flag? do you really need both?

[liberato (no reviews please), 2015/10/01 22:49:01]

in the current code, you're right -- we could just call it mroe than once, at
the cost of a search through a vector for duplicates.  however, in the new CL,
we do need both to handle the 'inserted into layout tree' case.

+            m_registeredWithLayoutObject = 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);

[esprehn, 2015/10/01 07:04:13]

currentLocation, don't abbreviate. I'd just call it current

[liberato (no reviews please), 2015/10/01 22:49:01]

renamed to currentLocation, here and elsewhere.

+    const bool inViewport = meetsVisibilityRequirements(curLocation);

[ojan, 2015/10/01 06:14:26]

Nit: We don't typically use const for local variables.

[liberato (no reviews please), 2015/10/01 22:49:02]

done, but why not?

+
+    // If the location has changed, then record the time of the last change.

[ojan, 2015/10/01 06:14:26]

Nit: This comment doesn't add value above what the code obviously does. Just
leave it out.

[liberato (no reviews please), 2015/10/01 22:49:02]

Done.

+    if (m_lastLocation != curLocation) {
+        m_lastLocationUpdateTime = monotonicallyIncreasingTime();
+        m_lastLocation = curLocation;
+    }
+
+    // If we have transitioned to be visible, then start the timer.

[ojan, 2015/10/01 06:14:26]

Ditto

[liberato (no reviews please), 2015/10/01 22:49:02]

Done.

+    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.

[ojan, 2015/10/01 06:14:26]

This comment is useful, but should focus on just the core part that explains
*why* we're doing this instead of also stating what the code clearly does.
Something like:
// Only reset the timer when we transition from not visible to visible to
// avoid resetting it too often since resetting the timer isn't cheap.

[liberato (no reviews please), 2015/10/01 22:49:01]

Done.

+        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_registeredWithLayoutObject) {
+        positionChanged();

[esprehn, 2015/10/01 07:04:14]

we often leave out the braces for single line ifs since it's shorter

[liberato (no reviews please), 2015/10/01 22:49:02]

i removed the whole check - it isn't needed with the new version of the
visibility check.

+    }
+
+    // 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() - 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)

[esprehn, 2015/10/01 07:04:14]

Can you just do a using on this Mode enum? The verbosity of doing
AutoplayExperimentConfig::Mode::... really makes this code hard to read.

I would probably just put it in the header as 

enum AutoplayMode {
  ...
};

and not use an enum class at all. The type safety thing is really not worth
making the code unreadable

[liberato (no reviews please), 2015/10/01 22:49:02]

done.  i moved the parser back to AutoplayExperimentHelper, too, where it began.

+        && !location.isPageVisible())
+        return false;
+
+    return !enabled(AutoplayExperimentConfig::Mode::IfViewport)
+        || location.isElementVisible();
+}
+
+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
+    // 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);

[esprehn, 2015/10/01 07:04:14]

Overloading like this is very confusing, don't have two methods with the same
name like this. It's also strange to reuse this LocationState object like this
both for storing state, and for creating on the stack

[liberato (no reviews please), 2015/10/01 22:49:02]

i'm not sure i see your point, but there's now only one of them anyway now that
LocationState has gone on a long quest for gold in a faraway browser.

+}
+
 bool AutoplayExperimentHelper::maybeStartPlaying()
 {
     // See if we're allowed to autoplay now.
-    if (!isEligible()) {
+    if (!isEligible() || !meetsVisibilityRequirements()) {
         return false;
     }
 
@@ -123,23 +269,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 +302,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 +318,91 @@ Document& AutoplayExperimentHelper::document() const
     return m_element.document();
 }
 
+AutoplayExperimentHelper::LocationState::LocationState(Element& element)

[ojan, 2015/10/01 06:14:26]

Mixing page visibility and element visibility into the same class is strange to
me. There's actually 0 overlap between the two codepaths. I don't think you need
a helper *class* for the page visibility stuff. A simple helper function
(isPageVisible) will do for page visibility.

[liberato (no reviews please), 2015/10/01 22:49:02]

Location state was removed.

+{
+    // Default to empty rectangles.  The screen is offset so that contains()
+    // returns false.

[ojan, 2015/10/01 06:14:26]

I'm not sure what value this provides. The only cases where you don't override
this are cases where we, e.g. don't have a layout object for the element, in
which case the two rects certainly won't be equal anyways.

[liberato (no reviews please), 2015/10/01 22:49:01]

removed.

+    m_element = IntRect();
+    m_screen = IntRect(-1, -1, 0, 0);
+    ASSERT(!m_screen.contains(m_element));

[ojan, 2015/10/01 06:14:26]

I don't think this assert provides value. It just tells us that contains doesn't
have a basic bug. :)

[liberato (no reviews please), 2015/10/01 22:49:01]

removed.

+    m_visibilityState = PageVisibilityStateHidden;
+
+    const LocalDOMWindow* domWindow = element.document().domWindow();

[esprehn, 2015/10/01 07:04:14]

Don't go through the DOMWindow to get the Frame or the Page. Get it from the
Document. You should almost never access the window

[liberato (no reviews please), 2015/10/01 22:49:02]

Done.

+    if (!domWindow)
+        return;
+
+    // Get the page visibility.
+    Frame* frame = domWindow->frame();
+    if (!frame)

[esprehn, 2015/10/01 07:04:14]

If this code is running you must have a frame and a page right? 

[liberato (no reviews please), 2015/10/01 22:49:01]

Removed.

+        return;
+
+    Page* page = frame->page();
+    if (!page)
+        return;
+
+    m_visibilityState = page->visibilityState();

[ojan, 2015/10/01 06:14:26]

You don't need to go through so much complexity to get the visiblityState. Just
do:
m_visibilityState = element.document().pageVisibilityState();

[liberato (no reviews please), 2015/10/01 22:49:01]

Done.

+
+    if (!element.layoutObject())

[esprehn, 2015/10/01 07:04:13]

How did the element end up in this code but without a layout object?

[liberato (no reviews please), 2015/10/01 22:49:02]

Done.

+        return;
+
+    const LayoutBox* elementBox = element.layoutObject()->enclosingBox();

[esprehn, 2015/10/01 07:04:14]

How do you know style and layout are updated? This reads stale values which is
bad, I think you want one of the methods on Element like getBoundingClientRect
(or the internal ones).

Also you should call document().updateLayout(). It'll be a no-op if layout is
not dirty

[liberato (no reviews please), 2015/10/01 22:49:02]

clientrect: using absolute..Rect(), per Ojan.

updateLayout: are you sure this is safe here?  the last time i forced a layout
update, everybody told me not to do that (and it crashed a lot).

+    if (!elementBox)
+        return;
+
+    float zoom = elementBox->style()->effectiveZoom();
+    m_element = IntRect(elementBox->offsetLeft().toInt()

[ojan, 2015/10/01 06:14:26]

This doesn't get you the right absolute coordinates. I believe you want
layoutObject->absoluteBoundingBoxFloatRect() as seen in
LayoutView::computeIntersectionObservations in
https://codereview.chromium.org/1330633003.

[liberato (no reviews please), 2015/10/01 22:49:01]

Done, though s/Float// since the viewport comes out as an IntRect anyway.

+        , elementBox->offsetTop().toInt()
+        , elementBox->clientWidth().toInt()
+        , elementBox->clientHeight().toInt());
+
+    m_screen = IntRect(domWindow->scrollX()*zoom

[ojan, 2015/10/01 06:14:26]

This isn't right. It doesn't walk up the tree to the root properly for frames.
You want to do something like what FrameView::computeIntersectionObservations
does in https://codereview.chromium.org/1330633003.

Also, do you need m_screen to be in LocationState? It seems to me that you could
just generate the m_screen value whenever you need it and pass it to
isElementVisible. I think the current code is doing equality checks that aren't
needed on m_screen. You don't need to care when the screen rect changes, just
when the video moves.

Now that we've removed page visibility and m_screen from this, all that's left
is the element rect. So, now I think we don't actually gain value from having a
helper class at all. You can just store the rect on the AutoplayExperimentHelper
and the two methods left in this class can just be helper methods on
AutoplayExperimentHelper instead.

That sound ok? I think that'd make the code easier to understand and removing
these m_screen equality checks would make the code faster.

[liberato (no reviews please), 2015/10/01 22:49:02]

i added a less cool version of mkb's code to FrameLayout.  Also removed
LocationState.  It still stores what the visible rect (what was m_screen) in the
AutoplayExperimentHelper class, so that it knows the most recent viewport
visibility.

+        , domWindow->scrollY()*zoom
+        , domWindow->innerWidth()*zoom
+        , domWindow->innerHeight()*zoom);
+}
+
+bool AutoplayExperimentHelper::LocationState::isElementVisible() const

[esprehn, 2015/10/01 07:04:13]

bool isElementVisble(IntRect screen, const Element& element)
{
  ...
}

you don't need this LocationState class

[liberato (no reviews please), 2015/10/01 22:49:02]

Done.

+{
+    // Check if we're in the viewport.  If the element is bigger than the
+    // viewport, then be happy if it covers it.
+
+    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.

[ojan, 2015/10/01 06:14:26]

What if the video is offset to the left a little bit (e.g. because of margins on
the body), but the right of the element is truncated.

I still think you should just look at the rect of the play button instead of the
whole video. It simplifies all this logic and means that whenever you start
autoplaying a video, the user is guaranteed to see the pause button, which seems
like a nice property that the current code doesn't give you.

[liberato (no reviews please), 2015/10/01 22:49:02]

offset to the left: the new code just insists that the video is visible in the
boundaries of all parents, or fills if it's too wide/high.  in your example,
some containing element would have to scroll to the right before it would
autoplay.

play button: i like the idea to check for the play button in principle, but we
have no way of knowing where the play button is unless it uses the default
controls.  we could do it for elements with the default controls, i guess, and
use some other behavior when they're not enabled, but that seems really hard to
explain to somebody.  if we can pin down this behavior better, we can compare it
to the current behavior in a future experiment.

+    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());
+    }
+
+    return m_screen.contains(element);
+}
+
+bool AutoplayExperimentHelper::LocationState::isPageVisible() const
+{
+    return m_visibilityState == PageVisibilityStateVisible;
+}
+
+bool AutoplayExperimentHelper::LocationState::operator==(const LocationState& them) const
+{
+    // As a special case, if either is missing position information, then
+    // they are not equal.
+    return !m_element.isEmpty() && !them.element().isEmpty()
+        && !m_screen.isEmpty() && !them.screen().isEmpty()
+        && m_visibilityState == them.visibilityState()
+        && m_screen == them.screen()
+        && m_element == them.element();
+}
+
+bool AutoplayExperimentHelper::LocationState::operator!=(const LocationState& them) const
+{
+    return !(*this == them);
+}
+
 }