Status

()

enhancement
RESOLVED FIXED
7 months ago
a month ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 months ago
Implement the FeaturePolicy support for autoplay media elements.
(Assignee)

Comment 1

7 months ago
Posted patch autoplay.patch (obsolete) — Splinter Review
Attachment #9015108 - Flags: review?(cpearce)
(Assignee)

Comment 2

7 months ago
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+
(Assignee)

Comment 4

7 months ago
> 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)

Comment 6

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f574c731fcb0
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Yeah, those WPT look like they cover us here.
Flags: needinfo?(cpearce)
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.