Closed Bug 1497034 Opened 2 years ago Closed 2 years ago

FeaturePolicy: autoplay

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Implement the FeaturePolicy support for autoplay media elements.
Attached patch autoplay.patch (obsolete) — Splinter Review
Attachment #9015108 - Flags: review?(cpearce)
Attached patch autoplay.patchSplinter Review
We don't pass so many WPTs because webdriver doesn't seem to have the support for user-interaction.
Attachment #9015108 - Attachment is obsolete: true
Attachment #9015108 - Flags: review?(cpearce)
Attachment #9015110 - Flags: review?(cpearce)
Comment on attachment 9015110 [details] [diff] [review]
autoplay.patch

Review of attachment 9015110 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/AutoplayPolicy.cpp
@@ +79,5 @@
>    if (!aWindow->GetExtantDoc()) {
>      return false;
>    }
>  
> +  if (!FeaturePolicyUtils::IsFeatureAllowed(aWindow->GetExtantDoc(),

Here you're (correctly) checking whether the current document is blocked via feature-policy, and further down we walk up the doc tree to the top level content document and check permissions etc on the top level content document. It would be incorrect to check the feature policy on the top level document, so can you add a comment here explaining why you're checking feature policy on the current document, rather than the "approver" document. Then we'll hopefully prevent future refactorings from making this incorrect.

Bonus points if you can write a mochitest to enforce this behaviour.
Attachment #9015110 - Flags: review?(cpearce) → review+
> Bonus points if you can write a mochitest to enforce this behaviour.

Thanks for the review. About this extra mochitest, all the existing featurePolicy WPTs are based on nested iframes. Is it similar to what you suggested? Here an example:

https://searchfox.org/mozilla-central/rev/29aea2a2a3bd0f5e25ce0b60a76053fb25ba5149/testing/web-platform/tests/html/semantics/embedded-content/media-elements/autoplay-allowed-by-feature-policy.https.sub.html#27

which uses iframes here:

https://searchfox.org/mozilla-central/rev/29aea2a2a3bd0f5e25ce0b60a76053fb25ba5149/testing/web-platform/tests/feature-policy/resources/featurepolicy.js#28-52

If I'm wrong, I'll add an extra WPT test in a follow up.
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/f574c731fcb0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Yeah, those WPT look like they cover us here.
Flags: needinfo?(cpearce)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.