Last Comment Bug 548715 - Start using Services.jsm in SeaMonkey
: Start using Services.jsm in SeaMonkey
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Robert Kaiser
:
:
Mentors:
Depends on: 512784
Blocks: 557022
  Show dependency treegraph
 
Reported: 2010-02-25 18:42 PST by Robert Kaiser
Modified: 2010-04-03 11:21 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Use it in utilityOverlay.js and navigator.js (57.07 KB, patch)
2010-02-26 08:10 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
Use it in utilityOverlay.js and navigator.js, v2 (58.87 KB, patch)
2010-03-02 07:04 PST, Robert Kaiser
neil: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-02-25 18:42:42 PST
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 2010-02-25 18:46:10 PST
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 2010-02-25 20:42:51 PST
Can't we make it part of FUEL/SMILE/STEEL instead of a separate services.jsm?
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-02-25 21:10:45 PST
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 2010-02-26 04:21:23 PST
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 2010-02-26 08:10:18 PST
Created attachment 429126 [details] [diff] [review]
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 2010-03-02 07:04:40 PST
Created attachment 429729 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-03-10 06:32:48 PST
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 2010-03-10 07:10:12 PST
(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 neil@parkwaycc.co.uk 2010-03-10 07:22:28 PST
(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 2010-03-10 08:56:31 PST
Pushed with nits fixed as http://hg.mozilla.org/comm-central/rev/715859e3dc66

Note You need to log in before you can comment on or make changes to this bug.