Implement autoplay gesture override experiment.

Implement autoplay gesture override experiment.
    
If the value of the autoplayExperimentMode setting is "enabled",
    then we don't require a user gesture to initiate autoplay.
    
    If one includes "-ifmobile", then we additionally require that the
    page is optimized for mobile (i.e., sets the viewport).
    
    If one includes "-ifmuted", then we additionally require that the
    media is muted or doesn't have an audio track.
    
    If one includes "-playmuted", then we will autoplay after muting the
    audio (if any), once any other conditions have been met.
    
    This behavior affects both attribute-based autoplay and JS calls (e.g.,
    play()/pause() ).    
BUG=487345, 402044
Committed: https://crrev.com/0ad5bea84a7d3978c9e53e8a97b65b052307a00a
git-svn-id: svn://svn.chromium.org/blink/trunk@202204 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[liberato (no reviews please), 2015-07-17 18:05:44]

liberato@chromium.org changed reviewers:
+ adamk@chromium.org, oysteine@chromium.org, philipj@opera.com

[liberato (no reviews please), 2015-07-17 21:18:29]

Hi all

This cl provides support for an experiment on the impact of relaxing the user
gesture requirement for media autoplay that we have on android.  CL summary
describes it.

There should be no change in functionality unless settings are changed.

philipj: i know you were involved in some related discussions, so i thought you
might be interested.  if not, could you still take a look at websettings as the
owner?  or let me know and i'll pass it off elsewhere if you'd prefer.

thanks
-fl

[philipj_slow, 2015-07-21 12:51:30]

http://code.google.com/p/chromium/issues/detail?id=487345 is 403, but I've taken
a brief look.

I'm a bit skeptical of any of these modes as end states to actually ship by
default, so I'm not sure what will be learned by testing them on users.
Specifically:

always: The on-screen check will not work for <audio>, maybe checking the
document visiblity state would produce roughly the intended effect and be easier
to describe in terms of web-exposed APIs? I'm also not sure about the "page is
optimized for mobile" check, why is that?

if-muted: This would be better than animated GIFs, but this isn't what web
developers are complaining about. It looks like it's possible to just unmute
after playback begins?

play-muted: Too magic IMHO, better to not play at all than to play in a state
where the web developer expects the video to be audible.

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3799:
document().domWindow()->addEventListener("scroll",
m_autoplayExperimentScrollListener, false);
I take it this will prevent smooth scrolling while dragging with the finger over
the video? There must be some way of learning about scrolling without falling of
the fast scrolling path?

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:658: friend class MyScrollListener;
MyScrollListener is nowhere to be found?

[philipj_slow, 2015-07-21 12:57:22]

I should also say that I think we should strive to have the final behavior
spec'd in https://html.spec.whatwg.org/ so that's why it's useful if it's
possible to express as much as possible in terms of already web-exposed
concepts. At the bottom of it all will still be a user agent-defined preference,
of course, but that could potentially be exposed somehow too.

[pangu, 2015-07-21 15:45:06]

> On Jul 21, 2015, at 5:51 AM, philipj@opera.com wrote:
> 
> http://code.google.com/p/chromium/issues/detail?id=487345 is 403, but I've
taken
> a brief look.
> 
> I'm a bit skeptical of any of these modes as end states to actually ship by
> default, so I'm not sure what will be learned by testing them on users.

Generally, the idea behind the experiments are as follows — #autoplay on media
elements on the mobile web is a subset of the autoplay all the time for battery
and bandwidth concerns. Additionally, autoplay on mobile needs to have clear
controls to the user , especially if there is playback when the user doesn’t
expect it. There is a call for a better system than the current don’t autoplay
anywhere.

The experiments are intended to provide some information on a) battery impact of
each state b) bandwidth impact of the state and c) user reaction to the states.

> Specifically:
> 
> always: The on-screen check will not work for <audio>, maybe checking the
> document visiblity state would produce roughly the intended effect and be
easier
> to describe in terms of web-exposed APIs?

@philipj: document visibility would only provide us with information if the tab
is active, right? We really don’t want to autoplay any media (esp video) where
it is *not* shown on the viewport.

> I'm also not sure about the "page is
> optimized for mobile" check, why is that?

This is particularly used to a) constrain to websites where the video will be
shown in the viewport intentionally and b) where there are likely only one
intentional media element that should be autoplayed.

> if-muted: This would be better than animated GIFs, but this isn't what web
> developers are complaining about.

We certainly got some specific feedback on the if-muted case. I don’t understand
why we still shouldn’t test this? Can you elaborate more on what you
specifically refer to the web developer complaints?

> It looks like it's possible to just unmute
> after playback begins?

Hmm: We will re-evaluate that. Thanks for the heads up. :)

> play-muted: Too magic IMHO, better to not play at all than to play in a state
> where the web developer expects the video to be audible.

Yes. We believe this is the most concerning state. We are specifically testing
this to better understand if audio is the main cause of annoyance in autoplay or
is it the motion. However, I agree that this is the least likely to win as a
candidate. 

— Pangu

>
https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
> Source/core/html/HTMLMediaElement.cpp:3799:
> document().domWindow()->addEventListener("scroll",
> m_autoplayExperimentScrollListener, false);
> I take it this will prevent smooth scrolling while dragging with the
> finger over the video? There must be some way of learning about
> scrolling without falling of the fast scrolling path?
> 
>
https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
> File Source/core/html/HTMLMediaElement.h (right):
> 
>
https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
> Source/core/html/HTMLMediaElement.h:658: friend class MyScrollListener;
> MyScrollListener is nowhere to be found?
> 
> https://codereview.chromium.org/1179223002/

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

[pangu, 2015-07-21 15:45:16]

+1. 

That’s the idea. :)
— Pangu

> On Jul 21, 2015, at 5:57 AM, philipj@opera.com wrote:
> 
> I should also say that I think we should strive to have the final behavior
> spec'd in https://html.spec.whatwg.org/ so that's why it's useful if it's
> possible to express as much as possible in terms of already web-exposed
> concepts. At the bottom of it all will still be a user agent-defined
preference,
> of course, but that could potentially be exposed somehow too.
> 
> https://codereview.chromium.org/1179223002/

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

[philipj_slow, 2015-07-22 07:49:28]

On 2015/07/21 15:45:06, pangu wrote:
> > On Jul 21, 2015, at 5:51 AM, mailto:philipj@opera.com wrote:
> > 
> > http://code.google.com/p/chromium/issues/detail?id=487345 is 403, but I've
> taken
> > a brief look.
> > 
> > I'm a bit skeptical of any of these modes as end states to actually ship by
> > default, so I'm not sure what will be learned by testing them on users.
> 
> Generally, the idea behind the experiments are as follows — #autoplay on media
> elements on the mobile web is a subset of the autoplay all the time for
battery
> and bandwidth concerns. Additionally, autoplay on mobile needs to have clear
> controls to the user , especially if there is playback when the user doesn’t
> expect it. There is a call for a better system than the current don’t autoplay
> anywhere.

The controls part of this is being solved by issue https://crbug.com/470600

> The experiments are intended to provide some information on a) battery impact
of
> each state b) bandwidth impact of the state and c) user reaction to the
states.
> 
> > Specifically:
> > 
> > always: The on-screen check will not work for <audio>, maybe checking the
> > document visiblity state would produce roughly the intended effect and be
> easier
> > to describe in terms of web-exposed APIs?
> 
> @philipj: document visibility would only provide us with information if the
tab
> is active, right? We really don’t want to autoplay any media (esp video) where
> it is *not* shown on the viewport.

A behavior that depends on what is visible seems very fragile to me. One web
developers understand what the rules are, and they still want to play video that
is not yet visible, they will move the video to where it is visible with some
trick not make it not actually observable (white poster image, opacity:0, 1x1
size), play it and then move it to where they actually want it.

Also, how about <audio>, that is never visible? 

> > I'm also not sure about the "page is
> > optimized for mobile" check, why is that?
> 
> This is particularly used to a) constrain to websites where the video will be
> shown in the viewport intentionally and b) where there are likely only one
> intentional media element that should be autoplayed.

Hmm, sounds like "optimized for mobile" is a proxy for some other aspects that
are more likely to be true on mobile sites than desktop sites. Should there be a
restriction on the number of media elements that can autoplay within a tab,
perhaps?

> > if-muted: This would be better than animated GIFs, but this isn't what web
> > developers are complaining about.
> 
> We certainly got some specific feedback on the if-muted case. I don’t
understand
> why we still shouldn’t test this? Can you elaborate more on what you
> specifically refer to the web developer complaints?

I mean that in https://crbug.com/178297, there is no discussion of muted, and
web developers commenting are saying things like:

"Many of our users are children, and having sounds automatically play helps keep
them focused and engaged."

"Another scenario is an instant messaging application, where a short alert sound
is played upon receiving a new message."

"The workaround is to AJAX the whole file and play it with the Web Audio API."

"I also have a need to autoplay an mp3 in various venues, for reasons that don't
only include entertainment."

"Just heard today from another client that needs auto play of mp3."

I'm not saying that nobody anywhere would benefit from playing muted, but from
that bug it sounds like the ability to play sounds is the main concern.

> > It looks like it's possible to just unmute
> > after playback begins?
> 
> Hmm: We will re-evaluate that. Thanks for the heads up. :)
> 
> > play-muted: Too magic IMHO, better to not play at all than to play in a
state
> > where the web developer expects the video to be audible.
> 
> Yes. We believe this is the most concerning state. We are specifically testing
> this to better understand if audio is the main cause of annoyance in autoplay
or
> is it the motion. However, I agree that this is the least likely to win as a
> candidate. 

Do you mean that if people pause an autoplaying but muted video, that suggests
that the motion is what they find annoying? It might also suggest that playing
the video with muted audio is annoying :)

Can you fill in some details on how the experiment will be run? Will a mode be
chosen at random for users of Chrome for Android?

[philipj_slow, 2015-07-22 07:50:26]

On 2015/07/22 07:49:28, philipj wrote:
> A behavior that depends on what is visible seems very fragile to me. One web
> developers understand what the rules are, and they still want to play video
that
> is not yet visible, they will move the video to where it is visible with some
> trick not make it not actually observable (white poster image, opacity:0, 1x1
> size), play it and then move it to where they actually want it.

s/One/Once/

[DaleCurtis, 2015-07-24 17:06:45]

Can you add issue 402044 to the bug listing here? I think it's more relevant.

[liberato (no reviews please), 2015-07-24 17:13:05]

On 2015/07/24 17:06:45, DaleCurtis wrote:
> Can you add issue 402044 to the bug listing here? I think it's more relevant.

done.

[liberato (no reviews please), 2015-07-27 06:03:09]

biggest change in this version is the visibility check, to make sure that we
don't lose the fast path scrolling.  for simple touch-based scrolling, it feels
as good or better than it did with the scroll listener.  thanks, philipj, for
pointing it out.

-fl

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3799:
document().domWindow()->addEventListener("scroll",
m_autoplayExperimentScrollListener, false);
On 2015/07/21 12:51:29, philipj wrote:
> I take it this will prevent smooth scrolling while dragging with the finger
over
> the video? There must be some way of learning about scrolling without falling
of
> the fast scrolling path?

good point.  we really only care if it ends up in view, not happens to scroll
by.

however, i wasn't able to find much else that catches all the cases of
scrolling.  the next CL will focus on touch end / cancel and poll a few times to
find the end of any inertial scroll.  scrollIntoView() won't be noticed, and
similar.

i'll collect some feedback on whether this is a reasonable approximation.  for
now, let's assume that it is.

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:658: friend class MyScrollListener;
On 2015/07/21 12:51:29, philipj wrote:
> MyScrollListener is nowhere to be found?

that was sloppy of me.  thanks.

https://codereview.chromium.org/1179223002/diff/220001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/1179223002/diff/220001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:2144: m_muted = muted;
@philipj: you are correct that we don't stop JS from simply unmuting here after
it passes the check (for either if-muted or play-muted) if not attached to a
user gesture.

however, as you've pointed out, none of this will stop a malicious site from
doing whatever it wants.  the visibility checks can be circumvented trivially
too, as you might have also pointed out.  rather, the goal for this experiment
is to find a compromise for users that provides a better experience on mobile
devices than the current (strict) gesture requirements for legitimate use-cases.

[philipj_slow, 2015-07-27 08:43:04]

What's the best place to have the discussion about which behaviors we should
investigate, i.e. which behaviors we think potentially make sense to eventually
ship?

[liberato (no reviews please), 2015-08-05 06:17:38]

this adds the play() failed metric, and addresses all outstanding CL feedback.

[philipj_slow, 2015-08-05 10:03:12]

OK, took a high-level view of things. The delayed play on scroll accounts for
most of the code, so I'd first investigate ways of simplifying that. As I said,
Rick Byers might have ideas.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:378: if (document.settings() &&
document.settings()->mediaPlaybackRequiresUserGesture())
Wow, this has actually been broken the whole time, you can already circumvent
the restriction entirely by getting a document with no settings object:

var video = document.querySelector("video");
var doc = document.implementation.createHTMLDocument();
doc.body.appendChild(video.cloneNode(true));
var autoplayVideo = doc.body.firstChild.cloneNode(true);
video.parentNode.replaceChild(autoplayVideo, video);
autoplayVideo.play();

So, yeah, we should probably make all of this not suck so much. Not sure what
would happen if this trick was widely known... o_O

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1608: if (autoplayExperimentIsVisible()) {
Perhaps rename this so that it's clear that it always returns true when
visibility isn't required, or move that checks to the call sites if that would
be clearer.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1618: m_paused = false;
This branch was previously predicated on the sandbox check passing, was that an
intentional change?

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:2117: if (progress < 0.5) {
Why the added {}?

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:2233: // playing, then start playing.  In
other words, start playing if
Really start playing by setting the muted attribute? Even with no intention of
shipping this, I'm not sure what could be learned from this.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3971: // Check visibility.
A page visibility check is missing, this ought to pass even if the tab is in the
background. Make sure to have a separate flag for page visibity, maybe
"pagevisible" and "inviewport" or something?

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3991: // Why are we always preparing? 
Just go!
Is this a question to the reviewer, or a rhetorical question? FWIW, I'm not sure
when we need to prepare and not, so if you know, please document it :)

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:291: // Media element with autoplay seen.
I'm not sure what this broad metric is for, but the documentation isn't
accurate, this counts both autoplay and play(). If it's possible to change the
enum names that could be more clear too.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:294: AutoplayStopped = 1,
Isn't it necessary to have one stopped entry for each start entry in order to
compare how users react, including measuring the stop and bailout metrics for
when the experiment is disabled entirely. (Maybe people bailout a lot even when
starting a play deliberately?)

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:307: AutoplayExperimentStartedByLoad = 7,
I think s/Load/AutoplayAttribute/, it's not really the load that causes the
play, it's the autoplay attribute which causes the load, and reaching a certain
point in loading then plays. It's probably worth testing if this actually works
at all on Android, or if it's only actually calling play() that will start the
load and eventually play.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:460: // Install an event listener to check
for changes in visibility.  If a
Instead of the touchend/touchcancel+timeout solution, can you investigate
hooking into FrameView::scrollPositionChanged()?
setTextTrackKindUserPreferenceForAllMediaElements is another method that takes a
Document* and does something for all media elements within.

I don't think there's really a good way to know that a scroll has ended, so
perhaps just continually resetting a 200ms (?) timeout and doing the check when
it finally runs would work? Rick Byers would be a good person to ask about
better ways here.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:627: bool m_autoplayExperimentPlayPending :
1;
It looks like the delayed play is only done for explicit play() calls and not
for autoplay?

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:646: ExperimentPlayMuted = 16
Bit fields are usually defined using 1 << n in Blink, see e.g. DelayedActionType
in this header.

[philipj_slow, 2015-08-05 14:10:32]

philipj@opera.com changed reviewers:
+ rbyers@chromium.org

[philipj_slow, 2015-08-05 14:10:33]

I'll be gone for the rest of the week, so I'd recommend grabbing a random
experienced Blink developer to work through the overall structure of this.
Adding Rick Byers as I think he'll be able to help you with the scrolling
problem.

[Rick Byers, 2015-08-05 14:27:59]

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3799:
document().domWindow()->addEventListener("scroll",
m_autoplayExperimentScrollListener, false);
On 2015/07/27 at 06:03:09, liberato wrote:
> On 2015/07/21 12:51:29, philipj wrote:
> > I take it this will prevent smooth scrolling while dragging with the finger
over
> > the video? There must be some way of learning about scrolling without
falling of
> > the fast scrolling path?
> 
> good point.  we really only care if it ends up in view, not happens to scroll
by.
> 
> however, i wasn't able to find much else that catches all the cases of
scrolling.  the next CL will focus on touch end / cancel and poll a few times to
find the end of any inertial scroll.  scrollIntoView() won't be noticed, and
similar.
> 
> i'll collect some feedback on whether this is a reasonable approximation.  for
now, let's assume that it is.

Listening to scroll events doesn't "prevent smooth scrolling".  scroll events
are async and don't block threaded scrolling (although they do tweak some
scheduler settings, but if that's visibly observable then it's probably a
schedule bug and not your issue).  Most major mobile websites have scroll
listeners on the document anyway, so adding one more isn't really a problem - as
long as you're careful to not do much work or dirty layout etc.

Touch event handlers, on the other hand, do block the start of scrolling.  In
general it's preferable to watch 'scroll' than lower level input like 'touch*',
'wheel', etc.

[Rick Byers, 2015-08-05 14:30:10]

On 2015/08/05 at 14:27:59, Rick Byers wrote:
>
https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
> Source/core/html/HTMLMediaElement.cpp:3799:
document().domWindow()->addEventListener("scroll",
m_autoplayExperimentScrollListener, false);
> On 2015/07/27 at 06:03:09, liberato wrote:
> > On 2015/07/21 12:51:29, philipj wrote:
> > > I take it this will prevent smooth scrolling while dragging with the
finger over
> > > the video? There must be some way of learning about scrolling without
falling of
> > > the fast scrolling path?
> > 
> > good point.  we really only care if it ends up in view, not happens to
scroll by.
> > 
> > however, i wasn't able to find much else that catches all the cases of
scrolling.  the next CL will focus on touch end / cancel and poll a few times to
find the end of any inertial scroll.  scrollIntoView() won't be noticed, and
similar.
> > 
> > i'll collect some feedback on whether this is a reasonable approximation. 
for now, let's assume that it is.
> 
> Listening to scroll events doesn't "prevent smooth scrolling".  scroll events
are async and don't block threaded scrolling (although they do tweak some
scheduler settings, but if that's visibly observable then it's probably a
schedule bug and not your issue).  Most major mobile websites have scroll
listeners on the document anyway, so adding one more isn't really a problem - as
long as you're careful to not do much work or dirty layout etc.
> 
> Touch event handlers, on the other hand, do block the start of scrolling.  In
general it's preferable to watch 'scroll' than lower level input like 'touch*',
'wheel', etc.

BTW, use "show potential scroll bottlenecks" in chrome devtools to see this
(scroll event listeners have no impact on this). 
https://developer.chrome.com/devtools/docs/rendering-settings#show-potential
scroll bottlenecks

[philipj_slow, 2015-08-05 14:40:12]

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3799:
document().domWindow()->addEventListener("scroll",
m_autoplayExperimentScrollListener, false);
On 2015/08/05 14:27:59, Rick Byers wrote:
> On 2015/07/27 at 06:03:09, liberato wrote:
> > On 2015/07/21 12:51:29, philipj wrote:
> > > I take it this will prevent smooth scrolling while dragging with the
finger
> over
> > > the video? There must be some way of learning about scrolling without
> falling of
> > > the fast scrolling path?
> > 
> > good point.  we really only care if it ends up in view, not happens to
scroll
> by.
> > 
> > however, i wasn't able to find much else that catches all the cases of
> scrolling.  the next CL will focus on touch end / cancel and poll a few times
to
> find the end of any inertial scroll.  scrollIntoView() won't be noticed, and
> similar.
> > 
> > i'll collect some feedback on whether this is a reasonable approximation. 
for
> now, let's assume that it is.
> 
> Listening to scroll events doesn't "prevent smooth scrolling".  scroll events
> are async and don't block threaded scrolling (although they do tweak some
> scheduler settings, but if that's visibly observable then it's probably a
> schedule bug and not your issue).  Most major mobile websites have scroll
> listeners on the document anyway, so adding one more isn't really a problem -
as
> long as you're careful to not do much work or dirty layout etc.
> 
> Touch event handlers, on the other hand, do block the start of scrolling.  In
> general it's preferable to watch 'scroll' than lower level input like
'touch*',
> 'wheel', etc.

Oh... http://rbyers.github.io/EventListenerOptions/EventListenerOptions.html
mentions "wheel" and but I remembered it as "scroll".

In that case, all of this will be much nicer. Is there a way to know for sure
that a scroll has ended?

[Rick Byers, 2015-08-06 04:03:15]

On 2015/08/05 14:40:12, philipj wrote:
>
https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/1179223002/diff/180001/Source/core/html/HTMLM...
> Source/core/html/HTMLMediaElement.cpp:3799:
> document().domWindow()->addEventListener("scroll",
> m_autoplayExperimentScrollListener, false);
> On 2015/08/05 14:27:59, Rick Byers wrote:
> > On 2015/07/27 at 06:03:09, liberato wrote:
> > > On 2015/07/21 12:51:29, philipj wrote:
> > > > I take it this will prevent smooth scrolling while dragging with the
> finger
> > over
> > > > the video? There must be some way of learning about scrolling without
> > falling of
> > > > the fast scrolling path?
> > > 
> > > good point.  we really only care if it ends up in view, not happens to
> scroll
> > by.
> > > 
> > > however, i wasn't able to find much else that catches all the cases of
> > scrolling.  the next CL will focus on touch end / cancel and poll a few
times
> to
> > find the end of any inertial scroll.  scrollIntoView() won't be noticed, and
> > similar.
> > > 
> > > i'll collect some feedback on whether this is a reasonable approximation. 
> for
> > now, let's assume that it is.
> > 
> > Listening to scroll events doesn't "prevent smooth scrolling".  scroll
events
> > are async and don't block threaded scrolling (although they do tweak some
> > scheduler settings, but if that's visibly observable then it's probably a
> > schedule bug and not your issue).  Most major mobile websites have scroll
> > listeners on the document anyway, so adding one more isn't really a problem
-
> as
> > long as you're careful to not do much work or dirty layout etc.
> > 
> > Touch event handlers, on the other hand, do block the start of scrolling. 
In
> > general it's preferable to watch 'scroll' than lower level input like
> 'touch*',
> > 'wheel', etc.
> 
> Oh... http://rbyers.github.io/EventListenerOptions/EventListenerOptions.html
> mentions "wheel" and but I remembered it as "scroll".

No worries, I know it's confusing (and I haven't done a great job
documenting/evangelizing here).

> In that case, all of this will be much nicer. Is there a way to know for sure
> that a scroll has ended?

Not web-exposed, no.  Blink has some idea, but it's complicated.  In the limit,
scrolling may be driven by a RAF loop in JS so we can't possibly know, lots of
other specific cases where we should be able to tell, but no general system. 
tdresser's scroll customization project is working to unify all this into a
general system (which will eventually be web exposed).

[liberato (no reviews please), 2015-08-06 06:37:58]

this addresses previous CL feedback.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:378: if (document.settings() &&
document.settings()->mediaPlaybackRequiresUserGesture())
On 2015/08/05 10:03:11, philipj wrote:
> Wow, this has actually been broken the whole time, you can already circumvent
> the restriction entirely by getting a document with no settings object:
> 
> var video = document.querySelector("video");
> var doc = document.implementation.createHTMLDocument();
> doc.body.appendChild(video.cloneNode(true));
> var autoplayVideo = doc.body.firstChild.cloneNode(true);
> video.parentNode.replaceChild(autoplayVideo, video);
> autoplayVideo.play();
> 
> So, yeah, we should probably make all of this not suck so much. Not sure what
> would happen if this trick was widely known... o_O

i'm glad you're one of the good guys.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1608: if (autoplayExperimentIsVisible()) {
On 2015/08/05 10:03:11, philipj wrote:
> Perhaps rename this so that it's clear that it always returns true when
> visibility isn't required, or move that checks to the call sites if that would
> be clearer.

Done.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1618: m_paused = false;
On 2015/08/05 10:03:11, philipj wrote:
> This branch was previously predicated on the sandbox check passing, was that
an
> intentional change?

it still is, though it's virtually impossible to tell from the formatting here.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:2117: if (progress < 0.5) {
On 2015/08/05 10:03:11, philipj wrote:
> Why the added {}?

whoops, thanks -- i had more code in there and forgot to remove.  i still have a
strong habit for {} that requires conscious effort to override.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:2233: // playing, then start playing.  In
other words, start playing if
On 2015/08/05 10:03:11, philipj wrote:
> Really start playing by setting the muted attribute? Even with no intention of
> shipping this, I'm not sure what could be learned from this.

there was interest in this from one of the original threads on the subject.

for me, comparing bailout rates of "visible", "visible but no audio" and
"visible and we turned off the audio" might be interesting.  particularly, i'm
curious to know if the latter two bailout rates are different.  it won't be
enough to figure out why, but it will tell us if there's something to look
deeper at.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3971: // Check visibility.
On 2015/08/05 10:03:11, philipj wrote:
> A page visibility check is missing, this ought to pass even if the tab is in
the
> background. Make sure to have a separate flag for page visibity, maybe
> "pagevisible" and "inviewport" or something?

i don't think it'll ever play unless in the foreground, at least on android. 
i'll add a comment to that effect and rename 'visible' to make it more clear
that it's viewport.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3991: // Why are we always preparing? 
Just go!
On 2015/08/05 10:03:12, philipj wrote:
> Is this a question to the reviewer, or a rhetorical question? FWIW, I'm not
sure
> when we need to prepare and not, so if you know, please document it :)

random movie quotes make reviewing more fun!

but yeah, i'd remove it before landing either way :)

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:291: // Media element with autoplay seen.
On 2015/08/05 10:03:12, philipj wrote:
> I'm not sure what this broad metric is for, but the documentation isn't
> accurate, this counts both autoplay and play(). If it's possible to change the
> enum names that could be more clear too.

i've added some comments and renamed some of the new ones to match.  i don't
want to fiddle with the existing ones, since they match up with descriptions
(stored elsewhere), that the original experiment owner might care about.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:294: AutoplayStopped = 1,
On 2015/08/05 10:03:12, philipj wrote:
> Isn't it necessary to have one stopped entry for each start entry in order to
> compare how users react, including measuring the stop and bailout metrics for
> when the experiment is disabled entirely. (Maybe people bailout a lot even
when
> starting a play deliberately?)

i gave this more thought today, but still believe that we don't need all the
cases.  I added one that counts any play/stop/bailout, whether user-initiated or
not.  that seems very useful, thanks!

as for the remainder, if this were the only round of measurement, then i would
agree.  however, we will be able to see which buckets have potentially
interesting things in them, and can design additional experiments more carefully
to target those buckets specifically.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:307: AutoplayExperimentStartedByLoad = 7,
On 2015/08/05 10:03:12, philipj wrote:
> I think s/Load/AutoplayAttribute/, it's not really the load that causes the
> play, it's the autoplay attribute which causes the load, and reaching a
certain
> point in loading then plays. It's probably worth testing if this actually
works
> at all on Android, or if it's only actually calling play() that will start the
> load and eventually play.

true, but this is from a previous experiment.  the text here currently lines up
with the description (elsewhere).  i'd prefer to keep them roughly in sync, and
i don't want to change the other description without the previous experiment
owner's agreement.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:460: // Install an event listener to check
for changes in visibility.  If a
On 2015/08/05 10:03:12, philipj wrote:
> Instead of the touchend/touchcancel+timeout solution, can you investigate
> hooking into FrameView::scrollPositionChanged()?
> setTextTrackKindUserPreferenceForAllMediaElements is another method that takes
a
> Document* and does something for all media elements within.
> 
> I don't think there's really a good way to know that a scroll has ended, so
> perhaps just continually resetting a 200ms (?) timeout and doing the check
when
> it finally runs would work? Rick Byers would be a good person to ask about
> better ways here.

based on all the comments around this, i've merged the two approaches.  we now
check for scroll events only, but use then to reset a 300msec timer. when the
timer fires, we declare that scrolling has stopped.  it does no work in the
event handler other than to set the timer, and seems to work quite well in
practice.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:627: bool m_autoplayExperimentPlayPending :
1;
On 2015/08/05 10:03:12, philipj wrote:
> It looks like the delayed play is only done for explicit play() calls and not
> for autoplay?

the delayed play will happen for autoplay attribute as well.  this just records
whether a play() has been issued without a subsequent pause().  the attr would
also cause a play in that case (...IsEligible())

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:646: ExperimentPlayMuted = 16
On 2015/08/05 10:03:12, philipj wrote:
> Bit fields are usually defined using 1 << n in Blink, see e.g.
DelayedActionType
> in this header.

Done.

[liberato (no reviews please), 2015-08-06 06:40:02]

thanks, CR tool!  now here's the message i was trying to type just a moment ago:

rbyers: i've modified it to use event-based scrolling.  does this seem like a
better approach?

thanks
-fl

[Rick Byers, 2015-08-06 15:04:26]

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:460: // Install an event listener to check
for changes in visibility.  If a
On 2015/08/06 06:37:58, liberato wrote:
> On 2015/08/05 10:03:12, philipj wrote:
> > Instead of the touchend/touchcancel+timeout solution, can you investigate
> > hooking into FrameView::scrollPositionChanged()?
> > setTextTrackKindUserPreferenceForAllMediaElements is another method that
takes
> a
> > Document* and does something for all media elements within.
> > 
> > I don't think there's really a good way to know that a scroll has ended, so
> > perhaps just continually resetting a 200ms (?) timeout and doing the check
> when
> > it finally runs would work? Rick Byers would be a good person to ask about
> > better ways here.
> 
> based on all the comments around this, i've merged the two approaches.  we now
> check for scroll events only, but use then to reset a 300msec timer. when the
> timer fires, we declare that scrolling has stopped.  it does no work in the
> event handler other than to set the timer, and seems to work quite well in
> practice.

This sounds like a good plan to me.  I'm sure websites do this sort of thing in
JS all the time.  You could of course debate over the timer value (it's not
uncommon for someone to pause briefly while scrolling with touch), but that's a
UI decision that should be entirely up to you I think.

[Rick Byers, 2015-08-06 15:07:49]

https://codereview.chromium.org/1179223002/diff/300001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/300001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3913: m_autoplayExperimentTouchListener =
adoptRef(new AutoplayExperimentTouchListener(this));
nit: please change the names from 'TouchListener' to 'ScrollListener' - there's
already enough confusion around the difference here ;-)

https://codereview.chromium.org/1179223002/diff/300001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3915: // don't try to catch programmatic
scrolls right now.
'scroll' fires for programmatic scrolls too.  Is that OK?  Maybe just update the
comment?

[liberato (no reviews please), 2015-08-06 15:32:28]

rbyers: i'll get some opinions on the timeout before landing.

adamk, Oystein: do either of you have any concerns about these changes?

thanks
-fl

https://codereview.chromium.org/1179223002/diff/300001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/300001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3913: m_autoplayExperimentTouchListener =
adoptRef(new AutoplayExperimentTouchListener(this));
On 2015/08/06 15:07:49, Rick Byers wrote:
> nit: please change the names from 'TouchListener' to 'ScrollListener' -
there's
> already enough confusion around the difference here ;-)

oops, thanks!  was tired, forgot to change the name back.

https://codereview.chromium.org/1179223002/diff/300001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3915: // don't try to catch programmatic
scrolls right now.
On 2015/08/06 15:07:49, Rick Byers wrote:
> 'scroll' fires for programmatic scrolls too.  Is that OK?  Maybe just update
the
> comment?

yeah, stale comment.  i really should push CLs out only in the morning.

[liberato (no reviews please), 2015-08-06 17:32:15]

liberato@chromium.org changed reviewers:
+ dglazkov@chromium.org

[liberato (no reviews please), 2015-08-06 18:27:52]

dglazkov: philipj is out of town, and suggested that someone replace him as a
reviewer on this.  do you have time to take a look?

the background is that we want to allow autoplay on clank without requiring user
gesture in all cases.  this is not the immediate goal of this CL, however.

instead, this CL is the first step to help us learn what user behavior actually
is around autoplay behavior.  here, we try a few different strategies for when
we require a user gesture to start autoplay on clank, and measure how this
impacts user behavior.  we do not intend any of these options on permanently.

instead, we'll take what we learn here, and devise more experiments.  once we
have a handle on what will improve user experience, we will try to figure out
how to spec and implement a solution.

thanks
-fl

[dglazkov, 2015-08-06 20:55:06]

dglazkov@chromium.org changed reviewers:
+ ojan@chromium.org

[dglazkov, 2015-08-06 20:55:23]

+ojan

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:289: class
HTMLMediaElement::AutoplayExperimentScrollListener : public EventListener  {
Is this the right approach? Adding Ojan, the TL for all things spatial.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:394: if (autoplayMode.contains("enabled"))
{
See my note above about not using strings for passing parameters.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3914:
document().domWindow()->addEventListener("scroll",
m_autoplayExperimentScrollListener, false);
Ojan, PTAL.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3960: FloatRect us(offsetLeft(),
offsetTop(), clientWidth(), clientHeight());
This will force layout, and something we should avoid -- especially coming off a
timer.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:439: bool isBailout() const;
This seems like it could be a separate CL.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:477: // vvvv Helpers for clank autoplay
investigation vvvv
Please no ASCII art -___-

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:481: void
autoplayExperimentInstallEventListenerIfNeeded();
I would like to avoid spamming already-large class with all these extra helpers.
If they are really necessary, consider splitting them off into a helper class.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:728: Timer<HTMLMediaElement>
m_autoplayViewportTimer;
Using timers here seems bad. Why?

https://codereview.chromium.org/1179223002/diff/320001/public/web/WebSettings.h
File public/web/WebSettings.h (right):

https://codereview.chromium.org/1179223002/diff/320001/public/web/WebSettings...
public/web/WebSettings.h:280: virtual void setAutoplayExperimentMode(const
WebString&) = 0;
Let's not use a String here. Take a look at how TextTrackKindUserPreference is
used for an alternative.

[liberato (no reviews please), 2015-08-07 14:33:16]

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3960: FloatRect us(offsetLeft(),
offsetTop(), clientWidth(), clientHeight());
On 2015/08/06 20:55:23, dglazkov wrote:
> This will force layout, and something we should avoid -- especially coming off
a
> timer.

one alternative is checking layoutObject()->needsLayout() and deferring while
that is true.  is this the sort of thing you're thinking about?

however, do you believe that this will trigger a layout very often in practice? 
it'll only happen once at the conclusion of a scroll (not scroll event).  plus,
there's a delay after the scroll ends, so if the page updates in response to the
scroll (e.g., JS scrolls something into view), it has time to do the layout.

[liberato (no reviews please), 2015-08-07 21:40:58]

Patchset #19 (id:360001) has been deleted

[liberato (no reviews please), 2015-08-07 21:53:17]

haven't addressed all of the comments, but wanted to see if the refactor is in
the right direction.

thanks
-fl

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:477: // vvvv Helpers for clank autoplay
investigation vvvv
On 2015/08/06 20:55:23, dglazkov wrote:
> Please no ASCII art -___-

                 .-'''-.                                    
_______         '   _    \                                  
\  ___ `'.    /   /` '.   \    _..._         __.....__      
 ' |--.\  \  .   |     \  '  .'     '.   .-''         '.    
 | |    \  ' |   '      |  '.   .-.   . /     .-''"'-.  `.  
 | |     |  '\    \     / / |  '   '  |/     /________\   \ 
 | |     |  | `.   ` ..' /  |  |   |  ||                  | 
 | |     ' .'    '-...-'`   |  |   |  |\    .-------------' 
 | |___.' /'                |  |   |  | \    '-.____...---. 
/_______.'/                 |  |   |  |  `.             .'  
\_______|/                  |  |   |  |    `''-...... -'    
                            |  |   |  |                     
                            '--'   '--'

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:481: void
autoplayExperimentInstallEventListenerIfNeeded();
On 2015/08/06 20:55:23, dglazkov wrote:
> I would like to avoid spamming already-large class with all these extra
helpers.
> If they are really necessary, consider splitting them off into a helper class.

this change makes me happy.  PS19 has most of the autoplay experiment logic
split out into AutoplayExperimentHelper.  It's a friend class so that the newly
exposed methods can be private.  encapsulating more than that doesn't seem
worthwhile.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:728: Timer<HTMLMediaElement>
m_autoplayViewportTimer;
On 2015/08/06 20:55:23, dglazkov wrote:
> Using timers here seems bad. Why?

we want to know when a scroll has (probably) ended, and that's the best idea
that i've been able to come up with.  suggestions welcome on alternate
approaches.

[ojan, 2015-08-11 02:45:21]

Here's some more code review feedback. Feel free to ping me on IM if you have
any questions. You've been waiting a long time, so I want to help you move
forward quickly.

I'm not sure I've wrapped my head around what the best experiments to run here
are. I'll think more on what I think the best end result would look like.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:728: Timer<HTMLMediaElement>
m_autoplayViewportTimer;
On 2015/08/07 at 21:53:17, liberato wrote:
> On 2015/08/06 20:55:23, dglazkov wrote:
> > Using timers here seems bad. Why?
> 
> we want to know when a scroll has (probably) ended, and that's the best idea
that i've been able to come up with.  suggestions welcome on alternate
approaches.

I think having a timer is good. The problem is what we're doing in the timer. :)

Instead of hooking at this layer, we should hook into the layout tree directly.
That will be much cheaper because it will run as part of the rendering pipeline
instead of artificially forcing the rendering pipeline to run during the timer
(which is what the offset* calls currently do). Also, then you don't need to
add/remove event listeners at all.

In term of concretes, I think what you want to do is:
1. Add a Vector<LayoutVideo*> to LayoutView (LayoutView is the root of the
Layout tree) and put any unplayed LayoutVideos in it.
2. Add a m_isVisible to LayoutVideo.
3. In FrameView::updatePostLifecycleData, loop over ever frame and update the
m_isVisible bit for each of the LayoutVideos in it. If the bit is turned on,
*then* start the timer. When the timer fires, you can read the bit to make the
appropriate decision.

In addition to being more efficient, this will fix bugs in your current patch
for when a video becomes visible due to things other than scrolling (e.g. an
animation over a transform:translate). It also means you won't do work for
display:none videos since they won't show up in the layout tree.

mpd: I added you to this review as an FYI because we'll want to incorporate this
walk into your PositionObserver walk (i.e. each video would be observed).

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:76: const String& autoplayMode =
document().settings()->autoplayExperimentMode();
Did you also want to measure the case Philip suggested of basing it off the
tab's visibility instead of the video element's?

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:96: if
(autoplayMode.contains("-playmuted")) {
For good measure, add a comment explaining this one too?

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:233: return screen.contains(us);
What if the video is larger than the screen or part of the video overflows off
the right-side?

Let me propose a different and simpler solution. How about we make it so that we
play when the screen contains the play button? That way we know the user can
pause it if they want to, it avoids all these edge cases and, in the common
case, it will still mean that the video will only play when the whole video is
visible.

[ojan, 2015-08-11 02:49:35]

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:76: const String& autoplayMode =
document().settings()->autoplayExperimentMode();
On 2015/08/11 at 02:45:21, ojan wrote:
> Did you also want to measure the case Philip suggested of basing it off the
tab's visibility instead of the video element's?

Also, I don't see anything for tab visibility. So, as it is, a background tab
could start playing a video?

[Rick Byers, 2015-08-11 13:45:26]

rbyers@chromium.org changed reviewers:
- rbyers@chromium.org

[philipj_slow, 2015-08-13 09:27:53]

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:2233: // playing, then start playing.  In
other words, start playing if
On 2015/08/06 06:37:57, liberato wrote:
> On 2015/08/05 10:03:11, philipj wrote:
> > Really start playing by setting the muted attribute? Even with no intention
of
> > shipping this, I'm not sure what could be learned from this.
> 
> there was interest in this from one of the original threads on the subject.
> 
> for me, comparing bailout rates of "visible", "visible but no audio" and
> "visible and we turned off the audio" might be interesting.  particularly, i'm
> curious to know if the latter two bailout rates are different.  it won't be
> enough to figure out why, but it will tell us if there's something to look
> deeper at.

Acknowledged.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3971: // Check visibility.
On 2015/08/06 06:37:57, liberato wrote:
> On 2015/08/05 10:03:11, philipj wrote:
> > A page visibility check is missing, this ought to pass even if the tab is in
> the
> > background. Make sure to have a separate flag for page visibity, maybe
> > "pagevisible" and "inviewport" or something?
> 
> i don't think it'll ever play unless in the foreground, at least on android. 
> i'll add a comment to that effect and rename 'visible' to make it more clear
> that it's viewport.

What is it that would prevent playback when not in the foreground? Video is
paused when going into the background, but I'm not sure if there's anything to
prevent from starting to play when the user gesture requirement is gone. For
audio, there's no pausing in the background either.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3991: // Why are we always preparing? 
Just go!
On 2015/08/06 06:37:57, liberato wrote:
> On 2015/08/05 10:03:12, philipj wrote:
> > Is this a question to the reviewer, or a rhetorical question? FWIW, I'm not
> sure
> > when we need to prepare and not, so if you know, please document it :)
> 
> random movie quotes make reviewing more fun!
> 
> but yeah, i'd remove it before landing either way :)

Ah, Spaceballs :)

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:291: // Media element with autoplay seen.
On 2015/08/06 06:37:58, liberato wrote:
> On 2015/08/05 10:03:12, philipj wrote:
> > I'm not sure what this broad metric is for, but the documentation isn't
> > accurate, this counts both autoplay and play(). If it's possible to change
the
> > enum names that could be more clear too.
> 
> i've added some comments and renamed some of the new ones to match.  i don't
> want to fiddle with the existing ones, since they match up with descriptions
> (stored elsewhere), that the original experiment owner might care about.

I see, so entries 0-5 should be left as they are.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:294: AutoplayStopped = 1,
On 2015/08/06 06:37:58, liberato wrote:
> On 2015/08/05 10:03:12, philipj wrote:
> > Isn't it necessary to have one stopped entry for each start entry in order
to
> > compare how users react, including measuring the stop and bailout metrics
for
> > when the experiment is disabled entirely. (Maybe people bailout a lot even
> when
> > starting a play deliberately?)
> 
> i gave this more thought today, but still believe that we don't need all the
> cases.  I added one that counts any play/stop/bailout, whether user-initiated
or
> not.  that seems very useful, thanks!
> 
> as for the remainder, if this were the only round of measurement, then i would
> agree.  however, we will be able to see which buckets have potentially
> interesting things in them, and can design additional experiments more
carefully
> to target those buckets specifically.

I don't think I understand, if we don't have any per-experiment data on bailout,
what can we learn about how users react to different kinds of restrictions?

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:627: bool m_autoplayExperimentPlayPending :
1;
On 2015/08/06 06:37:58, liberato wrote:
> On 2015/08/05 10:03:12, philipj wrote:
> > It looks like the delayed play is only done for explicit play() calls and
not
> > for autoplay?
> 
> the delayed play will happen for autoplay attribute as well.  this just
records
> whether a play() has been issued without a subsequent pause().  the attr would
> also cause a play in that case (...IsEligible())

OK, so the mechanism for delayed play is different for autoplay and play().

[philipj_slow, 2015-08-13 10:15:40]

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:65: AutoplayExperimentHelper*
m_helper;
It looks like the pointer is always passed and never cleared, so could this be a
reference so that the null check above goes away?

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:294: // the element is still
marked as eligible, then we'll probably
setMuted returns early if there's no change, so I don't think there could be
infinite recursion. Given the same fact, I think this could be simplified to
just if (m_mode & ExperimentPlayMuted) m_element.setMuted(true);

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:2: * Copyright (C) 2015 Google Inc.
All rights reserved.
This is new code, so I think you can use the short header.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:36: namespace blink {
Hmm, merge this into the below namespace blink block?

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:62: // Gestureless playback when
media scrolled into view.  We don't
I can't parse "Gestureless playback when media scrolled into view." :)

A blank line somewhere around here would help make it clear that "We don't
record whether it was a javascript or attribute autoplay request" is talking
about the scroll bit, not more generally. (I was scratching my head about
GesturelessPlaybackStartedByLoad which does distinguish.)

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:65: // Autoplay started by
experiment override during initial load.
If I'm reading this right, GesturelessPlaybackStartedByLoad will measure only
the cases where there was a user gesture requirement at the point where where
autoplaying flag is consulted, and playback was started at that very point. If
that's right, then GesturelessPlaybackStartedByAutoplayingFlag would make this
more clear to me. The documentation can say that it's only the synchronous case,
not anything that happens after scrolling.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:89: void onReadyToPlay();
The name had me thinking it was a callback based on a readyState change or
perhaps around prepareToPlay. Perhaps onAutoplayingWithoutUserGesture? Also,
would it make sense for either all or none of these to be called only if the
regular gesture requirement failed?

I can't find anything in the style guide, but it strikes me as unfamiliar to
have onFoo methods in Blink. More common seems to be method names in the past
tense (see e.g. WebMediaPlayerClient) and in some cases pre/post pairs, like in
pre/postDispatchEventHandler.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:111: // Set the mute flag on the
media if we're in an experiment mode that
s/mute/muted/

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:131: // Return our media player's
document.
s/player/element/

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:152: // the user has scrolled the
player into the viewport.
To be pedantic, it could also be scripts driving the scrolling.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:394: // If we are about to enter a stopped
state, call this to record
s/stop/pause/, media elements don't really have a concept of being stopped, and
it's easy to confuse this with HTMLMediaElement::stop(), which is something else
entirely, for ActiveDOMObject.

[philipj_slow, 2015-08-13 11:12:49]

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:681: void
HTMLMediaElement::recordMetricsIfStopping()
Since this AutoplayExperimentHelper is a friend class, can't this and basically
everything except a few callbacks be moved there? isBailout() doesn't even need
any internal state.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1984:
recordAutoplayMetric(PlayMethodFailed);
If recordAutoplayMetric is also moved to the helper, I guess replace this with a
callback.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:2156: m_autoplayHelper.onMuteChanged();
s/Mute/Muted/

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3130: recordMetricsIfStopping();
HTMLMediaElement::stop() is an ActiveDOMObject override, I don't think it
actually makes sense to include this in the metrics, not sure why it was done
originally. In order to capture cases where users close the tab in reaction to
autoplay, it would be better measured somewhere else explicitly, I think.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3751: m_initialPlayWithoutUserGestures =
value;
You didn't add m_initialPlayWithoutUserGestures, but if you want to rename it to
m_initialPlayWithoutUserGesture that'd be nice. The plural doesn't make sense to
me.

[philipj_slow, 2015-08-21 08:29:05]

Ping! The branch point is today, and it would be really nice to get this
experiment going.

[liberato (no reviews please), 2015-08-21 14:13:42]

On 2015/08/21 08:29:05, philipj wrote:
> Ping! The branch point is today, and it would be really nice to get this
> experiment going.

indeed. i was ooto unexpectedly, and just didn't finish the rewrite to
visibility detection that Ojan suggested on time.

-fl

[liberato (no reviews please), 2015-09-01 06:54:20]

i'm still potentially chasing down a few linker errors on some platforms, but
locally this works fine.  the changes to the visibility checks are sufficiently
interesting from the last published CL that i wanted to get it out there.

i doubt that the linker issue will change much of anything interesting, if this
most recent CL doesn't fix them outright.

thanks
-fl

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3971: // Check visibility.
On 2015/08/13 09:27:53, philipj wrote:
> On 2015/08/06 06:37:57, liberato wrote:
> > On 2015/08/05 10:03:11, philipj wrote:
> > > A page visibility check is missing, this ought to pass even if the tab is
in
> > the
> > > background. Make sure to have a separate flag for page visibity, maybe
> > > "pagevisible" and "inviewport" or something?
> > 
> > i don't think it'll ever play unless in the foreground, at least on android.

> > i'll add a comment to that effect and rename 'visible' to make it more clear
> > that it's viewport.
> 
> What is it that would prevent playback when not in the foreground? Video is
> paused when going into the background, but I'm not sure if there's anything to
> prevent from starting to play when the user gesture requirement is gone. For
> audio, there's no pausing in the background either.

Done.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:289: class
HTMLMediaElement::AutoplayExperimentScrollListener : public EventListener  {
On 2015/08/06 20:55:23, dglazkov wrote:
> Is this the right approach? Adding Ojan, the TL for all things spatial.

Done.

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/320001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:728: Timer<HTMLMediaElement>
m_autoplayViewportTimer;
On 2015/08/11 02:45:21, ojan wrote:
> On 2015/08/07 at 21:53:17, liberato wrote:
> > On 2015/08/06 20:55:23, dglazkov wrote:
> > > Using timers here seems bad. Why?
> > 
> > we want to know when a scroll has (probably) ended, and that's the best idea
> that i've been able to come up with.  suggestions welcome on alternate
> approaches.
> 
> I think having a timer is good. The problem is what we're doing in the timer.
:)
> 
> Instead of hooking at this layer, we should hook into the layout tree
directly.
> That will be much cheaper because it will run as part of the rendering
pipeline
> instead of artificially forcing the rendering pipeline to run during the timer
> (which is what the offset* calls currently do). Also, then you don't need to
> add/remove event listeners at all.
> 
> In term of concretes, I think what you want to do is:
> 1. Add a Vector<LayoutVideo*> to LayoutView (LayoutView is the root of the
> Layout tree) and put any unplayed LayoutVideos in it.
> 2. Add a m_isVisible to LayoutVideo.
> 3. In FrameView::updatePostLifecycleData, loop over ever frame and update the
> m_isVisible bit for each of the LayoutVideos in it. If the bit is turned on,
> *then* start the timer. When the timer fires, you can read the bit to make the
> appropriate decision.
> 
> In addition to being more efficient, this will fix bugs in your current patch
> for when a video becomes visible due to things other than scrolling (e.g. an
> animation over a transform:translate). It also means you won't do work for
> display:none videos since they won't show up in the layout tree.
> 
> mpd: I added you to this review as an FYI because we'll want to incorporate
this
> walk into your PositionObserver walk (i.e. each video would be observed).

thanks for the detailed description!  the most recent CL incorporates these
changes.  the only potential weirdness is that we get called back a lot -- while
testing on desktop, this includes a callback for any mouse movement or video
frame playback.  i've tried to keep the callback light, but i'd appreciate
feedback here.  is it worth it to add another timer to debounce the callbacks a
bit?

https://codereview.chromium.org/1179223002/diff/320001/public/web/WebSettings.h
File public/web/WebSettings.h (right):

https://codereview.chromium.org/1179223002/diff/320001/public/web/WebSettings...
public/web/WebSettings.h:280: virtual void setAutoplayExperimentMode(const
WebString&) = 0;
On 2015/08/06 20:55:23, dglazkov wrote:
> Let's not use a String here. Take a look at how TextTrackKindUserPreference is
> used for an alternative.

I tried several approaches to make Settings.in take an enum, and they all have
some significant drawbacks.  TextTrackKindUserPreference is accessed from
outside blink as an enum, which isn't what i want.  i'd like to send in
human-readable strings, so that (e.g.) the finch experiment config doesn't have
experiment mode "3", but rather "enabled-ifviewport".  so, i need a string
parser somewhere, both for the tests and for outside WebKit.

however, i could find no single place to put the parser such that it can be
accessed both from outside blink and from the layout tests.  InternalSettings is
where TextTrack* adds a parser for the tests, but that's not going to help blink
consumers.  if there is some way have one parser implementation that both
about::flags and that blink layout tests could use, then that would make it all
work out.

so, after spending a fair bit of time trying to make it better, i moved back to
strings in the settings.  i did encapsulate the parsing in a separate helper
class, just to clean it up a little.

if i've missed some approach that avoids these issues, please let me know.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:65: AutoplayExperimentHelper*
m_helper;
On 2015/08/13 10:15:40, philipj wrote:
> It looks like the pointer is always passed and never cleared, so could this be
a
> reference so that the null check above goes away?

indeed, but the listener has since been removed.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:76: const String& autoplayMode =
document().settings()->autoplayExperimentMode();
On 2015/08/11 02:49:34, ojan wrote:
> On 2015/08/11 at 02:45:21, ojan wrote:
> > Did you also want to measure the case Philip suggested of basing it off the
> tab's visibility instead of the video element's?
> 
> Also, I don't see anything for tab visibility. So, as it is, a background tab
> could start playing a video?

added a check for page()->visibilityState() to isInViewport().

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:96: if
(autoplayMode.contains("-playmuted")) {
On 2015/08/11 02:45:21, ojan wrote:
> For good measure, add a comment explaining this one too?

done, though the docs are with the enums in AutoplayExperimentConfig.h, rather
than in the parser.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:233: return screen.contains(us);
On 2015/08/11 02:45:21, ojan wrote:
> What if the video is larger than the screen or part of the video overflows off
> the right-side?
> 
> Let me propose a different and simpler solution. How about we make it so that
we
> play when the screen contains the play button? That way we know the user can
> pause it if they want to, it avoids all these edge cases and, in the common
> case, it will still mean that the video will only play when the whole video is
> visible.

for those videos that don't show controls, or use custom controls, we'd be out
of luck.

i'm going to leave it as-is for now, but some change does need to be made for
the "video-larger-than-viewport" case at least.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:294: // the element is still
marked as eligible, then we'll probably
On 2015/08/13 10:15:40, philipj wrote:
> setMuted returns early if there's no change, so I don't think there could be
> infinite recursion. Given the same fact, I think this could be simplified to
> just if (m_mode & ExperimentPlayMuted) m_element.setMuted(true);

infinite recursion: true, though it's not obvious that setMuted() should set
m_muted before calling onMuteChanged.  i was duplicating the check here because
it's cheap.  although, since !isEligible, it won't loop anyway, so i removed the
muted check.

simplified: indeed, but i kept the assert.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:2: * Copyright (C) 2015 Google Inc.
All rights reserved.
On 2015/08/13 10:15:40, philipj wrote:
> This is new code, so I think you can use the short header.

thanks, forgot about the short one.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:36: namespace blink {
On 2015/08/13 10:15:40, philipj wrote:
> Hmm, merge this into the below namespace blink block?

done.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:62: // Gestureless playback when
media scrolled into view.  We don't
On 2015/08/13 10:15:40, philipj wrote:
> I can't parse "Gestureless playback when media scrolled into view." :)
> 
> A blank line somewhere around here would help make it clear that "We don't
> record whether it was a javascript or attribute autoplay request" is talking
> about the scroll bit, not more generally. (I was scratching my head about
> GesturelessPlaybackStartedByLoad which does distinguish.)

Done.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:65: // Autoplay started by
experiment override during initial load.
On 2015/08/13 10:15:40, philipj wrote:
> If I'm reading this right, GesturelessPlaybackStartedByLoad will measure only
> the cases where there was a user gesture requirement at the point where where
> autoplaying flag is consulted, and playback was started at that very point. If
> that's right, then GesturelessPlaybackStartedByAutoplayingFlag would make this
> more clear to me. The documentation can say that it's only the synchronous
case,
> not anything that happens after scrolling.

i renamed these.  i also separated GesturelessPlaybackStartedByScroll into
'autoplay flag after scroll' and 'play() after scroll' cases.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:89: void onReadyToPlay();
On 2015/08/13 10:15:40, philipj wrote:
> The name had me thinking it was a callback based on a readyState change or
> perhaps around prepareToPlay. Perhaps onAutoplayingWithoutUserGesture? Also,
> would it make sense for either all or none of these to be called only if the
> regular gesture requirement failed?
> 
> I can't find anything in the style guide, but it strikes me as unfamiliar to
> have onFoo methods in Blink. More common seems to be method names in the past
> tense (see e.g. WebMediaPlayerClient) and in some cases pre/post pairs, like
in
> pre/postDispatchEventHandler.

Done.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:111: // Set the mute flag on the
media if we're in an experiment mode that
On 2015/08/13 10:15:40, philipj wrote:
> s/mute/muted/

Done.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:131: // Return our media player's
document.
On 2015/08/13 10:15:40, philipj wrote:
> s/player/element/

Done.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:152: // the user has scrolled the
player into the viewport.
On 2015/08/13 10:15:40, philipj wrote:
> To be pedantic, it could also be scripts driving the scrolling.

listener was removed, as part of Ojan's suggested changes.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:681: void
HTMLMediaElement::recordMetricsIfStopping()
On 2015/08/13 11:12:48, philipj wrote:
> Since this AutoplayExperimentHelper is a friend class, can't this and
basically
> everything except a few callbacks be moved there? isBailout() doesn't even
need
> any internal state.

they could, but they were part of the old autoplay experiment as well.  when we
remove the new one, i don't know if we'll also want to remove the old one.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1984:
recordAutoplayMetric(PlayMethodFailed);
On 2015/08/13 11:12:48, philipj wrote:
> If recordAutoplayMetric is also moved to the helper, I guess replace this with
a
> callback.

Acknowledged.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:2156: m_autoplayHelper.onMuteChanged();
On 2015/08/13 11:12:48, philipj wrote:
> s/Mute/Muted/

Done.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3751: m_initialPlayWithoutUserGestures =
value;
On 2015/08/13 11:12:48, philipj wrote:
> You didn't add m_initialPlayWithoutUserGestures, but if you want to rename it
to
> m_initialPlayWithoutUserGesture that'd be nice. The plural doesn't make sense
to
> me.

Done.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:394: // If we are about to enter a stopped
state, call this to record
On 2015/08/13 10:15:40, philipj wrote:
> s/stop/pause/, media elements don't really have a concept of being stopped,
and
> it's easy to confuse this with HTMLMediaElement::stop(), which is something
else
> entirely, for ActiveDOMObject.

updated, and the metric names.  was trying not to modify the old enum names, but
what the heck.

[liberato (no reviews please), 2015-09-01 06:54:24]

i'm still potentially chasing down a few linker errors on some platforms, but
locally this works fine.  the changes to the visibility checks are sufficiently
interesting from the last published CL that i wanted to get it out there.

i doubt that the linker issue will change much of anything interesting, if this
most recent CL doesn't fix them outright.

thanks
-fl

[ojan, 2015-09-01 20:20:10]

ojan@chromium.org changed reviewers:
+ esprehn@chromium.org

[ojan, 2015-09-01 20:20:12]

I think the performance of this code will be good once we fix the timer issue I
mention below. \o/

Unfortunately, I don't have time to do a thorough review right now and I'll be
on a plane soon and likely won't be able to take another look at this review
until next week.

Philip and/or Elliott, would you be willing to take a look at this review to see
if you have anything to add? I'd like to let Frank land this ASAP so we can
start getting some data.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:2484:
view->sendMediaPositionChangeNotifications();
Please move this to the end of the AllPhases branch right above here.

Also, add:
TODO(ojan): Make this more general to be used by PositionObserver and rAF
throttling.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:113: void
installEventListenerIfNeeded();
Nit: They're not event listeners anymore now that you're not using scroll
events. FWIW, it sounds like the name we're going to ship to web authors will be
IntersectionObserver.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
File Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
Source/core/layout/LayoutMedia.cpp:152: if (element)
Nit: Typical blink style is to do the following, since there's no chance of
human error in the case where you need to include the type (i.e. the compiler
will yell at you if you accidentally type == instead of =):
if (HTMLMediaElement* element = mediaElement())

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
Source/core/layout/LayoutMedia.cpp:153: element->notifyPositionMayHaveChanged();
How about having the LayoutMedia store a bit as to whether the video is visible?
That way you only have to notify when the video visibility changes and you don't
need to start timers every time the video moves. Adding timers has a performance
impact, so it's best to start them only when you actually need them.

In this world, HTMLMediaElement doesn't need to store a rect, it would just
store a bool as to whether it's visible or not.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
File Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
Source/core/layout/LayoutView.cpp:1032: for (LayoutMedia** media =
m_mediaForPositionNotification.begin();
C++11 auto to make this for loop more readable. See the following as an example:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[philipj_slow, 2015-09-02 09:24:12]

I have reviewed the core/html bits again.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentConfig.h (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentConfig.h:8: #include "wtf/text/WTFString.h"
Looks like this can be forward declared.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:122: LayoutVideo* layoutVideo =
(LayoutVideo*)layoutObject;
Use static_cast

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:124: // TODO(liberato): do we
really need to keep track of this?  it's
Is this TODO to be fixed before landing? Who knows the answer?

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:135: LayoutVideo* video =
(LayoutVideo*)obj;
static_cast

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:165: viewportTimerFired(0);
nullptr

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:171: // treat it as the end of
scroll.  Autoplay if we should.
I didn't follow the discussion around this very closely, but is there no way to
know within Blink whether a fling is ongoing or not?

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:192: if (!isEligible()
A bit much line breaking here, it shouldn't be very long as a single line.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:198:
prepareToPlay(m_element.autoplay()
autoplay() checks the content attribute, but there's also the autoplaying flag
that the enum name refers to. Perhaps factor out a shouldAutoplay() so that the
same condition is used in all places?

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:220: if (!m_playPending &&
!m_element.autoplay())
shouldAutoplay() here too. To write a test for the distinction, you can create a
media element, add the autoplay attribute, then call pause to clear the
autoplaying flag, then do whatever the test would usually do.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:236: // If media is muted, then
autoplay when it comes into view.
Move this outside the if and remove {} for consistency with the above.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:259: // This also causes
!isEligible, so that we don't alow autoplay more than
s/alow/allow/

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:163: bool m_registeredWithView;
Put this together with the other bool to get them packed together.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:681: if (!m_playing)
Use m_paused here unless there's a reason to use m_playing instead. We shouldn't
have both, and m_playing is the one that we should try to remove.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1562: if (m_autoplaying && m_paused &&
autoplay()) {
Maybe factor this into a shouldAutoplay() that can be used in the other places?

[philipj_slow, 2015-09-02 09:31:47]

Following up on some older issues...

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:76: const String& autoplayMode =
document().settings()->autoplayExperimentMode();
On 2015/09/01 06:54:19, liberato wrote:
> On 2015/08/11 02:49:34, ojan wrote:
> > On 2015/08/11 at 02:45:21, ojan wrote:
> > > Did you also want to measure the case Philip suggested of basing it off
the
> > tab's visibility instead of the video element's?
> > 
> > Also, I don't see anything for tab visibility. So, as it is, a background
tab
> > could start playing a video?
> 
> added a check for page()->visibilityState() to isInViewport().

What about the experiment to only require page visibility? That's one of the
better candidates for actually shipping, so I think we should measure it.

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:3130: recordMetricsIfStopping();
On 2015/08/13 11:12:48, philipj wrote:
> HTMLMediaElement::stop() is an ActiveDOMObject override, I don't think it
> actually makes sense to include this in the metrics, not sure why it was done
> originally. In order to capture cases where users close the tab in reaction to
> autoplay, it would be better measured somewhere else explicitly, I think.

Ping.

[philipj_slow, 2015-09-02 09:35:37]

One very important issue left dangling, hopefully I'm just wrong but if I'm not
then this is important.

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:294: AutoplayStopped = 1,
On 2015/08/13 09:27:53, philipj wrote:
> On 2015/08/06 06:37:58, liberato wrote:
> > On 2015/08/05 10:03:12, philipj wrote:
> > > Isn't it necessary to have one stopped entry for each start entry in order
> to
> > > compare how users react, including measuring the stop and bailout metrics
> for
> > > when the experiment is disabled entirely. (Maybe people bailout a lot even
> > when
> > > starting a play deliberately?)
> > 
> > i gave this more thought today, but still believe that we don't need all the
> > cases.  I added one that counts any play/stop/bailout, whether
user-initiated
> or
> > not.  that seems very useful, thanks!
> > 
> > as for the remainder, if this were the only round of measurement, then i
would
> > agree.  however, we will be able to see which buckets have potentially
> > interesting things in them, and can design additional experiments more
> carefully
> > to target those buckets specifically.
> 
> I don't think I understand, if we don't have any per-experiment data on
bailout,
> what can we learn about how users react to different kinds of restrictions?

Ping. If separate buckets are needed, as it seems to me that they should be,
then not having them will influence the whole experiment. It's only if there's
some external mechanism not visibile inside Blink that pairs up the experiment
mode with these results and maps them into separate buckets elsewhere that I
would understand how it works.

[liberato (no reviews please), 2015-09-02 18:14:01]

On 2015/09/02 09:35:37, philipj wrote:
> One very important issue left dangling, hopefully I'm just wrong but if I'm
not
> then this is important.
> 
>
https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
> File Source/core/html/HTMLMediaElement.h (right):
> 
>
https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
> Source/core/html/HTMLMediaElement.h:294: AutoplayStopped = 1,
> On 2015/08/13 09:27:53, philipj wrote:
> > On 2015/08/06 06:37:58, liberato wrote:
> > > On 2015/08/05 10:03:12, philipj wrote:
> > > > Isn't it necessary to have one stopped entry for each start entry in
order
> > to
> > > > compare how users react, including measuring the stop and bailout
metrics
> > for
> > > > when the experiment is disabled entirely. (Maybe people bailout a lot
even
> > > when
> > > > starting a play deliberately?)
> > > 
> > > i gave this more thought today, but still believe that we don't need all
the
> > > cases.  I added one that counts any play/stop/bailout, whether
> user-initiated
> > or
> > > not.  that seems very useful, thanks!
> > > 
> > > as for the remainder, if this were the only round of measurement, then i
> would
> > > agree.  however, we will be able to see which buckets have potentially
> > > interesting things in them, and can design additional experiments more
> > carefully
> > > to target those buckets specifically.
> > 
> > I don't think I understand, if we don't have any per-experiment data on
> bailout,
> > what can we learn about how users react to different kinds of restrictions?
> 
> Ping. If separate buckets are needed, as it seems to me that they should be,
> then not having them will influence the whole experiment. It's only if there's
> some external mechanism not visibile inside Blink that pairs up the experiment
> mode with these results and maps them into separate buckets elsewhere that I
> would understand how it works.

there is exactly such an external mechanism.  each of the counters defined in
blink is kept in per-experiment buckets.  on can ask for the bailout rate for
the "enabled-ifvisible-ifmuted" experiment, for example.

sorry i forgot to answer this one.  turns out that i misunderstood what you were
asking until just now, so my answer would have been unhelpful anyway :)

[esprehn, 2015-09-02 21:02:41]

This patch is massive, can we please do this in smaller steps?

[liberato (no reviews please), 2015-09-03 06:48:37]

On 2015/09/02 21:02:41, esprehn wrote:
> This patch is massive, can we please do this in smaller steps?

it does keep getting bigger with every PS, doesn't it?

i'll remove the visibility checks from the experiment into a second CL, and see
how that looks.

thanks
-fl

[philipj_slow, 2015-09-03 09:17:03]

On 2015/09/02 18:14:01, liberato wrote:
> On 2015/09/02 09:35:37, philipj wrote:
> > One very important issue left dangling, hopefully I'm just wrong but if I'm
> not
> > then this is important.
> > 
> >
>
https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
> > File Source/core/html/HTMLMediaElement.h (right):
> > 
> >
>
https://codereview.chromium.org/1179223002/diff/280001/Source/core/html/HTMLM...
> > Source/core/html/HTMLMediaElement.h:294: AutoplayStopped = 1,
> > On 2015/08/13 09:27:53, philipj wrote:
> > > On 2015/08/06 06:37:58, liberato wrote:
> > > > On 2015/08/05 10:03:12, philipj wrote:
> > > > > Isn't it necessary to have one stopped entry for each start entry in
> order
> > > to
> > > > > compare how users react, including measuring the stop and bailout
> metrics
> > > for
> > > > > when the experiment is disabled entirely. (Maybe people bailout a lot
> even
> > > > when
> > > > > starting a play deliberately?)
> > > > 
> > > > i gave this more thought today, but still believe that we don't need all
> the
> > > > cases.  I added one that counts any play/stop/bailout, whether
> > user-initiated
> > > or
> > > > not.  that seems very useful, thanks!
> > > > 
> > > > as for the remainder, if this were the only round of measurement, then i
> > would
> > > > agree.  however, we will be able to see which buckets have potentially
> > > > interesting things in them, and can design additional experiments more
> > > carefully
> > > > to target those buckets specifically.
> > > 
> > > I don't think I understand, if we don't have any per-experiment data on
> > bailout,
> > > what can we learn about how users react to different kinds of
restrictions?
> > 
> > Ping. If separate buckets are needed, as it seems to me that they should be,
> > then not having them will influence the whole experiment. It's only if
there's
> > some external mechanism not visibile inside Blink that pairs up the
experiment
> > mode with these results and maps them into separate buckets elsewhere that I
> > would understand how it works.
> 
> there is exactly such an external mechanism.  each of the counters defined in
> blink is kept in per-experiment buckets.  on can ask for the bailout rate for
> the "enabled-ifvisible-ifmuted" experiment, for example.
> 
> sorry i forgot to answer this one.  turns out that i misunderstood what you
were
> asking until just now, so my answer would have been unhelpful anyway :)

I see, then it all make sense :)

[liberato (no reviews please), 2015-09-04 06:49:46]

philipj: didn't get to the "m_paused" vs "m_playing" comment -- i don't have a
device handy on which to check it easily.  other comments are addressed.

esprehn: this PS removes all visibility checks in order to make the CL smaller.

thanks
-fl

https://codereview.chromium.org/1179223002/diff/500001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:2484:
view->sendMediaPositionChangeNotifications();
On 2015/09/01 20:20:11, ojan wrote:
> Please move this to the end of the AllPhases branch right above here.
> 
> Also, add:
> TODO(ojan): Make this more general to be used by PositionObserver and rAF
> throttling.

Done.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentConfig.h (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentConfig.h:8: #include "wtf/text/WTFString.h"
On 2015/09/02 09:24:11, philipj wrote:
> Looks like this can be forward declared.

Done.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:122: LayoutVideo* layoutVideo =
(LayoutVideo*)layoutObject;
On 2015/09/02 09:24:11, philipj wrote:
> Use static_cast

Done.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:124: // TODO(liberato): do we
really need to keep track of this?  it's
On 2015/09/02 09:24:11, philipj wrote:
> Is this TODO to be fixed before landing? Who knows the answer?

yes, it will.  i wanted to put it out for CL with the TODO to invite comment.

i'll keep it, since it prevents a search through a vector.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:135: LayoutVideo* video =
(LayoutVideo*)obj;
On 2015/09/02 09:24:11, philipj wrote:
> static_cast

thanks, forgot what year it is.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:165: viewportTimerFired(0);
On 2015/09/02 09:24:11, philipj wrote:
> nullptr

Done.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:171: // treat it as the end of
scroll.  Autoplay if we should.
On 2015/09/02 09:24:12, philipj wrote:
> I didn't follow the discussion around this very closely, but is there no way
to
> know within Blink whether a fling is ongoing or not?

no, unfortunately.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:192: if (!isEligible()
On 2015/09/02 09:24:11, philipj wrote:
> A bit much line breaking here, it shouldn't be very long as a single line.

Done.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:198:
prepareToPlay(m_element.autoplay()
On 2015/09/02 09:24:11, philipj wrote:
> autoplay() checks the content attribute, but there's also the autoplaying flag
> that the enum name refers to. Perhaps factor out a shouldAutoplay() so that
the
> same condition is used in all places?

good point.  i'll add this for clarity, but i think because all of this shuts
off with !m_userGestureRequired, it should be the same thing.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:220: if (!m_playPending &&
!m_element.autoplay())
On 2015/09/02 09:24:11, philipj wrote:
> shouldAutoplay() here too. To write a test for the distinction, you can create
a
> media element, add the autoplay attribute, then call pause to clear the
> autoplaying flag, then do whatever the test would usually do.

Done, including test.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:236: // If media is muted, then
autoplay when it comes into view.
On 2015/09/02 09:24:11, philipj wrote:
> Move this outside the if and remove {} for consistency with the above.

Done.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:259: // This also causes
!isEligible, so that we don't alow autoplay more than
On 2015/09/02 09:24:11, philipj wrote:
> s/alow/allow/

Done.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.h (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:113: void
installEventListenerIfNeeded();
On 2015/09/01 20:20:11, ojan wrote:
> Nit: They're not event listeners anymore now that you're not using scroll
> events. FWIW, it sounds like the name we're going to ship to web authors will
be
> IntersectionObserver.

i renamed to "[un]registerForPositionUpdatesIfNeeded".  wanted to avoid anything
confusingly similar to IO.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.h:163: bool m_registeredWithView;
On 2015/09/02 09:24:12, philipj wrote:
> Put this together with the other bool to get them packed together.

Done.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:681: if (!m_playing)
On 2015/09/02 09:24:12, philipj wrote:
> Use m_paused here unless there's a reason to use m_playing instead. We
shouldn't
> have both, and m_playing is the one that we should try to remove.

i think i used m_paused originally, but switched after testing the metrics
manually.  i don't remember what case wasn't covered.

i'll re-test and see if there's a way to get the behavior without using
m_playing.

note that i didn't get to this today, and i don't have a device handy to try it
now.  so, i'll revisit tomorrow morning.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1562: if (m_autoplaying && m_paused &&
autoplay()) {
On 2015/09/02 09:24:12, philipj wrote:
> Maybe factor this into a shouldAutoplay() that can be used in the other
places?

Done.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
File Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
Source/core/layout/LayoutMedia.cpp:152: if (element)
On 2015/09/01 20:20:11, ojan wrote:
> Nit: Typical blink style is to do the following, since there's no chance of
> human error in the case where you need to include the type (i.e. the compiler
> will yell at you if you accidentally type == instead of =):
> if (HTMLMediaElement* element = mediaElement())

never thought of that, thanks!

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
Source/core/layout/LayoutMedia.cpp:153: element->notifyPositionMayHaveChanged();
On 2015/09/01 20:20:11, ojan wrote:
> How about having the LayoutMedia store a bit as to whether the video is
visible?
> That way you only have to notify when the video visibility changes and you
don't
> need to start timers every time the video moves. Adding timers has a
performance
> impact, so it's best to start them only when you actually need them.
> 
> In this world, HTMLMediaElement doesn't need to store a rect, it would just
> store a bool as to whether it's visible or not.

i implemented the logic as you suggest.  however, i put it in
AutoplayExperimentHelper, so that it's all in one place.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
File Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/layout/Lay...
Source/core/layout/LayoutView.cpp:1032: for (LayoutMedia** media =
m_mediaForPositionNotification.begin();
On 2015/09/01 20:20:11, ojan wrote:
> C++11 auto to make this for loop more readable. See the following as an
example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Done.

[liberato (no reviews please), 2015-09-04 06:49:50]

philipj: didn't get to the "m_paused" vs "m_playing" comment -- i don't have a
device handy on which to check it easily.  other comments are addressed.

esprehn: this PS removes all visibility checks in order to make the CL smaller.

thanks
-fl

[philipj_slow, 2015-09-04 08:42:30]

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:124: // TODO(liberato): do we
really need to keep track of this?  it's
On 2015/09/04 06:49:46, liberato wrote:
> On 2015/09/02 09:24:11, philipj wrote:
> > Is this TODO to be fixed before landing? Who knows the answer?
> 
> yes, it will.  i wanted to put it out for CL with the TODO to invite comment.
> 
> i'll keep it, since it prevents a search through a vector.

OK, I see this will be done in a separate CL.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:171: // treat it as the end of
scroll.  Autoplay if we should.
On 2015/09/04 06:49:45, liberato wrote:
> On 2015/09/02 09:24:12, philipj wrote:
> > I didn't follow the discussion around this very closely, but is there no way
> to
> > know within Blink whether a fling is ongoing or not?
> 
> no, unfortunately.

Acknowledged.

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:198:
prepareToPlay(m_element.autoplay()
On 2015/09/04 06:49:45, liberato wrote:
> On 2015/09/02 09:24:11, philipj wrote:
> > autoplay() checks the content attribute, but there's also the autoplaying
flag
> > that the enum name refers to. Perhaps factor out a shouldAutoplay() so that
> the
> > same condition is used in all places?
> 
> good point.  i'll add this for clarity, but i think because all of this shuts
> off with !m_userGestureRequired, it should be the same thing.

It should be possible to clear the autoplaying flag by calling play() or pause()
without a user gesture which leaves m_userGestureRequired unchanged. But I'd
have to use a debugger to say for sure when this would make a difference. It's
clearer after the change, so we all win :)

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/500001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:681: if (!m_playing)
On 2015/09/04 06:49:46, liberato wrote:
> On 2015/09/02 09:24:12, philipj wrote:
> > Use m_paused here unless there's a reason to use m_playing instead. We
> shouldn't
> > have both, and m_playing is the one that we should try to remove.
> 
> i think i used m_paused originally, but switched after testing the metrics
> manually.  i don't remember what case wasn't covered.
> 
> i'll re-test and see if there's a way to get the behavior without using
> m_playing.
> 
> note that i didn't get to this today, and i don't have a device handy to try
it
> now.  so, i'll revisit tomorrow morning.

OK, whatever you find out please let me know, whatever the differences between
m_paused and !m_playing I assume that they are somewhat subtle, which is why I'd
like to get rid of m_playing. (Well, mostly because the spec only has m_paused.)

[philipj_slow, 2015-09-04 08:44:09]

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/380001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:76: const String& autoplayMode =
document().settings()->autoplayExperimentMode();
On 2015/09/02 09:31:47, philipj wrote:
> On 2015/09/01 06:54:19, liberato wrote:
> > On 2015/08/11 02:49:34, ojan wrote:
> > > On 2015/08/11 at 02:45:21, ojan wrote:
> > > > Did you also want to measure the case Philip suggested of basing it off
> the
> > > tab's visibility instead of the video element's?
> > > 
> > > Also, I don't see anything for tab visibility. So, as it is, a background
> tab
> > > could start playing a video?
> > 
> > added a check for page()->visibilityState() to isInViewport().
> 
> What about the experiment to only require page visibility? That's one of the
> better candidates for actually shipping, so I think we should measure it.

I haven't seen the other CL yet, so just a gentle reminder to carry this issue
over to it.

[philipj_slow, 2015-09-04 09:24:27]

Sourcec/core/html and tests LGTM with a bunch of minor test nits and a
suggestion for consolidating the "can we autoplay" tests that may be messy, but
not too bad I think.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
File LayoutTests/media/video-autoplay-experiment-just-once.html (right):

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:5: var mediaFile;
It looks like none of these need to be global, if you pass parent to
prepareVideo() and move findMediaFile() into prepareVideo()

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:12: return
!video.paused || video.ended;
A more direct way is to check if video.played.length > 0, as nothing you do
except reload can fool you into thinking that the video hasn't been played.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:18:
video.src=mediaFile;
Missing whitespace

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:19:
video.setAttribute("autoplay", "true");
video.autoplay = true works as it's a reflected attribute. (Also, the string
"true", "false" and "" all mean the same thing for boolean content attributes,
so I tend to use the empty string in markup and cases like this.)

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:29:
document.body.insertBefore(parent, document.body.firstChild);
Since body has no other children (except a text node with a newline) just using
appendChild would make me stop wondering if there's anything special going on
here.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:32:
window.internals.settings.setMediaPlaybackRequiresUserGesture(true);
Use just internals or window.internals on both lines.

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

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:27: Mute   - how is media
muted?
This really amounts to testing for bugs in our muted handling, since per spec
there's an internal muted flag and that's the only things we should be checking.
We actually do have a bug in when that flag is set, but testing for it here as
opposed to a dedicated test doesn't seem warranted to me. The bug is
https://code.google.com/p/chromium/issues/detail?id=350303 and I failed to fix
it in https://codereview.chromium.org/205683003/

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:64: return !video.paused
|| video.ended;
Same video.played.length test can be used here.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:70: var row =
document.getElementById("results").insertRow(-1);
Since you're always appending rows and cells and the arguments are optional, you
can remove them so that it doesn't look like there's anything interesting about
the insertion order.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:74:
row.insertCell(-1).innerText = (""+spec["experimentNumber"]);
You can always replace spec["foo"] with spec.foo. At least I use spec[x] mostly
when x is a number or a variable.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:114:
element.parentElement.removeChild(element);
Can use element.remove()

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:130:
element.setAttribute("muted", "true");
element.muted = true and element.autoplay = true below.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:150:
element.src=mediaFile;
Missing whitespace

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:155: "enabled-forvideo",
Is it possible to test -ifmobile?

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:138: // If media is muted, then
autoplay when it comes into view.
This "when it comes into view" and other comments like it (below) should be
moved to the following CL I think.

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1904: return m_autoplaying && autoplay();
Can you also add m_paused and the sandbox requirement to the definition, to
match the spec? It says "If the autoplaying flag is true, and the paused
attribute is true, and the media element has an autoplay attribute specified,
and the media element's node document's active sandboxing flag set does not have
the sandboxed automatic features browsing context flag set"

Then use this same helper also in HTMLMediaElement::setReadyState. I realize
that this creates some trouble for recording metrics, perhaps the reason for
disallowing autoplay can be an out variable, or this could be turned into
allowAutoplay(), kind of like allowFullscreen, which is internally two other
methods used by HTMLMediaElement::setReadyState with ASSERTS to guarantee that
the usage of these are the same in both places.

The important point, of course, is that we should use exactly the same
definition of "can we autoplay?" in all contexts.

[liberato (no reviews please), 2015-09-08 16:37:47]

Patchset #28 (id:560001) has been deleted

[liberato (no reviews please), 2015-09-08 21:58:02]

cleaned up tests, added -ifmobile test, shouldAutoplay() based on CL feedback.

also changed handling of mutedAttr (see AutoplayExperimentHelper.cpp:138).

thanks
-fl

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
File LayoutTests/media/video-autoplay-experiment-just-once.html (right):

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:5: var mediaFile;
On 2015/09/04 09:24:26, philipj wrote:
> It looks like none of these need to be global, if you pass parent to
> prepareVideo() and move findMediaFile() into prepareVideo()

Done.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:12: return
!video.paused || video.ended;
On 2015/09/04 09:24:26, philipj wrote:
> A more direct way is to check if video.played.length > 0, as nothing you do
> except reload can fool you into thinking that the video hasn't been played.

This didn't seem to work -- it always returned false.  i haven't checked where
that's set yet.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:18:
video.src=mediaFile;
On 2015/09/04 09:24:26, philipj wrote:
> Missing whitespace

Done.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:19:
video.setAttribute("autoplay", "true");
On 2015/09/04 09:24:26, philipj wrote:
> video.autoplay = true works as it's a reflected attribute. (Also, the string
> "true", "false" and "" all mean the same thing for boolean content attributes,
> so I tend to use the empty string in markup and cases like this.)

Done.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:29:
document.body.insertBefore(parent, document.body.firstChild);
On 2015/09/04 09:24:26, philipj wrote:
> Since body has no other children (except a text node with a newline) just
using
> appendChild would make me stop wondering if there's anything special going on
> here.

Done.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:32:
window.internals.settings.setMediaPlaybackRequiresUserGesture(true);
On 2015/09/04 09:24:26, philipj wrote:
> Use just internals or window.internals on both lines.

no idea.  done.

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

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:27: Mute   - how is media
muted?
On 2015/09/04 09:24:27, philipj wrote:
> This really amounts to testing for bugs in our muted handling, since per spec
> there's an internal muted flag and that's the only things we should be
checking.
> We actually do have a bug in when that flag is set, but testing for it here as
> opposed to a dedicated test doesn't seem warranted to me. The bug is
> https://code.google.com/p/chromium/issues/detail?id=350303 and I failed to fix
> it in https://codereview.chromium.org/205683003/

gladly removed, this test is too big.  done. (except the dedicated test).

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:64: return !video.paused
|| video.ended;
On 2015/09/04 09:24:26, philipj wrote:
> Same video.played.length test can be used here.

Always returned false, so i kept it this way.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:70: var row =
document.getElementById("results").insertRow(-1);
On 2015/09/04 09:24:27, philipj wrote:
> Since you're always appending rows and cells and the arguments are optional,
you
> can remove them so that it doesn't look like there's anything interesting
about
> the insertion order.

neat.  i thought that chrome prepended by default.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:74:
row.insertCell(-1).innerText = (""+spec["experimentNumber"]);
On 2015/09/04 09:24:26, philipj wrote:
> You can always replace spec["foo"] with spec.foo. At least I use spec[x]
mostly
> when x is a number or a variable.

Done.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:114:
element.parentElement.removeChild(element);
On 2015/09/04 09:24:26, philipj wrote:
> Can use element.remove()

Done.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:130:
element.setAttribute("muted", "true");
On 2015/09/04 09:24:27, philipj wrote:
> element.muted = true and element.autoplay = true below.

Done.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:150:
element.src=mediaFile;
On 2015/09/04 09:24:26, philipj wrote:
> Missing whitespace

this was the original, just-once was the copy...

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:155: "enabled-forvideo",
On 2015/09/04 09:24:26, philipj wrote:
> Is it possible to test -ifmobile?

that's cool.  it seems that it now is possible.  when i first implemented this,
the mobile test always returned false when compiled for, e.g., linux desktop.  i
tracked down why back then, but i don't remember the details.  the code to set
the viewport type to legacy wasn't being included, somewhere.

anyway, i tried again and now it work.  yay for progress.

done.

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:138: // If media is muted, then
autoplay when it comes into view.
On 2015/09/04 09:24:27, philipj wrote:
> This "when it comes into view" and other comments like it (below) should be
> moved to the following CL I think.

done, and also this does the wrong thing.  checking for mutedAttr will succeed
even if it were unmuted by JS.

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1904: return m_autoplaying && autoplay();
On 2015/09/04 09:24:27, philipj wrote:
> Can you also add m_paused and the sandbox requirement to the definition, to
> match the spec? It says "If the autoplaying flag is true, and the paused
> attribute is true, and the media element has an autoplay attribute specified,
> and the media element's node document's active sandboxing flag set does not
have
> the sandboxed automatic features browsing context flag set"
> 
> Then use this same helper also in HTMLMediaElement::setReadyState. I realize
> that this creates some trouble for recording metrics, perhaps the reason for
> disallowing autoplay can be an out variable, or this could be turned into
> allowAutoplay(), kind of like allowFullscreen, which is internally two other
> methods used by HTMLMediaElement::setReadyState with ASSERTS to guarantee that
> the usage of these are the same in both places.
> 
> The important point, of course, is that we should use exactly the same
> definition of "can we autoplay?" in all contexts.

done.  i just moved the metrics recording into shouldAutoplay() and sent in a
bool.  that keeps the logic in one place.

[philipj_slow, 2015-09-10 09:58:59]

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
File LayoutTests/media/video-autoplay-experiment-just-once.html (right):

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:5: var mediaFile;
On 2015/09/08 21:58:01, liberato wrote:
> On 2015/09/04 09:24:26, philipj wrote:
> > It looks like none of these need to be global, if you pass parent to
> > prepareVideo() and move findMediaFile() into prepareVideo()
> 
> Done.

It doesn't matter much, but mediaFile is still global.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:12: return
!video.paused || video.ended;
On 2015/09/08 21:58:01, liberato wrote:
> On 2015/09/04 09:24:26, philipj wrote:
> > A more direct way is to check if video.played.length > 0, as nothing you do
> > except reload can fool you into thinking that the video hasn't been played.
> 
> This didn't seem to work -- it always returned false.  i haven't checked where
> that's set yet.

That's surprising, but figuring out why isn't in scope of this CL.

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

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:27: Mute   - how is media
muted?
On 2015/09/08 21:58:01, liberato wrote:
> On 2015/09/04 09:24:27, philipj wrote:
> > This really amounts to testing for bugs in our muted handling, since per
spec
> > there's an internal muted flag and that's the only things we should be
> checking.
> > We actually do have a bug in when that flag is set, but testing for it here
as
> > opposed to a dedicated test doesn't seem warranted to me. The bug is
> > https://code.google.com/p/chromium/issues/detail?id=350303 and I failed to
fix
> > it in https://codereview.chromium.org/205683003/
> 
> gladly removed, this test is too big.  done. (except the dedicated test).

Yay :)

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

https://codereview.chromium.org/1179223002/diff/580001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:85:
mobileMetaTag.name="viewport";
More missing whitespace :)

[philipj_slow, 2015-09-10 10:05:13]

> also changed handling of mutedAttr (see AutoplayExperimentHelper.cpp:138).

Not sure if this was also missed when uploading,
m_element.fastHasAttribute(mutedAttr) is still in AutoplayExperimentHelper.cpp

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:138: // If media is muted, then
autoplay when it comes into view.
On 2015/09/08 21:58:01, liberato wrote:
> On 2015/09/04 09:24:27, philipj wrote:
> > This "when it comes into view" and other comments like it (below) should be
> > moved to the following CL I think.
> 
> done, and also this does the wrong thing.  checking for mutedAttr will succeed
> even if it were unmuted by JS.

I can't see the change for this in PS28 either.

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1904: return m_autoplaying && autoplay();
On 2015/09/08 21:58:01, liberato wrote:
> On 2015/09/04 09:24:27, philipj wrote:
> > Can you also add m_paused and the sandbox requirement to the definition, to
> > match the spec? It says "If the autoplaying flag is true, and the paused
> > attribute is true, and the media element has an autoplay attribute
specified,
> > and the media element's node document's active sandboxing flag set does not
> have
> > the sandboxed automatic features browsing context flag set"
> > 
> > Then use this same helper also in HTMLMediaElement::setReadyState. I realize
> > that this creates some trouble for recording metrics, perhaps the reason for
> > disallowing autoplay can be an out variable, or this could be turned into
> > allowAutoplay(), kind of like allowFullscreen, which is internally two other
> > methods used by HTMLMediaElement::setReadyState with ASSERTS to guarantee
that
> > the usage of these are the same in both places.
> > 
> > The important point, of course, is that we should use exactly the same
> > definition of "can we autoplay?" in all contexts.
> 
> done.  i just moved the metrics recording into shouldAutoplay() and sent in a
> bool.  that keeps the logic in one place.

Did you forget to upload some changes? I can't see this change between PS27 and
PS28.

[liberato (no reviews please), 2015-09-10 14:44:30]

Patchset #29 (id:600001) has been deleted

[liberato (no reviews please), 2015-09-10 14:52:28]

moved some changes into this CL that were accidentally included in a dependent
CL. sorry about the confusion.

thanks
-fl

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
File LayoutTests/media/video-autoplay-experiment-just-once.html (right):

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:5: var mediaFile;
On 2015/09/10 09:58:59, philipj wrote:
> On 2015/09/08 21:58:01, liberato wrote:
> > On 2015/09/04 09:24:26, philipj wrote:
> > > It looks like none of these need to be global, if you pass parent to
> > > prepareVideo() and move findMediaFile() into prepareVideo()
> > 
> > Done.
> 
> It doesn't matter much, but mediaFile is still global.

Done.

https://codereview.chromium.org/1179223002/diff/540001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:12: return
!video.paused || video.ended;
On 2015/09/10 09:58:59, philipj wrote:
> On 2015/09/08 21:58:01, liberato wrote:
> > On 2015/09/04 09:24:26, philipj wrote:
> > > A more direct way is to check if video.played.length > 0, as nothing you
do
> > > except reload can fool you into thinking that the video hasn't been
played.
> > 
> > This didn't seem to work -- it always returned false.  i haven't checked
where
> > that's set yet.
> 
> That's surprising, but figuring out why isn't in scope of this CL.

Acknowledged.

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/Autop...
File Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/Autop...
Source/core/html/AutoplayExperimentHelper.cpp:138: // If media is muted, then
autoplay when it comes into view.
On 2015/09/10 10:05:13, philipj wrote:
> On 2015/09/08 21:58:01, liberato wrote:
> > On 2015/09/04 09:24:27, philipj wrote:
> > > This "when it comes into view" and other comments like it (below) should
be
> > > moved to the following CL I think.
> > 
> > done, and also this does the wrong thing.  checking for mutedAttr will
succeed
> > even if it were unmuted by JS.
> 
> I can't see the change for this in PS28 either.

oh, bother.  i inadvertently switched to the branch with the visibility checks
when i made some of these changes.  sorry about that.

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1179223002/diff/540001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.cpp:1904: return m_autoplaying && autoplay();
On 2015/09/10 10:05:13, philipj wrote:
> On 2015/09/08 21:58:01, liberato wrote:
> > On 2015/09/04 09:24:27, philipj wrote:
> > > Can you also add m_paused and the sandbox requirement to the definition,
to
> > > match the spec? It says "If the autoplaying flag is true, and the paused
> > > attribute is true, and the media element has an autoplay attribute
> specified,
> > > and the media element's node document's active sandboxing flag set does
not
> > have
> > > the sandboxed automatic features browsing context flag set"
> > > 
> > > Then use this same helper also in HTMLMediaElement::setReadyState. I
realize
> > > that this creates some trouble for recording metrics, perhaps the reason
for
> > > disallowing autoplay can be an out variable, or this could be turned into
> > > allowAutoplay(), kind of like allowFullscreen, which is internally two
other
> > > methods used by HTMLMediaElement::setReadyState with ASSERTS to guarantee
> that
> > > the usage of these are the same in both places.
> > > 
> > > The important point, of course, is that we should use exactly the same
> > > definition of "can we autoplay?" in all contexts.
> > 
> > done.  i just moved the metrics recording into shouldAutoplay() and sent in
a
> > bool.  that keeps the logic in one place.
> 
> Did you forget to upload some changes? I can't see this change between PS27
and
> PS28.

this was applied to the branch with visibility checks inadvertently, too.  i've
moved it back here.

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

https://codereview.chromium.org/1179223002/diff/580001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-modes.html:85:
mobileMetaTag.name="viewport";
On 2015/09/10 09:58:59, philipj wrote:
> More missing whitespace :)

Done.

[philipj_slow, 2015-09-11 07:17:48]

Still LGTM with nits. Can someone take another look at the layout bits?

https://codereview.chromium.org/1179223002/diff/620001/LayoutTests/media/vide...
File LayoutTests/media/video-autoplay-experiment-just-once.html (right):

https://codereview.chromium.org/1179223002/diff/620001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:22: var mediaFile =
findMediaFile("video", "content/test");
So this actually works because prepareVideo() is in the same scope and called
after this, but I was thinking simply setting video.src = findMediaFile("video",
"content/test") in prepareVideo().

https://codereview.chromium.org/1179223002/diff/620001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/620001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:144: bool shouldAutoplay(bool recordMetrics
= false);
Create an enum for this, like for example "enum class LoopCondition { Included,
Ignored };"

[liberato (no reviews please), 2015-09-11 19:01:43]

https://codereview.chromium.org/1179223002/diff/620001/LayoutTests/media/vide...
File LayoutTests/media/video-autoplay-experiment-just-once.html (right):

https://codereview.chromium.org/1179223002/diff/620001/LayoutTests/media/vide...
LayoutTests/media/video-autoplay-experiment-just-once.html:22: var mediaFile =
findMediaFile("video", "content/test");
On 2015/09/11 07:17:47, philipj wrote:
> So this actually works because prepareVideo() is in the same scope and called
> after this, but I was thinking simply setting video.src =
findMediaFile("video",
> "content/test") in prepareVideo().

Done.

https://codereview.chromium.org/1179223002/diff/620001/Source/core/html/HTMLM...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1179223002/diff/620001/Source/core/html/HTMLM...
Source/core/html/HTMLMediaElement.h:144: bool shouldAutoplay(bool recordMetrics
= false);
On 2015/09/11 07:17:47, philipj wrote:
> Create an enum for this, like for example "enum class LoopCondition {
Included,
> Ignored };"

Done.

[liberato (no reviews please), 2015-09-11 19:01:51]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2015-09-11 19:01:51]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1179223002/#ps640001
(title: "cl comments, rebased.")

[commit-bot: I haz the power, 2015-09-11 19:02:01]

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

[commit-bot: I haz the power, 2015-09-11 19:16:43]

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

[commit-bot: I haz the power, 2015-09-11 19:16:45]

Try jobs failed on following builders:
  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-09-14 14:58:05]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2015-09-14 14:58:05]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1179223002/#ps660001
(title: "rebased.")

[commit-bot: I haz the power, 2015-09-14 14:58:12]

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

[commit-bot: I haz the power, 2015-09-14 16:12:46]

Committed patchset #31 (id:660001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202204

[commit-bot: I haz the power, 2015-09-23 12:32:42]

Patchset 31 (id:??) landed as
https://crrev.com/0ad5bea84a7d3978c9e53e8a97b65b052307a00a