Status

()

enhancement
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Implement the FeaturePolicy support for autoplay media elements.
Posted patch autoplay.patch (obsolete) — Splinter Review
Attachment #9015108 - Flags: review?(cpearce)
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: 9 months 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.