Closed Bug 548715 Opened 14 years ago Closed 14 years ago

Start using Services.jsm in SeaMonkey

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a1

People

(Reporter: kairo, Assigned: kairo)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Can't we make it part of FUEL/SMILE/STEEL instead of a separate services.jsm?
Services.jsm has nothing to do with FUEL, and there's no value to conflating them, as mentioned in bug 512784.
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.
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... :)
Attachment #429126 - Flags: review?(neil)
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).
Attachment #429126 - Attachment is obsolete: true
Attachment #429729 - Flags: review?(neil)
Attachment #429126 - Flags: review?(neil)
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.]
Attachment #429729 - Flags: review?(neil) → review+
(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?
(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? ;-)
Pushed with nits fixed as http://hg.mozilla.org/comm-central/rev/715859e3dc66
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.