649563 - Convert all existing uses of nsIPrefBranch to use SpecialPowers

Status: NEW

(Testing :: Mochitest, defect)

Description

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/search?string=%28get|set%29%28bool|int|char%29pref&regexp=1&find=\.html&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
http://mxr.mozilla.org/mozilla-central/search?string=%28get|set%29%28bool|int|char%29pref&regexp=1&find=\.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
http://mxr.mozilla.org/mozilla-central/search?string=%28get|set%29%28bool|int|char%29pref&regexp=1&find=\.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

This searches for (get|set)(char|bool|int)pref will do for a start.  Not every result needs to be changed, but

Comment 1

[Josh Matthews [:jdm]]

My goodness, comment 0 is quite poorly written.  Those searches will do for a start, but only mochitest tests (not mochitest-chrome or any of the other variants) need changing.  The basic formula is

1) delete any lines like

>25   var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>26             .getService(Components.interfaces.nsIPrefBranch);

2) change any line that uses |prefs| like

>27   var gOriginalHtml5Pref = prefs.getBoolPref("html5.parser.enable");
>28   prefs.setBoolPref("html5.parser.enable", true);

to read

>27   var gOriginalHtml5Pref = SpecialPowers.getBoolPref("html5.parser.enable");
>28   SpecialPowers.setBoolPref("html5.parser.enable", true);

3) make sure the test still passes

For extra credit, add another two steps:

4) delete any lines like

>netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

5) make sure the test still passes. If it doesn't undo step 4.

Comment 2

[Josh Matthews [:jdm]]

See this patch for an example of the changes required: https://bugzilla.mozilla.org/attachment.cgi?id=499705&action=diff

Comment 3

[Josh Matthews [:jdm]]

Hmm, my mistake, the second search for .js files is far far too broad.  Most of the results there are for non-test files.

Comment 4

[Josh Matthews [:jdm]]

And of course the third search is the same as the second.  Here's a better one for XUL files:

http://mxr.mozilla.org/mozilla-central/search?string=%28get|set%29%28bool|int|char%29pref&regexp=1&find=\.xul&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 5

[Robert Strong (they/them - no direct email)]

(In reply to comment #1)
> My goodness, comment 0 is quite poorly written.  Those searches will do for a
> start, but only mochitest tests (not mochitest-chrome or any of the other
> variants) need changing.  The basic formula is
For a new contributor it isn't clear what can be used to identify a mochitest-chrome test though they should be able to figure it out by the path name containing chrome. Specifically, I haven't checked the other links but the link in comment #4 contains many mochitest-chrome tests.

Comment 6

[Han Chang]

I've made the required changes, but how do I make sure the test still passes?

Comment 7

[arno renevier]

Han Chang:
to make sure test pass, run the Mochitest:
https://developer.mozilla.org/en/Mochitest

About this bug, many tests use prefHasUserValue to save a possible user pref and restore it later. So, I see two possibilites here:
either implement prefHasUserValue in SpecialPowers, either change the tests so they clearUserPref a value they have modified without restoring its previous value. After all, mochitest profile is only used for the tests, and there's not point keeping a user value between test. Or did I miss something ?

Comment 8

[Martijn Wargers (dead)]

Perhaps also test Fennec, where e10 is used?

Note bug 621363, where is being discussed that SpecialPowers setPref and getPref might have to be rewriteen.

Comment 9

[Josh Matthews [:jdm]]

I no longer think this is good first bug material, since the plan in bug 621363 is to rewrite all tests that set parameters to use a new system that doesn't exist yet.

Comment 10

[Han Chang]

Agreed, I'll leave it alone for now then, thanks for all of your help though!

Comment 11

[Martijn Wargers (dead)]

(In reply to Josh Matthews [:jdm] (away until 8/24) from comment #9)
> I no longer think this is good first bug material, since the plan in bug
> 621363 is to rewrite all tests that set parameters to use a new system that
> doesn't exist yet.

The new system has become SpecialPowers.pushPrefEnv. I can help people that want convert the existing callers.

The mochitests that need changing from prefbranch setting to SpecialPowers.pushPrefEnv:
content/html/content/test/test_anchor_ping.html
dom/plugins/test/mochitest/test_bug751809.html
dom/plugins/test/mochitest/test_plugin_tag_clicktoplay.html
dom/tests/mochitest/localstorage/test_localStorageQuotaPrivateBrowsing_perwindowpb.html
dom/base/test/test_bug1022229.html
dom/plugins/test/mochitest/test_secondPlugin.html
dom/tests/mochitest/bugs/test_bug61098.html
security/manager/ssl/tests/mochitest/mixedcontent/test_bug383369.html 
security/manager/ssl/tests/mochitest/stricttransportsecurity/test_sts_privatebrowsing_perwindowpb.html 
toolkit/components/passwordmgr/test/test_notifications.html
toolkit/components/passwordmgr/test/test_prompt.html 
dom/workers/test/test_suspend.html

I'm not sure if mochitest-chrome tests would need converting to SpecialPowers.pushPrefEnv. If they would need to run oop too, then they would need converting, indeed.
Afaik, browser-chrome tests don't need converting.

Comment 12

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #11)
> I'm not sure if mochitest-chrome tests would need converting to
> SpecialPowers.pushPrefEnv. If they would need to run oop too, then they
> would need converting, indeed.
> Afaik, browser-chrome tests don't need converting.

Actually, it would be better to convert those too, because they would benefit from the automatic reset of the preferences after the test has been run with the usage of SpecialPowers.pushPrefEnv.

Comment 13

[Masatoshi Kimura [:emk]]

We still have bunch of nsIPrefBranch usage, but it no longer depends on enablePrivilege.