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)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 2 obsolete files)
8.91 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
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 | |
Comment 1•9 years ago
|
||
Attachment #8599621 -
Flags: review?(cpearce)
![]() |
Assignee | |
Updated•9 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 2•9 years ago
|
||
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?
![]() |
Assignee | |
Comment 3•9 years ago
|
||
> 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....
![]() |
Assignee | |
Comment 4•9 years ago
|
||
Attachment #8599963 -
Flags: review?(cpearce)
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #8599621 -
Attachment is obsolete: true
Attachment #8599621 -
Flags: review?(cpearce)
![]() |
Assignee | |
Comment 5•9 years ago
|
||
Attachment #8600057 -
Flags: review?(cpearce)
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #8599963 -
Attachment is obsolete: true
Attachment #8599963 -
Flags: review?(cpearce)
Comment 6•9 years ago
|
||
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 8•9 years ago
|
||
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.
Description
•