There are a few common XPCOM services that are widely used in XUL apps. In Firefox's browser.js, for example, we've resorted to adding a smart getter to the window for the pref/io/observer services, to avoid the need to call getService() all the time. It seems like this is a common enough use case that just adding a caching getters for these available globally in JS. I'm not sure what the best way to implement that is - could potentially be a getter on XUL windows, or a "JavaScript global property", or maybe something else.

Can you just add a jsm that exports a Services object holding all the common ones (memoized of course)?

Where ever we put this stuff, we should use the work in bug 508850 to define the properties.

This takes the approach suggested by Mossop: a JSM that defines lazy getters. I also made browser.js and contentAreaUtils.js use it. This version has a low-memory observer that attempts to release the services, but I'm thinking now that I'll just remove it, since all of these have other long-lasting references anyways (I thought perhaps that the permission manager didn't, but I found out its kept alive by the content blocker which is in turn kept alive by the content policy stuff). The "release" function could be useful for breaking cycles on shutdown, but hopefully the cycle collector has our back there.

can i suggest changing ps name to prompt, since ps is a bit sounding like preferences service

Any reason we aren't using NetUtil.jsm's reference to the IO Service? We should probably just change the newURI calls to use it's helper too...

This module is meant for use outside of just browser.js (could even live in toolkit, as it is), including in files that may not already include NetUtil, and the duplication doesn't really cost anything.

In fact browser.js currently doesn't import NetUtil either, as far as I can tell.

Moves Services.jsm into toolkit. Will add other consumers in separate patches.

(In reply to comment #9) > Created an attachment (id=424058) [details] > move to toolkit > > Moves Services.jsm into toolkit. Will add other consumers in separate patches. Since you're no longer releasing the services ever (and I agree there is no point), I think Services.jsm is now needlessly complex. Can't you just do: var Services = {}; XPCOMUtils.defineLazyServiceGetter(Services, "search", "@mozilla.org/browser/search-service;1", "nsIBrowserSearchService"); etc... Or, I'm currently doing it this way in the extension manager code: http://hg.mozilla.org/users/dtownsend_mozilla.com/em-refactor/file/666a2cd0318c/toolkit/mozapps/extensions/XPIProvider.jsm#l508

I'm releasing them at the end of BrowserShutdown() (i.e. on each window close), because of the problems we had with the patch in bug 506111 / bug 507092. It may not be necessary (or particularly effective, given potential GC delays). I'll investigate and make that simplification if possible.

Comment on attachment 424058 [details] [diff] [review] move to toolkit >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js The same should be done for browser-places.js and browser-fullZoom.js, as done in attachment 391307 [details] [diff] [review]. >+function charsetLoadListener (event) { charsetLoadListener( >+ gCharsetMenu = Components.classes['@mozilla.org/rdf/datasource;1?name=charset-menu'].getService(Ci.nsICurrentCharsetListener); Cc instead of Components.classes >--- a/browser/base/content/utilityOverlay.js >+++ b/browser/base/content/utilityOverlay.js >+function getBoolPref (prefname, def) getBoolPref( >- var prefSvc = Components.classes["@mozilla.org/preferences-service;1"] >- .getService(Components.interfaces.nsIPrefService); >- prefSvc = prefSvc.getBranch(null); >+ Services.prefs.getBranch(null); > > // should we open it in a new tab? > var loadInBackground = true; > try { > loadInBackground = prefSvc.getBoolPref("browser.tabs.loadInBackground"); This is broken...

Comment on attachment 424058 [details] [diff] [review] move to toolkit >+++ b/browser/base/content/browser.js >+// Services = object with smart getters for common XPCOM services >+Cu.import("resource://gre/modules/Services.jsm"); I think you can omit this, as you're doing it in utilityOverlay.js already.

(In reply to comment #12) > The same should be done for browser-places.js and browser-fullZoom.js, as done > in attachment 391307 [details] [diff] [review]. Yeah, I'll do them in a followup. > This is broken... Oops, that was one of my last changes, and didn't qref the final patch.

You still have a + Services.release(); ..which appears to have been removed from the rest of the patch. Is this mainly for code clarity? The short names seem less readable than windowMediator / permissionManager, especially since the definition of |Services| happens in a different file (utilityOverlay) via an opaque C.u.import call... Too terse is not better than too long, especially since this is effectively an API.

Comment on attachment 424113 [details] [diff] [review] patch v2 >+ Services.release(); I don't see a release method in the module anymore, so this should probably be removed. > } else { >- var ss = Cc["@mozilla.org/browser/search-service;1"]. >- getService(Ci.nsIBrowserSearchService); >- var searchForm = ss.defaultEngine.searchForm; >+ var searchForm = Services.search.defaultEngine.searchForm; > openUILinkIn(searchForm, "current"); > } > }, Should use "let" here or better yet, no variable at all. >+function UpdateCharsetDetector() { >+ var prefvalue = "off"; >+ >+ try { >+ prefvalue = gPrefService.getComplexValue("intl.charset.detector", Ci.nsIPrefLocalizedString).data; >+ } >+ catch (ex) {} >+ >+ prefvalue = 'chardet.' + prefvalue; >+ >+ var menuitem = document.getElementById(prefvalue); >+ if (menuitem) >+ menuitem.setAttribute('checked', 'true'); > } Could replace ' with " while you're at it. > uninit: function () > { > try { >- var os = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService); >- os.removeObserver(this, "network:offline-status-changed"); >+ Services.obs.removeObserver(this, "network:offline-status-changed"); > } catch (ex) { > } > }, I don't think there's anything to catch here. >+ Services.pm.add(aDocument.documentURIObject, "offline-app", >+ Ci.nsIPermissionManager.DENY_ACTION); I think I'd prefer Services.pm.FOO instead of Ci.nsIPermissionManager.FOO throughout this patch. You're doing it with Services.prompt already. > let gPrivateBrowsingUI = { > _observerService: null, > _privateBrowsingService: null, > _searchBarValue: null, > _findBarValue: null, > > init: function PBUI_init() { >- this._observerService = Cc["@mozilla.org/observer-service;1"]. >- getService(Ci.nsIObserverService); >+ this._observerService = Services.obs; > this._observerService.addObserver(this, "private-browsing", false); > this._observerService.addObserver(this, "private-browsing-transition-complete", false); This looks like we should get rid of _observerService altogether. >+ var loadInBackground = getBoolPref("browser.tabs.loadInBackground", true); The second argument is superfluous, since browser.tabs.loadInBackground is set in firefox.js.

(In reply to comment #16) > You still have a > + Services.release(); > ..which appears to have been removed from the rest of the patch. Indeed :/ > The short names seem less readable than windowMediator / permissionManager, > especially since the definition of |Services| happens in a different file > (utilityOverlay) via an opaque C.u.import call... Too terse is not better than > too long, especially since this is effectively an API. I went back and forth on this. I expect that if use of this module becomes common enough, it won't be a problem (most of the uses are relatively obvious in context), and it's nice to avoid the need to use local variables just to keep things to a reasonable line length. I did purposefully add "Services =" to the comment in an attempt to make MXR results at least slightly easier to grok.

Previous patch caused some places tests to fail, because utilityOverlay.js is loaded into places.xul, and for some reason Cu isn't defined there. I thought at first that the problem was Cu being undefined in general, so I wrote some basic utilityOverlay tests, but then realized that didn't make sense because the browser would have been busted if it applied to browser.xul's scope. Oh well, extra tests don't hurt, even though they wouldn't catch that patch's bustage.

Comment on attachment 425518 [details] [diff] [review] patch v4 Looks ok, I'm slightly on the side of longer names but long lines are annoying so I guess it's fine as is. There are a couple of services I use frequently that I think we should add here: appinfo (@mozilla.org/xre/app-info;1) should QI as nsIXULAppInfo and nsIXULRuntime storage (@mozilla.org/storage/service;1) should QI as nsIStorageService Maybe also these: vc (@mozilla.org/xpcom/version-comparator;1) should QI as nsIVersionComparator locale (@mozilla.org/intl/nslocaleservice;1) should QI as nsILocaleService

Pushed to trunk: http://hg.mozilla.org/mozilla-central/rev/78e5543c0bc4

I blogged about his: http://www.gavinsharp.com/blog/2010/02/25/services-jsm/ I'll try to add some docs for this on MDC when I have a chance.

(In reply to comment #21) > There are a couple of services I use frequently that I think we should add > here Filed bug 548627 for that.

>> There are a couple of services I use frequently that I think we should add >> here > Filed bug 548627 for that. There seems to be a significant overlap between services.jsm and FUEL. Is it possible to merge these two. Otherwise there will be two ways to access nsIWindowMediator, nsIStorageManager etc.

(In reply to comment #25) > >> There are a couple of services I use frequently that I think we should add > >> here > > > Filed bug 548627 for that. > > There seems to be a significant overlap between services.jsm and FUEL. Is it > possible to merge these two. Otherwise there will be two ways to access > nsIWindowMediator, nsIStorageManager etc. FUEL is a nice JS wrapper around these XPCOM services, this on the other hand is just a quick way to get at these services directly without having to look up the magic contract ID etc. I don't believe FUEL provides direct access and isn't necessarily what is needed.

Wasn't there some issue with having an observer having a global reference to the observer service causing a shutdown leak? (It may have been fixed.)

mochitests were all green, so I think we're good there.

Comment on attachment 425518 [details] [diff] [review] patch v4 >+XPCOMUtils.defineLazyServiceGetter(Services, "search", >+ "@mozilla.org/browser/search-service;1", >+ "nsIBrowserSearchService"); Not everybody builds the browser search service!

(In reply to comment #29) > (From update of attachment 425518 [details] [diff] [review]) > >+XPCOMUtils.defineLazyServiceGetter(Services, "search", > >+ "@mozilla.org/browser/search-service;1", > >+ "nsIBrowserSearchService"); > Not everybody builds the browser search service! True. Still, doesn't this being a lazy getter mean that it only gets loaded once its requested?

(In reply to comment #30) > True. Still, doesn't this being a lazy getter mean that it only gets loaded > once its requested? Yes, so it should be fine.

Now documented: https://developer.mozilla.org/en/JavaScript_code_modules/Services.jsm