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

VERIFIED FIXED in seamonkey2.10

Status

SeaMonkey
Testing Infrastructure
--
trivial
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: ewong)

Tracking

Trunk
seamonkey2.10
Dependency tree / graph
Bug Flags:
in-testsuite +

SeaMonkey Tracking Flags

(seamonkey2.10 verified)

Details

Attachments

(3 attachments, 9 obsolete attachments)

29.41 KB, patch
neil@parkwaycc.co.uk
: review+
sgautherie
: feedback+
Details | Diff | Splinter Review
11.47 KB, patch
ewong
: review+
ewong
: feedback+
Details | Diff | Splinter Review
712 bytes, patch
neil@parkwaycc.co.uk
: review+
sgautherie
: feedback+
Details | Diff | Splinter Review

Updated

7 years ago
Blocks: 664838
(Assignee)

Updated

7 years ago
Assignee: nobody → ewong
Status: NEW → ASSIGNED
(Assignee)

Comment 1

7 years ago
Created attachment 584560 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey (v1)

Comment 2

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

Comment 3

7 years ago
Created attachment 584671 [details] [diff] [review]
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
Attachment #584560 - Attachment is obsolete: true
Attachment #584671 - Flags: review?(neil)
(Reporter)

Comment 4

7 years ago
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

7 years ago
> 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

7 years ago
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

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

Comment 8

7 years ago
(In reply to neil@parkwaycc.co.uk 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

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

Comment 10

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

Comment 11

6 years ago
Ping for update/rewiew.
(Assignee)

Comment 12

6 years ago
Created attachment 600603 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v3)
Attachment #584671 - Attachment is obsolete: true
Attachment #584671 - Flags: review?(neil)
Attachment #600603 - Flags: review?(neil)
(Assignee)

Comment 13

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

patch has been bitrotted.
Attachment #600603 - Flags: review?(neil)
(Assignee)

Comment 14

6 years ago
Created attachment 600608 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v4)
Attachment #600603 - Attachment is obsolete: true
Attachment #600608 - Flags: review?(neil)
(Reporter)

Comment 15

6 years ago
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 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.)
Attachment #600608 - Flags: review?(neil) → review+
(Assignee)

Comment 17

6 years ago
(In reply to neil@parkwaycc.co.uk 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.
(Assignee)

Updated

6 years ago
Attachment #600608 - Flags: feedback?(sgautherie.bz)
(Assignee)

Comment 18

6 years ago
Created attachment 600838 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v5)
Attachment #600608 - Attachment is obsolete: true
Attachment #600608 - Flags: feedback?(sgautherie.bz)
Attachment #600838 - Flags: review+
Attachment #600838 - Flags: feedback?(sgautherie.bz)
(Reporter)

Comment 19

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

(In reply to neil@parkwaycc.co.uk 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.
Attachment #600838 - Flags: review+
Attachment #600838 - Flags: feedback?(sgautherie.bz)
Attachment #600838 - Flags: feedback-
(Assignee)

Comment 20

6 years ago
Created attachment 603597 [details] [diff] [review]
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.
Attachment #600838 - Attachment is obsolete: true
Attachment #603597 - Flags: review?(neil)
(Reporter)

Comment 21

6 years ago
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).
Attachment #603597 - Flags: review?(neil) → feedback+
(Assignee)

Comment 22

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

Fixed nit from comment #21.
Attachment #603597 - Attachment is obsolete: true
Attachment #603938 - Flags: review?(neil)
(Reporter)

Comment 23

6 years ago
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)
Attachment #603938 - Flags: review?(neil)
(Assignee)

Comment 24

6 years ago
Created attachment 603958 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v8)

Misunderstood 'inlining'  Fixed.
Attachment #603938 - Attachment is obsolete: true
Attachment #603958 - Flags: review?(neil)
(Assignee)

Comment 25

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

Wrong patch added.  Fixed.
Attachment #603958 - Attachment is obsolete: true
Attachment #603958 - Flags: review?(neil)
Attachment #603959 - Flags: review?(neil)
Comment on attachment 603959 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (v8)

Fair enough, assuming it all works...
Attachment #603959 - Flags: review?(neil) → review+
(Reporter)

Updated

6 years ago
Attachment #603959 - Flags: feedback+
(Assignee)

Comment 27

6 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/714c2c71f323
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 28

6 years ago
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"
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: --- → seamonkey2.10
(Assignee)

Comment 29

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

Fixed outstanding occurrences.
Attachment #604743 - Flags: review?(neil)
Attachment #604743 - Flags: feedback?(sgautherie.bz)

Updated

6 years ago
Attachment #604743 - Flags: review?(neil) → review+
(Reporter)

Comment 30

6 years ago
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.
Attachment #604743 - Flags: feedback?(sgautherie.bz) → feedback+
(Assignee)

Comment 31

6 years ago
Created attachment 605270 [details] [diff] [review]
Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey (part 2) (v2)
Attachment #604743 - Attachment is obsolete: true
Attachment #605270 - Flags: review+
Attachment #605270 - Flags: feedback+
(Assignee)

Comment 32

6 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/f3dd079d3da9
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 33

6 years ago
V.Fixed, per MXR search.
Status: RESOLVED → VERIFIED
Summary: Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey → Use Services.prefs instead of preferences-service / gPrefService, in SeaMonkey tests
Whiteboard: [good first bug]
(Reporter)

Updated

6 years ago
Blocks: 735333
(Reporter)

Comment 34

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

Comment 35

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

Updated

6 years ago
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 36

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

Fixed test.
Attachment #607539 - Flags: review?(neil)
Attachment #607539 - Flags: feedback?(sgautherie.bz)
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.
Attachment #607539 - Flags: review?(neil) → review+
(Reporter)

Comment 38

6 years ago
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 :->
Attachment #607539 - Flags: feedback?(sgautherie.bz) → feedback+
(Assignee)

Comment 39

6 years ago
Pushed p3 to comm-central:
http://hg.mozilla.org/comm-central/rev/2326f20b3bc6
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 40

6 years ago
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
Status: RESOLVED → VERIFIED
(Reporter)

Comment 41

6 years ago
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.
Attachment #607539 - Flags: approval-comm-aurora?
(Reporter)

Comment 42

6 years ago
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.
Attachment #607539 - Flags: approval-comm-aurora? → approval-comm-beta?
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
Attachment #607539 - Flags: approval-comm-beta? → approval-comm-beta+
(Reporter)

Updated

6 years ago
status-seamonkey2.10: --- → affected

Comment 44

6 years ago
Checked in to comm-beta
http://hg.mozilla.org/releases/comm-beta/rev/b26e8f8161e2
status-seamonkey2.10: affected → fixed
(Reporter)

Comment 45

6 years ago
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.
status-seamonkey2.10: fixed → verified
(Reporter)

Updated

6 years ago
Attachment #607539 - Attachment description: Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p3 (v1) → Use Services.prefs instead of preferences-service / gPrefService in SeaMonkey p3 (v1) [Checked in: Comment 39 & 44]
You need to log in before you can comment on or make changes to this bug.