Add -ifsameorigin to autoplay experiment.

Add -ifsameorigin to autoplay experiment.

If -ifsameorigin is set in the autoplay experiment config, then
allow autoplay only for those things that are in the same origin.

BUG=599690
Committed: https://crrev.com/266cd3ce3ed7cbcb7018c11c5a7a7989ba52dad8
Cr-Commit-Position: refs/heads/master@{#390500}

[liberato (no reviews please), 2016-04-04 16:13:47]

Description was changed from

==========
Add -ifsameorigin and -ormuted to autoplay experiment.

If -ifsameorigin is set in the autoplay experiment config, then
allow autoplay for those things that are in the same origin.

If -ormuted is set, then -ifsameorigin will also permit muted
content to autoplay.  -ormuted without -ifsameorigin does nothing.

BUG=599690, 599695
==========

to

==========
Add -ifsameorigin and -ormuted to autoplay experiment.

If -ifsameorigin is set in the autoplay experiment config, then
allow autoplay for those things that are in the same origin.

If -ormuted is set, then -ifsameorigin will also permit muted
content to autoplay.  -ormuted without -ifsameorigin does nothing.

BUG=599690, 599695
==========

[liberato (no reviews please), 2016-04-04 16:13:47]

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

[liberato (no reviews please), 2016-04-04 16:27:26]

liberato@chromium.org changed reviewers:
- mlamouri@chromium.org, ojan@chromium.org, philipj@opera.com

[liberato (no reviews please), 2016-04-25 20:00:07]

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

[ojan, 2016-04-26 01:13:57]

Is the actual experiment we're going to run the following combined?

-forvideo
-foraudio
-ifpagevisible
-ifviewport
-ifsameorigin
-ormuted

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:305: const
SecurityOrigin* origin = frame->securityContext()->getSecurityOrigin();
Can you move this into a helper function on Frame.h? That way it can be used
here and in FrameView::notifyRenderThrottlingObservers().

[mlamouri (slow - plz ping), 2016-04-26 14:41:41]

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:312: }
I think you only want to check top() here. This is how we usually consider
cross-origin iframes, see:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:353: return
false;
Quite a bit of nesting, maybe you can merge `enabled(IfSameOrigin)` and
`!isSameOrigin()`

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right):

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:90:
MOCK_CONST_METHOD0(frame, LocalFrame*());
I wish we had more tests for this. Are there layout tests for the experiments?

[liberato (no reviews please), 2016-04-26 18:30:24]

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp (right):

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:305: const
SecurityOrigin* origin = frame->securityContext()->getSecurityOrigin();
On 2016/04/26 01:13:57, ojan wrote:
> Can you move this into a helper function on Frame.h? That way it can be used
> here and in FrameView::notifyRenderThrottlingObservers().

moved into a helper function.  however, i used mounir's UseCounter check rather
than one the one in notifyRender...(), so i didn't update the latter to use the
helper.  seemed pretty similar, but didn't want to make an unrelated change
that's not just refactoring.

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:312: }
On 2016/04/26 14:41:40, Mounir Lamouri wrote:
> I think you only want to check top() here. This is how we usually consider
> cross-origin iframes, see:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

done, and updated UseCounter::count...frame().

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayExperimentHelper.cpp:353: return
false;
On 2016/04/26 14:41:40, Mounir Lamouri wrote:
> Quite a bit of nesting, maybe you can merge `enabled(IfSameOrigin)` and
> `!isSameOrigin()`

Done.

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp (right):

https://codereview.chromium.org/1853543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayExperimentTest.cpp:90:
MOCK_CONST_METHOD0(frame, LocalFrame*());
On 2016/04/26 14:41:40, Mounir Lamouri wrote:
> I wish we had more tests for this. Are there layout tests for the experiments?

yes, there are layout tests for this also:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[philipj_slow, 2016-04-27 13:11:54]

philipj@opera.com changed reviewers:
- philipj@opera.com

[philipj_slow, 2016-04-27 13:11:55]

Last week at Opera, dropping self as reviewer.

[mlamouri (slow - plz ping), 2016-04-27 13:17:37]

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

[mlamouri (slow - plz ping), 2016-04-27 13:17:37]

I would to add things core/frame/ as a follow-up so you don't slow down this CL
by requiring more reviewers but that's up to you :)

Otherwise, could you add tests for the new flags?

https://codereview.chromium.org/1853543002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1853543002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Frame.h:124: // Return true if and only if
this frame is a cross-origin frame.
I think the comment should say cross-origin to what? (here, the top frame)

[philipj_slow, 2016-04-27 13:38:05]

philipj@opera.com changed reviewers:
- philipj@opera.com

[mlamouri (slow - plz ping), 2016-04-27 13:58:39]

Just realised that "-ormuted" isn't fully implemented because we actually expect
to do something when unmuting and this CL doesn't. Should we re-purpose this CL
to be only '-ifsameorigin' and leave '-ormuted' for another CL?

[ojan, 2016-04-28 03:03:51]

On 2016/04/27 at 13:17:37, mlamouri wrote:
> I would to add things core/frame/ as a follow-up so you don't slow down this
CL by requiring more reviewers but that's up to you :)

FWIW, I can lgtm for the core/frame part once we've resolved your question about
"-ormuted".

[liberato (no reviews please), 2016-04-28 05:24:50]

> Just realised that "-ormuted" isn't fully implemented

yup.  this was written while we were still unsure about the desired behavior.  i
was planning an additional CL at the time, but separating -ormuted out entirely
makes more sense at this point, as you suggest.

> FWIW, I can lg*m for the core/frame part

i think you accidentally did exactly that :)

-fl

[liberato (no reviews please), 2016-04-28 17:25:54]

Description was changed from

==========
Add -ifsameorigin and -ormuted to autoplay experiment.

If -ifsameorigin is set in the autoplay experiment config, then
allow autoplay for those things that are in the same origin.

If -ormuted is set, then -ifsameorigin will also permit muted
content to autoplay.  -ormuted without -ifsameorigin does nothing.

BUG=599690, 599695
==========

to

==========
Add -ifsameorigin to autoplay experiment.

If -ifsameorigin is set in the autoplay experiment config, then
allow autoplay only for those things that are in the same origin.

BUG=599690
==========

[liberato (no reviews please), 2016-04-28 17:32:35]

now with 12.5% more unit tests, and 100% fewer cross-origin exceptions for muted
playback.  as always, it's made with only the finest ingredients.

thanks
-fl

https://codereview.chromium.org/1853543002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1853543002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Frame.h:124: // Return true if and only if
this frame is a cross-origin frame.
On 2016/04/27 13:17:37, Mounir Lamouri wrote:
> I think the comment should say cross-origin to what? (here, the top frame)

Done.

[ojan, 2016-04-28 19:06:03]

The CQ bit was checked by ojan@chromium.org

[ojan, 2016-04-28 19:06:04]

lgtm

[commit-bot: I haz the power, 2016-04-28 19:06:29]

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

[commit-bot: I haz the power, 2016-04-28 20:40:33]

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

[commit-bot: I haz the power, 2016-04-28 20:40:35]

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

[liberato (no reviews please), 2016-04-28 21:36:25]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2016-04-28 21:36:49]

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

[commit-bot: I haz the power, 2016-04-28 22:05:39]

Committed patchset #9 (id:160001)

[mlamouri (slow - plz ping), 2016-04-29 10:28:16]

lgtm

[mlamouri (slow - plz ping), 2016-04-29 10:28:40]

Note though that LayoutTests would have been good to add :)

[liberato (no reviews please), 2016-04-29 14:13:01]

On 2016/04/29 10:28:40, Mounir Lamouri wrote:
> Note though that LayoutTests would have been good to add :)

i agree.  i attempted one before switching to the unit test.  i couldn't find an
implementation that wasn't really messy -- getting results back from the iframes
was convoluted, due to, i'm sure, my lack of experience with layout tests.

i'll take another try with the -ormuted cl.

[commit-bot: I haz the power, 2016-04-30 17:21:52]

Patchset 9 (id:??) landed as
https://crrev.com/266cd3ce3ed7cbcb7018c11c5a7a7989ba52dad8
Cr-Commit-Position: refs/heads/master@{#390500}

[commit-bot: I haz the power, 2016-04-30 17:57:51]

Description was changed from

==========
Add -ifsameorigin to autoplay experiment.

If -ifsameorigin is set in the autoplay experiment config, then
allow autoplay only for those things that are in the same origin.

BUG=599690
==========

to

==========
Add -ifsameorigin to autoplay experiment.

If -ifsameorigin is set in the autoplay experiment config, then
allow autoplay only for those things that are in the same origin.

BUG=599690

Committed: https://crrev.com/266cd3ce3ed7cbcb7018c11c5a7a7989ba52dad8
Cr-Commit-Position: refs/heads/master@{#390500}
==========

[dcheng, 2016-05-05 23:14:49]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-05-05 23:14:50]

https://codereview.chromium.org/1853543002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1853543002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Frame.h:126: bool isCrossOrigin() const;
Sorry for missing this... I've updated my WATCHLISTS.

Can we rename this to be clearer? Something like isCrossOriginToMainFrame(). I
know it's in the comment, but it's not at all clear in the call sites.