1458383 - Pinned tabs and blocked autoplay

Status: RESOLVED FIXED

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

Description

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

Should pinned tabs be able to autoplay?

Our current default behaviour (before block-autoplay) is for tabs loaded in the background to have playback delayed until the user focuses the tab. So if a page tries to play before the page has been foregrounded, we'll only actually start to play the media when user brings the tab to the foreground.

This is different for pinned tabs, which are allowed to play upon session restore without being foregrounded first.

This was to enable pinned mail/chat tabs being able to play sounds when showing notifications for new messages etc.

Rationale/previous decision:
https://bugzilla.mozilla.org/show_bug.cgi?id=1347791#c25

Unfortunately some users pin tabs to enable quick re-access upon session restore, and some users pin tabs for sites that like to autoplay media, but the users don't want them to autoplay, for example YouTube tabs:
bug 1449022, and bug 1378172 comment 4.

Also note that many of these media sites we expect will be whitelisted to allow autoplay, further defeating some users' expectations. IMO, the whitelist should be consulted *after* we've checked whether the page has been in the foreground to avoid that issue.

Comment 1

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

I'd overlooked that the block-autoplay UX spec specifies that pinned tabs should no longer be able to autoplay, but should instead show an audio tab indicator:

https://mozilla.invisionapp.com/share/N8F2K0I93#/screens/287176980

Having discussed this with Cindy, we agreed this is a good behaviour. Coupled with delaying playback start, this ensures that pinned tabs, even if for whitelisted origins, will not be a nuisance. If a tab is trying to play sounds to cause a notification, the audio tab indicator will show which brings attention to the tab, including if it's pinned.

Comment 2

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

Resolving, as we've decided to not have any special handling for pinned tabs. So nothing to do here.

Comment 3

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

(In reply to Chris Pearce (:cpearce) from comment #2)
> So nothing to do here.

This is incorrect.

With media.autoplay.enabled=false and media.autoplay.enabled.user-gestures-needed=true we're still autoplay in pinned tabs in whitelisted origins upon session restore. We need to partially backout Bug 1347791 (I think just patch part 4, commit 49b533231388, to stop the block status being session restored), and also we need to change nsPIDOMWindowOuter::mMediaSuspend so that it's initialized by default to SUSPENDED_BLOCK when gesture activation is required.

Comment 4

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

Attachment: Bug 1458383 - Don't session restore browser.mediaBlocked.

This is a backout of Bug 1347791 part 4; 49b533231388.

49b533231388 took the mediaBlocked attribute and stored it in session store,
and caused us to call browser.resumeMedia() or blockMedia() as appropriate
upon restore. We don't want to session restore whether tabs have had delay
playback start unblocked anymore, so remove the code that session stores
that attribute.

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

Comment 5

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

Attachment: Bug 1458383 - Remove browser mediaBlocked attribute.

This is a partial backout of Bug 1347791 part 3; a5fbb7e2d1d0.

a5fbb7e2d1d0 added a mediaBlocked attribute to the browser element, which
tracked whether the tab has ever been in the foreground, so that this could
be saved and restored in session restore. We don't session restore this
anymore, so we can remove this attribute and associated event handlers.

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

Comment 6

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

Attachment: Bug 1458383 - Don't dispatch mediaBlockStop notification.

This is a partial backout of Bug 1347791 part 3; a5fbb7e2d1d0.

We don't need to track in the front end code when a tab is foregrounded,
so we don't need to dispatch this message any more.

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

Comment 7

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

Attachment: Bug 1458383 - Assume tabs delay playback start of media.

Regardless of whether autoplay is enabled or not, we want to delay playback
start in tabs until the tab has been in the foreground. So initialize
nsPIDOMWindowOuter::mMediaSuspend to SUSPENDED_BLOCK if the controlling
pref for the delay-playback-start feature is enabled.

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

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8974252 [details]
Bug 1458383 - Don't dispatch mediaBlockStop notification.

https://reviewboard.mozilla.org/r/242574/#review248484

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8974253 [details]
Bug 1458383 - Assume tabs delay playback start of media.

https://reviewboard.mozilla.org/r/242576/#review248486

Comment 10

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

Comment on attachment 8974250 [details]
Bug 1458383 - Don't session restore browser.mediaBlocked.

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

Comment 11

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

Comment on attachment 8974251 [details]
Bug 1458383 - Remove browser mediaBlocked attribute.

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

Comment 12

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

Comment on attachment 8974252 [details]
Bug 1458383 - Don't dispatch mediaBlockStop notification.

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

Comment 13

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

Comment on attachment 8974253 [details]
Bug 1458383 - Assume tabs delay playback start of media.

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

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8974250 [details]
Bug 1458383 - Don't session restore browser.mediaBlocked.

https://reviewboard.mozilla.org/r/242570/#review249206

I trust that you can resolve the issue below without the need to have me check this again ;-)

::: browser/base/content/tabbrowser.js
(Diff revision 2)
> -            };
> -          break;
>          case "userTypedValue":
>          case "userTypedClear":
> -        case "mediaBlocked":
> -          getter = () => SessionStore.getLazyTabValue(aTab, name);

You're removing too much here! Please keep `getter = () => SessionStore.getLazyTabValue(aTab, name);`, otherwise urlbar content restoration will be broken.

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8974251 [details]
Bug 1458383 - Remove browser mediaBlocked attribute.

https://reviewboard.mozilla.org/r/242572/#review249210

Cool! Nice cleanup.

Comment 17

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

Comment on attachment 8974250 [details]
Bug 1458383 - Don't session restore browser.mediaBlocked.

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

Comment 18

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

Comment on attachment 8974251 [details]
Bug 1458383 - Remove browser mediaBlocked attribute.

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

Comment 19

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

Comment on attachment 8974252 [details]
Bug 1458383 - Don't dispatch mediaBlockStop notification.

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

Comment 20

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

Comment on attachment 8974253 [details]
Bug 1458383 - Assume tabs delay playback start of media.

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

Comment 21

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

Comment on attachment 8974250 [details]
Bug 1458383 - Don't session restore browser.mediaBlocked.

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

Comment 22

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

Comment on attachment 8974251 [details]
Bug 1458383 - Remove browser mediaBlocked attribute.

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

Comment 23

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

Comment on attachment 8974252 [details]
Bug 1458383 - Don't dispatch mediaBlockStop notification.

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

Comment 24

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

Comment on attachment 8974253 [details]
Bug 1458383 - Assume tabs delay playback start of media.

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a80de077d740
Don't session restore browser.mediaBlocked. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/510dfe436e4e
Remove browser mediaBlocked attribute. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/97e9b87a9a55
Don't dispatch mediaBlockStop notification. r=baku
https://hg.mozilla.org/integration/autoland/rev/74dc3b5bb9aa
Assume tabs delay playback start of media. r=baku

Comment 26

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a80de077d740
https://hg.mozilla.org/mozilla-central/rev/510dfe436e4e
https://hg.mozilla.org/mozilla-central/rev/97e9b87a9a55
https://hg.mozilla.org/mozilla-central/rev/74dc3b5bb9aa

Comment 27

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Hi Chris. I saw that here are some automated tests here. Does this require manual testing in these circumstances?

Comment 28

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

I don't think we need any more manual QA here. Thanks!