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

RESOLVED FIXED in Firefox 40

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla40
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 8599621 [details] [diff] [review]
Stop forcing the media.eme.apiVisible preference to be true in our test harness
Attachment #8599621 - Flags: review?(cpearce)
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....
Created attachment 8599963 [details] [diff] [review]
With better comments, hopefully
Attachment #8599963 - Flags: review?(cpearce)
Attachment #8599621 - Attachment is obsolete: true
Attachment #8599621 - Flags: review?(cpearce)
Created attachment 8600057 [details] [diff] [review]
Stop forcing the media.eme.apiVisible preference to be true in our test harness
Attachment #8600057 - 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+

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b39354cd8c5d
https://hg.mozilla.org/mozilla-central/rev/b39354cd8c5d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.