548715 - Start using Services.jsm in SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

Bug 512784 added a way to globally get and cache a few common services, we should start using that in SeaMonkey.
As that original Firefox bug uses it in utilityOverlay.js and browser.js for a start, I'm doing this first patch for the same - just that the latter is navigator.js on our side.

Comment 1

[Robert Kaiser]

Oh, as a note for the future, if we have other services we are using a lot, we could possibly request them to be added in bug 548627.

Comment 2

[Philip Chee]

Can't we make it part of FUEL/SMILE/STEEL instead of a separate services.jsm?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Services.jsm has nothing to do with FUEL, and there's no value to conflating them, as mentioned in bug 512784.

Comment 4

[Robert Kaiser]

I guess that Philip's concern is that we now have Services.prefs and Application.prefs which are mostly (completely?) doing the same thing.

While that might be true, Services.jsm is a lightweight shorthand (and possibly perf improvement) just for getting (cached) XPCOM Services while FUEL/SMILE/STEEL is a rather full-fledged library/API for much more complicated things.

Comment 5

[Robert Kaiser]

Attachment: Use it in utilityOverlay.js and navigator.js

This uses the currently available parts where possible in utilityOverlay.js and navigator.js - nicely, e.g. Services.prefs is an instance of all of nsIPrefService, nsIPrefBranch and nsIPrefBranch, so we can use it for all those.

diffstat says this patch has 119 insertions(+), 210 deletions(-) even though I changed a few long lines to break as near to 80 characters as possible - I guess that stat means that the code should get small and hopefully more readable by doing that... :)

Comment 6

[Robert Kaiser]

Attachment: Use it in utilityOverlay.js and navigator.js, v2

Bug 548627 added a few more services to be lazily retrieved and cached in Services.jsm, most don't affect those two files I'm using it in here, but I added a few re-indentations for the 80-char-"limit" and whitespace deletions in this second version (in areas I was touching anyhow).

Comment 7

[[e-mail]]

Comment on attachment 429729 [details] [diff] [review]
Use it in utilityOverlay.js and navigator.js, v2

>-var pref = null;
Can we be sure it's safe to remove this?

>-  Application.prefs.setValue("browser.sessionstore.resume_session_once", true);
>+  Services.prefs.setValue("browser.sessionstore.resume_session_once", true);
Not quite right - Application.prefs works differently ;-)

>+  var prefName = Services.prefs.getBoolPref("extensions.dss.switchPending")
>+                 ? "extensions.lastSelectedSkin"
>+                 : "general.skins.selectedSkin";
?: belongs at the end of the line, e.g.
var prefName = Services.prefs.getBoolPref("extensions.dss.switchPending") ?
               "extensions.lastSelectedSkin" : "general.skins.selectedSkin";

[The rewrapping of the getComplexValue looks quite odd. Perhaps we should create a const nsIPrefLocalizedString to allow us to avoid the rewrap.]

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> (From update of attachment 429729 [details] [diff] [review])
> >-var pref = null;
> Can we be sure it's safe to remove this?

I saw no errors with doing it, I watched carefully for them, as I knew it might be risky. Actually, I wondered a bit myself that it seemed to be safe.
I guess it's best to have it removed and if any errors come up that I haven't spotted in testing, we can actually fix them by using Services.prefs there as well.

> >-  Application.prefs.setValue("browser.sessionstore.resume_session_once", true);
> >+  Services.prefs.setValue("browser.sessionstore.resume_session_once", true);
> Not quite right - Application.prefs works differently ;-)

Oops, thanks for spotting that!

> [The rewrapping of the getComplexValue looks quite odd. Perhaps we should
> create a const nsIPrefLocalizedString to allow us to avoid the rewrap.]

Will do that. Should I stick it in utilityOverlay.js and just use it from navigator.js or only declare it in navigator.js and only use it there?

Will your r+ extend to that?

Comment 9

[[e-mail]]

(In reply to comment #8)
> > [The rewrapping of the getComplexValue looks quite odd. Perhaps we should
> > create a const nsIPrefLocalizedString to allow us to avoid the rewrap.]
> Will do that. Should I stick it in utilityOverlay.js and just use it from
> navigator.js or only declare it in navigator.js and only use it there?
I'd go for utilityOverlay.js, since other Services.prefs users may want it.

> Will your r+ extend to that?
Sure, what could go wrong with search and replace? ;-)

Comment 10

[Robert Kaiser]

Pushed with nits fixed as http://hg.mozilla.org/comm-central/rev/715859e3dc66