Include viewport visibility checks for autoplay experiment.

CLOSED in favor of the post-blink-merge https://codereview.chromium.org/1370723002 .  Please redirect reviews there!

====

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.

BUG=487345, 402044

[liberato (no reviews please), 2015-09-04 06:52:18]

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

[liberato (no reviews please), 2015-09-04 06:58:16]

Here's where the other half of the autoplay experiment CL went :)

it does address (i believe) Ojan's most recent comments about the visibility
checking.

i haven't tested this most recent rebase manually yet (no device handy).  i
wanted to get the CL out anyway since most of it has been reviewed already, and
the timezone thing is always problematic.

thanks
-fl

[philipj_slow, 2015-09-04 08:47:56]

https://codereview.chromium.org/1329853004/diff/1/Source/core/html/AutoplayEx...
File Source/core/html/AutoplayExperimentConfig.h (right):

https://codereview.chromium.org/1329853004/diff/1/Source/core/html/AutoplayEx...
Source/core/html/AutoplayExperimentConfig.h:30: IfViewport = 1 << 2,
Ah, here it is. Here's where a document.visibility-only experiment would be
useful.

[liberato (no reviews please), 2015-09-10 18:40:27]

Patchset #3 (id:40001) has been deleted

[liberato (no reviews please), 2015-09-14 14:50:12]

I've separated the viewport visibility check into two: one that does page
visibility only, and one that checks for location in the viewport.  One must
enable them both to get the old behavior.

I've also updated the code that checks for when a scroll ends.  I just didn't
like how it was behaving after the change to FrameView, since it would trigger
even if the scroll hadn't completed. It now avoids using timers too much, while
also waiting for scrolling to stop before overriding the user gesture
requirement for playback.

thanks
-fl

[liberato (no reviews please), 2015-09-16 20:12:47]

all feedback is welcome!

thanks
-fl

[esprehn, 2015-09-16 21:26:25]

I'll review this today, thanks for being patient!

On Wed, Sep 16, 2015 at 1:12 PM, <liberato@chromium.org> wrote:

> all feedback is welcome!
>
>
> thanks
> -fl
>
> https://codereview.chromium.org/1329853004/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2015-09-17 07:09:49]

I'm still reviewing this, I'm not a fan of "Helper" classes like this. We don't
generally do that in blink. Also why are there so many nested classes inside it?

Do we have a long term owner for this code? Who will merge it into LayoutMedia
when we finish the experiment?

Historically helper classes that are mixins for "experiments" have ended up
being left around forever.

[liberato (no reviews please), 2015-09-17 13:58:19]

On 2015/09/17 07:09:49, esprehn wrote:
> I'm still reviewing this, I'm not a fan of "Helper" classes like this. We
don't
> generally do that in blink. Also why are there so many nested classes inside
it?
> 
> Do we have a long term owner for this code? Who will merge it into LayoutMedia
> when we finish the experiment?
> 
> Historically helper classes that are mixins for "experiments" have ended up
> being left around forever.

thanks for the feedback!

helper classes: i'd like to understand this concern better.  the code is cleanly
separable, and HTMLMediaElement is already huge.  merging them seems like it
would tangle things up more, and make removal harder.

ownership: i'll take responsibility for removing it when we're done with it.

merge with LayoutMedia: thankfully, it will not be merged.  there is work in
progress (https://codereview.chromium.org/1330633003/) that will provide the
functionality we need for detecting visibility changes.  this makes me very
happy :)

nested classes: i did this specifically to keep removal simple.  i wanted to
discourage using them elsewhere.

[liberato (no reviews please), 2015-09-21 14:04:20]

hi all

friendly reminder!

thanks
-fl

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

I've looked at core/html, if possible I'd like to leave core/layout to one of
the other reviewers.

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
File LayoutTests/media/video-autoplay-experiment-modes.html (right):

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:16: V      - "ifviewport"
Also test ifpagevisible

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:169: // Pick whether the
element will be visible when canPlayThrough.
What does the "when canPlayThrough" bit mean? The layout will be done roughly
instantly.

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:204:
window.internals.triggerAutoplayViewportCheck(element);
What's going on here, isn't the scroll itself supposed to trigger the play, or
was that idea abandoned? (I don't think it's a sane behavior, but IIRC it would
still be tested to see how people react.)

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:205: // Once those are
return, the state should be correct.
Once the above two methods have returned? Maybe "The new state should be
observable synchronously"?

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:83: // Wait for viewport
visibility.
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.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:127: m_registeredWithView = true;
Here we're registering with the layout object, so should m_registeredWithView be
m_registeredWithLayoutObject?

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:136: LayoutMedia* layoutMedia =
(LayoutMedia*)obj;
static_cast

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:184: return;
Is this code path reached and if so when *is* positionChanged called? If it
isn't reached, then ASSERT

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:188: // past to allow the test to
succeed.
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?

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:224: // callers need to do it
separately, and we don't want to check more
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.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:343: if (!element.layoutObject())
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?

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:366: // viewport, then be happy if
it covers it.
This all seems pretty elaborate, why not just return element.intersects(screen)?

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:385: // See if elemet is contained
by the screen.
Typo if this line doesn't go *poof*

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:391: // Check if we're in the
viewport.  If the element is bigger than the
This comment is copied from the above and isn't accurate here.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:407: return !((*this) == them);
Don't need () around *this I think.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
File Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:87: // "not valid", in case some of
the information isn't available.
I looks like PageVisibilityState is doesn't have an "invalid" state, so I guess
this is for m_element and m_screen. Can't those invalid states then simply be
expressed by one or both of those being the empty rect? Less stuff to keep in
sync.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:101: // Return true if and only if
the player is visible.
s/player/element/ and maybe rename to isElementVisible() as that's a nice pair
with isPageVisibile().

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:127: // not visible, and we care
that it must be visible.
It's pretty hard to tell from this header when you should pass the LocationState
and when you don't need to. It looks like it has to do with comparing the
before/after state.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:130: // Return false if and only if
the player is not visible, and we care
s/player/element/?

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

i believe that this addresses the outstanding feedback, though i understand that
the layout code is unreviewed.

@ojan: can you take a look at the layout code?

the experiment-modes test is getting close to 6sec, i should break it up into
multiple tests, or at least mark it as slow.

thanks
-fl

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
File LayoutTests/media/video-autoplay-experiment-modes.html (right):

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:16: V      - "ifviewport"
On 2015/09/21 15:02:04, philipj wrote:
> Also test ifpagevisible

done.

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:169: // Pick whether the
element will be visible when canPlayThrough.
On 2015/09/21 15:02:04, philipj wrote:
> What does the "when canPlayThrough" bit mean? The layout will be done roughly
> instantly.

At some point, canplaythrough will be generated, and the autoplay experiment
checks will be done.  we are choosing if we want to pass or fail the visibility
checks when that happens.

why does this necessarily happen right away, though?

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:204:
window.internals.triggerAutoplayViewportCheck(element);
On 2015/09/21 15:02:04, philipj wrote:
> What's going on here, isn't the scroll itself supposed to trigger the play, or
> was that idea abandoned? (I don't think it's a sane behavior, but IIRC it
would
> still be tested to see how people react.)

the trigger happens after scrolling stops, not when it starts.  as in, flinging
the page to the bottom won't automatically start every video on the page.

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:205: // Once those are
return, the state should be correct.
On 2015/09/21 15:02:04, philipj wrote:
> Once the above two methods have returned? Maybe "The new state should be
> observable synchronously"?

Done.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:83: // Wait for viewport
visibility.
On 2015/09/21 15:02:04, philipj wrote:
> 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.

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

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:127: m_registeredWithView = true;
On 2015/09/21 15:02:05, philipj wrote:
> Here we're registering with the layout object, so should m_registeredWithView
be
> m_registeredWithLayoutObject?

Done.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:136: LayoutMedia* layoutMedia =
(LayoutMedia*)obj;
On 2015/09/21 15:02:04, philipj wrote:
> static_cast

Done.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:184: return;
On 2015/09/21 15:02:04, philipj wrote:
> Is this code path reached and if so when *is* positionChanged called? If it
> isn't reached, then ASSERT

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

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:188: // past to allow the test to
succeed.
On 2015/09/21 15:02:04, philipj wrote:
> 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?

Done.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:224: // callers need to do it
separately, and we don't want to check more
On 2015/09/21 15:02:05, philipj wrote:
> 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.

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.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:343: if (!element.layoutObject())
On 2015/09/21 15:02:04, philipj wrote:
> 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?

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.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:366: // viewport, then be happy if
it covers it.
On 2015/09/21 15:02:04, philipj wrote:
> This all seems pretty elaborate, why not just return
element.intersects(screen)?

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

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:385: // See if elemet is contained
by the screen.
On 2015/09/21 15:02:04, philipj wrote:
> Typo if this line doesn't go *poof*

poof.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:391: // Check if we're in the
viewport.  If the element is bigger than the
On 2015/09/21 15:02:05, philipj wrote:
> This comment is copied from the above and isn't accurate here.

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

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:407: return !((*this) == them);
On 2015/09/21 15:02:04, philipj wrote:
> Don't need () around *this I think.

Done.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
File Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:87: // "not valid", in case some of
the information isn't available.
On 2015/09/21 15:02:05, philipj wrote:
> I looks like PageVisibilityState is doesn't have an "invalid" state, so I
guess
> this is for m_element and m_screen. Can't those invalid states then simply be
> expressed by one or both of those being the empty rect? Less stuff to keep in
> sync.

i don't see a way to make it work automatically.  intersects() always returns
false for empty rectangles, but contains() does not.  we'd still have to check
explicitly for isEmpty().

anyway, it does remove the flag so i'll give it a try.  i suspect that it will
be less clear.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:101: // Return true if and only if
the player is visible.
On 2015/09/21 15:02:05, philipj wrote:
> s/player/element/ and maybe rename to isElementVisible() as that's a nice pair
> with isPageVisibile().

Done.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:127: // not visible, and we care
that it must be visible.
On 2015/09/21 15:02:05, philipj wrote:
> It's pretty hard to tell from this header when you should pass the
LocationState
> and when you don't need to. It looks like it has to do with comparing the
> before/after state.

if yuo have LocationState, pass it in.  if not, then the second form will
construct one for you.  i'll update the docs to make that clearer.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:130: // Return false if and only if
the player is not visible, and we care
On 2015/09/21 15:02:05, philipj wrote:
> s/player/element/?

Done.

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

i believe that this addresses the outstanding feedback, though i understand that
the layout code is unreviewed.

@ojan: can you take a look at the layout code?

the experiment-modes test is getting close to 6sec, i should break it up into
multiple tests, or at least mark it as slow.

thanks
-fl

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:83: // Wait for viewport
visibility.
On 2015/09/21 15:02:04, philipj wrote:
> 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.

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

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
File Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:87: // "not valid", in case some of
the information isn't available.
On 2015/09/21 15:02:05, philipj wrote:
> I looks like PageVisibilityState is doesn't have an "invalid" state, so I
guess
> this is for m_element and m_screen. Can't those invalid states then simply be
> expressed by one or both of those being the empty rect? Less stuff to keep in
> sync.

i don't see a way to make it work automatically.  intersects() always returns
false for empty rectangles, but contains() does not.  we'd still have to check
explicitly for isEmpty().

anyway, it does remove the flag so i'll give it a try.  i suspect that it will
be less clear.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:101: // Return true if and only if
the player is visible.
On 2015/09/21 15:02:05, philipj wrote:
> s/player/element/ and maybe rename to isElementVisible() as that's a nice pair
> with isPageVisibile().

Done.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:127: // not visible, and we care
that it must be visible.
On 2015/09/21 15:02:05, philipj wrote:
> It's pretty hard to tell from this header when you should pass the
LocationState
> and when you don't need to. It looks like it has to do with comparing the
> before/after state.

if yuo have LocationState, pass it in.  if not, then the second form will
construct one for you.  i'll update the docs to make that clearer.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:130: // Return false if and only if
the player is not visible, and we care
On 2015/09/21 15:02:05, philipj wrote:
> s/player/element/?

Done.

[esprehn, 2015-09-23 06:55:06]

I'll try to review this tomorrow.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[philipj_slow, 2015-09-25 15:32:28]

My comments seem to have been addressed, so LGTM. However, the base URL is still
blink.git, so I think you'll have to create a new review. When you do, it would
be great if you start it with PS4, so I can see the whole diff to PS5, here I
had to look and compare by hand, and probably missed some things.

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
File LayoutTests/media/video-autoplay-experiment-modes.html (right):

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:169: // Pick whether the
element will be visible when canPlayThrough.
On 2015/09/23 06:14:56, liberato wrote:
> On 2015/09/21 15:02:04, philipj wrote:
> > What does the "when canPlayThrough" bit mean? The layout will be done
roughly
> > instantly.
> 
> At some point, canplaythrough will be generated, and the autoplay experiment
> checks will be done.  we are choosing if we want to pass or fail the
visibility
> checks when that happens.
> 
> why does this necessarily happen right away, though?

I mean that this code is appending the element either to an onscreen element or
an offscreen element right now, but the comment makes it sound like it has
something to do with the canplaythrough event.

https://codereview.chromium.org/1329853004/diff/80001/LayoutTests/media/video...
LayoutTests/media/video-autoplay-experiment-modes.html:204:
window.internals.triggerAutoplayViewportCheck(element);
On 2015/09/23 06:14:56, liberato wrote:
> On 2015/09/21 15:02:04, philipj wrote:
> > What's going on here, isn't the scroll itself supposed to trigger the play,
or
> > was that idea abandoned? (I don't think it's a sane behavior, but IIRC it
> would
> > still be tested to see how people react.)
> 
> the trigger happens after scrolling stops, not when it starts.  as in,
flinging
> the page to the bottom won't automatically start every video on the page.

Yes, but shouldn't the call to element.scrollIntoView() eventually trigger the
check again, or why is the manual triggerAutoplayViewportCheck needed here?

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:83: // Wait for viewport
visibility.
On 2015/09/23 06:14:57, liberato wrote:
> On 2015/09/21 15:02:04, philipj wrote:
> > 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.
> 
> it doesn't register for page visibility because the callback from FrameView
> covers it.  we just register for that always.

Acknowledged.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:184: return;
On 2015/09/23 06:14:56, liberato wrote:
> On 2015/09/21 15:02:04, philipj wrote:
> > Is this code path reached and if so when *is* positionChanged called? If it
> > isn't reached, then ASSERT
> 
> actually, the more i think about it, we don't want to check at all in case the
> timer did fire.

I see this block was removed.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:224: // callers need to do it
separately, and we don't want to check more
On 2015/09/23 06:14:56, liberato wrote:
> On 2015/09/21 15:02:05, philipj wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:343: if (!element.layoutObject())
On 2015/09/23 06:14:56, liberato wrote:
> On 2015/09/21 15:02:04, philipj wrote:
> > 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?
> 
> 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.

Acknowledged.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.cpp:366: // viewport, then be happy if
it covers it.
On 2015/09/23 06:14:56, liberato wrote:
> On 2015/09/21 15:02:04, philipj wrote:
> > This all seems pretty elaborate, why not just return
> element.intersects(screen)?
> 
> "contained in" just seemed nicer than "intersects with" when playing with it
on
> a device.

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.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
File Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:87: // "not valid", in case some of
the information isn't available.
On 2015/09/23 06:14:57, liberato wrote:
> On 2015/09/21 15:02:05, philipj wrote:
> > I looks like PageVisibilityState is doesn't have an "invalid" state, so I
> guess
> > this is for m_element and m_screen. Can't those invalid states then simply
be
> > expressed by one or both of those being the empty rect? Less stuff to keep
in
> > sync.
> 
> i don't see a way to make it work automatically.  intersects() always returns
> false for empty rectangles, but contains() does not.  we'd still have to check
> explicitly for isEmpty().
> 
> anyway, it does remove the flag so i'll give it a try.  i suspect that it will
> be less clear.

OK, looks like it was successfully removed.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:121: // Return true if any only if
this player meets (most) of the eligibility
Another "player" here if you want to fix it.

https://codereview.chromium.org/1329853004/diff/80001/Source/core/html/Autopl...
Source/core/html/AutoplayExperimentHelper.h:127: // not visible, and we care
that it must be visible.
On 2015/09/23 06:14:57, liberato wrote:
> On 2015/09/21 15:02:05, philipj wrote:
> > It's pretty hard to tell from this header when you should pass the
> LocationState
> > and when you don't need to. It looks like it has to do with comparing the
> > before/after state.
> 
> if yuo have LocationState, pass it in.  if not, then the second form will
> construct one for you.  i'll update the docs to make that clearer.

Acknowledged.