Closed Bug 1159755 Opened 9 years ago Closed 9 years ago

Stop forcing media.eme.apiVisible to true in the test harness

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 2 obsolete files)

This makes it impossible to properly test that we don't ship support when we don't mean to, for example.

The tests that rely on this preference should set it as needed.  Assuming they're running on non-desktop platforms at all, of course.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8599621 [details] [diff] [review]
Stop forcing the media.eme.apiVisible preference to be true in our test harness

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

::: dom/media/test/manifest.js
@@ +3,5 @@
>  // "bogus/duh" in each list.
>  
> +// Make sure to not touch navigator in here, since we want to push prefs that
> +// will affect the APIs it exposes.  Create a private navigator instance for our
> +// use.

I don't understand why changing the APIs that navigator exposes would mean we don't want to touch navigator? When should we use manifestNavigator(), and when |navigator|? Is this something to do with the async nature of pushPref?

@@ +7,5 @@
> +// use.
> +var gManifestNavigatorSource = document.documentElement.appendChild(document.createElement("iframe"));
> +gManifestNavigatorSource.style.display = "none";
> +function manifestNavigator() {
> +    return gManifestNavigatorSource.contentWindow.navigator;

Indent is 2 spaces in this file, not 4.

@@ +9,5 @@
> +gManifestNavigatorSource.style.display = "none";
> +function manifestNavigator() {
> +    return gManifestNavigatorSource.contentWindow.navigator;
> +}
> +function manifestVideo() {

Ditto for manifestVideo(), when should we use this, and when document.createElement('video') and why?
> When should we use manifestNavigator(), and when |navigator|?

You should use manifestNavigator() for any code that can run before the pushPrefEnv happens.

> Is this something to do with the async nature of pushPref?

No, that's not relevant.  The manifestNavigator() bits are running when manifest.js is loaded, before the pushPrefEnv has even started.

> Indent is 2 spaces in this file, not 4.

Will fix.

> when should we use this, and when document.createElement('video') and why?

The issue here is that eme.js is creating a video element as part of SetupEMEPref.  If it does that before the pushPrefEnv, then that will create HTMLMediaElement.prototype using the then-current pref values.  So we want to create the video in a different document so we don't mess with this document's prototypes until after the prefs have been set up the way we want.

I can add some comments to explain that all a bit more clearly....
Attached patch With better comments, hopefully (obsolete) — Splinter Review
Attachment #8599963 - Flags: review?(cpearce)
Attachment #8599621 - Attachment is obsolete: true
Attachment #8599621 - Flags: review?(cpearce)
Attachment #8599963 - Attachment is obsolete: true
Attachment #8599963 - Flags: review?(cpearce)
Comment on attachment 8600057 [details] [diff] [review]
Stop forcing the media.eme.apiVisible preference to be true in our test harness

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

Thanks, that makes sense.
Attachment #8600057 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/b39354cd8c5d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.