707786 - Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey tests

Status: VERIFIED FIXED

(SeaMonkey :: Testing Infrastructure, defect)

Description

[Serge Gautherie (:sgautherie)]

http://mxr.mozilla.org/comm-central/search?string=gPrefService&case=1&find=%2Fsuite%2F.*%2Ftest
"Found 9 matching lines in 2 files"

http://mxr.mozilla.org/comm-central/search?string=preferences-service&case=1&find=%2Fsuite%2F.*%2Ftest
"Found 15 matching lines in 12 files"

Comment 1

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey (v1)

Comment 2

[[e-mail]]

Comment on attachment 584560 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey (v1)

> function test()
> {
>+  Components.utils.import("resource://gre/modules/Services.jsm");
Importing Services inside a function is not useful.

>+      this.prefs = Services.prefs;
You're supposed to replace this.prefs with Services.prefs, not assign it...

Comment 3

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v2)

Comment on attachment 584560 [details] [diff] [review] [diff] [details] [review]
Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey (v1)

>> function test()
>> {
>>+  Components.utils.import("resource://gre/modules/Services.jsm");
>Importing Services inside a function is not useful.

fixed

>>+      this.prefs = Services.prefs;
>You're supposed to replace this.prefs with Services.prefs, not assign it...

fixed

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 584671 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v2)

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

See
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Services.jsm

::: suite/common/downloads/tests/chrome/test_ui_stays_open_on_alert_clickback.xul
@@ +70,4 @@
>  
>    // Close the UI if necessary
>    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>                       .getService(Components.interfaces.nsIWindowMediator);

Not strictly this bug, but, while there, would you care to update the other services in the (test) files you touch?

::: suite/common/places/tests/unit/test_browserGlue_smartBookmarks.js
@@ +41,5 @@
>   * by the user or by other components.
>   */
>  
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");

Fwiw,
Neil, do we want both imports?
Edmund, do we need either? (I'm guessing 'yes' in xpcshell, I'm 'not sure' in (all/some) mochitest...)

Comment 5

[Philip Chee]

> Not strictly this bug, but, while there, would you care to update the other services in
> the (test) files you touch?
Out of scope in this bug.

Comment 6

[[e-mail]]

Comment on attachment 584671 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v2)

>   <![CDATA[
>+Components.utils.import("resource://gre/modules/Services.jsm");
> 
> function test()
> {
>+
Nit: spare blank line here. (Might move it to after CDATA, see below.)

>  * ***** END LICENSE BLOCK ***** */
>+Components.utils.import("resource://gre/modules/Services.jsm");
Although not so bad in some other places, it looks particularly bad here to have the import statements jammed up against something else. I think it would look neatest to ensure there is a blank line both above and below the import(s) in each file.

> // Get pref service
> try {
>-  var pref =  Cc["@mozilla.org/preferences-service;1"].
>-              getService(Ci.nsIPrefBranch);
>+  var pref =  Services.prefs;
> } catch(ex) {
>   do_throw("Could not get Preferences service\n");
> }
Just replace pref with Services.prefs (sorry if I missed this last time).

> Components.utils.import("resource:///modules/Sanitizer.jsm");
>+Components.utils.import("resource://gre/modules/Services.jsm");
> ok(typeof Sanitizer != "undefined", "Sanitizer module imported")
The import of Sanitizer.jsm is actually a test, so you should import Services.jsm before it.

Comment 7

[[e-mail]]

(In reply to Serge Gautherie from comment #4)
> (From update of attachment 584671 [details] [diff] [review])
> ::: suite/common/places/tests/unit/test_browserGlue_smartBookmarks.js
> @@ +41,5 @@
> >   * by the user or by other components.
> >   */
> >  
> >  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Components.utils.import("resource://gre/modules/Services.jsm");
> do we want both imports?
If you can prove that XPCOMUtils isn't necessary then file a bug to remove it.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #7)
> (In reply to Serge Gautherie from comment #4)
> > >  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > > +Components.utils.import("resource://gre/modules/Services.jsm");
> > do we want both imports?
> If you can prove that XPCOMUtils isn't necessary then file a bug to remove
> it.

My question assumed that Services.jsm importing XPCOMUtils.jsm would subsume doing it explicitely:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Services.jsm
43 Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

I'll check that if it is not already done in this bug.

Comment 9

[Philip Chee]

> My question assumed that Services.jsm importing XPCOMUtils.jsm would subsume doing it
> explicitely:
Services.jsm is a JS Module. It doesn't export XPCOMUtils so XPCOMUtils is not available to callers of Services.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #9)
> Services.jsm is a JS Module. It doesn't export XPCOMUtils so XPCOMUtils is
> not available to callers of Services.

I stand corrected.

Nonetheless, I tested test_browserGlue_smartBookmarks.js and it passes even without importing XPCOMUtils.jsm.
Iiuc, XPCOMUtils.jsm is already imported by head_common.js through head_bookmarks.js.
And head_bookmarks.js even already imports Services.jsm (for its own use).

Then, I doubt even more that Services.jsm imports need to be "re-added" to most/all of these tests: it looks like old getService() can simply be removed.

Comment 11

[Serge Gautherie (:sgautherie)]

Ping for update/rewiew.

Comment 12

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v3)

Comment 13

[Edmund Wong (:ewong)]

Comment on attachment 600603 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v3)

patch has been bitrotted.

Comment 14

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v4)

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 600608 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v4)

What about my previous comments about not needing some of the imports?

Comment 16

[[e-mail]]

Comment on attachment 600608 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v4)

> let tempScope = {};
>+Components.utils.import("resource://gre/modules/Services.jsm");
I don't remember that line being there before but now it's there you should import Services before it. r=me with that fixed.

Serge could be right about Services already being imported in some of the tests but he should identify which ones (e.g. "all the Places tests") so you know which ones you can remove the imports from again. (That change doesn't need my review but you may want to request feedback from him to double-check.)

Comment 17

[Edmund Wong (:ewong)]

(In reply to [e-mail] from comment #16)
> Comment on attachment 600608 [details] [diff] [review]
> Use Services.prefs instead of preferences-service / gPrefService, in
> SeaMonkey (v4)
> 
> > let tempScope = {};
> >+Components.utils.import("resource://gre/modules/Services.jsm");
> I don't remember that line being there before but now it's there you should
> import Services before it. r=me with that fixed.

That line's new (not part of my patch).  That line (amongst others) bitrotted
me.

Comment 18

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v5)

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 600838 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v5)

(In reply to [e-mail] from comment #16)

> > let tempScope = {};
> >+Components.utils.import("resource://gre/modules/Services.jsm");
> I don't remember that line being there before but now it's there you should

I added it in bug 725529.

> import Services before it. r=me with that fixed.

That's wrong: either (re-)importing Services.jsm in browser-chrome tests is not needed or it should be done through tempScope too. (See that bug.)

> Serge could be right about Services already being imported in some of the
> tests but he should identify which ones (e.g. "all the Places tests") so you
> know which ones you can remove the imports from again.

I already started to do this in my previous comments.
Wrt to the other cases, it's ease to comment the added import out then check whether/why the test passes.

Comment 20

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v6)

It seems as if none of the tests require the import statement, so I removed the import statement from all tests.

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 603597 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v6)

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

::: suite/modules/test/browser_sanitizer.js
@@ +302,2 @@
>  
> +  var poppref = Services.prefs.getBranch("privacy.sanitize.");

Nit: while here, you could just get rid of poppref by inlining "privacy.sanitize." where it is used (twice).

Comment 22

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v7)

Fixed nit from comment #21.

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 603938 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v7)

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

(I don't think Neil needs/wants to review this again.)

::: suite/modules/test/browser_sanitizer.js
@@ +302,3 @@
>  
> +  Services.prefs.getBranch("privacy.sanitize.")
> +                .setBoolPref("promptOnSanitize", false);

Doesn't "privacy.sanitize.promptOnSanitize" just work? (both places)

Comment 24

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v8)

Misunderstood 'inlining'  Fixed.

Comment 25

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v8)

Wrong patch added.  Fixed.

Comment 26

[[e-mail]]

Comment on attachment 603959 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v8)

Fair enough, assuming it all works...

Comment 27

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/714c2c71f323

Comment 28

[Serge Gautherie (:sgautherie)]

Not fully fixed yet:

Tests:
http://mxr.mozilla.org/comm-central/search?string=preferences-service&case=1&find=%2Fsuite%2F.*%2Ftest
"Found 2 matching lines in 2 files"


Shall we fix application code too while here? Or in a separate bug?
http://mxr.mozilla.org/comm-central/search?string=gPrefService&case=1&find=%2Fsuite%2F
"Found 10 matching lines in 3 files"
http://mxr.mozilla.org/comm-central/search?string=preferences-service&case=1&find=%2Fsuite%2F
"Found 77 matching lines in 44 files"

Comment 29

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p2 (v1)

Fixed outstanding occurrences.

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 604743 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p2 (v1)

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

::: suite/common/places/tests/unit/test_384370.js
@@ -59,5 @@
>  
>    // get places import/export service
>    var importer = Cc["@mozilla.org/browser/places/import-export-service;1"].getService(Ci.nsIPlacesImportExportService);
>  
> -  // avoid creating the places smart folder during tests

Nit: Leave this comment in, or fix it if it's wrong.

Comment 31

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (part 2) (v2)

Comment 32

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/f3dd079d3da9

Comment 33

[Serge Gautherie (:sgautherie)]

V.Fixed, per MXR search.

Comment 34

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #28)
> Shall we fix application code too while here? Or in a separate bug?

I filed bug 735333.

Comment 35

[Serge Gautherie (:sgautherie)]

One of the test fails: (apparently on Linux and Windows, not OSX 10.6)
{
13144 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/suite/common/downloads/tests/test_ui_stays_open_on_alert_clickback.xul | an unexpected uncaught JS exception reported through window.onerror - Services is not defined at chrome://mochitests/content/chrome/suite/common/downloads/tests/test_ui_stays_open_on_alert_clickback.xul:66
}

Comment 36

[Edmund Wong (:ewong)]

Attachment: Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p3 (v1) [Checked in: Comment 39 & 44]

Fixed test.

Comment 37

[[e-mail]]

Comment on attachment 607539 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p3 (v1)
[Checked in: Comment 39 & 44]

Verified patch allows test to pass.

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 607539 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p3 (v1)
[Checked in: Comment 39 & 44]

That's most likely it :->

Comment 39

[Edmund Wong (:ewong)]

Pushed p3 to comm-central:
http://hg.mozilla.org/comm-central/rev/2326f20b3bc6

Comment 40

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1332499182.1332503873.20181.gz
Linux comm-central-trunk debug test mochitest-other on 2012/03/23 03:39:42

V.Fixed

Comment 41

[Serge Gautherie (:sgautherie)]

Comment on attachment 607539 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p3 (v1)
[Checked in: Comment 39 & 44]

[Approval Request Comment]
Regression happened in SM 2.10 initially.
No risk, test only.

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 607539 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p3 (v1)
[Checked in: Comment 39 & 44]

Missed Aurora-2.10.

Comment 43

[Justin Wood (:Callek)]

Comment on attachment 607539 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p3 (v1)
[Checked in: Comment 39 & 44]

a=test-only

Comment 44

[Philip Chee]

Checked in to comm-beta
http://hg.mozilla.org/releases/comm-beta/rev/b26e8f8161e2

Comment 45

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1337595251.1337600055.1708.gz
WINNT 5.2 comm-beta debug test mochitest-other on 2012/05/21 03:14:11

seamonkey2.10: verified.