Autoplay experiment metric fixes and additions.

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

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/dom/Document.h"
+#include "core/html/AutoplayExperimentHelper.h"
+#include "platform/UserGestureIndicator.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace blink {
+
+using namespace testing;
+using RecordMetricsBehavior = AutoplayExperimentHelper::Client::RecordMetricsBehavior;
+
+class MockAutoplayClient : public AutoplayExperimentHelper::Client {
+public:
+    enum ElementType { VIDEO,
+        AUDIO };

[philipj_slow, 2016/01/12 15:12:18]

Enum on a single line or each entry on a separate line.

[liberato (no reviews please), 2016/01/29 08:25:24]

Done.

+
+    MockAutoplayClient(const char* mode = "enabled-forvideo", ElementType type = VIDEO)

[philipj_slow, 2016/01/12 15:12:18]

The default value of at least mode doesn't make the tests below clearer.

[liberato (no reviews please), 2016/01/29 08:25:24]

Done.

+        : m_mode(mode)
+        , m_duration(100)
+    {
+        // Set up default behaviors that the helper is allowed to use or cache
+        // during construction.
+
+        EXPECT_CALL(*this, autoplayExperimentMode())
+            .WillRepeatedly(Return(m_mode));
+
+        // Use m_isVideo to answer these.
+        EXPECT_CALL(*this, isHTMLVideoElement())
+            .WillRepeatedly(Return(type == VIDEO));
+        EXPECT_CALL(*this, isHTMLAudioElement())
+            .WillRepeatedly(Return(type != VIDEO));

[philipj_slow, 2016/01/12 15:12:18]

type == AUDIO?

[liberato (no reviews please), 2016/01/29 08:25:24]

Done.

+
+        // Now set up some useful defaults.
+        // Remember that this are only evaluated once.
+        EXPECT_CALL(*this, duration())

[philipj_slow, 2016/01/12 15:12:18]

Reading
http://stackoverflow.com/questions/13933475/gmock-setting-default-actions-on-...
I'm thinking maybe ON_CALL(*this, duration()).WillByDefault(Return(m_duration))
would be a clearer way to achieve these defaults?

[liberato (no reviews please), 2016/01/29 08:25:24]

Done, plus NiceMock to supress the warnings.

[philipj_slow, 2016/02/02 08:08:15]

It worked, yay!

+            .WillRepeatedly(Return(m_duration));
+        EXPECT_CALL(*this, currentTime())
+            .WillRepeatedly(Return(0));
+
+        // Default to "not optimized for mobile" page.
+        EXPECT_CALL(*this, isLegacyViewportType())
+            .WillRepeatedly(Return(false));
+
+        // Other handy defaults.
+        EXPECT_CALL(*this, paused())
+            .WillRepeatedly(Return(true));
+        EXPECT_CALL(*this, pageVisibilityState())
+            .WillRepeatedly(Return(PageVisibilityStateVisible));
+        EXPECT_CALL(*this, absoluteBoundingBoxRect())
+            .WillRepeatedly(Return(
+                IntRect(10, 10, 100, 100)));
+
+        // Normally, the autoplay experiment should not modify lots of other
+        // state unless we explicitly expect it.
+        EXPECT_CALL(*this, setMuted(_))
+            .Times(0);
+        EXPECT_CALL(*this, removeUserGestureRequirement())
+            .Times(0);
+        EXPECT_CALL(*this, setRequestPositionUpdates(true))
+            .Times(0);
+        EXPECT_CALL(*this, recordAutoplayMetric(_))
+            .Times(0);
+    }
+
+    virtual ~MockAutoplayClient() {}
+
+    MOCK_CONST_METHOD0(currentTime, double());
+    MOCK_CONST_METHOD0(duration, double());
+    MOCK_CONST_METHOD0(paused, bool());
+    MOCK_CONST_METHOD0(muted, bool());
+    MOCK_METHOD1(setMuted, void(bool));
+    MOCK_METHOD0(playInternal, void());
+    MOCK_CONST_METHOD0(isUserGestureRequiredForPlay, bool());
+    MOCK_METHOD0(removeUserGestureRequirement, void());
+    MOCK_METHOD1(recordAutoplayMetric, void(AutoplayMetrics));
+    MOCK_METHOD1(shouldAutoplay, bool(RecordMetricsBehavior));
+    MOCK_CONST_METHOD0(isHTMLVideoElement, bool());
+    MOCK_CONST_METHOD0(isHTMLAudioElement, bool());
+    MOCK_METHOD0(isLegacyViewportType, bool());
+    MOCK_CONST_METHOD0(pageVisibilityState, PageVisibilityState());
+    MOCK_CONST_METHOD0(autoplayExperimentMode, String());
+    MOCK_METHOD1(setRequestPositionUpdates, void(bool));
+    MOCK_CONST_METHOD0(absoluteBoundingBoxRect, IntRect());
+
+    const char* m_mode;
+    // const since changes to it won't affect the mocked value.
+    const double m_duration;
+};
+
+class AutoplayExperimentTest : public ::testing::Test {
+public:
+    AutoplayExperimentTest()
+        : client(nullptr)
+        , helper(nullptr)
+    {
+    }
+    ~AutoplayExperimentTest()
+    {
+        if (helper)
+            delete helper;

[philipj_slow, 2016/01/12 15:12:18]

Looks like you really want an auto pointer, so OwnPtr<>?

[liberato (no reviews please), 2016/01/29 08:25:25]

Done.

+
+        if (client)
+            delete client;
+    }
+
+    bool isEligible()
+    {
+        return helper->isEligible();
+    }
+
+    void setInterface(MockAutoplayClient* client)
+    {
+        this->client = client;
+
+        // Set some defaults.
+        setUserGestureRequiredForPlay(true);
+        setShouldAutoplay(true);
+        setIsMuted(false);
+
+        helper = new AutoplayExperimentHelper(*client);
+    }
+
+    MockAutoplayClient* client;

[philipj_slow, 2016/01/12 15:12:18]

https://www.chromium.org/blink/coding-style#TOC-Names says m_client and
m_helper.

[liberato (no reviews please), 2016/01/29 08:25:24]

don't know why i did that.  thanks.

+    AutoplayExperimentHelper* helper;
+
+    // Mirror updatePlayState to transition to play.
+    void startPlayback()
+    {
+        EXPECT_CALL(*client, paused())
+            .WillRepeatedly(Return(false));
+        EXPECT_CALL(*client, recordAutoplayMetric(AnyPlaybackStarted))
+            .Times(1);
+        helper->playbackStarted();
+    }
+
+    void startPlaybackWithoutUserGesture()
+    {
+        EXPECT_FALSE(UserGestureIndicator::processingUserGesture());
+        startPlayback();
+    }
+
+    void startPlaybackWithUserGesture()
+    {
+        UserGestureIndicator indicator(DefinitelyProcessingUserGesture);
+        EXPECT_TRUE(UserGestureIndicator::processingUserGesture());
+        startPlayback();
+    }
+
+    void setCurrentTimeToEnd()
+    {
+        EXPECT_CALL(*client, currentTime())

[philipj_slow, 2016/01/12 15:12:18]

Is this how unit tests are normally written? I'm used to seeing EXPECT_CALL to
actually expect that a method will be called (and fail otherwise), but here and
many places in this test it's apparently used only for the side effect of
causing m_duration to be returned.

Maybe use ON_CALL? Or have the state on some object and directly update that
state?

[liberato (no reviews please), 2016/01/29 08:25:24]

moved to ON_CALL, and make a NiceMock out of it to supress the warnings.  that
was the only reason i used EXPECT.

+            .WillRepeatedly(Return(client->m_duration));
+    }
+
+    void setUserGestureRequiredForPlay(bool required)
+    {
+        EXPECT_CALL(*client, isUserGestureRequiredForPlay())
+            .WillRepeatedly(Return(required));
+    }
+
+    void setShouldAutoplay(bool should)
+    {
+        EXPECT_CALL(*client, shouldAutoplay(_))
+            .WillRepeatedly(Return(should));
+    }
+
+    void setIsMuted(bool isMuted)
+    {
+        EXPECT_CALL(*client, muted())
+            .WillRepeatedly(Return(isMuted));
+    }
+
+    void pausePlaybackExpectingBailout(bool expectingAutoplay)
+    {
+        EXPECT_CALL(*client, recordAutoplayMetric(AnyPlaybackPaused))
+            .Times(1);
+        EXPECT_CALL(*client, recordAutoplayMetric(AnyPlaybackBailout))
+            .Times(1);
+        if (expectingAutoplay) {
+            EXPECT_CALL(*client, recordAutoplayMetric(AutoplayPaused))
+                .Times(1);
+            EXPECT_CALL(*client, recordAutoplayMetric(AutoplayBailout))
+                .Times(1);
+        }
+        helper->pauseMethodCalled();
+    }
+
+    void pausePlaybackNotExpectingBailout(bool expectingAutoplay)

[philipj_slow, 2016/01/12 15:12:18]

Doesn't this have to EXPECT_CALL *something* to fail if
recordAutoplayMetric(AnyPlaybackBailout) is called anyway?

[liberato (no reviews please), 2016/01/29 08:25:24]

added Times(0) for both bailout cases.

+    {
+        EXPECT_CALL(*client, recordAutoplayMetric(AnyPlaybackPaused))
+            .Times(1);
+        if (expectingAutoplay)
+            EXPECT_CALL(*client, recordAutoplayMetric(AutoplayPaused)).Times(1);
+        setCurrentTimeToEnd();
+        helper->pauseMethodCalled();
+    }
+
+    void endPlayback(bool expectingAutoplay)
+    {
+        EXPECT_CALL(*client, recordAutoplayMetric(AnyPlaybackComplete))
+            .Times(1);
+        if (expectingAutoplay)
+            EXPECT_CALL(*client, recordAutoplayMetric(AutoplayComplete)).Times(1);
+        helper->playbackEnded();
+    }
+
+    void moveIntoViewport()
+    {
+        helper->positionChanged(IntRect(0, 0, 200, 200));
+        helper->triggerAutoplayViewportCheckForTesting();
+    }
+};
+
+TEST_F(AutoplayExperimentTest, IsNotEligibleWithEmptyMode)
+{
+    setInterface(new MockAutoplayClient(""));
+    EXPECT_FALSE(isEligible());
+}
+
+TEST_F(AutoplayExperimentTest, IsVideoEligibleForVideoMode)
+{
+    // Video should be eligible in "forvideo" mode.
+    setInterface(new MockAutoplayClient());
+    EXPECT_TRUE(isEligible());
+}
+
+TEST_F(AutoplayExperimentTest, IsAudioNotEligibleForVideoMode)
+{
+    // Audio should not be eligible for video mode.
+    setInterface(new MockAutoplayClient("enabled-forvideo", MockAutoplayClient::AUDIO));
+    EXPECT_FALSE(isEligible());
+}
+
+TEST_F(AutoplayExperimentTest, IsEligibleRequiresUserGesture)
+{
+    setInterface(new MockAutoplayClient());
+    // If a user gesture is not required, then we're not eligible.
+    EXPECT_CALL(*client, isUserGestureRequiredForPlay())
+        .WillRepeatedly(Return(false));
+    EXPECT_FALSE(isEligible());
+}
+
+TEST_F(AutoplayExperimentTest, IsEligibleRequiresShouldAutoplay)
+{
+    setInterface(new MockAutoplayClient());
+    // If we shouldn't autoplay, then we're not eligible.
+    EXPECT_CALL(*client, shouldAutoplay(_))
+        .WillRepeatedly(Return(false));
+    EXPECT_FALSE(isEligible());
+}
+
+TEST_F(AutoplayExperimentTest, IsAudioEligibleForAudioMode)
+{
+    setInterface(new MockAutoplayClient("enabled-foraudio", MockAutoplayClient::AUDIO));
+    EXPECT_TRUE(isEligible());
+}
+
+TEST_F(AutoplayExperimentTest, EligibleIfOptimizedForMobile)
+{
+    setInterface(new MockAutoplayClient("enabled-forvideo-ifmobile"));
+    // Should not be eligible with our default of "not mobile".
+    EXPECT_FALSE(isEligible());
+
+    EXPECT_CALL(*client, isLegacyViewportType())
+        .WillRepeatedly(Return(true));
+    EXPECT_TRUE(isEligible());
+}
+
+TEST_F(AutoplayExperimentTest, EligibleIfMuted)
+{
+    setInterface(new MockAutoplayClient("enabled-forvideo-ifmuted"));
+    // Should not be eligible with our default of "not muted".
+    EXPECT_FALSE(isEligible());
+
+    EXPECT_CALL(*client, muted())
+        .WillRepeatedly(Return(true));
+    EXPECT_TRUE(isEligible());
+}
+
+TEST_F(AutoplayExperimentTest, BecameReadyAutoplayThenBailout)
+{
+    setInterface(new MockAutoplayClient());
+
+    EXPECT_CALL(*client, removeUserGestureRequirement())
+        .Times(1);
+    EXPECT_CALL(*client, recordAutoplayMetric(AutoplayMediaFound))
+        .Times(1);
+    helper->becameReadyToPlay();
+
+    EXPECT_CALL(*client, recordAutoplayMetric(GesturelessPlaybackStartedByAutoplayFlagImmediately))
+        .Times(1);
+    startPlaybackWithoutUserGesture();
+
+    pausePlaybackExpectingBailout(true);
+}
+
+TEST_F(AutoplayExperimentTest, BecameReadyAutoplayThenPause)
+{
+    setInterface(new MockAutoplayClient());
+
+    EXPECT_CALL(*client, removeUserGestureRequirement())
+        .Times(1);
+    EXPECT_CALL(*client, recordAutoplayMetric(AutoplayMediaFound))
+        .Times(1);
+    helper->becameReadyToPlay();
+
+    EXPECT_CALL(*client, recordAutoplayMetric(GesturelessPlaybackStartedByAutoplayFlagImmediately))
+        .Times(1);
+    startPlaybackWithoutUserGesture();
+
+    pausePlaybackNotExpectingBailout(true);
+}
+
+TEST_F(AutoplayExperimentTest, BecameReadyAutoplayThenComplete)
+{
+    setInterface(new MockAutoplayClient());
+
+    EXPECT_CALL(*client, removeUserGestureRequirement())
+        .Times(1);
+    EXPECT_CALL(*client, recordAutoplayMetric(AutoplayMediaFound))
+        .Times(1);
+    helper->becameReadyToPlay();
+
+    EXPECT_CALL(*client, recordAutoplayMetric(GesturelessPlaybackStartedByAutoplayFlagImmediately))
+        .Times(1);
+    startPlaybackWithoutUserGesture();
+
+    // Now stop at the end.
+    endPlayback(true);
+}
+
+TEST_F(AutoplayExperimentTest, NoUserGestureNeededShouldNotOverride)
+{
+    // Make sure that we don't override the user gesture if it isn't needed.
+    setInterface(new MockAutoplayClient());
+    setUserGestureRequiredForPlay(false);
+
+    // It is still autoplay media, though.
+    EXPECT_CALL(*client, recordAutoplayMetric(AutoplayMediaFound))
+        .Times(1);
+    helper->becameReadyToPlay();
+
+    EXPECT_CALL(*client, recordAutoplayMetric(GesturelessPlaybackUnknownReason))
+        .Times(1);
+    startPlaybackWithoutUserGesture();
+}
+
+TEST_F(AutoplayExperimentTest, NoAutoplayMetricsIfNoAutoplay)
+{
+    // If playback is started while processing a user gesture, then nothing
+    // should be overridden or logged about autoplay.
+    setInterface(new MockAutoplayClient());
+    setUserGestureRequiredForPlay(false);
+    setShouldAutoplay(false);
+    startPlaybackWithUserGesture();
+
+    // Expect bailout, but not from autoplay.
+    pausePlaybackExpectingBailout(false);
+}
+
+TEST_F(AutoplayExperimentTest, PlayMethodThenBailout)
+{
+    setInterface(new MockAutoplayClient());
+    setShouldAutoplay(false); // No autoplay attribute.
+
+    EXPECT_CALL(*client, removeUserGestureRequirement())
+        .Times(1);
+    EXPECT_CALL(*client, recordAutoplayMetric(AutoplayMediaFound))
+        .Times(1);
+    helper->playMethodCalled();
+
+    EXPECT_CALL(*client, recordAutoplayMetric(GesturelessPlaybackStartedByPlayMethodImmediately))
+        .Times(1);
+    startPlaybackWithoutUserGesture();
+
+    pausePlaybackExpectingBailout(true);
+}
+
+TEST_F(AutoplayExperimentTest, DeferAutoplayUntilMuted)
+{
+    setInterface(new MockAutoplayClient("enabled-forvideo-ifmuted"));
+
+    // Should not override the gesture requirement yet.
+    EXPECT_CALL(*client, recordAutoplayMetric(AutoplayMediaFound))
+        .Times(1);
+    helper->becameReadyToPlay();
+
+    // When we toggle the muted attribute, it should start.
+    EXPECT_CALL(*client, removeUserGestureRequirement())
+        .Times(1);
+    EXPECT_CALL(*client, playInternal())
+        .Times(1);
+    setIsMuted(true);
+    helper->mutedChanged();
+
+    // When playback starts (in response to playInternal()), it should also
+    // record why.  'After scroll' isn't the best name, but this isn't a common case.
+    EXPECT_CALL(*client, recordAutoplayMetric(GesturelessPlaybackStartedByAutoplayFlagAfterScroll))
+        .Times(1);
+    startPlaybackWithoutUserGesture();
+}
+
+TEST_F(AutoplayExperimentTest, DeferPlaybackUntilInViewport)
+{
+    setInterface(new MockAutoplayClient("enabled-forvideo-ifviewport"));
+
+    // Should not override the gesture requirement yet.
+    EXPECT_CALL(*client, recordAutoplayMetric(AutoplayMediaFound))
+        .Times(1);
+    EXPECT_CALL(*client, setRequestPositionUpdates(true))
+        .Times(1);
+    helper->becameReadyToPlay();
+
+    EXPECT_CALL(*client, removeUserGestureRequirement())
+        .Times(1);
+    EXPECT_CALL(*client, playInternal())
+        .Times(1);
+    EXPECT_CALL(*client, setRequestPositionUpdates(false))
+        .Times(1);
+    moveIntoViewport();
+}
+}