1477415 - Autoplay is blocked even on standalone top level audio/video document

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Tom S [:evilpie]]

I think people expect top level audio/video to play automatically. For example: https://www.sample-videos.com/audio/mp3/crowd-cheering.mp3

Comment 1

[Alastor Wu [:alwu]]

Agree, Chrome allows video document to autoplay as well.

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1477415 - part1 : allow video document autoplay.

Allow autoplay for video document when user is directly viewing a video/audio by visiting
its url.

Review commit: https://reviewboard.mozilla.org/r/258856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258856/

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1477415 - part2 : add test.

Review commit: https://reviewboard.mozilla.org/r/258858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258858/

Comment 4

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6151fa01c044d02cb17af130be916792ae6688d9

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8994330 [details]
Bug 1477415 - part1 : allow video document autoplay.

https://reviewboard.mozilla.org/r/258856/#review265908

r+ with HTMLMediaElement::mIsCreatedByVideoDocument removed.

::: dom/media/AutoplayPolicy.cpp:123
(Diff revision 1)
>    if (aElement.Volume() == 0.0 || aElement.Muted()) {
>      return nsIAutoplay::ALLOWED;
>    }
>  
> +  // Allow autoplay when user directly views the video/audio url
> +  if (aElement.IsCreatedByVideoDocument()) {

I think you can just check whether aElement.OwnerDoc()->MediaDocumentKind() == MediaDocumentKind::Video here instead of having to set a flag to store whether the HTMLMediaElement is in a MediaDocument.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8994331 [details]
Bug 1477415 - part2 : add test.

https://reviewboard.mozilla.org/r/258858/#review265910

::: toolkit/content/tests/browser/browser_autoplay_videoDocument.js:8
(Diff revision 1)
> +const PAGE = "https://example.com/browser/toolkit/content/tests/browser/audio.ogg";
> +
> +function setup_test_preference() {
> +  return SpecialPowers.pushPrefEnv({"set": [
> +    ["media.autoplay.default", SpecialPowers.Ci.nsIAutoplay.PROMPT],
> +    ["media.autoplay.enabled", false],

The "media.autoplay.enabled" pref has been removed, so you can remove it here too.

::: toolkit/content/tests/browser/browser_autoplay_videoDocument.js:14
(Diff revision 1)
> +    ["media.autoplay.enabled.user-gestures-needed", true],
> +    ["media.autoplay.ask-permission", true],
> +  ]});
> +}
> +
> +function checkIsVideoDocumentAutoplay(browser) {

There's a few things wrong here...

When you have an async function, the interpreter wraps the function with a state machine that yields control when you "await", returning a promise, and resuming execution when the promise the async function is awaiting on fulfils and the event loop is free.

If your async function runs to completion, the automatically generated promise resolves with the return value (undefined if your async function didn't return anything). If anything inside the async function threw an exception, the promise is rejected with the thrown exception.

Whereas here, you're returning a promise that contains an async function. The extra promise is unnecessary, as you have one being automatically generated here by the interpreter because your function has the "async" keyword.

Also it looks like you're manually creating a new promise here so that you can reject() on failure, but you're not actually checking whether the promise returned by checkIsVideoDocumentAutoplay() is resolved or rejected, so the extra promise is unnececssary.

Also, you're handling the case of the video having played to completion or potentially having nto started playback yet. This means sometimes you're testing that cideo documents play automatically, and other times you're testing that calling play() in a MediaDocument works.

You also can make the test cleaner by just testing whether calling play() in a MediaDocument works. I think that's sufficient.

So you can should be able to make this:

    return ContentTask.spawn(browser, null, async () => {
      let video = content.document.getElementsByTagName("video")[0];
      let played = video && await video.play().then(() => true, () => false);
      ok(played, "Should be able to play in video document");
    });

::: toolkit/content/tests/browser/browser_autoplay_videoDocument.js:44
(Diff revision 1)
> +add_task(async function testAutoplayVideoDocument() {
> +  info("- setup test preference -");
> +  await setup_test_preference();
> +
> +  info("- open new foreground tab -");
> +  let tab = await BrowserTestUtils.openNewForegroundTab(window.gBrowser,

You can use BrowserTestUtils.withNewTab() to automatically load the URI and remove the tab afterwards. See browser_autoplay_policy_request_permission.js as an example.

Comment 7

[Alastor Wu [:alwu]]

Thank for your suggestion!
I'll modify my patches and upload them later.

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8994330 [details]
Bug 1477415 - part1 : allow video document autoplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258856/diff/1-2/

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 8994331 [details]
Bug 1477415 - part2 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258858/diff/1-2/

Comment 10

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=754edff84c0c50e421e2590663f4d817965c01aa

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8994331 [details]
Bug 1477415 - part2 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258858/diff/2-3/

Comment 12

[Tom S [:evilpie]]

I think you need to make sure it is a top-level document, otherwise <iframe src="http://xxx/xxx.mp3"> would be allowed to play automatically.

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 8994331 [details]
Bug 1477415 - part2 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258858/diff/3-4/

Comment 14

[Alastor Wu [:alwu]]

(In reply to Tom Schuster [:evilpie] from comment #12)
> I think you need to make sure it is a top-level document, otherwise <iframe
> src="http://xxx/xxx.mp3"> would be allowed to play automatically.

We make the video document always can autoplay that is a document which only contains one media element. It happens when user visit video or audio by directly visiting its url.

Comment 15

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 33761c9a58efb2c7268f978d38a89c092061b1ce -d 5389bb094bc4: rebasing 474474:33761c9a58ef "Bug 1477415 - part1 : allow video document autoplay. r=cpearce"
merging dom/media/AutoplayPolicy.cpp
warning: conflicts while merging dom/media/AutoplayPolicy.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[Alastor Wu [:alwu]]

Comment on attachment 8994330 [details]
Bug 1477415 - part1 : allow video document autoplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258856/diff/2-3/

Comment 17

[Alastor Wu [:alwu]]

Comment on attachment 8994331 [details]
Bug 1477415 - part2 : add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258858/diff/4-5/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/784d6695694c
part1 : allow video document autoplay. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8890b8f7e9cc
part2 : add test. r=cpearce

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/784d6695694c
https://hg.mozilla.org/mozilla-central/rev/8890b8f7e9cc

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8994330 [details]
Bug 1477415 - part1 : allow video document autoplay.

https://reviewboard.mozilla.org/r/258856/#review268700

::: dom/media/AutoplayPolicy.cpp:107
(Diff revision 3)
>  
>  static bool
>  IsMediaElementAllowedToPlay(const HTMLMediaElement& aElement)
>  {
>    return ((aElement.Volume() == 0.0 || aElement.Muted()) &&
> -          Preferences::GetBool("media.autoplay.allow-muted", true)) ||
> +           Preferences::GetBool("media.autoplay.allow-muted", true)) ||

Indentation is wrong here...

Comment 22

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #21)
> ::: dom/media/AutoplayPolicy.cpp:107
> (Diff revision 3)
> >  
> >  static bool
> >  IsMediaElementAllowedToPlay(const HTMLMediaElement& aElement)
> >  {
> >    return ((aElement.Volume() == 0.0 || aElement.Muted()) &&
> > -          Preferences::GetBool("media.autoplay.allow-muted", true)) ||
> > +           Preferences::GetBool("media.autoplay.allow-muted", true)) ||
> 
> Indentation is wrong here...

This has been fixed :)

Comment 23

[Gyula Palko[:gpalko]]

Verified on Nightly 63.0a1(20180819221915), that autoplay is not blocked on top level audio document.