Convert all existing uses of nsIPrefBranch to use SpecialPowers

NEW
Unassigned

Status

7 years ago
2 years ago

People

(Reporter: jdm, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js])

(Reporter)

Comment 1

7 years ago
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.
(Reporter)

Comment 2

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

Comment 3

7 years ago
Hmm, my mistake, the second search for .js files is far far too broad.  Most of the results there are for non-test files.
(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

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

Comment 7

7 years ago
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 ?
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.
(Reporter)

Comment 9

7 years ago
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.
Whiteboard: [good first bug] [mentor=jdm]

Comment 10

7 years ago
Agreed, I'll leave it alone for now then, thanks for all of your help though!
Depends on: 868439
(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.
See Also: → bug 1056851
(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.
Mentor: martijn.martijn
(Reporter)

Updated

4 years ago
Whiteboard: [lang=js]
Mentor: martijn.martijn
We still have bunch of nsIPrefBranch usage, but it no longer depends on enablePrivilege.
No longer blocks: 462483
You need to log in before you can comment on or make changes to this bug.