Include viewport visibility checks for autoplay experiment.

Include viewport visibility checks for autoplay experiment.

If the value of the autoplayExperimentMode setting contains
"-ifviewport", then the autoplay experiment will additionally
require that the media is located in the viewport bounds.
Autoplay is deferred until it comes into view, assuming that
it passes any other autoplay experiment checks that have been
turned on.

If the value of the autoplayExperimentMode "-ifpagevisible",
then we require that the page is visible.

BUG=487345, 402044
Committed: https://crrev.com/e894363cd4e9b052486c26c2c3ab2adf6a500ae5
Cr-Commit-Position: refs/heads/master@{#357911}

[liberato (no reviews please), 2015-09-25 16:42:41]

liberato@chromium.org changed reviewers:
+ dglazkov@chromium.org, esprehn@chromium.org, ojan@chromium.org,
philipj@opera.com

[liberato (no reviews please), 2015-09-25 16:42:41]

Hi folks

updated CL for the blink merge!

This CL is identical to PS4 of https://codereview.chromium.org/1329853004/
except rebased and post-blink-merge.  The latter has been closed.

thanks
-fl

[esprehn, 2015-09-26 03:43:13]

Okay I'll review this soon!

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

Sorry again it took so long to review this. It's a large patch and quite
daunting even after you've split it up. :)

Hopefully the next review will be much faster since I will understand the code
better.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html
(right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:13:
Flags  - autoplay experiment setting being tested.
You might want to test viewport-based visibility for a video inside an iframe as
well (i.e. the video is visible inside the iframe, but the iframe itself is
offscreen). I'm pretty sure the current code gets that wrong.

The other case that I think the current code gets wrong is a video inside of a
position:absolute or transform:translate2d container where the top/left or the
transform puts it offscreen.

These are both things that should be fixed by my suggestions above.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:25: // scroll
events are going to arrive?
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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:26: static
const double viewportTimerPollDelay = 0.5;
We typically prefix static variables with a "k".

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:124: if
(layoutObject && layoutObject->isMedia()) {
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);
  ...
}

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:135: if (obj &&
obj->isMedia()) {
ditto

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:153: const bool
inViewport = meetsVisibilityRequirements(curLocation);
Nit: We don't typically use const for local variables.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:155: // If the
location has changed, then record the time of the last change.
Nit: This comment doesn't add value above what the code obviously does. Just
leave it out.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:161: // If we
have transitioned to be visible, then start the timer.
Ditto

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:165: //
resetting the timer too often in this callback.
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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:321:
AutoplayExperimentHelper::LocationState::LocationState(Element& element)
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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:324: // returns
false.
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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:327:
ASSERT(!m_screen.contains(m_element));
I don't think this assert provides value. It just tells us that contains doesn't
have a basic bug. :)

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:343:
m_visibilityState = page->visibilityState();
You don't need to go through so much complexity to get the visiblityState. Just
do:
m_visibilityState = element.document().pageVisibilityState();

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:353: m_element
= IntRect(elementBox->offsetLeft().toInt()
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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:358: m_screen =
IntRect(domWindow->scrollX()*zoom
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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:371: // match
the screen.  Any element that is wider just has to cover.
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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:158: if (!view())
Pretty sure this isn't possible.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutView.h (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutView.h:205: void
registerMediaForPositionChangeNotification(LayoutMedia*);
Please add:
TODO(ojan): Merge this with IntersectionObserver once it lands.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/testing/Internals.idl:357:
[TypeChecking=Interface] void triggerAutoplayViewportCheck(HTMLMediaElement
mediaElement);
We typically try to avoid adding one-off APIs like this. Unfortunately, since we
don't have a great track record of unittests, that means we handle things via
regression tests (i.e. layout tests), which aren't very friendly to exposing the
guts of the system. As an aside, feel free to write unittests whenever you can.
We're happy to have more unittests and fewer layout tests where it's not too
onerous to get setup.

Since this will go away when we remove the experiment code, I'm OK with taking
on this small bit of technical debt temporarily. But, please add a
TODO(liberato) to remove this once the experiment is done in all the headers
this method is defined in and in the calling code (i.e. in the layout test).

[esprehn, 2015-10-01 07:04:15]

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html
(right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:123:
smallType += (type.indexOf("-ifpagevisible")>-1)?"P":"";
this test is huge and complicated, it should really be broken into multiple
tests

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:295:
&& spec.experimentType.indexOf("-foraudio") > -1)
string.includes is what you want.

spec.experimentType.includes("-foraudio")

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:301:
else if (spec.experimentType.indexOf("-ifmuted") > -1
ditto for all of these

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:319:
&& spec.experimentType.indexOf("-ifpagevisible") <= -1)
!string.includes(...)

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:26: static
const double viewportTimerPollDelay = 0.5;
On 2015/10/01 at 06:14:26, ojan wrote:
> We typically prefix static variables with a "k".

that is constants, s_ or c prefixes are also okay

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:120: &&
!enabled(AutoplayExperimentConfig::Mode::IfPageVisible))
nit: I would do these as two separate if statements, then you don't need the
funny wrapping.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:137:
layoutMedia->setRequestPositionUpdates(false);
is the requestPositionUpdates flag the same as the m_registeredWithLayoutObject
flag? do you really need both?

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:152:
LocationState curLocation(m_element);
currentLocation, don't abbreviate. I'd just call it current

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:178:
positionChanged();
we often leave out the braces for single line ifs since it's shorter

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:207: if
(enabled(AutoplayExperimentConfig::Mode::IfPageVisible)
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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:227: return
meetsVisibilityRequirements(location);
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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:330: const
LocalDOMWindow* domWindow = element.document().domWindow();
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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:336: if
(!frame)
If this code is running you must have a frame and a page right?

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:345: if
(!element.layoutObject())
How did the element end up in this code but without a layout object?

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:348: const
LayoutBox* elementBox = element.layoutObject()->enclosingBox();
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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:364: bool
AutoplayExperimentHelper::LocationState::isElementVisible() const
bool isElementVisble(IntRect screen, const Element& element)
{
  ...
}

you don't need this LocationState class

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:90:
LocationState(Element&);
const Element& ?

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:93: const
IntRect& element() const { return m_element; }
elementRect() ? element is confusing

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:94: const
IntRect& screen() const { return m_screen; }
screenRect

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:179: friend class
Internals;
We don't generally do friends like this, we just add ForTesting() methods. I
know this isn't part of this patch though.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:126:
updatePositionChangeRegistration(false);
inline the remove case here

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:136:
updatePositionChangeRegistration(m_wantPositionChangeNotifications);
remove this, you don't need to hook inserted at all

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:139: void
LayoutMedia::willBeRemovedFromTree()
why do you hook removal? I don't think you need this, just destroyed

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:166: if (doRegister)
Remove this method

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:177:
updatePositionChangeRegistration(m_wantPositionChangeNotifications);
inline the positive case here

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutMedia.h (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.h:97: bool
m_wantPositionChangeNotifications;
: 1; for both

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutView.cpp:959: void
LayoutView::registerMediaForPositionChangeNotification(LayoutMedia* media)
reference

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutView.cpp:965: void
LayoutView::unregisterMediaForPositionChangeNotification(LayoutMedia* media)
reference

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutView.cpp:975:
media->notifyPositionMayHaveChanged();
This isn't really the same timing as intersection observer, it would be on a
Timer, I think we should do that for this too otherwise you'll depend on the
synchronous dispatch which is bad

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/testing/Internals.idl:357:
[TypeChecking=Interface] void triggerAutoplayViewportCheck(HTMLMediaElement
mediaElement);
You should be able to write a Sim test for this, see
DocumentLoadingRenderingTest. that would remove the need for this

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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:26: static
const double viewportTimerPollDelay = 0.5;
On 2015/10/01 at 07:04:14, esprehn wrote:
> On 2015/10/01 at 06:14:26, ojan wrote:
> > We typically prefix static variables with a "k".
> 
> that is constants, s_ or c prefixes are also okay

This is a constant, no?

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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:26: static
const double viewportTimerPollDelay = 0.5;
On 2015/10/01 at 18:00:19, ojan wrote:
> On 2015/10/01 at 07:04:14, esprehn wrote:
> > On 2015/10/01 at 06:14:26, ojan wrote:
> > > We typically prefix static variables with a "k".
> > 
> > that is constants, s_ or c prefixes are also okay
> 
> This is a constant, no?

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:03]

i think this addresses all of the cl feedback, except as noted in my comments.

it works with nested iframes and browser zoom.  i'll do more manual testing on
it, since it was a lot of changes, but so far so good.

wanted to get this out now, though, just in case it can still make M47 to get
this experiment underway.

thanks
-fl

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html
(right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:13:
Flags  - autoplay experiment setting being tested.
On 2015/10/01 06:14:26, ojan wrote:
> You might want to test viewport-based visibility for a video inside an iframe
as
> well (i.e. the video is visible inside the iframe, but the iframe itself is
> offscreen). I'm pretty sure the current code gets that wrong.
> 
> The other case that I think the current code gets wrong is a video inside of a
> position:absolute or transform:translate2d container where the top/left or the
> transform puts it offscreen.
> 
> These are both things that should be fixed by my suggestions above.

i tried lots of iframes manually, and they work as expected.  i haven't managed
to produce a test for them yet.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:123:
smallType += (type.indexOf("-ifpagevisible")>-1)?"P":"";
On 2015/10/01 07:04:13, esprehn wrote:
> this test is huge and complicated, it should really be broken into multiple
> tests

it's mostly setup code for the test cases.  i can split them up, but i don't
think it would be any simpler.

since we all agree that this shouldn't be a layout test anyway, it doesn't seem
like it would buy anything.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:295:
&& spec.experimentType.indexOf("-foraudio") > -1)
On 2015/10/01 07:04:13, esprehn wrote:
> string.includes is what you want.
> 
> spec.experimentType.includes("-foraudio")

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:301:
else if (spec.experimentType.indexOf("-ifmuted") > -1
On 2015/10/01 07:04:13, esprehn wrote:
> ditto for all of these

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/media/video-autoplay-experiment-modes.html:319:
&& spec.experimentType.indexOf("-ifpagevisible") <= -1)
On 2015/10/01 07:04:13, esprehn wrote:
> !string.includes(...)

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:25: // scroll
events are going to arrive?
On 2015/10/01 06:14:26, ojan wrote:
> 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.

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:26: static
const double viewportTimerPollDelay = 0.5;
On 2015/10/01 07:04:14, esprehn wrote:
> On 2015/10/01 at 06:14:26, ojan wrote:
> > We typically prefix static variables with a "k".
> 
> that is constants, s_ or c prefixes are also okay

Acknowledged.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:26: static
const double viewportTimerPollDelay = 0.5;
On 2015/10/01 06:14:26, ojan wrote:
> We typically prefix static variables with a "k".

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:120: &&
!enabled(AutoplayExperimentConfig::Mode::IfPageVisible))
On 2015/10/01 07:04:13, esprehn wrote:
> nit: I would do these as two separate if statements, then you don't need the
> funny wrapping.

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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:124: if
(layoutObject && layoutObject->isMedia()) {
On 2015/10/01 06:14:26, ojan wrote:
> 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);
>   ...
> }

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:135: if (obj &&
obj->isMedia()) {
On 2015/10/01 06:14:26, ojan wrote:
> ditto

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:137:
layoutMedia->setRequestPositionUpdates(false);
On 2015/10/01 07:04:14, esprehn wrote:
> is the requestPositionUpdates flag the same as the
m_registeredWithLayoutObject
> flag? do you really need both?

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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:152:
LocationState curLocation(m_element);
On 2015/10/01 07:04:13, esprehn wrote:
> currentLocation, don't abbreviate. I'd just call it current

renamed to currentLocation, here and elsewhere.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:153: const bool
inViewport = meetsVisibilityRequirements(curLocation);
On 2015/10/01 06:14:26, ojan wrote:
> Nit: We don't typically use const for local variables.

done, but why not?

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:155: // If the
location has changed, then record the time of the last change.
On 2015/10/01 06:14:26, ojan wrote:
> Nit: This comment doesn't add value above what the code obviously does. Just
> leave it out.

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:161: // If we
have transitioned to be visible, then start the timer.
On 2015/10/01 06:14:26, ojan wrote:
> Ditto

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:165: //
resetting the timer too often in this callback.
On 2015/10/01 06:14:26, ojan wrote:
> 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.

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:178:
positionChanged();
On 2015/10/01 07:04:14, esprehn wrote:
> we often leave out the braces for single line ifs since it's shorter

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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:207: if
(enabled(AutoplayExperimentConfig::Mode::IfPageVisible)
On 2015/10/01 07:04:14, esprehn wrote:
> 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

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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:227: return
meetsVisibilityRequirements(location);
On 2015/10/01 07:04:14, esprehn wrote:
> 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

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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:321:
AutoplayExperimentHelper::LocationState::LocationState(Element& element)
On 2015/10/01 06:14:26, ojan wrote:
> 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.

Location state was removed.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:324: // returns
false.
On 2015/10/01 06:14:26, ojan wrote:
> 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.

removed.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:327:
ASSERT(!m_screen.contains(m_element));
On 2015/10/01 06:14:26, ojan wrote:
> I don't think this assert provides value. It just tells us that contains
doesn't
> have a basic bug. :)

removed.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:330: const
LocalDOMWindow* domWindow = element.document().domWindow();
On 2015/10/01 07:04:14, esprehn wrote:
> 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

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:336: if
(!frame)
On 2015/10/01 07:04:14, esprehn wrote:
> If this code is running you must have a frame and a page right? 

Removed.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:343:
m_visibilityState = page->visibilityState();
On 2015/10/01 06:14:26, ojan wrote:
> You don't need to go through so much complexity to get the visiblityState.
Just
> do:
> m_visibilityState = element.document().pageVisibilityState();

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:345: if
(!element.layoutObject())
On 2015/10/01 07:04:13, esprehn wrote:
> How did the element end up in this code but without a layout object?

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:348: const
LayoutBox* elementBox = element.layoutObject()->enclosingBox();
On 2015/10/01 07:04:14, esprehn wrote:
> 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

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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:353: m_element
= IntRect(elementBox->offsetLeft().toInt()
On 2015/10/01 06:14:26, ojan wrote:
> 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.

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

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:358: m_screen =
IntRect(domWindow->scrollX()*zoom
On 2015/10/01 06:14:26, ojan wrote:
> 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.

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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:364: bool
AutoplayExperimentHelper::LocationState::isElementVisible() const
On 2015/10/01 07:04:13, esprehn wrote:
> bool isElementVisble(IntRect screen, const Element& element)
> {
>   ...
> }
> 
> you don't need this LocationState class

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:371: // match
the screen.  Any element that is wider just has to cover.
On 2015/10/01 06:14:26, ojan wrote:
> 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.

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.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:90:
LocationState(Element&);
On 2015/10/01 07:04:14, esprehn wrote:
> const Element& ?

locationstate removed.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:93: const
IntRect& element() const { return m_element; }
On 2015/10/01 07:04:14, esprehn wrote:
> elementRect() ? element is confusing

locationstate removed.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:94: const
IntRect& screen() const { return m_screen; }
On 2015/10/01 07:04:14, esprehn wrote:
> screenRect

ditto.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.h:179: friend class
Internals;
On 2015/10/01 07:04:14, esprehn wrote:
> We don't generally do friends like this, we just add ForTesting() methods. I
> know this isn't part of this patch though.

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:126:
updatePositionChangeRegistration(false);
On 2015/10/01 07:04:14, esprehn wrote:
> inline the remove case here

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:136:
updatePositionChangeRegistration(m_wantPositionChangeNotifications);
On 2015/10/01 07:04:14, esprehn wrote:
> remove this, you don't need to hook inserted at all

inserted is needed to catch cases where an element is removed and re-added to
the tree via JS.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:139: void
LayoutMedia::willBeRemovedFromTree()
On 2015/10/01 07:04:14, esprehn wrote:
> why do you hook removal? I don't think you need this, just destroyed

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:158: if (!view())
On 2015/10/01 06:14:26, ojan wrote:
> Pretty sure this isn't possible.

it does happen during the test:

#0 0x000000463b2b base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7f12c31c6340 <unknown>
#2 0x000001aa8454
blink::LayoutView::unregisterMediaForPositionChangeNotification()
#3 0x000001a5e8af blink::LayoutMedia::willBeDestroyed()
#4 0x000001a6e02d blink::LayoutObject::destroy()
#5 0x0000014bf081 blink::Node::detach()
#6 0x0000014782c8 blink::ContainerNode::detach()
#7 0x0000014a69a5 blink::Element::detach()
#8 0x0000014782af blink::ContainerNode::detach()
#9 0x0000014a69a5 blink::Element::detach()
#10 0x0000014782af blink::ContainerNode::detach()
#11 0x0000014a69a5 blink::Element::detach()
#12 0x0000014782af blink::ContainerNode::detach()
#13 0x0000014a69a5 blink::Element::detach()
#14 0x0000014782af blink::ContainerNode::detach()
#15 0x00000148cf74 blink::Document::detach()
#16 0x00000189c855 blink::FrameLoader::prepareForCommit()
#17 0x00000189c9e2 blink::FrameLoader::commitProvisionalLoad()
#18 0x00000188bc1a blink::DocumentLoader::finishedLoading()
#19 0x00000188dc3f blink::DocumentLoader::maybeLoadEmpty()
#20 0x00000188e0e0 blink::DocumentLoader::startLoadingMainResource()
#21 0x00000189c1a8 blink::FrameLoader::startLoad()
#22 0x000001899751 blink::FrameLoader::load()
#23 0x00000103eb75 blink::WebLocalFrameImpl::load()
#24 0x00000103ec23 blink::WebLocalFrameImpl::loadRequest()
#25 0x000000450d5f content::BlinkTestRunner::OnReset()
#26 0x0000004507b3 content::BlinkTestRunner::OnMessageReceived()
#27 0x0000022bfad6 content::RenderViewImpl::OnMessageReceived()
#28 0x00000265ef43 content::MessageRouter::RouteMessage()
#29 0x00000265eee9 content::MessageRouter::OnMessageReceived()
#30 0x000002235624 content::ChildThreadImpl::OnMessageReceived()
#31 0x000002690f8c IPC::ChannelProxy::Context::OnDispatchMessage()
#32 0x0000004aa0ed base::debug::TaskAnnotator::RunTask()
#33 0x00000228c148 scheduler::TaskQueueManager::ProcessTaskFromWorkQueue()
#34 0x00000228ad0e scheduler::TaskQueueManager::DoWork()
#35 0x0000004aa0ed base::debug::TaskAnnotator::RunTask()
#36 0x00000047042b base::MessageLoop::RunTask()
#37 0x000000470739 base::MessageLoop::DeferOrRunPendingTask()
#38 0x00000047091d base::MessageLoop::DoWork()
#39 0x000000471c6a base::MessagePumpDefault::Run()
#40 0x00000047bcd0 base::RunLoop::Run()
#41 0x00000046fb8a base::MessageLoop::Run()
#42 0x0000022da4a4 content::RendererMain()
#43 0x000000453b17 content::RunZygote()
#44 0x0000004547f1 content::ContentMainRunnerImpl::Run()
#45 0x000000453790 content::ContentMain()

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:166: if (doRegister)
On 2015/10/01 07:04:14, esprehn wrote:
> Remove this method

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:177:
updatePositionChangeRegistration(m_wantPositionChangeNotifications);
On 2015/10/01 07:04:14, esprehn wrote:
> inline the positive case here

Done, though both cases are used.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutMedia.h (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutMedia.h:97: bool
m_wantPositionChangeNotifications;
On 2015/10/01 07:04:14, esprehn wrote:
> : 1; for both

thanks, done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutView.cpp:959: void
LayoutView::registerMediaForPositionChangeNotification(LayoutMedia* media)
On 2015/10/01 07:04:15, esprehn wrote:
> reference

done, though the vector must still be LayoutMedia* and the callers now send
*this.  why is reference preferable here?

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutView.cpp:965: void
LayoutView::unregisterMediaForPositionChangeNotification(LayoutMedia* media)
On 2015/10/01 07:04:15, esprehn wrote:
> reference

ditto.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutView.cpp:975:
media->notifyPositionMayHaveChanged();
On 2015/10/01 07:04:15, esprehn wrote:
> This isn't really the same timing as intersection observer, it would be on a
> Timer, I think we should do that for this too otherwise you'll depend on the
> synchronous dispatch which is bad

One of the objections to a prior iteration of this CL was setting timers too
often on scroll events.  those events happen less often than
sendMediaPositionChangeNotification() is called now.

plus, the callback does most of the real work on a (debounced) timer anyway. 
i'll leave it for now.  i believe it will work with the IntersectionObserver as
is.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutView.h (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutView.h:205: void
registerMediaForPositionChangeNotification(LayoutMedia*);
On 2015/10/01 06:14:26, ojan wrote:
> Please add:
> TODO(ojan): Merge this with IntersectionObserver once it lands.

Done.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/testing/Internals.idl:357:
[TypeChecking=Interface] void triggerAutoplayViewportCheck(HTMLMediaElement
mediaElement);
On 2015/10/01 06:14:26, ojan wrote:
> We typically try to avoid adding one-off APIs like this. Unfortunately, since
we
> don't have a great track record of unittests, that means we handle things via
> regression tests (i.e. layout tests), which aren't very friendly to exposing
the
> guts of the system. As an aside, feel free to write unittests whenever you
can.
> We're happy to have more unittests and fewer layout tests where it's not too
> onerous to get setup.
> 
> Since this will go away when we remove the experiment code, I'm OK with taking
> on this small bit of technical debt temporarily. But, please add a
> TODO(liberato) to remove this once the experiment is done in all the headers
> this method is defined in and in the calling code (i.e. in the layout test).

added the TODO.

https://codereview.chromium.org/1370723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/testing/Internals.idl:357:
[TypeChecking=Interface] void triggerAutoplayViewportCheck(HTMLMediaElement
mediaElement);
On 2015/10/01 07:04:15, esprehn wrote:
> You should be able to write a Sim test for this, see
> DocumentLoadingRenderingTest. that would remove the need for this

will look at this test, but for now leaving with TODO.

[liberato (no reviews please), 2015-10-23 18:02:58]

esprehn, ojan: do you have any additional feedback on this, or may i land it as
is?

thanks
-fl

[philipj_slow, 2015-11-02 15:56:21]

Ping?

[ojan, 2015-11-04 07:42:16]

The CQ bit was checked by ojan@chromium.org

[ojan, 2015-11-04 07:42:17]

lgtm

[commit-bot: I haz the power, 2015-11-04 07:42:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1370723002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1370723002/40001

[commit-bot: I haz the power, 2015-11-04 07:44:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-04 07:44:20]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[liberato (no reviews please), 2015-11-04 16:21:12]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2015-11-04 16:21:13]

The patchset sent to the CQ was uploaded after l-g-t-m from ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/1370723002/#ps60001
(title: "rebased.")

[commit-bot: I haz the power, 2015-11-04 16:21:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1370723002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1370723002/60001

[commit-bot: I haz the power, 2015-11-04 18:04:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-04 18:04:51]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[liberato (no reviews please), 2015-11-04 21:24:08]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2015-11-04 21:24:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1370723002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1370723002/60001

[commit-bot: I haz the power, 2015-11-04 22:25:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-04 22:26:13]

Patchset 4 (id:??) landed as
https://crrev.com/e894363cd4e9b052486c26c2c3ab2adf6a500ae5
Cr-Commit-Position: refs/heads/master@{#357911}