Closed
Bug 311735
Opened 20 years ago
Closed 20 years ago
when --disable-libxul, build a little xul.dll with the proper exports
Categories
(Toolkit Graveyard :: XULRunner, defect, P2)
Toolkit Graveyard
XULRunner
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
Attachments
(2 files, 2 obsolete files)
33.10 KB,
patch
|
darin.moz
:
first-review+
|
Details | Diff | Splinter Review |
630 bytes,
patch
|
benjamin
:
first-review+
|
Details | Diff | Splinter Review |
When we're building a "developer" build of libxul with --disable-libxul (to
reduce the static-linking pain), we should still create a xul.dll with the XRE_*
exports. This will also significantly simplify some of the pain of the
toolkit/xre -> browser/app static/dynamic/libxul ifdefs.
Assignee | ||
Updated•20 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha1
Assignee | ||
Comment 1•20 years ago
|
||
This patch builds a little libxul with firefox dynamic and with xulrunner --disable-libxul. I couldn't get it to work with Firefox static but that's ok. It also gets rid of some copy/pasted code in xulrunner/app which was for xulrunner static builds, which is not something it makes sense to support.
Attachment #203170 -
Flags: first-review?(darin)
Comment 2•20 years ago
|
||
Comment on attachment 203170 [details] [diff] [review]
Build a little libxul with ffox dynamic and xulrunner --disable-libxul
>Index: xpcom/base/nscore.h
>+/**
>+ * Import/export macros for libXUL APIs.
>+ */
>+#ifdef XPCOM_GLUE
>+#define XUL_API(type, name, params) type NS_FROZENCALL (*name) params
>+#elif defined(IMPL_XULAPI)
>+#define XUL_API(type, name, params) EXPORT_XPCOM_API(type) name params
>+#else
>+#define XUL_API(type, name, params) IMPORT_XPCOM_API(type) name params
>+#endif
Do these have to be here? Can they instead be in the header file
where the LibXUL exports are defined? Or in some common LibXUL
header file? XPCOM is not XUL just as it is not GFX. We don't
define the GFX utility library's similar macros in nscore.h, so
we shouldn't define LibXUL's there either IMO ;-)
>Index: toolkit/xre/nsXULAppAPI.h
>+XUL_API(int,
>+ XRE_main, (int argc, char* argv[], const nsXREAppData* sAppData));
Hrm... the macro'ization of this is not very pretty, but I guess you
are doing it so that you can turn these into global function pointers
when XPCOM_GLUE is defined. Hrm... we don't do this for XPCOM. Are
you planning similar changes to XPCOM?
I also don't see these function pointers being assigned values anywhere.
Is this a work in progress?
All of these symbols are prefixed with "XRE_"... why not "XRE_API" then?
Or, why not change the symbol names to "NS_" to reduce acronyms and unify
our frozen API namespace? We have the chance to define this API from the
start, so why not make it consistent in style?
Assignee | ||
Comment 3•20 years ago
|
||
> Do these have to be here? Can they instead be in the header file
> where the LibXUL exports are defined? Or in some common LibXUL
> header file? XPCOM is not XUL just as it is not GFX. We don't
> define the GFX utility library's similar macros in nscore.h, so
> we shouldn't define LibXUL's there either IMO ;-)
Down with XPCOM! ok, I'll make a xulcore.h somewhere.
> Hrm... the macro'ization of this is not very pretty, but I guess you
> are doing it so that you can turn these into global function pointers
> when XPCOM_GLUE is defined. Hrm... we don't do this for XPCOM. Are
> you planning similar changes to XPCOM?
Yes, these are function pointers when XPCOM_GLUE is defined, and they will be loaded by the function in bug 316098. (extra glue to do this easily not yet written, but it's not necessary for this patch)
> All of these symbols are prefixed with "XRE_"... why not "XRE_API" then?
> Or, why not change the symbol names to "NS_" to reduce acronyms and unify
> our frozen API namespace? We have the chance to define this API from the
> start, so why not make it consistent in style?
I'd like to have the libxul APIs with a different prefix than the NS_ APIs which are exported from xpcom.dll, and in fact I'd like to make the Current NS_*_P symbols use the same prefix (NS_InitXPCOM3 from xpcom.dll, and XRE_InitXPCOM3 from xul.dll). I can make it XRE_API instead of XUL_API if that's preferable, or change the name of the exports to XUL_main.
Comment 4•20 years ago
|
||
Alright... I guess I'm OK with "XRE_" for all of the symbols exported from XUL.dll, and XRE_API sounds like it would be more consistent with that. I think it is unfortunate for newcomers to have to deal with "PR_", "NS_", and now "XRE_", but OK :-/
Assignee | ||
Comment 5•20 years ago
|
||
This has xrecore.h, XRE_API, and this shows how winembed (and other future standalone-glue embedders) should use the function-pointer declarations to get at the libxul symbols, modulo the XP loading code in bug 316098.
Attachment #203170 -
Attachment is obsolete: true
Attachment #203292 -
Flags: first-review?(darin)
Attachment #203170 -
Flags: first-review?(darin)
Comment 6•20 years ago
|
||
why not name the file "xre.h" rather than "xrecore.h"?
Assignee | ||
Comment 7•20 years ago
|
||
It's parallel to "nscore.h" and "prtypes.h"... we may want an overarching "XRE.h" header which includes a bunch of subheaders at some point.
Comment 8•20 years ago
|
||
Comment on attachment 203292 [details] [diff] [review]
Build a little libxul with ffox dynamic and xulrunner --disable-libxul, rev. 2
r=darin
Attachment #203292 -
Flags: first-review?(darin) → first-review+
Assignee | ||
Comment 9•20 years ago
|
||
Fixed on trunk, with a couple bustage fixes (-lXUL doesn't work on mac, you have to specify the file explicitly).
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 10•20 years ago
|
||
Broke build with following :-
In file included from e:/mozilla/source/mozilla/embedding/tests/winEmbed/winEmbe
d.cpp:46:
../../../dist/include/xulapp/nsXULAppAPI.h:182: error: ISO C++ forbids declarati
on of `XRE_mainType' with no type
../../../dist/include/xulapp/nsXULAppAPI.h:189: error: ISO C++ forbids declarati
on of `XRE_GetFileFromPathType' with no type
../../../dist/include/xulapp/nsXULAppAPI.h:198: error: ISO C++ forbids declarati
on of `XRE_GetBinaryPathType' with no type
../../../dist/include/xulapp/nsXULAppAPI.h:204: error: ISO C++ forbids declarati
on of `XRE_GetStaticComponentsType' with no type
../../../dist/include/xulapp/nsXULAppAPI.h:233: error: ISO C++ forbids declarati
on of `XRE_InitEmbeddingType' with no type
../../../dist/include/xulapp/nsXULAppAPI.h:240: error: ISO C++ forbids declarati
on of `XRE_TermEmbeddingType' with no type
I think some extra commas need to be sorted out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•20 years ago
|
||
David, what build environment are you using? I have built this patch successfully.
Comment 12•20 years ago
|
||
MingW/Cygwin/Win32
However, the error seems to be indicating that INVALID C++ code is being used. I'm rather concerned that your compiler didn't notice this invalid code.
Assignee | ||
Comment 13•20 years ago
|
||
Could you point out *how* it's invalid c++?
http://lxr.mozilla.org/seamonkey/source/toolkit/xre/xrecore.h#48
Comment 14•20 years ago
|
||
I think you are looking at the wrong file.
http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsXULAppAPI.h#182
Line 183 has the undefined use of XRE_main. And I really don't understand the logic of int,XRE_main, ?
Assignee | ||
Comment 15•20 years ago
|
||
David, the link I pointed to defines the XRE_API macro which is used to either declare this as a function (imported/exported) or as a function pointer (for use in the standalone situation).
Comment 16•20 years ago
|
||
I'm not up to speed with macros in c++.
All I can tell you is that gcc 3.4.4 in complaining about invalid code due to declarations with no type.
Comment 17•20 years ago
|
||
Yes, this broke building on MingW, attaching the errors I get.
Comment 18•20 years ago
|
||
Attachment #204300 -
Attachment is obsolete: true
Attachment #204335 -
Flags: first-review?(benjamin)
Assignee | ||
Comment 19•20 years ago
|
||
Comment on attachment 204335 [details] [diff] [review]
Add missing type declaration
Doh.
Attachment #204335 -
Flags: first-review?(benjamin) → first-review+
Comment 20•20 years ago
|
||
The mingw fix has been checked in.
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Comment 21•19 years ago
|
||
*** Bug 311055 has been marked as a duplicate of this bug. ***
Updated•9 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•