1463919 - Request users' approval to play audible media on tabs which are not allowed to play media with sound

Status: RESOLVED FIXED

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

Description

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

We've decided to prompt the user to approve/deny playing audible media on tab which have not had user interaction.

This bug covers landing the code send the request from HTMLMediaElement to front end.

Comment 1

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

WIP patch:
https://hg.mozilla.org/try/rev/3105db7dedc476d2b0e45c246f33433ebe166e0b

Comment 2

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

Attachment: Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

Add an implementation of nsIContentPermissionRequest to encapsulate requesting
permission from the user to autoplay audible media.

All documents in the tab request permission using the top level document's
origin, so the AutoplayRequest instance for a tab is stored on the top level
content window of the tab.

AutoplayRequest ensures that there's only a single prompt shown at once.

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

Comment 3

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

Attachment: Bug 1463919 - Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked.

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

Comment 4

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

Attachment: Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

When autoplay is requested by setting the "autoplay" attribute, we should
check whether autoplay is allowed in HTMLMediaElement::CheckAutoplayDataReady()
and if not we should prompt for user consent.

This ensures that <video ... autoplay/> will prompt for consent when used on
a page without a pre-existing allow/block permission.

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

Comment 5

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

Attachment: Bug 1463919 - Adjust mochitests to pass after changes.

Prior to the changes in this bug, a document would request autoplay permission
for its own origin, and not use the top level document's origin for the
permission check. Since now we use the top level document's origin for
requesting autoplay permission, some mochitests need to change, namely:

* test_autoplay_policy_permission.html can use
file_autoplay_policy_activation_frame.html directly. The test was failing
because its helper page was loaded same origin and the helper page was testing
if cross origins iframes could play; since we use the top level document's
permission request, this no longer effectively tests whether the cross origin
child can autoplay, as the cross origin child just uses the top level window's
origin for requests. So we can instead load the helper window cross orgin
instead, and remove one helper layer. Also an issue here is the way I was
waiting for a new window to load was racy, so now we wait for loading windows
to send us a "ready" message.

* test_autoplay_policy_activation.html; this test's helper needs to wait for
loading windows to send it a "ready" message, as its helper is shared with the
above test.

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

Comment 6

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

Attachment: Bug 1463919 - Ensure resuming via audio tab indicator bypasses autoplay permission check.

If the user opens a tab in the background, and that tab tries to play media,
we'll delay playing that media until the tab is brought to the foreground.
But the user can explicitly start playback of such delayed media by clicking
the "play" icon we show in the tab indicator. Then if autoplay is disabled,
we'll block the play (unless the origin is whitelisted). This is bad, as the
user has clearly indicated intent to play media in this tab.

So this patch adds an override flag that we set when the user starts playback
via the audio tab indicator that short-circuits the autoplay blocking logic,
ensuring we play in this case.

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

Comment 7

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

Attachment: Bug 1463919 - Test that starting play from tab audio indicator overrides block autoplay.

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

Comment 8

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

Attachment: Bug 1463919 - Tests for prompting for permission to autoplay.

Test that video that autoplays via a play() call or via autoplay attribute
plays or is blocked or prompts with a doorhanger for approval and plays when
"allow" clicked and doesn't when "block" clicked.

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

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8988064 [details]
Bug 1463919 - Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked.

https://reviewboard.mozilla.org/r/253304/#review259930

::: dom/html/HTMLMediaElement.h:59
(Diff revision 1)
>  class MediaInputPort;
>  class MediaStream;
>  class MediaStreamGraph;
>  class VideoFrameContainer;
>  namespace dom {
> +class AutoplayRequest;

Should that be in the DOM namespace?

This isn't a DOM object.

::: dom/html/HTMLMediaElement.cpp:4058
(Diff revision 1)
> +    // will call Play() again if the tab is brought to the foreground, or the
> +    // audio tab indicator is clicked, which will resolve the promise if we end
> +    // up playing.
> +    LOG(LogLevel::Debug, ("%p Play() call delayed by AudioChannelAgent", this));
> +    MaybeDoLoad();
> -  mPendingPlayPromises.AppendElement(promise);
> +    mPendingPlayPromises.AppendElement(promise);

Do we need to use multiple play promises?

why not re-use an existing promise if one is pending?

::: dom/html/HTMLMediaElement.cpp:4104
(Diff revision 1)
> +    AutoplayPolicy::RequestFor(WrapNotNull(OwnerDoc()));
> +  if (!request) {
> +    AsyncRejectPendingPlayPromises(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return;
> +  }
> +  RefPtr<HTMLMediaElement> self(this);

nit: I prefer RefPtr<HTLMMediaElement> self = this 
as this is one it's mostly used style-wise in the dom/media code.

I see one lonely existing self(this) form in this file...

::: dom/html/HTMLMediaElement.cpp:4111
(Diff revision 1)
> +    ->Then(mAbstractMainThread,
> +           __func__,
> +           [ self, handlingUserInput = aHandlingUserInput, request ](
> +             bool aApproved) {
> +             self->mAutoplayPermissionRequest.Complete();
> +             MOZ_RELEASE_ASSERT(!self->mAutoplayPermissionRequest.Exists());

unecessary, as you've called Complete() just above.

With the promise semantics, the assert is redundant, you can't be there if the promise request holder has been disconnected.

And I don't see the point in asserting the line of code above worked as documented.

::: dom/html/HTMLMediaElement.cpp:4121
(Diff revision 1)
> +             self->PlayInternal(handlingUserInput);
> +             self->UpdateCustomPolicyAfterPlayed();
> +           },
> +           [self, request](nsresult aError) {
> +             self->mAutoplayPermissionRequest.Complete();
> +             MOZ_RELEASE_ASSERT(!self->mAutoplayPermissionRequest.Exists());

same as above

::: dom/html/HTMLMediaElement.cpp:7895
(Diff revision 1)
>  }
>  
>  void
>  HTMLMediaElement::AsyncRejectPendingPlayPromises(nsresult aError)
>  {
> +  mAutoplayPermissionRequest.DisconnectIfExists();

So we disconnect AsyncRejectPendingPlayPromises (it will already be disconnected except when called from Pause())

Does that mean the prompt stays displayed even though we no longer care for it?

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

https://reviewboard.mozilla.org/r/253312/#review259938

::: dom/html/HTMLMediaElement.cpp:6237
(Diff revision 1)
>    if (!CanActivateAutoplay()) {
>      return;
>    }
>  
> +  if (!AutoplayPolicy::IsMediaElementAllowedToPlay(WrapNotNull(this))) {
> +    if (Preferences::GetBool("media.autoplay.ask-permission", false)) {

shouldn't the handling on this pref done within the Autoplay policy?

such that HTMLMediaElement doesn't need to know about specific exception to such autoplay policy.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8988066 [details]
Bug 1463919 - Adjust mochitests to pass after changes.

https://reviewboard.mozilla.org/r/253314/#review259940

::: commit-message-eb6c2:17
(Diff revision 1)
> +permission request, this no longer effectively tests whether the cross origin
> +child can autoplay, as the cross origin child just uses the top level window's
> +origin for requests. So we can instead load the helper window cross orgin
> +instead, and remove one helper layer. Also an issue here is the way I was
> +waiting for a new window to load was racy, so now we wait for loading windows
> +to send us a "ready" message.

tldr.

can you break in paragraphs?

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

https://reviewboard.mozilla.org/r/253310/#review259942

::: dom/html/HTMLMediaElement.cpp:4076
(Diff revision 1)
>    // ask for and are granted permission by the user.
>  
>    if (!Preferences::GetBool("media.autoplay.ask-permission", false)) {
>      LOG(LogLevel::Debug, ("%p play not allowed and prompting disabled.", this));
>      promise->MaybeReject(NS_ERROR_DOM_MEDIA_NOT_ALLOWED_ERR);
> +    if (Preferences::GetBool("media.autoplay.block-event.enabled", false)) {

I'd prefer a StaticPref access, makes it easier if there's any desire one day to change the pref name

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

https://reviewboard.mozilla.org/r/253302/#review259944

::: dom/html/AutoplayRequest.h:18
(Diff revision 1)
> +
> +class nsPIDOMWindowInner;
> +class nsIEventTarget;
> +
> +namespace mozilla {
> +namespace dom {

I don't think this object should be in the DOM namespace.

Comment 14

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

Comment on attachment 8988064 [details]
Bug 1463919 - Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked.

https://reviewboard.mozilla.org/r/253304/#review259930

> Do we need to use multiple play promises?
> 
> why not re-use an existing promise if one is pending?

The spec says to allocate a new one... https://html.spec.whatwg.org/multipage/media.html#playing-the-media-resource:media-element-28

> So we disconnect AsyncRejectPendingPlayPromises (it will already be disconnected except when called from Pause())
> 
> Does that mean the prompt stays displayed even though we no longer care for it?

Yes, Bug 1471485 will help with that somewhat... I think leaving the prompt showing is OK here, as if we hid it on promise reject, we could end up getting into a show/hide/show/hide loop on pages which were constantly playing/pausing/playing/pausing, which would be hard on the eyes.

Comment 15

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

Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

https://reviewboard.mozilla.org/r/253312/#review260326

::: dom/html/HTMLMediaElement.cpp:6237
(Diff revision 1)
>    if (!CanActivateAutoplay()) {
>      return;
>    }
>  
> +  if (!AutoplayPolicy::IsMediaElementAllowedToPlay(WrapNotNull(this))) {
> +    if (Preferences::GetBool("media.autoplay.ask-permission", false)) {

We could have AutoplayPolicy::IsMediaElementAllowedToPlay return an enum, {Allowed,Blocked,AskPermission}, would that suffice?

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

https://reviewboard.mozilla.org/r/253312/#review260326

> We could have AutoplayPolicy::IsMediaElementAllowedToPlay return an enum, {Allowed,Blocked,AskPermission}, would that suffice?

yes,

such 3 states thing is something netflix mentioned about too

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8988067 [details]
Bug 1463919 - Ensure resuming via audio tab indicator bypasses autoplay permission check.

https://reviewboard.mozilla.org/r/253306/#review260474

This looks solid to me. Thanks, cpearce!

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8988068 [details]
Bug 1463919 - Test that starting play from tab audio indicator overrides block autoplay.

https://reviewboard.mozilla.org/r/253316/#review260476

Thanks for the test!

::: toolkit/content/tests/browser/browser_block_autoplay_media.js:69
(Diff revision 1)
>    await ContentTask.spawn(tab2.linkedBrowser, SuspendedType.SUSPENDED_BLOCK,
>                            check_audio_suspended);
>  
> +
> +  // Test 4: Disable autoplay, enable asking for permission, and verify
> +  // that when a tab is opened in the background has had its playback

Nit: "when a tab is opened in the background has had its start delayed" -> "when a tab is opened in the background and has had its start delayed"

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8988067 [details]
Bug 1463919 - Ensure resuming via audio tab indicator bypasses autoplay permission check.

https://reviewboard.mozilla.org/r/253306/#review260478

::: toolkit/content/browser-content.js:532
(Diff revision 1)
>          break;
>        case "resumeMedia":
> +        // Override the autoplay blocking logic, so that resuming media
> +        // via the audio tab indicator will bypass the permission check
> +        // and allow media to autoplay.
> +        utils.allowAutoplayOverride = true;

Actually, out of curiosity, is this going to survive the lifetime of the document? Like... are we okay if this tab will be able to autoplay content going into the future even at different sites?

Comment 20

[Mike Conley (:mconley) (:⚙️)]

ni?ing cpearce for comment 19. Just making sure I understand what rights we're actually giving a tab when we allow it to play like this (is it intentional to be for the tab lifetime, or did we want this to be for the site lifetime?)

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

https://reviewboard.mozilla.org/r/253310/#review260480

Test looks good - thanks for adding it! Most of what I'm suggesting here is pretty minor.

::: commit-message-d191a:3
(Diff revision 1)
> +Test that video that autoplays via a play() call or via autoplay attribute
> +plays or is blocked or prompts with a doorhanger for approval and plays when
> +"allow" clicked and doesn't when "block" clicked.

Nit: This commit message has a lot of clauses and can probably be broken down into several sentences. I had to read it a few times to understand what it was saying.

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: I'm pretty sure our tests are supposed to be Public Domain licensed. At least, that was the last I heard about it.

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:20
(Diff revision 1)
> +    ["media.autoplay.ask-permission", true],
> +    ["media.autoplay.block-event.enabled", true],
> +  ]});
> +});
> +
> +// Content script that creates an autoplay video.

Let's document what the args are expected to be here.

Also, perhaps to make it clearer that this is a content script, perhaps this function should take care of calling ContentTask.spawn, and then we call it like

```js
await loadAutoplayVideo({ browser, ... });
```

in your test helper. I think that's a more typical pattern in our browser mochitest testing code rather than separate outside functions that get passed to ContentTask.spawn.

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:59
(Diff revision 1)
> +}
> +
> +// Content script that checks whether the video created by loadAutoplayVideo()
> +// started playing.
> +async function checkVideoDidPlay(args) {
> +  let video = content.document.getElementById("v1");

Similar to the above, this function should probably be responsible for calling ContentTask.spawn

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:72
(Diff revision 1)
> +  let tab = await BrowserTestUtils.openNewForegroundTab(window.gBrowser,
> +                                                        "about:blank");
> +  tab.linkedBrowser.loadURI(VIDEO_PAGE);
> +  await BrowserTestUtils.browserLoaded(tab.linkedBrowser);

See my other comment on how you can simplify this with `BrowserTestUtils.withNewTab`.

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:128
(Diff revision 1)
> +  let tab = await BrowserTestUtils.openNewForegroundTab(window.gBrowser,
> +                                                        "about:blank");

You can simplify this a bit, and take care of the removal of the tab all in one by using `BrowserTestUtils.withNewTab`, like so:

```js
await BrowserTestUtils.withNewTab({
  gBrowser,
  url: VIDEO_PAGE,
}, browser => {
  // At this point, VIDEO_PAGE is loaded in the
  // new tab, and you have access to its browser
  // element.
  
  // Rest of test goes in here.

});

// Tab gets automatically closed for you.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

https://reviewboard.mozilla.org/r/253302/#review260500

r- mainly because I feel like I'm missing something regarding user input handling.

::: dom/html/AutoplayRequest.cpp:40
(Diff revision 1)
> +}
> +
> +AutoplayRequest::~AutoplayRequest() {}
> +
> +already_AddRefed<AutoplayRequest>
> +AutoplayRequest::Create(nsPIDOMWindowInner* aWindow, bool aIsHandlingUserInput)

I'm clearly missing something here. aIsHandlingUserInput is always false.
Why do we then even need mIsHandlingUserInput member variable?

::: dom/html/AutoplayRequest.cpp:40
(Diff revision 1)
> +}
> +
> +AutoplayRequest::~AutoplayRequest() {}
> +
> +already_AddRefed<AutoplayRequest>
> +AutoplayRequest::Create(nsPIDOMWindowInner* aWindow, bool aIsHandlingUserInput)

Perhaps pass in nsGlobalWindowInner, since it has GetPrincipal() method. Then there isn't need to ever use GetDoc()

::: dom/media/AutoplayPolicy.cpp:78
(Diff revision 1)
>    return false;
>  }
>  
> +/* static */
> +already_AddRefed<AutoplayRequest>
> +AutoplayPolicy::RequestFor(NotNull<nsIDocument*> aDocument)

nsIDocument& aDocument as param? oh, other code in this file uses NonNull too.
I wonder why.

Comment 23

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

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #20)
> ni?ing cpearce for comment 19. Just making sure I understand what rights
> we're actually giving a tab when we allow it to play like this (is it
> intentional to be for the tab lifetime, or did we want this to be for the
> site lifetime?)

The override should not survive the lifetime of the document. That is, if we load a new document into a tab, the override should not persist. So that means what I have here isn't going to work, thanks for pointing that out!

Maybe we need to store the override somewhere else, like on the inner window.

Comment 24

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

Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

https://reviewboard.mozilla.org/r/253302/#review260500

> I'm clearly missing something here. aIsHandlingUserInput is always false.
> Why do we then even need mIsHandlingUserInput member variable?

You're right, mIsHandlingUserInput should always be false, if we'd had user input, we'd not need to prompt at all. I will remove this field and assume it's always false.

> nsIDocument& aDocument as param? oh, other code in this file uses NonNull too.
> I wonder why.

Sure, I was merely copying the precedent of using NotNull set by the previous author of this code.

Comment 25

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

Comment on attachment 8988064 [details]
Bug 1463919 - Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked.

https://reviewboard.mozilla.org/r/253304/#review260576

::: dom/html/HTMLMediaElement.h:59
(Diff revision 1)
>  class MediaInputPort;
>  class MediaStream;
>  class MediaStreamGraph;
>  class VideoFrameContainer;
>  namespace dom {
> +class AutoplayRequest;

I've removed AutoplayRequest out dom namespace, it's just in mozilla namespace now.

Comment 26

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

Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

https://reviewboard.mozilla.org/r/253310/#review260590

::: toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

You're correct! According to https://www.mozilla.org/en-US/MPL/license-policy/ 
"Trivial bits of Mozilla Code, such as testcases [...] should be put in the public domain"

Comment 27

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

I'll move the code to override the autoplay logic if the audio tab indicator is pressed into a new bug, so that I can land what we have here sooner since Mike will be out for holiday on Monday.

Comment 28

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

Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

https://reviewboard.mozilla.org/r/253310/#review260480

> You can simplify this a bit, and take care of the removal of the tab all in one by using `BrowserTestUtils.withNewTab`, like so:
> 
> ```js
> await BrowserTestUtils.withNewTab({
>   gBrowser,
>   url: VIDEO_PAGE,
> }, browser => {
>   // At this point, VIDEO_PAGE is loaded in the
>   // new tab, and you have access to its browser
>   // element.
>   
>   // Rest of test goes in here.
> 
> });
> 
> // Tab gets automatically closed for you.

Thanks, that makes it a lot nicer!

Comment 29

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

Comment on attachment 8988067 [details]
Bug 1463919 - Ensure resuming via audio tab indicator bypasses autoplay permission check.

https://reviewboard.mozilla.org/r/253306/#review260478

> Actually, out of curiosity, is this going to survive the lifetime of the document? Like... are we okay if this tab will be able to autoplay content going into the future even at different sites?

Will continue working on this patch in Bug 1472580 in order to land the rest here independently ASAP.

Comment 30

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

Attachment: Bug 1463919 - Check AutoplayPolicy before activating attribute based autoplay.

When autoplay is requested by setting the "autoplay" attribute, we should
check whether autoplay is allowed in HTMLMediaElement::CheckAutoplayDataReady()
and if not we should prompt for user consent.

This ensures that <video ... autoplay/> will prompt for consent when used on
a page without a pre-existing allow/block permission.

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

Comment 31

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

Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

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

Comment 32

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

Comment on attachment 8988064 [details]
Bug 1463919 - Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked.

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

Comment 33

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

Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

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

Comment 34

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

Comment on attachment 8988066 [details]
Bug 1463919 - Adjust mochitests to pass after changes.

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

Comment 35

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

Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

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

Comment 36

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

Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

Re-requesting review on "Move ask autoplay permission check into AutoplayPolicy", as I think it's changed enough since r+ to warrant re-review, and MozReview won't let me me re-request for some reason.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

https://reviewboard.mozilla.org/r/253302/#review260956

::: dom/media/AutoplayPolicy.cpp:101
(Diff revision 2)
>    }
>  
> +  // TODO : this old way would be removed when user-gestures-needed becomes
> +  // as a default option to block autoplay.
>    if (!Preferences::GetBool("media.autoplay.enabled.user-gestures-needed", false)) {
> +    // If elelement is blessed, it would always be allowed to play().

element

Comment 38

[Jean-Yves Avenard [:jya]]

Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

https://reviewboard.mozilla.org/r/253312/#review261098

Much more readable that way...

::: dom/media/AutoplayPolicy.h:23
(Diff revision 2)
>  namespace dom {
>  
>  class HTMLMediaElement;
>  class AudioContext;
>  
> +enum class Authorization {

{ on new line...

Comment 39

[Jean-Yves Avenard [:jya]]

Comment on attachment 8989095 [details]
Bug 1463919 - Check AutoplayPolicy before activating attribute based autoplay.

https://reviewboard.mozilla.org/r/254156/#review261100

::: dom/html/HTMLMediaElement.cpp:6234
(Diff revision 1)
>    if (!CanActivateAutoplay()) {
>      return;
>    }
>  
> +  switch (AutoplayPolicy::IsAllowedToPlay(*this)) {
> +    case Authorization::Blocked: {

{ } block isn't necessary here

::: dom/html/HTMLMediaElement.cpp:6238
(Diff revision 1)
> +  switch (AutoplayPolicy::IsAllowedToPlay(*this)) {
> +    case Authorization::Blocked: {
> +      return;
> +    }
> +    case Authorization::Prompt: {
> +      EnsureAutoplayRequested(false);

same

Comment 40

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

Comment on attachment 8988063 [details]
Bug 1463919 - Add AutoplayRequest to encapsulate asking for autoplay permission.

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

Comment 41

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

Comment on attachment 8988064 [details]
Bug 1463919 - Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked.

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

Comment 42

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

Comment on attachment 8988065 [details]
Bug 1463919 - Move ask autoplay permission check into AutoplayPolicy.

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

Comment 43

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

Comment on attachment 8989095 [details]
Bug 1463919 - Check AutoplayPolicy before activating attribute based autoplay.

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

Comment 44

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

Comment on attachment 8988066 [details]
Bug 1463919 - Adjust mochitests to pass after changes.

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

Comment 45

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

Comment on attachment 8988069 [details]
Bug 1463919 - Tests for prompting for permission to autoplay.

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

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc42cfddb48c
Add AutoplayRequest to encapsulate asking for autoplay permission. r=smaug
https://hg.mozilla.org/integration/autoland/rev/75087df2875a
Have HTMLMediaElement ask for autoplay permission when playback otherwise blocked. r=jya
https://hg.mozilla.org/integration/autoland/rev/a03d5236779d
Move ask autoplay permission check into AutoplayPolicy. r=jya
https://hg.mozilla.org/integration/autoland/rev/1810cad130d4
Check AutoplayPolicy before activating attribute based autoplay. r=jya
https://hg.mozilla.org/integration/autoland/rev/11017d3044e2
Adjust mochitests to pass after changes. r=jya
https://hg.mozilla.org/integration/autoland/rev/e780c6b275a2
Tests for prompting for permission to autoplay. r=jya,mconley

Comment 47

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/fc42cfddb48c
https://hg.mozilla.org/mozilla-central/rev/75087df2875a
https://hg.mozilla.org/mozilla-central/rev/a03d5236779d
https://hg.mozilla.org/mozilla-central/rev/1810cad130d4
https://hg.mozilla.org/mozilla-central/rev/11017d3044e2
https://hg.mozilla.org/mozilla-central/rev/e780c6b275a2