868439 - Convert some uses of nsIPrefBranch to SpecialPowers

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Martijn Wargers (dead)]

Attachment: patch

See url search, I guess I'm partially doing work from bug 649563.
I need to see if those tests would pass now on b2g mochitest, then I can remove those from the exlude list from b2g.json file.

Bug 649563, comment 9 is mentioning using SpecialPowers.pushPrefenv preferably, so perhaps this patch could still be improved.

test_bug260264_nested.html is failing while trying to run as single test, btw. I think that is a bug. It was annoying at least, because I thought I broke something at first.

Comment 1

[Martijn Wargers (dead)]

Oh, the +var splat = []; part should be removed.

Comment 2

[Martijn Wargers (dead)]

Some other old and now wrong use of pref service in tests here:
http://mxr.mozilla.org/mozilla-central/search?string=Components.interfaces.nsIPrefService
I'll try to fix those instances in a follow-up bug.

Comment 3

[(not currently active) Ted Mielczarek]

If you want these tests to be B2G-friendly then you really do want to use pushPrefEnv. That ensures that the pref is actually set in the content process before calling the callback.

Comment 4

[Martijn Wargers (dead)]

Regarding comment 3, chatted with ted on irc about this, it seems that all preference setting has to be rewritten to pushPrefEnv, ideally.

Comment 5

[Martijn Wargers (dead)]

In:
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/bugs/utils_bug260264.js
There is also some permissions code that has to be rewritten to use SpecialPowers.pushPermissions, btw.

Comment 6

[Martijn Wargers (dead)]

Working on converting test_bug260264.html, it's quite difficult.
Note, I'm currently getting this error at:
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#537
537           cleeanupTodo.value = originalValue;
That definitely seems like a typo there.

Comment 7

[Martijn Wargers (dead)]

Attachment: test_bug260264.html

This is what I currently have for test_bug260264.html.
It's really complicated to change it pushPrefEnv and pushPermissions, because of all the nested calls. It's as good as a complete rewrite of the test, but it's probably not testing absolutely the same anymore.
I get 12 unexpected-fails with the test, not sure where those are coming from.

Comment 8

[Martijn Wargers (dead)]

Ok, the unexpected-fails are happening because UNKNOWN value is not supported, this is related to bug 859401, basically.

Comment 9

[Martijn Wargers (dead)]

Attachment: wip patch

Ok, it is getting too complicated.
I'll first try to fix bug 859401, so test_bug260264.html can be properly converted.
But I should probably spin off a new bug for that, because rewriting that test is quite a bit of work and quite different from other tests.

Comment 10

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #9)
> But I should probably spin off a new bug for that, because rewriting that
> test is quite a bit of work and quite different from other tests.

I filed bug 873403 for it.

Comment 11

[Martijn Wargers (dead)]

Attachment: patch

Ok, this has mainly become changes in the media tests now.
These files are changed:
M content/media/test/Makefile.in
M content/media/test/manifest.js
M content/media/test/test_buffered.html
M content/media/test/test_bug493187.html
M content/media/test/test_can_play_type_mpeg.html
M content/media/test/test_closing_connections.html
M content/media/test/test_decode_error.html
M content/media/test/test_preload_actions.html
M content/media/test/test_preload_suspend.html
M content/media/test/test_progress.html
M content/media/test/test_video_to_canvas.html
M layout/forms/test/test_bug411236.html
M toolkit/content/tests/widgets/Makefile.in
M toolkit/content/tests/widgets/test_audiocontrols_dimensions.html
M toolkit/content/tests/widgets/test_videocontrols.html
M toolkit/content/tests/widgets/test_videocontrols_audio_direction.html
M toolkit/content/tests/widgets/test_videocontrols_video_direction.html
M toolkit/content/tests/widgets/videocontrols_direction_test.js
R content/media/test/use_large_cache.js
R toolkit/content/tests/widgets/use_large_cache.js

I guess the layout/forms/test/test_bug411236.html file is a little out of place.

I'm a little worried about toolkit/content/tests/widgets/test_videocontrols.html.
Because I'm setting the pref with pushPrefEnv and the invoked function is having a window load eventlistener. Theoretically, I think the pushPrefEnv invoked function can be invoked after the window load event, right? Although, it didn't seem to give me trouble when I tested it. Do you have an idea on how to circumvent this, or perhaps this isn't an issue?

Btw, I changed content/media/test/test_preload_suspend.html , but that test is disabled, see bug 493692.

Comment 12

[Martijn Wargers (dead)]

manifest.js and use_large_cache.js is used by quite a lot of files, btw:
http://mxr.mozilla.org/mozilla-central/search?string=manifest.js
http://mxr.mozilla.org/mozilla-central/search?string=use_large_cache.js

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 751338 [details] [diff] [review]
patch

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

just a couple nits here.  Thanks for making the big pass through here and using the latest and greatest SpecialPowers code!

::: content/media/test/test_closing_connections.html
@@ +46,4 @@
>  } else {
>    todo(false, "No types supported");
>  }
> +}

can you indent the code inside of beginTest here?

::: layout/forms/test/test_bug411236.html
@@ +60,5 @@
>  
>  function do_test() {
> +  // accessibility.tabfocus must be set to value 7 before running test also
> +  // on a mac.
> +  SpecialPowers.pushPrefEnv({"set": [["accessibility.tabfocus", 7]]}, do_test2);

I would prefer to call do_test beginTest() and not use do_test2, but do_test()

Comment 14

[Martijn Wargers (dead)]

Attachment: patch for checkin

Ok, made those changes. I also rechecked the affected tests to make sure they run properly on my macbook.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb1973f06b9

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android mochitest-2 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a830609f619

https://tbpl.mozilla.org/php/getParsedLog.php?id=23205284&tree=Mozilla-Inbound

Comment 17

[Martijn Wargers (dead)]

Currently, there is bug 875092, which prevents me from running mochitests on Fennec locally.

Comment 18

[Martijn Wargers (dead)]

Ok, I think I know what's going on with test_access_control.html. On Android the media.preload.default is 1, while on desktop it is 2. 
I put the prefs setting in manifest.js inside the MediaTestManager.runTests function, but that is not called at all by test_access_control.html, hence the time out there.
I fixed that by setting the prefs in the test_access_control.html file itself and combine it with a function to test on. There are more of those cases with this problem:
http://mxr.mozilla.org/mozilla-central/search?string=+src=%22manifest.js%22
test_autoplay.html, test_bug463162.xhtml, test_error_in_video_document.html, test_delay_load.html, test_defaultMuted.html, test_contentDuration7.html, test_closing_connections.html, test_can_play_type_webm.html, test_can_play_type_wave.html, test_can_play_type_ogg.html,  test_can_play_type_no_webm.html, test_can_play_type_no_wave.html, test_can_play_type_no_ogg.html, test_can_play_type.html, test_paused.html, test_no_load_event.html,  test_networkState.html, test_mixed_principals.html, test_load_source.html, test_load.html, test_error_on_404.html, test_seekable2.html, test_source.html, test_reactivate.html, test_referer.html, test_source_media.html, test_bug726904.html, test_source_null.html, test_streams_element_capture_reset.html, test_too_many_elements.html, test_standalone.html, test_source_write.html, test_streams_autoplay.html, test_seekable3.html, test_reset_src.html,  test_dash_detect_stream_switch.html, test_streams_gc.html, test_can_play_type_no_dash.html, test_can_play_type_dash.html, test_can_play_type_mpeg.html and test_streams_srcObject.html

I guess I should write a manager.setPrefs function in manifest.js that has to be called from those test files.

I have no idea why test_audio_event_adopt.html would be timing out. I haven't changed that file and it doesn't use manifest.js

test_bug465498.html is timing out after the opus test-pass, that one is related to the pref setting in manifest.js that this file is using. But I have no idea why it is failing.
test_bug495145.html seems to be even timing out earlier. It's using the same file array as test_bug465498.html: gSmallTests.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

can you get a patch up and try server run linked where you put setPrefs in manifest.js ?

Comment 20

[Martijn Wargers (dead)]

I can't put setPrefs in manifest.js. It uses the SpecialPowers.pushPrefEnv function, which is an asynchronous call and expects to run some function after it.

Comment 21

[Martijn Wargers (dead)]

Attachment: patch

Ok, I'm leaving manifest.js as it is for now and will try to fix that in a follow-up bug.
This is only changing the use_large_cache.js thing (and some unrelated fix).

I'll push this to try.

Comment 22

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6605c884338c

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 762195 [details] [diff] [review]
patch

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

This looks great.  r=me assuming you resolve the one comment in test-closing_connections.html.

::: content/media/test/test_closing_connections.html
@@ +43,5 @@
> +      v.src = resource.name;
> +      document.body.appendChild(v);
> +    }
> +  } else {
> +    todo(false, "No types supported");

in this case we will always waitForExplicitFinish, whereas before we would only do it if resource != null.

Comment 24

[Martijn Wargers (dead)]

(In reply to Joel Maher (:jmaher) from comment #23)
> in this case we will always waitForExplicitFinish, whereas before we would
> only do it if resource != null.

Yeah, that's a good observation. However, it seems to me that previous code was wrong, because this would potentially allow a Simpletest.finish() to be called without a Simpletest.waitForExplicitFinish() call.
So I think my patch actually fixes that.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 762195 [details] [diff] [review]
patch

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

::: content/media/test/test_closing_connections.html
@@ +43,5 @@
> +      v.src = resource.name;
> +      document.body.appendChild(v);
> +    }
> +  } else {
> +    todo(false, "No types supported");

in looking at this further, the waitForExplicitFinish is just fine, I do see in the source file a script src=use_large_cache.js reference, did we forget to remove that?

Comment 26

[Martijn Wargers (dead)]

Attachment: patch for checkin

Yes, you're right. No idea how those files got missing from the patch.

Comment 27

[Martijn Wargers (dead)]

I've pushed this to try, just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=2e80ddaadab1

Comment 28

[Martijn Wargers (dead)]

The try run seems fine, it's not completely finished, but I'm confident that the patch is fine.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09fcb384c6a0

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09fcb384c6a0