The default bug view has changed. See this FAQ.

Remove the external string API from comm-central

RESOLVED FIXED in Thunderbird 54.0

Status

MailNews Core
Backend
RESOLVED FIXED
2 months ago
15 days ago

People

(Reporter: Magnus Melin, Assigned: Jorg K (GMT+1))

Tracking

Trunk
Thunderbird 54.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

2 months ago
+++ This bug was initially created as a clone of Bug #1332639 +++

With the removal of the XPCOM glue, the external string API is dead. We need to remove it. I intend to:

* remove nsStringAPI.h and nsStringAPI.cpp
* remove nsStringGlue.h and change current includers to nsString.h
* remove nsEmbedString.h and change current includers to nsString.h
* remove nsProfileStringTypes.h and fixup callsites
(Assignee)

Comment 1

2 months ago
Have I interpreted the "I intend to ..." incorrectly?
Assignee: nobody → mkmelin+mozilla
(Reporter)

Comment 2

2 months ago
Ah, that came from the cloning.
Assignee: mkmelin+mozilla → nobody
(Assignee)

Comment 3

2 months ago
If Magnus doesn't want it, then it might be an anti-bustage job for us, right, Richard? Let's keep an eye on bug 1332639.
(Assignee)

Comment 4

2 months ago
nsStringAPI.h: One include in calUtils.h.
nsStringGlue.h: Included a lot, that's an job for
  find . -name *.cpp -exec sed -i -e 's/nsStringGlue.h/nsString.h/g' {} \;
nsEmbedString.h: We don't use.
nsProfileStringTypes.h: We don't use.

So when the time comes, I can do this ;-)
(Reporter)

Comment 5

2 months ago
Well that's one thing. There's also the fairly massive removal off all the code that's (not) conditional on ifdef MOZILLA_INTERNAL_API.

All this is of course possible to do already with no harm, but it it's not done before bug 1332639 there will be bustage.
(Assignee)

Comment 6

2 months ago
Can you be more specific, please. What should happen to, for example:

#ifdef MOZILLA_INTERNAL_API
    if (extn.EqualsIgnoreCase("s")) {
      url->GetFileName(fileName);
      break;
    }
#else
    if (extn.Equals("s", CaseInsensitiveCompare)) {
      url->GetFileName(fileName);
      break;
    }
#endif

Do we compile with MOZILLA_INTERNAL_API defined or undefined. Excuse the ignorance :-(
(Reporter)

Comment 7

2 months ago
It's true, from here I think: https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/library/moz.build#9
So you would remove the ifdefs, and all the code in the code in the else block.
(Assignee)

Comment 8

26 days ago
I'll do what's needed in the next few days.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
(Reporter)

Comment 9

26 days ago
A lot of it can be automated. Let me have a look
(Assignee)

Comment 10

26 days ago
Created attachment 8841718 [details] [diff] [review]
1332717-replace-nsStringGlue.patch

Very boring review.
Generated with:
find . -name *.h -exec sed -i -e 's/#include \"nsStringGlue.h\"/#include \"nsString.h\"/' {} \; 
find . -name *.cpp -exec sed -i -e 's/#include \"nsStringGlue.h\"/#include \"nsString.h\"/' {} \;

Run on im, ldap, mail, mailnews, suite.

Still compiles ;-)

So far #include "nsStringGlue.h" can't be removed from nsMailApp.cpp, since that causes a compile failure. I asked in bug 1332639 comment #7.

So I guess we can prepare two patches here, this one and the one removing code (not) dependent on MOZILLA_INTERNAL_API but we'll need another patch to see where M-C goes in bug 1332639.
Attachment #8841718 - Flags: review?(mkmelin+mozilla)
(Reporter)

Comment 11

26 days ago
Created attachment 8841720 [details] [diff] [review]
bug1332717_remove_cc_external_api.patch

This is as far as I got. Untested, but looks ok by inspection
Attachment #8841720 - Flags: review?(jorgk)
(Reporter)

Comment 12

26 days ago
That was from 

 find mail/ mailnews/ editor/ im/ calendar/ suite/ chat/ ldap/ db/ -name "*.c*" -type f -print | xargs unifdef -m -D MOZILLA_INTERNAL_API

 find mail/ mailnews/ editor/ im/ calendar/ suite/ chat/ ldap/ db/ -name "*.c*" -type f -print0 | xargs -0 sed -i 's/#include "nsStringGlue.h"/#include "nsString.h"\n#include "nsReadableUtils.h"/g'
(Assignee)

Comment 13

26 days ago
Created attachment 8841729 [details] [diff] [review]
1332717-remove-moz-internal-api.patch

This was done by hand. Still compiles.
Attachment #8841729 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 14

26 days ago
Comment on attachment 8841720 [details] [diff] [review]
bug1332717_remove_cc_external_api.patch

I'm not sure why we're wasting time doing the same work twice :-(
I clearly assigned the bug to myself (hoping that you might spend time on the .show() bug) and stated that I'd work on it. Was there any doubt?

Scripts can only go so far:
Modified Binary File: mailnews/import/test/unit/resources/utf16_addressbook.csv

Should be r- for that already.
Attachment #8841720 - Flags: review?(jorgk)
(Assignee)

Comment 15

26 days ago
(In reply to Jorg K (GMT+1) from comment #10)
> So far #include "nsStringGlue.h" can't be removed from nsMailApp.cpp, since
> that causes a compile failure. I asked in bug 1332639 comment #7.
> 
> So I guess we can prepare two patches here, this one and the one removing
> code (not) dependent on MOZILLA_INTERNAL_API but we'll need another patch to
> see where M-C goes in bug 1332639.

Yes, we can only complete work here after bug 1332639 lands since they're adding some #ifdef that will allow removal of nsStringGlue.h from nsMailApp.cpp.
(Assignee)

Comment 16

26 days ago
(In reply to Magnus Melin from comment #5)
> Well that's one thing. There's also the fairly massive removal off all the
> code that's (not) conditional on ifdef MOZILLA_INTERNAL_API.
> All this is of course possible to do already with no harm, but it it's not
> done before bug 1332639 there will be bustage.
I don't think that's right. Bug 1332639 will remove nsStringGlue.h (causing bustage) but it will *not* remove MOZILLA_INTERNAL_API. Check M-C on DXR, it's widely used and bug 1332639 doesn't remove all that. In fact, bug 1332639 will even add a new use in BinaryPath.h, so inclusion in the (external) main program won't cause a compile error since an internal string API is not available. Check BinaryPath.h in https://reviewboard.mozilla.org/r/115828/diff/1#index_header.
(Reporter)

Comment 17

26 days ago
Well you said you'd work on in the next few days, and I did say I'd take a look. (BTW, the .show() bug is almost done.)
(Reporter)

Comment 18

26 days ago
(In reply to Jorg K (GMT+1) from comment #16)
> (In reply to Magnus Melin from comment #5)
> > Well that's one thing. There's also the fairly massive removal off all the
> > code that's (not) conditional on ifdef MOZILLA_INTERNAL_API.
> > All this is of course possible to do already with no harm, but it it's not
> > done before bug 1332639 there will be bustage.
> I don't think that's right. Bug 1332639 will remove nsStringGlue.h (causing
> bustage) but it will *not* remove MOZILLA_INTERNAL_API. 

It doesn't remove it, but if MOZILLA_INTERNAL_API is undefed nothing would work, so essentially dead code.
(Assignee)

Comment 19

26 days ago
(In reply to Magnus Melin from comment #17)
> Well you said you'd work on in the next few days, and I did say I'd take a
> look.
No damage done. Nice that you tried with a script, but you missed stuff and hit unrelated files (utf16_addressbook.csv). Doing it by hand also removed extra blank lines and comments which no long apply. The removal wasn't massive, so you can review this in 10 minutes.

> (BTW, the .show() bug is almost done.)
Great news!

(In reply to Magnus Melin from comment #18)
> It doesn't remove it, but if MOZILLA_INTERNAL_API is undefed nothing would
> work, so essentially dead code.
Sure, but that's also not going to happen since they have plenty of code left relying on it (and adding more).

Anyway, we're on top of it ;-)
(Reporter)

Comment 20

25 days ago
Comment on attachment 8841729 [details] [diff] [review]
1332717-remove-moz-internal-api.patch

Review of attachment 8841729 [details] [diff] [review]:
-----------------------------------------------------------------

I'm surprised include nsReadableUtils.h wasn't nneeded, but if it compiles without then great! r=mkmelin
Attachment #8841729 - Flags: review?(mkmelin+mozilla) → review+
(Reporter)

Updated

25 days ago
Attachment #8841720 - Attachment is obsolete: true
(Reporter)

Comment 21

25 days ago
Comment on attachment 8841718 [details] [diff] [review]
1332717-replace-nsStringGlue.patch

Review of attachment 8841718 [details] [diff] [review]:
-----------------------------------------------------------------

(The other comment was meant for this patch)
Attachment #8841718 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 22

24 days ago
(In reply to Magnus Melin from bug 1332639 comment #20)
> (In reply to Mike Hommey [:glandium] from bug 1332639 comment #17)
> > Comment on attachment 8841668 [details]
> > Bug 1332639 - Remove the RDF "standalone" library which isn't used,
> Well, it's used in comm-central...

Magnus, any more insights on this?

https://hg.mozilla.org/integration/mozilla-inbound/rev/9825f34adc3d
doesn't remove any code, right? Will this cause us problems?
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 23

24 days ago
Magnus, I've been looking at what M-C will land:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bcada51f54b
Remove unneeded #include so that this file builds, r=gsvelto

https://hg.mozilla.org/integration/mozilla-inbound/rev/9825f34adc3d
Remove the RDF "standalone" library which isn't used, r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab3684fe0488
Stop building the media/mtransport/testlib library which isn't used, r=dminor

https://hg.mozilla.org/integration/mozilla-inbound/rev/e761b88de9de
Remove the external unicharutil library which isn't used and is now unlinkable, r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/cae4a255be3f
Fix nsBrowserApp.cpp and related headers to compile without the external string API, r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f45b2afbf3
Remove the external string API: nsStringAPI.h/cpp and nsEmbedString.h, r=glandium

Looking at the last changeset, nsStringGlue.h will be maintained, only it will error out if MOZILLA_INTERNAL_API isn't set.

So we don't really need any of the two patches we have prepared here.

What we do need to port is the change in the main program
https://hg.mozilla.org/integration/mozilla-inbound/rev/cae4a255be3f#l1.12
since this is external. There we need to apply these two changes:

-#include "nsStringGlue.h"
and
-#include "mozilla/Telemetry.h"
+#include "mozilla/StartupTimeline.h"

How do you want to proceed?

The other thing is that I replaced #include "nsStringGlue.h" with #include "nsString.h", where as nsStringGlue.h also includes nsReadableUtils.h. My local build still compiles since which I don't quite understand since C-C uses for example LossyCopyUTF16toASCII() which is coming from nsReadableUtils.h

I'll import the M-I changes now to check it out.
(Assignee)

Comment 24

24 days ago
Created attachment 8842496 [details] [diff] [review]
1332717-changes-to-nsMailApp.patch - ONLY PATCH NEEDED!!

This is the only patch that's really required, all the rest was a MacGuffin. I'm not sure whether we want to land the other ones.

With this patch, everything compiles links and runs (when the patches from bug 1332639 are applied).
Attachment #8842496 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 25

24 days ago
OK, I applied the other two patches as well, and everything compiles, links and runs.

So what do I land in the end?

Comment 26

24 days ago
If the removed parts are dead code I would land everything.
(Assignee)

Comment 27

24 days ago
(In reply to Frank-Rainer Grahl from comment #26)
> If the removed parts are dead code I would land everything.
I must admit that I don't understand it fully. Apparently there is and internal API that things inside "libxul" can use and an external API that other things, like the main program, can use.

As far as I understand it, all C-C code, apart from the mainlines, is internal. So we can lose the code ifdef-ed away by (not) MOZILLA_INTERNAL_API.

Since nsStringGlue.h (which was *not* removed other than announced) is only a redirect to nsString.h we could change that since we know everything is internal.

I don't know M-C's plans. Magnus was afraid that the define MOZILLA_INTERNAL_API would be removed which would have exposed a lot of "external" code. But that's not the case. M-C has MOZILLA_INTERNAL_API all over the place.
(Reporter)

Updated

24 days ago
Attachment #8842496 - Flags: review?(mkmelin+mozilla) → review+
(Reporter)

Comment 28

24 days ago
They plan to remove all code that's external api, so let's just land all the patches we can.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 29

24 days ago
Created attachment 8842583 [details] [diff] [review]
1332717-changes-to-nsMailApp.patch (v2).

I'm nice and I'm fixing IM and SM's mainlines as well.
Attachment #8842496 - Attachment is obsolete: true
Attachment #8842583 - Flags: review+
Comment on attachment 8841718 [details] [diff] [review]
1332717-replace-nsStringGlue.patch

Why is in /im and /suite #include "nsStringGlue.h" changed to #include "nsString.h" but not in /mail?
(Assignee)

Comment 31

24 days ago
That's a mistake. All will be well when I'm landing it.
(Assignee)

Comment 32

24 days ago
https://hg.mozilla.org/comm-central/rev/e0b1fb0cdc2930373a2b694b913f33e62db1ff80
https://hg.mozilla.org/comm-central/rev/f0451b35ac4ab672c0f383f08c56d8fcc099ab00
https://hg.mozilla.org/comm-central/rev/417a5d45d130379d57a9b5502c1089e3a6f50d32
Status: ASSIGNED → RESOLVED
Last Resolved: 24 days ago
OS: Unspecified → All
Hardware: Unspecified → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
Version: unspecified → Trunk

Comment 33

21 days ago
Thanks for the cleanups.
Good that Neil does not have to watch this and get sad.
(Assignee)

Comment 34

21 days ago
Whereas ignorance is bliss. I don't even know what the external interface was. All I know is that it's gone now.

Comment 35

21 days ago
I also didn't use it.
But it should be related to external-linkage which had its benefits for some cases (like faster builds) and I know Neil was a fan of it and kept it alive when we broke something in c-c (so that it worked with internal but not external version).
(Assignee)

Comment 36

15 days ago
Created attachment 8846144 [details] [diff] [review]
1332717-missed-replacements.patch

Oops, I was checking DXR and I had missed some. Now they're fixed:
https://hg.mozilla.org/comm-central/rev/07b68e86a6928b6c33f98c4d4eb8692408a1d7d2
Attachment #8846144 - Flags: review+
You need to log in before you can comment on or make changes to this bug.