1684656 - Remaining conversion to Services.jsm in SeaMonkey code

Status: RESOLVED FIXED

(SeaMonkey :: General, task)

Description

[Ian Neal]

There are still places where the SeaMonkey code can be switched to use Services.jsm

Comment 1

[Ian Neal]

Attachment: 1684656-services-mailnews-2537.patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53.7
Risk to taking this patch (and alternatives if risky): something might break
String changes made by this patch: none

Comment 2

[Ian Neal]

Attachment: 1684656-services-browser-2537.patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53.7
Risk to taking this patch (and alternatives if risky): things might break
String changes made by this patch: none

Comment 3

[Ian Neal]

Attachment: 1684656-services-base-2537.patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53.7
Risk to taking this patch (and alternatives if risky): some things might not work
String changes made by this patch: none

Comment 4

[Ian Neal]

Attachment: 1684656-services-bindings-2537.patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53.7
Risk to taking this patch (and alternatives if risky): some things might break
String changes made by this patch: none

Comment 5

[Ian Neal]

Attachment: 1684656-services-components-2537.patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: none
Testing completed (on m-c, etc.): 2.53.7
Risk to taking this patch (and alternatives if risky): minimal
String changes made by this patch: none

Comment 6

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9195063 [details] [diff] [review]
1684656-services-base-2537.patch

suite/base/content/safeMode.js
+const appStartup = Services.startup;

Do we need the const? Only used in 3 places and not hot code. Same in content/viewApplyThemeOverlay.js.

r/a+ either way. LGTM

Comment 7

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9195064 [details] [diff] [review]
1684656-services-bindings-2537.patch

suite/components/bindings/notification.xml
const nsIPermissionManager = Ci.nsIPermissionManager;

The patch cleans out a few uses of this. There are still some other "const nsI* = Ci.nsI*" left. Would it make sense to remove/replace them too?

r/a+ either way. LGTM

Comment 8

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9195037 [details] [diff] [review]
1684656-services-browser-2537.patch

/browser/tabbrowser.xml

Needs "Servies.uriFixup." typo fixed. Otherwise LGTM

Comment 9

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9195066 [details] [diff] [review]
1684656-services-components-2537.patch

suite/components/helpviewer/content/help.js

NIT indention off by 1:

function log(aText) {
...

• Services.console.logStringMessage(aText);

components/profile/content/profileSelection.js

• var ps = Services.prompt;

I would probably just use Services.prompt. everywhere because not hot code but just a personal preference. Fine with me as is.

LGTM r/a+

Comment 10

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9195036 [details] [diff] [review]
1684656-services-mailnews-2537.patch

suite/mailnews/content/mailViewList.js

• let ps = Services.prompt;
  Seems only used in one place so I would use it directly.

LGTM either way r/a+

Comment 11

[Ian Neal]

(In reply to Frank-Rainer Grahl (:frg) from comment #6)

Comment on attachment 9195063 [details] [diff] [review]
1684656-services-base-2537.patch

suite/base/content/safeMode.js
+const appStartup = Services.startup;

Do we need the const? Only used in 3 places and not hot code. Same in content/viewApplyThemeOverlay.js.

r/a+ either way. LGTM

Matching how it is done in mozilla code, so easier to port patches (and makes it more readable).

Comment 12

[Ian Neal]

(In reply to Frank-Rainer Grahl (:frg) from comment #7)

Comment on attachment 9195064 [details] [diff] [review]
1684656-services-bindings-2537.patch

suite/components/bindings/notification.xml
const nsIPermissionManager = Ci.nsIPermissionManager;

The patch cleans out a few uses of this. There are still some other "const nsI* = Ci.nsI*" left. Would it make sense to remove/replace them too?

r/a+ either way. LGTM

I did think about that, but decided it would be better in a separate patch so as not to confuse this one.

Comment 13

[Ian Neal]

(In reply to Frank-Rainer Grahl (:frg) from comment #9)

Comment on attachment 9195066 [details] [diff] [review]
1684656-services-components-2537.patch

components/profile/content/profileSelection.js

• var ps = Services.prompt;

I would probably just use Services.prompt. everywhere because not hot code but just a personal preference. Fine with me as is.

I think using ps makes the code more readable (especially when using vi at 80 character width).

Comment 14

[Ian Neal]

(In reply to Frank-Rainer Grahl (:frg) from comment #10)

Comment on attachment 9195036 [details] [diff] [review]
1684656-services-mailnews-2537.patch

suite/mailnews/content/mailViewList.js

• let ps = Services.prompt;
  Seems only used in one place so I would use it directly.

Yeah, probably okay, only makes a minor difference in readability.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0ade240416a7
Remaining conversion to Services.jsm in SeaMonkey code - mailnews part. r=frg
https://hg.mozilla.org/comm-central/rev/11d15386eaa3
Remaining conversion to Services.jsm in SeaMonkey code - browser part. r=frg
https://hg.mozilla.org/comm-central/rev/8e38a8558338
Remaining conversion to Services.jsm in SeaMonkey code - base part. r=frg
https://hg.mozilla.org/comm-central/rev/8888ab3dadb1
Remaining conversion to Services.jsm in SeaMonkey code - bindings part. r=frg
https://hg.mozilla.org/comm-central/rev/5ca19d5395ff
Remaining conversion to Services.jsm in SeaMonkey code - components part. r=frg

Comment 16

[Frank-Rainer Grahl (:frg)]

https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/bc253331eb48c29eaf93f571f9dee4541624905d
Remaining conversion to Services.jsm in SeaMonkey code - mailnews part. r=frg a=frg
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/d20df46f03cd85488bf6752a7b07a6316621583d
Remaining conversion to Services.jsm in SeaMonkey code - browser part. r=frg a=frg
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/91a027a2b6aafb7a1e4a933b099d955855ea1010
Remaining conversion to Services.jsm in SeaMonkey code - base part. r=frg a=frg
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/94986585cabd1c7bb7947163aecac4afc2bb0f59
Remaining conversion to Services.jsm in SeaMonkey code - bindings part. r=frg a=frg
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/dd3294eca6b27231d76c697f774fe9af47a9bbe5
Remaining conversion to Services.jsm in SeaMonkey code - components part. r=frg a=frg
Target 2.53.7