315087 - Convert necko tests to use the glue

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Benjamin Smedberg]

In preparation for stopping exporting nonfrozen functions from libxul, I'm going through the tree and fixing the various tests that currently use the internal linkage to use frozen linkage.

Comment 1

[Benjamin Smedberg]

Attachment: necko tests use xpcom glue

Some of the tests use PL_InitEvent, which is not an exported function, which is why they are not built when MOZ_ENABLE_LIBXUL is set.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

> Some of the tests use PL_InitEvent, which is not an exported function

maybe it should be... that function is required for using nsIEventTarget::PostEvent.

although, it'd be also nice if there was a scriptable way to post events.

Comment 3

[Darin Fisher]

I think we should support posting nsIRunnable instances to nsIEventTarget's.

Comment 4

[Darin Fisher]

Comment on attachment 201864 [details] [diff] [review]
necko tests use xpcom glue

>Index: netwerk/base/public/nsNetUtil.h

>+#ifdef MOZILLA_INTERNAL_API
> inline nsresult
> NS_ExamineForProxy(const char    *scheme,
>                    const char    *host,
>                    PRInt32        port, 
>                    nsIProxyInfo **proxyInfo)
> {
>     nsresult rv;
>     static NS_DEFINE_CID(kPPSServiceCID, NS_PROTOCOLPROXYSERVICE_CID);
>@@ -671,16 +673,17 @@ NS_ExamineForProxy(const char    *scheme
>         if (NS_SUCCEEDED(rv)) {
>             rv = uri->SetSpec(spec);
>             if (NS_SUCCEEDED(rv))
>                 rv = pps->Resolve(uri, 0, proxyInfo);
>         }
>     }
>     return rv;
> }
>+#endif

This function is currently used by mailnews FWIW, so you probably
do want to implement this for XPCOM glue users eventually.


>+#ifdef MOZILLA_INTERNAL_API
>     return NS_NewCStringInputStream(result, data);
>+#else
>+    nsCOMPtr<nsIStringInputStream> stream
>+        (do_CreateInstance("@mozilla.org/io/string-input-stream;1", &rv));
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    rv = stream->SetData(PromiseFlatCString(data).get(), data.Length());
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    NS_ADDREF(*result = stream);
>+    return NS_OK;
>+#endif
> }

NS_NewPostDataStream isn't used by FF or TB, and I want to just
remove it at some point.  So, I'd be fine with using the CI path
in all cases.

NOTE: You should use data.BeginReading() instead of calling
PromiseFlatCString.  That'd be more efficient.


>Index: netwerk/streamconv/test/Converters.cpp

>     buf[read] = '\0';
...
>+    rv = str->SetData(buf, strlen(buf));

The length of |buf| is known... it's |read| :)


>Index: netwerk/streamconv/test/TestStreamConv.cpp

> nsresult SendData(const char * aData, nsIStreamListener* aListener, nsIRequest* request) {

>-    nsCOMPtr<nsIInputStream> dataStream;
>-    nsresult rv = NS_NewCharInputStream(getter_AddRefs(dataStream), aData);
>-    if (NS_FAILED(rv)) return rv;

Perhaps you should just implement these for glue users.


>Index: netwerk/test/Makefile.in

>+ifndef MOZ_ENABLE_LIBXUL
>+CPPSRCS += \
>+		TestPerf.cpp \
>+		TestIDN.cpp \
>+		TestSocketTransport.cpp \
>+		TestStreamTransport.cpp \
>+		TestStreamChannel.cpp \
>+		TestStreamPump.cpp \
>+		TestProtocols.cpp \
>+		TestIOThreads.cpp \
>+		$(NULL)
>+endif

You've killed off most of the useful tests :)


>Index: netwerk/test/TestBlockingSocket.cpp

>-#include "nsString.h"
>+#include "nsStringAPI.h"

Why not nsStringGlue.h?  Are these no longer able to be built
with MOZILLA_INTERNAL_API defined?


>Index: netwerk/test/TestDNS.cpp

>         for (int j=0; j<2; ++j) {
>             for (int i=1; i<argc; ++i) {
>                 // assume non-ASCII input is given in the native charset 
>                 nsCAutoString hostBuf;
>-                if (nsCRT::IsAscii(argv[i]))
>+                PRBool isAscii = PR_TRUE;
>+                for (char *s = argv[i]; *s; ++s) {
>+                  if (*s & 0x80)
>+                    isAscii = PR_FALSE;
>+                }

Indentation!  Write a static helper function instead please?

Comment 5

[Benjamin Smedberg]

> > NS_ExamineForProxy(const char    *scheme,

> This function is currently used by mailnews FWIW, so you probably
> do want to implement this for XPCOM glue users eventually.

Yeah, but AppendInt is hard.

> Perhaps you should just implement these for glue users.

I'd prefer to stabilize nsIByteArrayInputStream or nsIStringInputStream first... bytearray can only be gotten through the NS_NewByteArrayInputStream symbol, and nsIStringInputStream should really be using a byte-array, not a string in the IDL.

> You've killed off most of the useful tests :)

Yes, I'll file a followup bug about a scriptable (nsIRunnable) version of PL_InitEvent.

> Why not nsStringGlue.h?  Are these no longer able to be built
> with MOZILLA_INTERNAL_API defined?

There's no need to allow both ways... I was just switching them over completely.

I'll have a new patch Tuesday/Wednesday, need to do the nsStringAPI-compatibility-header-thing first.

Comment 6

[Darin Fisher]

> > > NS_ExamineForProxy(const char    *scheme,
> 
> > This function is currently used by mailnews FWIW, so you probably
> > do want to implement this for XPCOM glue users eventually.
> 
> Yeah, but AppendInt is hard.

I was thinking about having it call snprintf, but yeah... we can punt on it.


> > Perhaps you should just implement these for glue users.
> 
> I'd prefer to stabilize nsIByteArrayInputStream or nsIStringInputStream
> first... bytearray can only be gotten through the NS_NewByteArrayInputStream
> symbol, and nsIStringInputStream should really be using a byte-array, not a
> string in the IDL.

I think it should be implemented as a ACString.  That's nicer because it allows us to have a single parameter from JS, it supports intermittent null bytes, and it is more efficient than a byte array in JS.  nsIScriptableInputStream should really have a Read method that returns a ACString instead of a string as well.  That combined with nsIScriptableUnicodeConverter makes a good solution for JS stream consumers.


> > You've killed off most of the useful tests :)
> 
> Yes, I'll file a followup bug about a scriptable (nsIRunnable) version of
> PL_InitEvent.

We should discuss the design of this.  The current nsIEventTarget is nice from C++ because you can inherit from PLEvent, which has the property of being both a "nsIRunnable"-like thingy and a "list-node" thingy.  Given only a nsIRunnable, we will be forced to allocate a "list-node" thingy for inserting it into the event queue.  That means that it is not a good idea to replace all uses of PLEvent with nsIRunnable IMO.  So, nsIScriptableEventTarget might be a reasonable solution.  However, I don't like that name either since we are also designing an API that would be good for C++ extension authors to use.  Perhaps nsIEventTarget should have a second method, but that's still ugly because nsIEventTarget exposes PLEvent.


> > Why not nsStringGlue.h?  Are these no longer able to be built
> > with MOZILLA_INTERNAL_API defined?
> 
> There's no need to allow both ways... I was just switching them over
> completely.

ok

Comment 7

[Benjamin Smedberg]

PL_InitEvent stuff is covered in bug 315442.

Comment 8

[Benjamin Smedberg]

Attachment: necko tests use the xpcom glue, rev. 2

Comment 9

[Darin Fisher]

Comment on attachment 202254 [details] [diff] [review]
necko tests use the xpcom glue, rev. 2

>Index: netwerk/test/TestProtocols.cpp

>+static void
>+StripChar(nsCString& buffer, char c)
>+{
>+    const char *b;
>+    PRUint32 len = NS_CStringGetData(buffer, &b) - 1;
>+
>+    for (; len > 0; --len) {
>+        if (b[len] == c)
>+            buffer.Cut(len, 1);
>+    }
>+}

After cutting the string, you have to call NS_CStringGetData
again to retrieve the possibly new buffer pointer.  You might
be able to use NS_CStringGetMutableData in a clever way to
avoid this, but it's probably not worth it.


r=darin with that fix

Comment 10

[Benjamin Smedberg]

Fixed with the TestProtocols cleanup.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 202254 [details] [diff] [review]
necko tests use the xpcom glue, rev. 2

-                fileBuffer.Left(urlString, offset);
+                urlString = Substring(fileBuffer, offset);

Er, are those really equivalent? Doesn't .Left get the leftmost offset characters, while substring gets the characters starting at 17, until the end of the   string?

Comment 12

[Darin Fisher]

biesi: i think you are right.  it should be Substring(fileBuffer, 0, offset)

Comment 13

[Darin Fisher]

FYI, I checked in a fix for TestProtocols.

Comment 14

[Boris Zbarsky [:bzbarsky]]

This broke TestCookie (bug 327163) because it changed the string type to one that doesn't return null on .get() without changing the surrounding test...