Closed
Bug 1332639
Opened 7 years ago
Closed 7 years ago
Remove the external string API
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
Attachments
(6 files)
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dminor
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emk
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
gsvelto
:
review+
|
Details |
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 This is currently blocked on some webrtc refactoring in bug 1332622
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Comment 7•7 years ago
|
||
Benjamin, you removed #include "nsStringGlue.h" from browser/app/nsBrowserApp.cpp. When I do the same in Thunderbird's "main" program, which is 99% the same, I get a compile error on Windwos: c:\mozilla-source\comm-central\mozilla\xpcom\build\BinaryPath.h(185): error C3861: 'nsDependentString': identifier not found coming from #ifdef XP_WIN rv = NS_NewLocalFile(nsDependentString(exePath), true, Have you compiled your patches on Windows? BTW, if I change the include to nsString.h I get: nsStringFwd.h(15): fatal error C1189: #error: Internal string headers are not available from external-linkage code.
Updated•7 years ago
|
Attachment #8841668 -
Flags: review?(l10n) → review?(mh+mozilla)
Comment 8•7 years ago
|
||
Forwarded the RDF bits from me to glandium, as that change is really build config, IMHO.
Assignee | ||
Comment 9•7 years ago
|
||
I have a windows try run going, but I did include a set of #ifdefs in BinaryPath.h that should cover this case.
Comment 10•7 years ago
|
||
I can't see your try run. Are you saying the BinaryPath.h in the try doesn't match your patch here?
Assignee | ||
Comment 11•7 years ago
|
||
The only NS_NewLocalFile in BinaryPath.h is now in a MOZILLA_INTERNAL_API block. So I guess you're missing something from the patch stack?
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8841672 [details] Bug 1332639 - Remove unneeded #include so that this file builds, https://reviewboard.mozilla.org/r/115830/#review117258 LGTM, I've double-checked the commit that introduced that inclusion (e96c297c8723) and it seems that it was already unused back then.
Attachment #8841672 -
Flags: review?(gsvelto) → review+
Comment 13•7 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #11) > The only NS_NewLocalFile in BinaryPath.h is now in a MOZILLA_INTERNAL_API > block. So I guess you're missing something from the patch stack? Thanks. I haven't applied your patches. I'm just preparing C-C by changing nsStringGlue.h to nsString.h. I hadn't noticed the newly added MOZILLA_INTERNAL_API in BinaryPath.h. So I guess I'll complete the TB work when this has landed.
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8841670 [details] Bug 1332639 - Remove the external unicharutil library which isn't used and is now unlinkable, https://reviewboard.mozilla.org/r/115826/#review117286 Bug 1114997 sucks, but OK.
Attachment #8841670 -
Flags: review?(VYV03354) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8841669 [details] Bug 1332639 - Stop building the media/mtransport/testlib library which isn't used, https://reviewboard.mozilla.org/r/115824/#review117386 lgtm
Attachment #8841669 -
Flags: review?(dminor) → review+
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8841667 [details] Bug 1332639 - Remove the external string API: nsStringAPI.h/cpp and nsEmbedString.h, https://reviewboard.mozilla.org/r/115820/#review117678 Note, it seems to me this should be last (or close to last) in the patch queue, not first. ::: netwerk/base/nsNetUtil.h:59 (Diff revision 1) > namespace mozilla { class OriginAttributes; } > > template <class> class nsCOMPtr; > template <typename> struct already_AddRefed; > > #ifdef MOZILLA_INTERNAL_API Without the external string API, the file obviously doesn't build without MOZILLA_INTERNAL_API anymore. Seems like the non MOZILLA_INTERNAL_API branch should be removed in a followup. ::: xpcom/glue/nsStringGlue.h:18 (Diff revision 1) > > #ifndef nsStringGlue_h__ > #define nsStringGlue_h__ > > -#ifdef MOZILLA_INTERNAL_API > +#ifndef MOZILLA_INTERNAL_API > +#error "Everything using XPCOM strings must be compiled with MOZILLA_INTERNAL_API." Maybe "Using XPCOM strings is limited to libxul" would be better than kind of implying that one can work around the limitation with a dubious define.
Attachment #8841667 -
Flags: review?(mh+mozilla) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8841668 [details] Bug 1332639 - Remove the RDF "standalone" library which isn't used, https://reviewboard.mozilla.org/r/115822/#review117684
Attachment #8841668 -
Flags: review?(mh+mozilla) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8841671 [details] Bug 1332639 - Fix nsBrowserApp.cpp and related headers to compile without the external string API, https://reviewboard.mozilla.org/r/115828/#review117692
Attachment #8841671 -
Flags: review?(mh+mozilla) → review+
Comment 19•7 years ago
|
||
Pushed by bsmedberg@mozilla.com: 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
Comment 20•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #17) > Comment on attachment 8841668 [details] > Bug 1332639 - Remove the RDF "standalone" library which isn't used, > > https://reviewboard.mozilla.org/r/115822/#review117684 Well, it's used in comm-central...
Assignee | ||
Comment 21•7 years ago
|
||
I don't know how it could be; it doesn't actually link without the string API which hasn't been built since we removed the XPCOM glue.
Comment 22•7 years ago
|
||
Right, the standalone API. I misinterpreted it as nsRDFResource was going away.
Assignee | ||
Comment 23•7 years ago
|
||
No it just lives in libxul.
Comment 24•7 years ago
|
||
(In reply to Magnus Melin from comment #20) > (In reply to Mike Hommey [:glandium] from comment #17) > > Comment on attachment 8841668 [details] > > Bug 1332639 - Remove the RDF "standalone" library which isn't used, > > > > https://reviewboard.mozilla.org/r/115822/#review117684 > > Well, it's used in comm-central... was. past tense. Removed in bug 1318966
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1bcada51f54b https://hg.mozilla.org/mozilla-central/rev/9825f34adc3d https://hg.mozilla.org/mozilla-central/rev/ab3684fe0488 https://hg.mozilla.org/mozilla-central/rev/e761b88de9de https://hg.mozilla.org/mozilla-central/rev/cae4a255be3f https://hg.mozilla.org/mozilla-central/rev/a8f45b2afbf3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 26•7 years ago
|
||
Is it deliberate that nsStringAPI.* are still present in the repo?
Flags: needinfo?(benjamin)
Comment 27•7 years ago
|
||
I noticed that nsString.h includes nsReadableUtils.h, so it's not required in nsReadableUtils.h.
Assignee | ||
Comment 28•7 years ago
|
||
dmajor no! I thought I had hg rm'ed it but apparently not, I'll do that.
Flags: needinfo?(benjamin)
Comment 29•7 years ago
|
||
Pushed by bsmedberg@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d60dce18350a followup. I apparently didn't actually remove the nsStringAPI.* files, but that was the intention, r=oops
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d60dce18350a
You need to log in
before you can comment on or make changes to this bug.
Description
•