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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Stop forcing the media.eme.apiVisible preference to be true in our test harness

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: With better comments, hopefully

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Stop forcing the media.eme.apiVisible preference to be true in our test harness

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b39354cd8c5d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b39354cd8c5d