1069795 - Fix test_can_play_type_ogg.html on B2G

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: use_push_env

Comment 2

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

Comment on attachment 8492040 [details] [diff] [review]
use_push_env

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

JW: can you review this please?

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 4

[Alfredo Yang (:alfredo)]

Attachment: use_push_env

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 6

[Alfredo Yang (:alfredo)]

(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

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 8

[Alfredo Yang (:alfredo)]

Attachment: use_push_env

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 10

[Alfredo Yang (:alfredo)]

Attachment: use_push_env

Update nit.

Comment 11

[Alfredo Yang (:alfredo)]

https://tbpl.mozilla.org/?tree=Try&rev=336ecee07519

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7fef64119cf4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7fef64119cf4

Comment 14

[Sudhir]

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