Last Comment Bug 560772 - Make use of mozilla::services for comm-central
: Make use of mozilla::services for comm-central
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 13.0
Assigned To: Justin Wood (:Callek) (Away until Aug 29)
:
Mentors:
https://developer.mozilla.org/en/XPCO...
: 562709 (view as bug list)
Depends on: 516085 560095
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-20 21:17 PDT by Justin Wood (:Callek) (Away until Aug 29)
Modified: 2012-03-21 07:52 PDT (History)
7 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
suite/* (2.57 KB, patch)
2010-04-20 21:30 PDT, Justin Wood (:Callek) (Away until Aug 29)
no flags Details | Diff | Splinter Review
WIP 1 (27.56 KB, patch)
2012-01-01 22:23 PST, Justin Wood (:Callek) (Away until Aug 29)
standard8: feedback+
Details | Diff | Splinter Review
v1 (170.25 KB, patch)
2012-01-06 18:48 PST, Justin Wood (:Callek) (Away until Aug 29)
bugspam.Callek: feedback+
Details | Diff | Splinter Review
v1.1 (unbitrotted) (106.31 KB, patch)
2012-01-26 11:55 PST, Justin Wood (:Callek) (Away until Aug 29)
standard8: review-
Details | Diff | Splinter Review
v2 interdiff against v1 (73.42 KB, patch)
2012-02-05 22:56 PST, Justin Wood (:Callek) (Away until Aug 29)
no flags Details | Diff | Splinter Review
v2 (109.46 KB, patch)
2012-02-05 22:57 PST, Justin Wood (:Callek) (Away until Aug 29)
standard8: review+
Details | Diff | Splinter Review

Description Justin Wood (:Callek) (Away until Aug 29) 2010-04-20 21:17:54 PDT
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 1 Justin Wood (:Callek) (Away until Aug 29) 2010-04-20 21:30:39 PDT
Created attachment 440419 [details] [diff] [review]
suite/*
Comment 2 neil@parkwaycc.co.uk 2010-04-21 03:50:24 PDT
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 3 Robert Kaiser 2010-04-21 05:34:12 PDT
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.
Comment 4 Justin Wood (:Callek) (Away until Aug 29) 2010-05-22 18:55:42 PDT
*** Bug 562709 has been marked as a duplicate of this bug. ***
Comment 5 Justin Wood (:Callek) (Away until Aug 29) 2010-05-30 20:01:27 PDT
With Neil's concern, dropping myself off the assignee until M-C supports this externally linked.
Comment 6 Justin Wood (:Callek) (Away until Aug 29) 2010-09-29 16:57:33 PDT
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 7 Serge Gautherie (:sgautherie) 2010-09-29 17:50:43 PDT
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?
Comment 8 Justin Wood (:Callek) (Away until Aug 29) 2010-09-29 19:36:35 PDT
(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 9 neil@parkwaycc.co.uk 2010-09-30 05:38:12 PDT
Comment on attachment 440419 [details] [diff] [review]
suite/*

KaiRo just bitrotted you again ;-)
Comment 10 Ian Neal 2010-10-04 15:45:20 PDT
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 11 Ian Neal 2010-10-19 16:52:21 PDT
Comment on attachment 440419 [details] [diff] [review]
suite/*

Canceling request as current patch seems broken :(
Comment 12 Justin Wood (:Callek) (Away until Aug 29) 2012-01-01 22:23:25 PST
Created attachment 585255 [details] [diff] [review]
WIP 1

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
Comment 13 neil@parkwaycc.co.uk 2012-01-02 02:34:01 PST
(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?
Comment 14 Justin Wood (:Callek) (Away until Aug 29) 2012-01-02 11:04:49 PST
(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 15 Mark Banner (:standard8) 2012-01-03 01:52:35 PST
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.
Comment 16 Justin Wood (:Callek) (Away until Aug 29) 2012-01-03 14:48:28 PST
(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.
Comment 17 neil@parkwaycc.co.uk 2012-01-04 14:03:30 PST
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)
Comment 18 Justin Wood (:Callek) (Away until Aug 29) 2012-01-04 14:11:40 PST
(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 :-)
Comment 19 Justin Wood (:Callek) (Away until Aug 29) 2012-01-06 18:48:02 PST
Created attachment 586634 [details] [diff] [review]
v1

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 20 Justin Wood (:Callek) (Away until Aug 29) 2012-01-06 18:55:47 PST
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
Comment 21 Justin Wood (:Callek) (Away until Aug 29) 2012-01-21 05:13:15 PST
Ping for review?
Comment 22 Justin Wood (:Callek) (Away until Aug 29) 2012-01-26 11:55:59 PST
Created attachment 591883 [details] [diff] [review]
v1.1 (unbitrotted)

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 23 Mark Banner (:standard8) 2012-01-27 04:48:42 PST
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?
Comment 24 Justin Wood (:Callek) (Away until Aug 29) 2012-02-05 22:56:39 PST
Created attachment 594648 [details] [diff] [review]
v2 interdiff against v1

Interdiff of suggested fixes.
Comment 25 Justin Wood (:Callek) (Away until Aug 29) 2012-02-05 22:57:59 PST
Created attachment 594649 [details] [diff] [review]
v2

v2
Comment 26 Justin Wood (:Callek) (Away until Aug 29) 2012-02-13 09:59:04 PST
http://hg.mozilla.org/comm-central/rev/233670788020
Comment 27 Serge Gautherie (:sgautherie) 2012-02-14 21:31:20 PST
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?
Comment 28 Jan Horak 2012-03-20 06:50:35 PDT
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?
Comment 29 Mark Banner (:standard8) 2012-03-20 13:59:43 PDT
(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...
Comment 30 Jan Horak 2012-03-21 07:52:44 PDT
(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).

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