Closed Bug 1069795 Opened 8 years ago Closed 7 years ago

Fix test_can_play_type_ogg.html on B2G

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch use_push_env (obsolete) — Splinter Review
Attachment #8492040 - Flags: review?(cpearce)
Comment on attachment 8492040 [details] [diff] [review]
use_push_env

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

JW: can you review this please?
Attachment #8492040 - Flags: review?(cpearce) → review?(jwwang)
Comment on attachment 8492040 [details] [diff] [review]
use_push_env

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

::: content/media/test/can_play_type_ogg.js
@@ +46,5 @@
>    if (OpusEnabled !== undefined) {
> +    opus_enable().then(function() {
> +      return opus_disable();
> +    }).then(function() {
> +      SpecialPowers.setBoolPref("media.opus.enabled", OpusEnabled);

Please use SpecialPowers.pushPrefEnv instread, we want to remove all usage of SpecialPowers.setXXXPref.

@@ +52,3 @@
>    }
>  
>    // Unsupported Ogg codecs

This part needs to be wrapped in Promise as well to ensure the order of tests doesn't change.

::: content/media/test/mochitest.ini
@@ +319,5 @@
>  [test_can_play_type_mpeg.html]
>  skip-if = buildapp == 'b2g' # bug 1021675
>  [test_can_play_type_no_ogg.html]
>  [test_can_play_type_ogg.html]
> +skip-if = buildapp == e10s

Can't it pass e10s?
Attachment #8492040 - Flags: review?(jwwang)
Attached patch use_push_env (obsolete) — Splinter Review
Attachment #8492040 - Attachment is obsolete: true
Attachment #8492835 - Flags: review?(jwwang)
Comment on attachment 8492835 [details] [diff] [review]
use_push_env

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

r- for this change breaks test_can_play_type_no_ogg.html.

::: content/media/test/can_play_type_ogg.js
@@ +1,2 @@
> +
> +SimpleTest.waitForExplicitFinish();

This should be put in test_can_play_type_ogg.html as test_can_play_type_no_ogg.html did.

@@ +68,5 @@
> +    check("video/ogg; codecs=xyz", "");
> +    check("video/ogg; codecs=xyz,vorbis", "");
> +    check("video/ogg; codecs=vorbis,xyz", "");
> +
> +    SimpleTest.finish();

This will conflict with test_can_play_type_no_ogg.html which also call SimpleTest.finish() explicitly.

@@ +78,5 @@
> +    return opus_enable();
> +  }).then(function() {
> +    return opus_disable();
> +  }).then(function() {
> +    SpecialPowers.pushPrefEnv({"set": [['media.opus.enabled', OpusEnabled]]},

Call SpecialPowers.popPrefEnv() to restore pref settings so that you don't need OpusEnabled to keep the original value.
Attachment #8492835 - Flags: review?(jwwang) → review-
(In reply to JW Wang [:jwwang] from comment #5)
> Comment on attachment 8492835 [details] [diff] [review]
> use_push_env
> 
> Review of attachment 8492835 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +78,5 @@
> > +    return opus_enable();
> > +  }).then(function() {
> > +    return opus_disable();
> > +  }).then(function() {
> > +    SpecialPowers.pushPrefEnv({"set": [['media.opus.enabled', OpusEnabled]]},
> 
> Call SpecialPowers.popPrefEnv() to restore pref settings so that you don't

Actually, I think we don't even need to call these undo functions. These prefs will be undo automatically when test completed.
What do you think?

http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#860
(In reply to Alfredo Yang from comment #6)
> Actually, I think we don't even need to call these undo functions. These
> prefs will be undo automatically when test completed.
> What do you think?
> 
> http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/
> SimpleTest/SimpleTest.js#860

OK.

tip: click "[review]" to add review comments, it is easier to track all comments in one place.
Attached patch use_push_env (obsolete) — Splinter Review
Attachment #8492835 - Attachment is obsolete: true
Attachment #8492927 - Flags: review?(jwwang)
Comment on attachment 8492927 [details] [diff] [review]
use_push_env

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

::: content/media/test/can_play_type_ogg.js
@@ +70,5 @@
> +  }
> +
> +  basic_test().then(function() {
> +    return verify_opus_support();
> +  }).then(function() {

nit: can we just say:

basic_test()
.then(verify_opus_support)
.then(opus_enable)
.then(...)

which is more concise.
Attachment #8492927 - Flags: review?(jwwang) → review+
Attached patch use_push_envSplinter Review
Update nit.
Attachment #8492927 - Attachment is obsolete: true
Attachment #8492962 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7fef64119cf4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Can I pick up this bug for testing, if so can someone update test-steps for the same?

PS: Assuming this bug is fixed and ready for QA
You need to log in before you can comment on or make changes to this bug.