Stop exporting nonfrozen XPCOM functions

RESOLVED FIXED

Status

()

defect
P3
normal
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

(Depends on 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

14 years ago
Part of the libxul plan to improve performance and reduce embedding headaches is
to stop exporting the nonfrozen symbols. This bug is about the XPCOM symbols.
(Assignee)

Updated

14 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

14 years ago
Depends on: 306615
(Assignee)

Updated

14 years ago
Depends on: 315087
(Assignee)

Updated

14 years ago
Depends on: 315089
(Assignee)

Updated

14 years ago
Depends on: 315090
(Assignee)

Comment 1

14 years ago
This uses a new macro for the frozen XPCOM API, and keeps NS_COM for internal symbols. It doesn't actually stop exporting the NS_COM symbols from libxul yet, pending tree fixup (see blocking bugs).
Attachment #201843 - Flags: review?(darin)

Comment 2

14 years ago
Comment on attachment 201843 [details] [diff] [review]
Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 1

>+#  Can't build internal xptcall tests that use symbols which are not exported.
>+ifndef MOZ_ENABLE_LIBXUL
>+TOOL_DIRS += \
> 	reflect/xptinfo/tests \
> 	reflect/xptcall/tests \
>-	proxy/tests
>+	$(NULL)
>+endif
> endif

We need to figure out how to expose xptcall symbols like
XPTC_InokeByIndex because it is needed to do things like
implement python and java bindings.


>Index: xpcom/string/public/nsStringAPI.h

>+inline char*
>+ToNewUTF8String(const nsAString& aSource)
>+{
>+  nsCString temp;
>+  CopyUTF16toUTF8(aSource, temp);
>+  return NS_CStringCloneData(temp);
>+}

So, we have to decide whether nsStringAPI.h should be an
attempt to implement an API-compatible equivalent to our
internal string API, or if it should be a clean API ;-)
I vote for a cleaner API.  Can we please avoid bringing
in baggage from the internal string API?  Maybe move this
function and others like it into a separate compat header
file?


>+#define PromiseFlatString nsString
>+#define PromiseFlatCString nsCString
>+
>+inline char*
>+ToNewCString(const nsACString& aStr)
>+{
>+  return NS_CStringCloneData(aStr);
>+}

ditto for these...


>+typedef nsCString nsCAutoString;
>+typedef nsString nsAutoString;
>+
>+#define nsXPIDLString nsString
>+#define nsXPIDLCString nsCString

and these...

why not use typedef for nsXPIDLString?


>Index: xpcom/tests/TestMinStringAPI.cpp

>     NS_StringSetData(s, kUnicodeData, PR_UINT32_MAX);
>     len = NS_StringGetData(s, &ptr);
>+    if (len != sizeof(kUnicodeData)/2 - 1)
>       {
>         NS_ERROR("unexpected result");
>         return PR_FALSE;
>       }
>+    if (ptr == nsnull || memcmp(ptr, kUnicodeData, sizeof(kUnicodeData)/2 - 1) != 0)

This looks wrong.  If you are using memcmp, then you are comparing
bytes, so you should pass a length value that expresses the number
of bytes.  I think that sizeof(kUnicodeData) is correct because it
also includes the null terminator, which we can assert exists.


>+    if (ptr == nsnull || memcmp(ptr, kUnicodeData, sizeof(kUnicodeData)/2 - 1) != 0)

ditto.


>-    nsMemory::Free(clone);
>+    NS_Free(clone);

Is nsMemory::Free no longer part of the glue?
Attachment #201843 - Flags: review?(darin) → review-
(Assignee)

Comment 3

14 years ago
> We need to figure out how to expose xptcall symbols like
> XPTC_InokeByIndex because it is needed to do things like
> implement python and java bindings.

Are we going to freeze them? I don't mind freezing XPTC_InvokeByIndex as-is, but nsXPTCStubBase/nsXPTCVariant should probably be in the glue since there's no good way to freeze it as it depends on the C++ ABI.

> Is nsMemory::Free no longer part of the glue?

It is, but it inlines directly to NS_Free

Comment 4

14 years ago
> Are we going to freeze them? I don't mind freezing XPTC_InvokeByIndex as-is,
> but nsXPTCStubBase/nsXPTCVariant should probably be in the glue since there's
> no good way to freeze it as it depends on the C++ ABI.

Last time I asked about this, shaver said that these should not be frozen as-is.  I don't know the issues, but I do care about supporting language bindings via extensions, so we need to figure out what we want to do.


> > Is nsMemory::Free no longer part of the glue?
> 
> It is, but it inlines directly to NS_Free

OK, that's what I figured.  You were just changing the code to use NS_Free for aesthetic reasons then ;-)
(Assignee)

Comment 5

14 years ago
Shaver, can we just move all of xptcall into the glue (actually probably its own glue-like library)?
(Assignee)

Updated

14 years ago
Depends on: 315401
(Assignee)

Comment 6

14 years ago
After discussing with darin, we are going to leave a lot of the CopyUTF8toUTF16 glue in nsStringAPI.h, and mark certain things (nsXPIDLString and nsAutoString) as deprecated. This patch also happens to include bits of bug 315401.
Attachment #201843 - Attachment is obsolete: true
Attachment #202133 - Flags: review?(darin)

Comment 7

14 years ago
Comment on attachment 202133 [details] [diff] [review]
Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2

>Index: xpcom/string/public/nsStringAPI.h

>+#define nsXPIDLString                  nsXPIDLString_external
>+#define nsXPIDLCString                 nsXPIDLCString_external

Since these are deprecated, let's move them down to where
nsXPIDLString is defined.


>+inline char*
>+ToNewCString(const nsACString& aStr)
>+{
>+  return NS_CStringCloneData(aStr);
>+}
>+
>+inline PRUnichar*
>+ToNewUnicode(const nsAString& aStr)
>+{
>+  return NS_StringCloneData(aStr);
>+}

I think these should be in the deprecated section since the names of these
functions are terrible.  "ToNewCString"... sounds like "new nsCString" but
it isn't.  "ToNewUnicode"... unicode... what?!?


>+/**
>+ * The following declarations are *deprecated*, and are included here only
>+ * to make porting from existing code that doesn't use the frozen string API
>+ * easier. They may disappear in the future.
>  */
> 
>+class nsXPIDLString : public nsString
>+{
>+public:
>+  operator const PRUnichar*() { return get(); }
>+};
>+
>+class nsXPIDLCString : public nsCString
>+{
>+public:
>+  operator const char*() { return get(); }
>+};

If you are going to support these, what about supporting the NULL return
value from .get() and the cast for nsXPIDLString?  That would seem to be
a very important part of nsXPIDLString that is not being supported here.


> #define EmptyCString() nsCString()
> #define EmptyString() nsString()

EmptyCString/EmptyString are not deprecated.  please move them
out of this section.
(Assignee)

Comment 8

14 years ago
> >+class nsXPIDLString : public nsString

> >+  operator const PRUnichar*() { return get(); }

> If you are going to support these, what about supporting the NULL return
> value from .get() and the cast for nsXPIDLString?  That would seem to be
> a very important part of nsXPIDLString that is not being supported here.

I don't know what you mean, especially WRT to casting. I will try removing these casts and see whether my existing code compiles, until I can figure out what you're talking about ;-)

Comment 9

14 years ago
> > If you are going to support these, what about supporting the NULL return
> > value from .get() and the cast for nsXPIDLString?  That would seem to be
> > a very important part of nsXPIDLString that is not being supported here.
> 
> I don't know what you mean, especially WRT to casting. I will try removing
> these casts and see whether my existing code compiles, until I can figure out
> what you're talking about ;-)

Take this code for example:

  {
    nsXPIDLString s;
    foo->GetBar(getter_Copies(s));
    if (s) {
      // do stuff with |s|
    }
  }

Notice that this code depends on "nsXPIDLString::operator const PRUnichar*()" evaluating to null when GetBar returns a null out param.  nsString::get() never returns null, so nsString is not a substitute for nsXPIDLString in the above sample code.  This is a very common use-case for nsXPIDLString, so I suspect that you must have encountered code that is depending on this.
(Assignee)

Updated

14 years ago
Depends on: 315562
(Assignee)

Comment 10

14 years ago
OK, I didn't need the xpidlstring stuff... Since this doesn't flip the NS_COM switch yet, I'd to land this as-is and then look at how to deal with the xptcall stuff.
Attachment #202133 - Attachment is obsolete: true
Attachment #202253 - Flags: review?(darin)
Attachment #202133 - Flags: review?(darin)
(Assignee)

Updated

14 years ago
Depends on: 315563

Comment 11

14 years ago
Comment on attachment 202253 [details] [diff] [review]
Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2.1 [checked in]

r=darin with the typedefs for nsXPIDLC?String removed from nsStringAPI.h -- it's just really worrisome to have those there if they do not have the "null" property that I mentioned previously.  they are likely to be misused.
Attachment #202253 - Flags: review?(darin) → review+
(Assignee)

Comment 12

14 years ago
Comment on attachment 202253 [details] [diff] [review]
Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2.1 [checked in]

Attachment 202253 [details] [diff] checked in with xpidlc?string removed and CopyUTF16toASCII renamed with a Lossy to match the nonfrozen version.
Attachment #202253 - Attachment description: Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2.1 → Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2.1 [checked in]
(Assignee)

Updated

14 years ago
Depends on: 315584
(Assignee)

Updated

14 years ago
Depends on: 315601
(Assignee)

Updated

14 years ago
Depends on: 315890
(Assignee)

Updated

14 years ago
Depends on: 316778
(Assignee)

Updated

14 years ago
Depends on: 319997
(Assignee)

Updated

14 years ago
Depends on: 320002
(Assignee)

Updated

14 years ago
Depends on: 320148
(Assignee)

Updated

14 years ago
Depends on: 320540
(Assignee)

Updated

14 years ago
Depends on: 320553
(Assignee)

Updated

14 years ago
Depends on: 320820
(Assignee)

Updated

14 years ago
Depends on: 320972
(Assignee)

Updated

14 years ago
Depends on: 320988
(Assignee)

Updated

13 years ago
Depends on: 331476
(Assignee)

Updated

13 years ago
Depends on: 332045
(Assignee)

Updated

13 years ago
Depends on: 332115
(Assignee)

Updated

13 years ago
Attachment #216559 - Attachment description: Stop exporting GFX symbols, rev. 1 → Stop exporting GFX symbols, rev. 1 [checked in]
(Assignee)

Comment 14

13 years ago
I'm thinking that we never want to override IMETHOD_VISIBILITY to NS_VISIBILITY_DEFAULT, always preferring to define it to empty and use the class-scope visibility declaration.
Attachment #218722 - Flags: review?(darin)

Comment 15

13 years ago
> I'm thinking that we never want to override IMETHOD_VISIBILITY to
> NS_VISIBILITY_DEFAULT, always preferring to define it to empty and use the
> class-scope visibility declaration.

And that works properly on Linux?  NS_COM is empty on Linux, no?

Comment 17

13 years ago
Comment on attachment 218722 [details] [diff] [review]
Various XPCOM macros, rev. 1 [checked in]

ok, but i think bryner should review this.  i thought that older compilers lacked support for specifying visibility at the class level.
Attachment #218722 - Flags: superreview?(bryner)
Attachment #218722 - Flags: review?(darin)
Attachment #218722 - Flags: review+
(Assignee)

Comment 18

13 years ago
They do, but on those compilers we don't use pragmas nor -fvisibility=hidden, so the default visibility is fine.
Comment on attachment 218722 [details] [diff] [review]
Various XPCOM macros, rev. 1 [checked in]

I wonder if we should change the way the visibility macros work so that it's not necessary to duplicate this somewhat non-intuitive #defining.  In any case, sr=me.
Attachment #218722 - Flags: superreview?(bryner) → superreview+
(Assignee)

Updated

13 years ago
Attachment #218722 - Attachment description: Various XPCOM macros, rev. 1 → Various XPCOM macros, rev. 1 [checked in]
(Assignee)

Updated

13 years ago
Depends on: 335248
(Assignee)

Updated

13 years ago
Depends on: 337731
(Assignee)

Comment 20

13 years ago
The widget classes are not NS_EXPORT or called across dll boundaries, no need to override the default visibility. The NSReg symbols are all statically linked, no need for dllexport mojo. The OJI symbol just needed visibility love.
Attachment #222874 - Flags: review?(bryner)
Attachment #222874 - Flags: review?(bryner) → review+
(Assignee)

Updated

13 years ago
Attachment #222874 - Attachment description: No need to export widgetshared or mozreg symbols, rev. 1 → No need to export widgetshared or mozreg symbols, rev. 1 [checked in]
(Assignee)

Updated

13 years ago
Depends on: 341385
Blocks: 347727
Blocks: 347731
Blocks: 347735
(Assignee)

Updated

13 years ago
Depends on: 348234
(Assignee)

Updated

12 years ago
Priority: P2 → P3
Target Milestone: mozilla1.9alpha1 → ---
Depends on: 393142
(Assignee)

Comment 21

11 years ago
This is done well enough to call it fixed.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.