Closed Bug 560772 Opened 10 years ago Closed 8 years ago

Make use of mozilla::services for comm-central

Categories

(MailNews Core :: Backend, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 13.0

People

(Reporter: Callek, Assigned: Callek)

References

()

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch suite/* (obsolete) — Splinter Review
Attachment #440419 - Flags: superreview?(neil)
Attachment #440419 - Flags: review?
Attachment #440419 - Flags: review? → review?(kairo)
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.
Attachment #440419 - Flags: superreview?(neil) → superreview-
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.
Attachment #440419 - Flags: review?(kairo)
Component: Build Config → Backend
QA Contact: build-config → backend
Duplicate of this bug: 562709
With Neil's concern, dropping myself off the assignee until M-C supports this externally linked.
Assignee: bugspam.Callek → nobody
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.
Attachment #440419 - Flags: superreview?(neil)
Attachment #440419 - Flags: superreview-
Attachment #440419 - Flags: review?(iann_bugzilla)
Assignee: nobody → bugspam.Callek
Status: NEW → ASSIGNED
Depends on: 560095
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.
No longer depends on: C192Branch
Comment on attachment 440419 [details] [diff] [review]
suite/*

KaiRo just bitrotted you again ;-)
Attachment #440419 - Flags: superreview?(neil)
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 :(
Attachment #440419 - Flags: review?(iann_bugzilla)
Attached patch WIP 1 (obsolete) — Splinter Review
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
Attachment #440419 - Attachment is obsolete: true
Attachment #585255 - Flags: feedback?(neil)
Attachment #585255 - Flags: feedback?(mbanner)
(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.
Attachment #585255 - Flags: feedback?(mbanner) → feedback+
(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.
See Also: → 714967
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 :-)
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #585255 - Attachment is obsolete: true
Attachment #586634 - Flags: feedback?(bugspam.Callek)
Attachment #585255 - Flags: feedback?(neil)
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
Attachment #586634 - Flags: review?(mbanner)
Attachment #586634 - Flags: feedback?(bugspam.Callek)
Attachment #586634 - Flags: feedback+
Ping for review?
Attached patch v1.1 (unbitrotted) (obsolete) — Splinter 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?
Attachment #591883 - Flags: review-
Attachment #586634 - Flags: review?(mbanner)
Interdiff of suggested fixes.
Attachment #586634 - Attachment is obsolete: true
Attachment #591883 - Attachment is obsolete: true
Attached patch v2Splinter Review
v2
Attachment #594649 - Flags: review?(mbanner)
Attachment #594649 - Flags: review?(mbanner) → review+
http://hg.mozilla.org/comm-central/rev/233670788020
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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?
Flags: in-testsuite-
Target Milestone: --- → Thunderbird 13.0
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).
You need to log in before you can comment on or make changes to this bug.