The suite/ portions I am to do can land now (pending review) whereas the mailnews and mail, etc. I feel is best without ifdefs and thus can land after c-c branch.

Comment on attachment 440419 [details] [diff] [review] suite/* Given that each of these calls runs at most once each, and that we don't yet know about external linkage libxul compatibility, I wouldn't bother with this.

Comment on attachment 440419 [details] [diff] [review] suite/* Sorry, I can't review C++ code. Still, I wonder if it makes sense to patch nsBookmarksService.cpp at all, given its removal in bug 498596. I'd love to see nsInternetSearchService.cpp be killed for 2.1 as well, but nobody has done actual work on the OpenSearch switch yet, AFAIK.

With Neil's concern, dropping myself off the assignee until M-C supports this externally linked.

Comment on attachment 440419 [details] [diff] [review] suite/* re-requesting review, this patch is already written and since we link into libxul now I *think* this is now Ok. I did not double-check if it bitrotted or if all the places I patched here link correctly, but I *think* thats all right.

Comment on attachment 440419 [details] [diff] [review] suite/* >diff --git a/suite/browser/src/nsBookmarksService.cpp b/suite/browser/src/nsBookmarksService.cpp This file doesn't exist anymore. *** http://mxr.mozilla.org/comm-central/search?string=do_GetService&find=%2Fsuite%2F It looks like 4 |do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv)| could be converted too, couldn't they?

(In reply to comment #7) > converted too, couldn't they? Was a first-shot, I didn't delve too deep here. So if this gets review I will do more.

Comment on attachment 440419 [details] [diff] [review] suite/* KaiRo just bitrotted you again ;-)

When trying to compile with this patch (or the remaining bit of it), I get: In function `nsNetscapeProfileMigratorBase::nsNetscapeProfileMigratorBase()': /mozdev/commsrc/suite/profile/migration/src/nsNetscapeProfileMigratorBase.cpp:101: undefined reference to `mozilla::services::GetObserverService()' /usr/bin/ld: libsuite.so: hidden symbol `mozilla::services::GetObserverService()' isn't defined /usr/bin/ld: final link failed: Nonrepresentable section on output collect2: ld returned 1 exit status

Comment on attachment 440419 [details] [diff] [review] suite/* Canceling request as current patch seems broken :(

This is a WIP I just started, and am tackling all of c-c right now... I have not yet tested the patch locally, and do have more to do. And I also do intend to run it against TB try-server. I'm looking if there is any repetitive mistakes I made, or if there is any major problems with this, not looking for a detailed review yet

(In reply to Justin Wood from comment #12) > I'm looking if there is any major problems with this Such as (per comment #2 and comment #5) linking to, rather than with, libxul?

(In reply to neil@parkwaycc.co.uk from comment #13) > (In reply to Justin Wood from comment #12) > > I'm looking if there is any major problems with this > Such as (per comment #2 and comment #5) linking to, rather than with, libxul? Well c#5 was written before we linked with libxul here.

Comment on attachment 585255 [details] [diff] [review] WIP 1 I'd be generally happy with this, but it probably would be useful to see if we can find a way around external linkage issues.

(In reply to neil@parkwaycc.co.uk from comment #13) > (In reply to Justin Wood from comment #12) > > I'm looking if there is any major problems with this > Such as (per comment #2 and comment #5) linking to, rather than with, libxul? Filed Bug 714967 - with a patch. Now all thats left is to advocate to them if its turned down. I intend to move forward with this bug, regardless of the outcome there.

How about adding the following to nsMsgUtils.h: (#ifdef MOZILLA_INTERNAL_API) #include "mozilla/Services.h" using namespace mozilla::services; (#else) #define GetObserverService() do_GetService("@mozilla.org/observer-service;1") (etc.) (#endif)

(In reply to neil@parkwaycc.co.uk from comment #17) > How about adding the following to nsMsgUtils.h: > #include "mozilla/Services.h" > using namespace mozilla::services; I'm not really a fan of |using namespace| like this, makes it harder to identify where/what something is from. And easier for name conflicts. That said if the bug about making this external falls through, and you can't convince ben why it might be good, I can give it a shot. But in the mean time I am moving forward with trying to make this work as-is :-)

This compiles fine for SeaMonkey with the services external patch (incase it matters). I want to review the patch before handing review over, as well as doing a try run.

Comment on attachment 586634 [details] [diff] [review] v1 Review of attachment 586634 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/shell/nsMailGNOMEIntegration.cpp @@ +301,5 @@ > } > > if (giovfs) { > nsCOMPtr<nsIStringBundleService> bundleService = > + mozilla::services::GetObserverService(); This should have been GetStringBundleService() fixed locally

Ping for review?

This is just an unbitrotted version of v1, incase Mark requires it to review. I really want to land this before the train leaves the station

Comment on attachment 591883 [details] [diff] [review] v1.1 (unbitrotted) Review of attachment 591883 [details] [diff] [review]: ----------------------------------------------------------------- This generally looks good, however I think we should be tidying up the indentation and length of lines whilst we're touching all of these locations. Generally I think that mozilla::services::... should be on a new line with 2-space indent. You'll get the ideas from the start of the comments where I was commenting individually to being with. ::: mailnews/addrbook/src/nsAbCardProperty.cpp @@ +725,4 @@ > > // Get Address Book string and set it as title of XML document > nsCOMPtr<nsIStringBundle> bundle; > + nsCOMPtr<nsIStringBundleService> stringBundleService = mozilla::services::GetStringBundleService(); nit: start mozilla on the next line with 2-space indent please. ditto with the other case in this file. ::: mailnews/addrbook/src/nsAbManager.cpp @@ +175,4 @@ > > nsresult rv; > nsCOMPtr<nsIObserverService> observerService = > + mozilla::services::GetObserverService(); nit: here you could move nsresult rv to be initialised directly on the observerService call a couple of lines down (and for the removeObserver call later in this file). @@ +541,4 @@ > nsCOMPtr<nsIFilePicker> filePicker = do_CreateInstance("@mozilla.org/filepicker;1", &rv); > NS_ENSURE_SUCCESS(rv, rv); > > + nsCOMPtr<nsIStringBundleService> bundleService = mozilla::services::GetStringBundleService(); nit mozilla::... on a new line please. @@ +667,4 @@ > PRUint32 writeCount; > PRUint32 length; > > + nsCOMPtr<nsIStringBundleService> bundleService = mozilla::services::GetStringBundleService(); ditto ::: mailnews/addrbook/src/nsAbView.cpp @@ +1198,4 @@ > nsString str; > pls->ToString(getter_Copies(str)); > displayNameLastnamefirst = str.EqualsLiteral("true"); > + nsCOMPtr<nsIStringBundleService> bundleService = mozilla::services::GetStringBundleService(); nit: new line again. ::: mailnews/addrbook/src/nsAddbookProtocolHandler.cpp @@ +266,4 @@ > > // Get Address Book string and set it as title of XML document > nsCOMPtr<nsIStringBundle> bundle; > + nsCOMPtr<nsIStringBundleService> stringBundleService = mozilla::services::GetStringBundleService(); nit: new line ::: mailnews/base/src/nsMailDirProvider.cpp @@ +229,4 @@ > (nsISimpleEnumerator* aBase) : > mBase(aBase) > { > + nsCOMPtr<nsIXULChromeRegistry> packageRegistry = mozilla::services::GetXULChromeRegistryService(); nit: start on a new line please. ::: mailnews/base/src/nsMessenger.cpp @@ +1989,5 @@ > const char propertyURL[] = MESSENGER_STRING_URL; > nsCOMPtr<nsIStringBundleService> sBundleService = > + mozilla::services::GetStringBundleService(); > + NS_ENSURE_TRUE(sBundleService, NS_ERROR_UNEXPECTED); > + res = sBundleService->CreateBundle(propertyURL, Might as well drop the "res" variable, turn this into a function where you have: if (mStringBundle) return NS_OK; ... return sBundleService->Create.... ::: mailnews/base/src/nsMessengerUnixIntegration.cpp @@ +180,2 @@ > nsCOMPtr<nsIStringBundle> bundle; > + bundleService->CreateBundle("chrome://messenger/locale/messenger.properties", getter_AddRefs(bundle)); nit: put the second parameter on a new line please (align with the " after the open bracket). ::: mailnews/base/src/nsMessengerWinIntegration.cpp @@ +467,2 @@ > nsCOMPtr<nsIStringBundle> bundle; > + bundleService->CreateBundle("chrome://messenger/locale/messenger.properties", getter_AddRefs(bundle)); nit: put the second parameter on a new line please (align with the " after the open bracket). ::: mailnews/base/src/nsMsgAccountManager.cpp @@ +194,5 @@ > Shutdown(); > //Don't remove from Observer service in Shutdown because Shutdown also gets called > //from xpcom shutdown observer. And we don't want to remove from the service in that case. > nsCOMPtr<nsIObserverService> observerService = > + mozilla::services::GetObserverService(); nit: these need re-indenting to two-space (same with other instances in this file). ::: mailnews/base/src/nsMsgOfflineManager.cpp @@ +283,5 @@ > > nsCOMPtr<nsIStringBundleService> sBundleService = > + mozilla::services::GetStringBundleService(); > + NS_ENSURE_TRUE(sBundleService, NS_ERROR_UNEXPECTED); > + res = sBundleService->CreateBundle(propertyURL, getter_AddRefs(mStringBundle)); res isn't used here, so we can just drop that and move its definition into the if (mStringBundle) below. Whilst you're here, please drop the propertyURL wrapping of MESSENGER_STRING_URL - that's just unnecessary. @@ +360,5 @@ > { > nsresult rv; > + nsCOMPtr<nsIIOService> netService = mozilla::services::GetIOService(); > + NS_ENSURE_TRUE(netService, NS_ERROR_UNEXPECTED); > + rv = netService->SetOffline(!online); Lets drop the intermediate variable now and just return the result straight away. ::: mailnews/base/util/nsMsgIncomingServer.cpp @@ +1621,5 @@ > NS_ENSURE_ARG_POINTER(aMsgWindow); > > nsresult rv; > + nsCOMPtr<nsIStringBundleService> bundleService = mozilla::services::GetStringBundleService(); > + NS_ENSURE_TRUE(bundleService, NS_ERROR_UNEXPECTED); nit: move the nsresult rv down so that it is initialised on the same line as it is first used. ::: mailnews/compose/src/nsSmtpProtocol.cpp @@ +452,5 @@ > if (m_statusFeedback) > { > nsresult rv; > + nsCOMPtr<nsIStringBundleService> bundleService = mozilla::services::GetStringBundleService(); > + if (!bundleService) return; nit: move result values in this file (several places) ::: mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp @@ +136,5 @@ > nsresult rv; > > + nsCOMPtr<nsIStringBundleService> bundleService = > + mozilla::services::GetStringBundleService(); > + NS_ENSURE_TRUE(bundleService, NS_ERROR_UNEXPECTED); nit: move result values in this file. ::: mailnews/imap/src/nsImapIncomingServer.cpp @@ +1978,4 @@ > > nsresult nsImapIncomingServer::GetStringBundle() > { > + nsresult res = NS_OK; nit: I think this should be an early return on if (m_stringBundle), then do the rest and just return the result straight away without using res. ::: mailnews/news/src/nsNntpService.cpp @@ +87,4 @@ > #include "nsIMsgMailNewsUrl.h" > #include "nsIMsgMailSession.h" > #include "nsISupportsPrimitives.h" > +#include "mozilla/Services.h" No other changes in this file, so is this unnecessary?

Interdiff of suggested fixes.

v2

http://mxr.mozilla.org/comm-central/search?string=do_GetService&find=%2Fsuite%2F "Found 20 matching lines in 7 files" http://mxr.mozilla.org/comm-central/search?string=do_GetService&find=%2Fmail%2F "Found 23 matching lines in 8 files" etc Should a follow-up bug be filed to (be able to) use other services, most notably NS_PREFSERVICE_CONTRACTID?

Why this change has been introduced? It breaks building thunderbird with libxul in linking phase: /home/jhorak/mozilla/9mozilla-central/obj-dir/dist/include/mozilla/ServiceList.h:10: undefined reference to `mozilla::services::_external_GetStringBundleService()' Do I have to link affected modules directly against libxul?

(In reply to jhorak from comment #28) > Why this change has been introduced? It breaks building thunderbird with > libxul in linking phase: > /home/jhorak/mozilla/9mozilla-central/obj-dir/dist/include/mozilla/ > ServiceList.h:10: undefined reference to > `mozilla::services::_external_GetStringBundleService()' > Do I have to link affected modules directly against libxul? Please see earlier comments in this bug, especially comment 16 & references of bug 714967 which imply this was considered before fixing this bug. Its possible of course that something has regressed since this landed...

(In reply to Mark Banner (:standard8) from comment #29) > Please see earlier comments in this bug, especially comment 16 & references > of bug 714967 which imply this was considered before fixing this bug. Its > possible of course that something has regressed since this landed... Oh, I see. Solution in comment 17 has been refused. I'd like to continue on TB & libxul (bug #306324) and I don't know how to deal with this issue (undefined reference to `mozilla::services::_external_GetStringBundleService()' which is only in libxul.so).