Make use of mozilla::services for comm-central

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
Backend
--
minor
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Callek, Assigned: Callek)

Tracking

Trunk
Thunderbird 13.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 440419 [details] [diff] [review]
suite/*
Attachment #440419 - Flags: superreview?(neil)
Attachment #440419 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #440419 - Flags: review? → review?(kairo)

Comment 2

7 years ago
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 3

7 years ago
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)
(Assignee)

Updated

7 years ago
Component: Build Config → Backend
QA Contact: build-config → backend
(Assignee)

Updated

7 years ago
Duplicate of this bug: 562709
(Assignee)

Comment 5

7 years ago
With Neil's concern, dropping myself off the assignee until M-C supports this externally linked.
Assignee: bugspam.Callek → nobody
(Assignee)

Comment 6

7 years ago
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)

Updated

7 years ago
Assignee: nobody → bugspam.Callek
Status: NEW → ASSIGNED
Depends on: 560095
Depends on: 544237
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?
(Assignee)

Comment 8

7 years ago
(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: 544237

Comment 9

7 years ago
Comment on attachment 440419 [details] [diff] [review]
suite/*

KaiRo just bitrotted you again ;-)
Attachment #440419 - Flags: superreview?(neil)

Comment 10

7 years ago
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

7 years ago
Comment on attachment 440419 [details] [diff] [review]
suite/*

Canceling request as current patch seems broken :(
Attachment #440419 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 12

6 years ago
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
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?
(Assignee)

Comment 14

6 years ago
(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+
(Assignee)

Comment 16

6 years ago
(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: → bug 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)
(Assignee)

Comment 18

6 years ago
(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 :-)
(Assignee)

Comment 19

6 years ago
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.
Attachment #585255 - Attachment is obsolete: true
Attachment #586634 - Flags: feedback?(bugspam.Callek)
Attachment #585255 - Flags: feedback?(neil)
(Assignee)

Comment 20

6 years ago
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+
(Assignee)

Comment 21

6 years ago
Ping for review?
(Assignee)

Comment 22

6 years ago
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 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)
(Assignee)

Comment 24

6 years ago
Created attachment 594648 [details] [diff] [review]
v2 interdiff against v1

Interdiff of suggested fixes.
Attachment #586634 - Attachment is obsolete: true
Attachment #591883 - Attachment is obsolete: true
(Assignee)

Comment 25

6 years ago
Created attachment 594649 [details] [diff] [review]
v2

v2
Attachment #594649 - Flags: review?(mbanner)
Attachment #594649 - Flags: review?(mbanner) → review+
(Assignee)

Comment 26

6 years ago
http://hg.mozilla.org/comm-central/rev/233670788020
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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

Comment 28

6 years ago
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...

Comment 30

6 years ago
(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.