Following the great work done in bug 516085 to provide easy access for common global services, we can now start replacing do_GetService() calls with mozilla::services::Get*Service(). In forthcoming patches I'm doing so with GetObserverService(), but I got a bit carried away and nuked hundreds of unnecessary {} brackets in the process.

Our style guide says > Always brace controlled statements, even a single-line consequent of an if else > else. This is redundant typically, but it avoids dangling else bugs, so it's > safer at scale than fine-tuning. So removing those {}s is not right.

What a horrible style rule.

Comment on attachment 439763 [details] [diff] [review] netwerk (observer service, v1) +++ b/netwerk/cache/src/nsCacheService.cpp + if (!observerService) + return NS_ERROR_FAILURE; NS_ENSURE_ARG(observerService); you don't need both of those checks + if (obs) + for (unsigned int i=0; i<NS_ARRAY_LENGTH(observerList); i++) + obs->RemoveObserver(this, observerList[i]); please keep the {}, I find multi-line bodies easier to read if they're bracketed even when they're a single statement.

> What a horrible style rule. It's there because not having it led to serious bugs, including security bugs, in the past. Put the curlies back, please.

Review comments addressed.

Note: I changed "obssvc" to "obsSvc" for a bit of consistency within one file.

I didn't realise the toolkit component encompassed "chrome" and "profile" directories.

Sorry for all the bug spam. There has to be a better way to do this.

Do we actually need to use nsCOMPtr to hold the service? I don't think we do.

(In reply to comment #26) > Do we actually need to use nsCOMPtr to hold the service? I don't think we do. The services still need to be reference-counted as they are also accessed via JS. I'd rather not have someone accidentally hold on to a service until after it is shutdown.

(In reply to comment #27) > (In reply to comment #26) > > Do we actually need to use nsCOMPtr to hold the service? I don't think we do. > > The services still need to be reference-counted as they are also accessed via > JS. But isn't the mozilla::service layer holding a reference to the service? Why does layout etc need to add a reference?

(In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #26) > > > Do we actually need to use nsCOMPtr to hold the service? I don't think we do. > > > > The services still need to be reference-counted as they are also accessed via > > JS. > > But isn't the mozilla::service layer holding a reference to the service? Why > does layout etc need to add a reference? It holds one reference, until xpcom shutdown when it goes away. At which point any references via raw pointers will be bad.

OK but most of the uses here are storing the service reference in an nsCOMPtr local variable. Those surely aren't needed.

(In reply to comment #30) > OK but most of the uses here are storing the service reference in an nsCOMPtr > local variable. Those surely aren't needed. Indeed, but I'd rather not risk having that leak(or have xpcom shutdown midmethod). As far as saving cpu time it's pointless, getservice-type things just don't get called enough for reference counting to make any measurable difference.

XPCOM can shut down mid-method? I seriously hope not! If the caller doesn't add a reference, it can't leak.

(In reply to comment #32) > XPCOM can shut down mid-method? I seriously hope not! yes if the method initiates a shutdown. > > If the caller doesn't add a reference, it can't leak. Worried about dereferencing free()ed memory, not leaks. My original idea was to not ref count, but ref counting turned out to be safer.

OK, but XPCOM can't shut down without spinning a nested event loop, right? Lots of code holds raw pointers across function calls because we assume/know that the callee can't spin a nested event loop. The same should be true here.

Comment on attachment 439796 [details] [diff] [review] toolkit (observer service, v3) >diff --git a/toolkit/xre/nsNativeAppSupportUnix.cpp b/toolkit/xre/nsNativeAppSupportUnix.cpp >@@ -323,24 +323,24 @@ static void OssoHardwareCallback(osso_hw > if (state->memory_low_ind) { > if (! ourState->memory_low_ind) { >- nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1"); >+ nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService(); > if (os) > os->NotifyObservers(nsnull, "memory-pressure", NS_LITERAL_STRING("low-memory").get()); > } > } Yeesh, what's going on here? Hate to scope creep, but this is so ugly I can't help it - can you make this a single conditional rather than nested if()s, and fix the indent? Please undo all of the brace removals. r=me with that.

Restored some braces.

Restored some more braces.

Comment on attachment 439806 [details] [diff] [review] storage (observer service, v1) r=sdwilsh

+ nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService(); This doesn't need to be an nsCOMPtr. + nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService(); Neither does this. etc... I guess we can take these patches for now, but at some point --- perhaps when we have infrastructure for annotating/analyzing which methods can spin the event loop --- I would like to remove the nsCOMPtrs.

mozilla::services returns already_AddRefed objects, so you do need a comptr.

It shouldn't.

(In reply to comment #41) > mozilla::services returns already_AddRefed objects, so you do need a comptr. You can do mozilla::services::GetIOService().get(), so technically you don't. I think making nsCOMPtr<>s optional is an interesting project, that should be investigated as part of another bug. It would be easier to make that change after giving services explicit getters like this bug is doing. Ridding services of nsCOMPtrs is not a low-hanging performance fruit, so didn't consider it as important as providing a way to bypass the massive GetService() slowness. One was to accomplish a comptr-less operation would be to define a wrapper class that lets one dereference services, but not store them. Ie allow mozilla::services::IOService->...., but not nsIIOService *io = mozilla::services::IOService.

(In reply to comment #43) > You can do mozilla::services::GetIOService().get(), so technically you don't. Well sure, but then you need to call release manually, so you might as well use a comptr.

Comment on attachment 439800 [details] [diff] [review] dom (observer service, v1) Please put back the braces to keep with the bracing stile of these files. (It also makes future patches easier to read) Looks good otherwise.

Comment on attachment 440438 [details] [diff] [review] dom (observer service, v2) Stealing review from sicking...

Comment on attachment 439810 [details] [diff] [review] xpinstall (observer service, v1) r=dvedit

TryServer picked up a missing include. Carrying over r+.

Ditto.

Comment on attachment 439845 [details] [diff] [review] toolkit (observer service, v4) this patch failed a try-build, nsNativeAppSupportUnix.cpp doesn't have "mozilla/services.h" in scope. The other files in this patch that don't directly include it get it for free from NetUtils.h. I'm not sure if toolkit would want explicit #includes in any file that uses mozilla::services though; so re-pushed the patchset to try without this...

Comment on attachment 439801 [details] [diff] [review] embedding (observer service, v1) mozilla::services is not externally linkable, therefore this fails to compile [and link] TestGtkEmbed.cpp please revert in this file.

Fixed. Thanks, Justin.

This amalgamation of all above patches passes TryServer, and should be ready for check-in. Patch includes Hg changeset summary.