Closed Bug 1457048 Opened 6 years ago Closed 6 years ago

Check if site is whitelisted before blocking autoplay

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(3 files)

Bug 1457048 - Test that whitelisted origins are able to autoplay.
59 bytes, text/x-review-board-request
bryce
: review+
Details
Bug 1457048 - Ensure origins with autoplay-media exact permission can autoplay.
59 bytes, text/x-review-board-request
bryce
: review+
johannh
: review+
Details
Bug 1457048 - Fix permissions tests to accomodate autoplay-media.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
We intent do whitelist some origins to allow them to autoplay videos.

To implement this, we'll have a permission for autoplay. We need to use nsIPermissionManager to read the permission. I've been told that in the content process nsIPermissionManager does a synchronous IPC to the main process to lookup  permmission, and we don't want to be adding more sync IPC, so we'll need to find a non-synchronous way to check against the whitelist.
When adding the permission please make sure it's added to the appropriate pieces on the front-end (I know there's bug 1457425, too, I just wanted to explicitly mention it). I'm mostly thinking of https://searchfox.org/mozilla-central/rev/68fdb6cf4f40bea1a1f6c07531ebf58fb8ab060b/browser/modules/SitePermissions.jsm#578 for inclusion in the site-specific permission UI (the identity popup and page info).

(In reply to Chris Pearce (:cpearce) from comment #0)
> To implement this, we'll have a permission for autoplay. We need to use
> nsIPermissionManager to read the permission. I've been told that in the
> content process nsIPermissionManager does a synchronous IPC to the main
> process to lookup  permmission, and we don't want to be adding more sync
> IPC, so we'll need to find a non-synchronous way to check against the
> whitelist.

Does it? I thought the content process maintains its own copy of permissions that get sent down via async IPC (see bug 1337056). Maybe I'm wrong about this, in which case it would be cool to get the bug for that. Nika definitely knows.

Thanks for doing this!
Comment on attachment 8972241 [details]
Bug 1457048 - Test that whitelisted origins are able to autoplay.

https://reviewboard.mozilla.org/r/240892/#review246796

::: dom/media/test/test_autoplay_policy_permission.html:53
(Diff revision 1)
> +
> +          // Now we should be able to play...
> +          await canPlayIn({
> +            origin: "http://mochi.test:8888",
> +            shouldPlay: true,
> +            message: "Should be able to play since unwhitelisted."

Should 'unwhitelisted' be 'whitelisted'?
Attachment #8972241 - Flags: review?(bvandyk) → review+
Comment on attachment 8972242 [details]
Bug 1457048 - Ensure origins with autoplay-media exact permission can autoplay.

https://reviewboard.mozilla.org/r/240894/#review246808
Attachment #8972242 - Flags: review?(bvandyk) → review+
Comment on attachment 8972242 [details]
Bug 1457048 - Ensure origins with autoplay-media exact permission can autoplay.

https://reviewboard.mozilla.org/r/240894/#review246972

Hm I wanted to try this out but my latest OSX update seems to have busted compilation...

I assume this works, but please see the comment about the default state below.

Thanks!

::: browser/modules/SitePermissions.jsm:606
(Diff revision 2)
>     */
>  
> +  "autoplay-media": {
> +    exactHostMatch: true,
> +    getDefault() {
> +      return SitePermissions.BLOCK;

Shouldn't this be either BLOCK or ALLOW based on "media.autoplay.enabled"?

::: dom/media/AutoplayPolicy.cpp:59
(Diff revision 2)
>      return true;
>    }
>  
> -  return AutoplayPolicy::IsDocumentAllowedToPlay(aElement->OwnerDoc());
> +  // Whitelisted.
> +  if (nsContentUtils::IsExactSitePermAllow(
> +        aElement->OwnerDoc()->NodePrincipal(), "autoplay-media")) {

nit: aElement->NodePrincipal()
Attachment #8972242 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #8)
> ::: browser/modules/SitePermissions.jsm:606
> (Diff revision 2)
> >     */
> >  
> > +  "autoplay-media": {
> > +    exactHostMatch: true,
> > +    getDefault() {
> > +      return SitePermissions.BLOCK;
> 
> Shouldn't this be either BLOCK or ALLOW based on "media.autoplay.enabled"?

The current platform side implementation checks the pref before checking the whitelist, but I can add a check here on the pref to ensure this code is more resilient to changes elsewhere.

Thanks for the review!
Comment on attachment 8972241 [details]
Bug 1457048 - Test that whitelisted origins are able to autoplay.

https://reviewboard.mozilla.org/r/240892/#review246796

> Should 'unwhitelisted' be 'whitelisted'?

Yes! Thank you!
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd70fc188bc8
Test that whitelisted origins are able to autoplay. r=bryce
https://hg.mozilla.org/integration/autoland/rev/a5d71f8bf413
Ensure origins with autoplay-media exact permission can autoplay. r=bryce,johannh
Ah, sorry, I can explain those failures, I missed this in my review. Luckily the fix should be very straightforward.

For the xpcshell failure, you will need to just add your new permission to this test (in two places):

https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/browser/modules/test/unit/test_SitePermissions.js#13,109

For the other two failures, these are failing because you haven't added a label for the permission setting, you will need to add one here (the label needs to be permission.autoplay-media.label:

https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/browser/locales/en-US/chrome/browser/sitePermissions.properties#31

If you don't have a UX spec for the feature, you can probably just come up with something yourself. The labels are usually in imperative form (e.g. "Automatically Play Media").

I'll put writing a guide for adding new permissions on my todo list...

Thanks!
(In reply to Johann Hofmann [:johannh] from comment #16)
> Ah, sorry, I can explain those failures, I missed this in my review. Luckily
> the fix should be very straightforward.
> 
> For the xpcshell failure, you will need to just add your new permission to
> this test (in two places):
> 
> https://searchfox.org/mozilla-central/rev/
> ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/browser/modules/test/unit/
> test_SitePermissions.js#13,109

The test still fails here:
https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/browser/modules/test/unit/test_SitePermissions.js#130

This is because the test assumes that the default permission state is UNKNOWN, whereas for block-autoplay our default is BLOCK.


> For the other two failures, these are failing because you haven't added a
> label for the permission setting, you will need to add one here (the label
> needs to be permission.autoplay-media.label

Thanks seems to have fixed it, thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b81adbdc7350cd6a40918b1e31021b984be51f6
Comment on attachment 8973113 [details]
Bug 1457048 - Fix permissions tests to accomodate autoplay-media.

https://reviewboard.mozilla.org/r/241620/#review247500

Perfect, thanks!

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:31
(Diff revision 1)
>  state.multichoice.alwaysAsk = Always Ask
>  state.multichoice.allow = Allow
>  state.multichoice.allowForSession = Allow for Session
>  state.multichoice.block = Block
>  
> +permission.autoplay-media.label = Automatically play media with sound

nit: please uppercase the first letter of each word (except "with"), like in the other labels
Attachment #8973113 - Flags: review?(jhofmann) → review+
A quick note that adding the string makes this impossible to uplift, just in case you were interested in landing in 61 still. In that case you should land quickly :)
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36e9545c963a
Test that whitelisted origins are able to autoplay. r=bryce
https://hg.mozilla.org/integration/autoland/rev/99e1f4ca625c
Ensure origins with autoplay-media exact permission can autoplay. r=bryce,johannh
https://hg.mozilla.org/integration/autoland/rev/ad49e0534fd3
Fix permissions tests to accomodate autoplay-media. r=johannh
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: