In MS scripthosts, there is a VARIANT type which is treated specially by the scripting engine. Basically, it permits the caller in the script to pass an arbitrary type for a VARIANT parameter to a method. The engine then creates a VARIANT structure with the type and contents of the parameter set appropriately, and passes that to the method. For porting existing COM/scripthost stuff to XPCOM/JS/XPConnect, some sort of equivalent functionality is required if the interface is to be kept as similar as possible. I've got a set of patches which implements basic variants in XPCOM and XPConnect as follows: 1) a new nsIVariant interface in xpcom/ds/nsIVariant.idl 2) a nsVariant class placed in xpcom/ds/nsVariant.* 3) update Unix and Windows makefiles appropriately 4) registration for nsVariant in xpcom/build/nsXPComInit.cpp 5) modifications to js/src/xpconnect/src/xpcconvert.cpp which work as follows: a) [in] - if type is nsIVariant, create a nsVariant object and insert the parameter type and value (after conversion from JS type), and pass that to the method b) [out] - if retval is nsIVariant, extract the type and value, and return the associated JS value The nice thing about this approach is that there is nothing special about nsIVariant on the XPCOM side - it's merely another class. This avoids modifications to XPIDL et al. Thus, modifications are localized to 2 places in the JS <-> XPCOM conversion routines in XPConnect. The nsIVariant I've done to date only has support for some types (such as int, double, string, object, null), but can be easily extended. Also, I've tried to make the nsVariant class check everything strictly, so you can't (for example) try to read out an object as a string; thus, the semantics are a bit different from MS variants. In addition, I've not added methods for type coercion, although again that could easily be done. Patches to be attached momentarily. //Mark

OK, I'm softening up on the variant issue. This basically looks good. There are details... This needs to be further fleshed out and the type names are going to have to change (VT_R8?). XPConnect is going to need an implementation of the class - there is no way that it is going to go through the component manager to create variant objects in order to make method calls. There is also no need to further replicate so much type conversion code. Like XPC_JSArgumentFormatter this code could use XPCConvert::JSData2Native and XPCConvert::NativeData2JS to do the bulk of the conversion work. I'm also not very happy with the use of (essentially) JS typeof in determining the type of the resultant value of the variant. This puts the burden on the JS author to use the right type in a loosly typed language, and the burden on the C++ callee to deal with unexpected types. I realize that there is precedence for this; e.g. the overloaded '+' operator. But, I think we can do better. One of the schemes previouly suggested would allow nsIVariant to defer the data conversion. The C++ code could then express a preference (or hint) for what type of data the variant would supply. There are various ways to make this work. Some are pretty complicated. But, I think that the conversions need not be completely flexible and the scope of how long type commitment can be deferred can be limited. We could use... short GetType(in short hint); ... and make no promises that you get what you want. xpconnect's impl of the interface can leave the conversion undone until asked or until it needs to relenquish its hold on the data to be converted. It can then commit to some type and let go of the underlying data reference. An implementation of the nsIVariant for C++ -> C++ calls might commit to the type at init time and "short GetType(in short hint);" would simply ignore the hint. I argued against all this before. But, if we are going to do this then I think that we should allow for the flexibility. Some additional scripting specific xpcom/xpidl issues have come up recently. I'll post a set of proposals to the xpcom newsgroup.

mass reassign of xpconnect bugs to dbradley@netscape.com

Removing Rob as QA contact

Taking this bug back. I'm working on this.

If you are working on this, maybe you should change the target milestone? ;) nsIXMLHttpRequest could use this as well in two places, at least. The parameter to send() can be document, string or stream. Right now it is impossible to pass JS string to the function, though. Hopefully variant will provide a way. Also, responseBody data member might be variant (need object to hold binary data and contain information how long it is).

Ok. Time to surface this. I'll attach an unfinished patch. It still needs: - a type conversion matrix test. - fleshing out of the type conversions. - implmentation of the array support. The scheme I have here is not exactly what I started out planning to do. Discussion with rayw about how he would use it in his low-level SOAP support influenced the plan significantly. The basic idea is an immutable nsIVariant interface with a method to get the type of the contained data and accessors that clients can call to get the data as converted to any of a range of types. The *minimum* supported type conversions will be specified as part of the interface contract. Conversions will either succeed or will fail with an error code. The available types match the xptcall data types (with the addition of an 'empty' type - to indicate an empty container). The objects implementing nsIVariant can optionally implement nsIWritableVariant. This interface adds 'SetAs' methods for each of the supported types. The interface also has a read/write 'writable' attribute. This allows for 'sealing' the container contents. Clients can call GetWritable() to determing if the object is mutable and they can call SetWritable(PR_FALSE) to make it immutable. This allows the default (generic) nsIVariant implementation in xpcom/ds to support clients that want mutable *and* immutable objects without also having more than one implementation or some fancy factory to make the things. Note that the COM rules preclude having an object implment interfaces A and B at one point in time and only A at a later time. So, shaving off the writable interface before handing the object to consumers was not an option. I was not originally excited about defining and supporting a range of data conversions in the core implementation. But, assuming this is used widely, it will free many clients of the code from duplicating a bunch of logic. I've implmented this by defining a public struct type containing unions (nsDiscriminatedUnion) that is capable of holding any of the supported types and a PRUint16 that identifies the type contained. There are also reusable public *static* methods of nsVariant (the xpcom default nsIVariant impl class) that do the various data conversions, data setting, and data cleanup in terms of a passed in nsDiscriminatedUnion reference. This is intended to allow other implmentors of the nsIVariant interface to leverage the core code without being required to use C++ inheritance or sneek under the covers of any other xpcom object. In addition to the generic xpcom nsIVariant implementation, for JS I have a minimal xpconnect implmentation and support for wrapping and unwrapped arbitrary data types passed via parameters declared as nsIVariant. I have this basically working. As I wrote above, this is not quite finished. The next step is a JS driven test suite to drive moving data across the language boundary and multiple hops between xpconnect and xpcom nsIVariant objects to test the range of data conversions. This will force me to define and write the various value-based conversion routines. Anyway, I'm throwing this out for comments. Does this look reasonable? Are you ActiveState guys OK with implmenting this for your languages?

This all looks good to me. I am not sure I see the whole point of the nsIWritableVariant (as opposed to having nsIVariant simply fail on the Set methods when immutable), but I am sure it will become clearer as I see code. It should be quite trivial to hook this support into Python, which I will do as soon as some patches become available. I have seen the lack of a Variant as a minor weakness in xpcom, so I am very happy to see this being addressed - good stuff!

MarkH: I'm glad to hear this scheme seems OK to you. My reason for spliting off nsIWritabableVariant was to allow for nsIVariant implementations that don't want to implement *any* of the Set methods. For instance... XPCVariant implements only nsIVariant. As I see things, XPCVariants only need to be built by the xpconnect internals and don't need to be mutable. I didn't want to have to stub out a bunch of methods when there was a clean way to do an interface split.

retargeting

OK. I'm backing down on my rayw-argument-induced stance on data conversion. He had me convinced that data conversions calls to this core code should succeed or fail based on the values being converted and not just the types. So, for instance, converting the float value 1234.0 into a PRUint8 would yield an error code bacause the conversion can not be done without loss of significant data. We might even go so far as non-NS_OK *success* codes for cases like 12.1 -> PRInt32 where we want to signal loss of insignificant data but still do the conversion since significant bits are not lost. This extends to things like wstring -> string conversions. Now I'm becoming convinced that there is little to be gained by this. The client still needs to write code to deal with these error cases. The client might as well gather the data in its 'natural' form and do local conversions if they *really* need some specific type but require the value to fall in a particular range. I'm not convincd that all callers should pay for 'bounds checking' if few callers require it. I argue that those few callers can supply their own conversion code. Unless someone makes a very compelling argument, I'm going to finish coding a simpler scheme - more like languages we're used to; e.g. the ECMAScript data conversion spec - where the caller can rely on data conversions from type A to type B succeeding regardless of data value.

The patch I just attached is functional but still unfinished. It still needs: array support, a fuller matrix of tests, and a few more conversions. Bit, I don't expect the interfaces to change and it should generally work if anyone wants to give it a try.

A conversion which loses insignificant bits is OK. A conversion which loses most significant bits is seldom if ever OK. Detecting the situation versus dealing with it are two different things. Certainly an application has to deal with the situation. But the conversion should do something, like returning an error condition or raising an exception, to allow the caller to detect that the problem has occurred. The conversion can detect the situation quite naturally as part of the conversion process. An application, on the other hand, might have to install large switch statements around each conversion just to detect the situation. This destroys all the advantage that the application would have in relying on built-in conversions. The application may as well write a new conversion library if it has to be able to check every different type of data it may be converting from to obtain the desired type for it's own specific error conditions.

+// However, currently these types can not be used for 'in' params. And, methods +// that use them as 'out' params *must* be declared [noxpcom] (with an explicit s/noxpcom/notxpcom/ +// return type of nsresult). This makes such methods implicitly not scriptable. +// Use of these typed in methods without a [notxpcom] declaration will cause s/typed/types/ + + COMPONENT(FAST_LOAD_SERVICE, nsFastLoadService::Create), - COMPONENT(FAST_LOAD_SERVICE, nsFastLoadService::Create) + COMPONENT(VARIANT, nsVariantConstructor) Ooh, your patch is out of date (FASTLOADSERVICE nowadays, following the prevailing naming convention) -- can you update your tree? I think if you can flag (via a boolean out param) conversion loss of precision, without adding a lot of costs, then the conversion code should do so, so that one or more callers do not have to recapitulate the checks and flag themselves. But I haven't looked at the patch enough to say whether that's cheap and easy. /be

> s/noxpcom/notxpcom/ > s/typed/types/ fixed. thanks. > FASTLOADSERVICE cvs found that for me last night when I updated. thanks. OK, consensus is building for loss of precision checking. brendan: I guess we've been through this before... I was leaning toward encoding this info in the nsresult rather than an additional param. I don't want to go as far as rayw suggests and make these errors - I'd rather let the caller decide if he wants to use the lossy result. So, I was toying with the idea of success codes. The old arguments against success codes are coming to mind. So, an additional out param is looking better. The fact that scripts are not going to be dealing with this problem explicitly makes additional out params look less bad. I do want to push toward only applying this to the numeric types... I think losses in wstring -> string conversion are not worth detecting. I think that getAsBool is inherently lossy. Is a bool to flag loss of precision rich enough? Don't we want to distinguish rounding losses from loss of significant bits? e.g. 12.1 -> PRInt32 v. 518 -> PRInt8 What about the loss of the meaning of the sign bit? e.g. -12 -> PRUint32 Do we assume that the caller just wanted the bit pattern? Or that he should be warned that the value is out of range for the specified type? I'd be very happy with some guidance here. Thanks.

As Brendan notes, it would be nice to be able to have info come back about loss of precision. Please note that in my previous comment, I was NOT worried about loss of precision, but rather loss of the most significant bits, which is a completely different scenario as I see it. People expect loss of precision and as long as they get the precision they asked for, it is reasonable. They ask for a number as a float and lose precision because it was a double or a 64 bit integer, but at least the answer is often close enough to work and as close as it can be given their request, as long as it did not overflow the float. But when they ask for an unsigned byte, for example (0 - 255) and the value was 999, any answer they coud possibly recieve would be quite meaningless in most circumstances, so an exception seems appropriate. I find a conversion function that does not inform the caller of this type of complete failure not usable for most use cases.

Assuming that you mean UCS-2 to ASCII when you say "wstring to string", I think we should indicate a conversion error if there are non-ASCII characters in the source string. Just stripping the high bit is data corruption, really, and we should be flagging it as such.. (Alternatively, we could use the "loss of precision" flag to indicate that we couldn't fit it in ASCII, and so have return a UTF-8 encoded string "instead".)

I think the idea of alternate completion codes is very good. I think that if you get as a boolean, you should have a status that warns you that the value was not one of the recognized boolean forms of true / false. I think this is clearly applicable to string conversions as well, even though it is a fair amount of work. If wide characters are being replaced with nearly-equivalents from the target set, I think it looks like a loss in precision. Of course, there can be evil results. Just because the not equal symbol is almost like the equal symbol does not mean that replacing not equal with equal is a good idea just to get the set into a 7 bit representation, but perhaps you can replace it with "!=". If you have characters that are completely unrepresentable, then perhaps dropping those characters and keeping others is like a loss in precision, because the result may still be mostly usable even if it lost something, but that could be tricky. On the other hand, stripping the top non-zero bits from wide characters seems just as evil as stripping the non-zero most significant bits from a number. An exception seems preferrable to that. This would be value-based, because you can obviously strip away as long as the most significan bits are zero, and callers will want to do that.

shaver: I continue to argue that xpidl's 'string' *never* means UTF-8 encoded (it's in the spec!). So you're suggesting I implement (what I think will be) the *only* UCS-2 to ASCII in our code base that bothers to flag this for the caller? I'm suggesting that for some conversions the caller can *see* that the variant is claiming to hold a UCS-2. If they asl for ASCII they get what they get. rayw: in my book non-zero is a recognized form of true. I'd like us to settle on a reasonable (and hopefully simple) set of implmentable rules. I don't think they need to be all singing and all dancing. I'm about *this* close to declaring that nsIVariant will promise no conversions at all and that the default nsVariant in xpcom will do zero value checking and few conversions and that *someone else* can write an nsIVariant exposing 'filter' object that callers can use to wrap their calls to their target variants and the filer object(s) can do any darn error detection that y'all want.

So we have the following conditions: 1. Straight conversion: PRUInt32(5) -> PRUint32 2. Conversion no loss: PRUint32(5) -> PRUint16 or double(1.0) -> PRUint16 3. Conversion with insignificant loss: double(5.5) -> PRUint16 4. Conversion with significant loss: PRUint32(70000) -> PRUint16 or PRInt32(-1) -> PRUint32 5. Conversion from illegal value -> string("Text") -> PRUint32 6. Illegal conversion: PRUint32 -> ISupports Thought it might help to enumerate the scenarios. In any case, the questions sound strikingly similar to what will need to be answered for bug 13428

Some other thoughts I had. Something to consider, why not have array be part of the type, a flag in the type. You'd reduce the overall size of the union by two bytes. Would it be worth creating an error type? Basically a variant that can hold an nsresult value. I'm not sure I'm real sold on the idea. It seems it might have some value, but it also creates a separate error handling mechanism. The biggest benefit is used for return values. The variant could hold a valid value or an error. MS does it in their variant, but I'm not convinced it's something worth emulating. Also is there any value for being able to signify a null with a type. Database variants often do this. I've seen it done in other variants, but I can't remember a specific use for it. Wanted to through it out in case someone else might see a value. Lastly I know the type conversion issue is getting a bit messy. Would it be better to do as you suggest, jband, and just get a simple implementation with minimal or no conversions. We can then extend this variant to support conversions at a later date after we determined the appropriate thing to do. I think the addition of variant without type conversion would probably be of big benefit to many and the absence of conversions wouldn't make it unusable.

> Something to consider, why not have array be part of the type, a flag in the > type. You'd reduce the overall size of the union by two bytes. You missed the nsID member of the union. This dominates the size of the union. I don't think that optimizing this structure for size is a big concern. I *really* hope that there will rarely be more than a handful of nsDiscriminatedUnion objects alive at any one time. > Would it be worth creating an error type? I'm trying to just mirror the xpidl types here. We typedef nsresult as PRUint32 for xpidl. I'm thinking that that is sufficient. > Also is there any value for being able to signify a null with a type. TYPE_EMPTY covers that AFAIC. I've tightened up the code a bit since that last patch to assume the rule that none of the pointer types for the variant can hold a value of null. e.g. setAsString(nsnull) is illegal and getAsString can not succeed *and* return null. The contract will be that the variant can't hold a typed null - if the value is null then the type is neither string or nsISupports or any other pointer type - the only type with 'nullness' is TYPE_EMPTY. I know I'm going back and forth on this conversion errors issue. But at this point I'm ready to go ahead and define and implment strict rules. The code I have will need little change. I'm thinking (as dbradley pointed out) that we may well leverage those rules (and the nsVariant static methods) for bug 13428. At present xpconnect calls into the JS engine for data conversion. For the numerical conversions it might was well call into the nsVariant static conversion routines so that out of range values are not silently converted to garbage in calls from JS to native interfaces. As much as I'm tired of messing with this, we might as well get it right. Thanks for the comments David (even if I do disagree :) John.

*** Bug 77530 has been marked as a duplicate of this bug. ***

I'm back to working on this again. Added some range checking and tests. More to come. I'm going to chart out the conversions rules. I'd like to get this all working soon and get it in.

That last one has the nsIPropertyBag support for bug 98209 and completely untested array core support. Most of the conversion range checking is in. Not yet checking for loss of data in conversions from double to float or wide to narrow strings. I *hate* that this has become so large and macro filled. I know rayw wants to get to using this stuff and is less concerned about some edge cases in the near term. I'm inclined to get this all in pretty soon and fix the trailing bugs after. I'm out until next tuesday. I'm pretty sure I got all the bits in here in case anyone wants to apply it and give it a try.

retarget -> 0.9.6

To simplify things I checked in the new files (NOT PART OF THE BUILD) These are: mozilla/xpcom/ds/nsIPropertyBag.idl mozilla/xpcom/ds/nsIVariant.idl mozilla/xpcom/ds/nsVariant.cpp mozilla/xpcom/ds/nsVariant.h mozilla/js/src/xpconnect/src/xpcvariant.cpp mozilla/js/src/xpconnect/tests/components/xpctest_variant.cpp mozilla/js/src/xpconnect/tests/js/old/xpctest_variant.js mozilla/js/src/xpconnect/tests/js/old/xpctest_propertybag.js The last patch contains only changes. Of course, review of the change includes reviewing the stuff in the tree and the patches.

I have been unable to get the MOZ_CO_DATE sticky date to work (under Linux) with client.mk to go back to a tree where this latest patch worked. You didn't happen to put in a tag at that point, did you? Or is there a better strategy, like moving onto a branch if it cannot be put back soon?

I have tried compiling the IDL files you put back w/o the patch so I could at least write code against it, and I get errors: nsIVariant.idl:92: `nsID' undeclared identifier How can I best work with this stuff, which is critical throughout my SOAP

Sorry, I miscounted the .rej files after I ran the thing multiple times. It was only two failures in makefiles in the main directory, which I think I can fix.

rayw: sorry, I've ben distracted while transfering my work environment to a new computer. Did you get this working? If not, the freshened patch might help. It is unfortunate that Patch screws up on files in different directories with identical names. I'm inclined to get this reviewed and checked in asap. We can then file bugs and fix those remaining problems later: mostly array support and some broken range checking (floats, wide strings, and int8).

Attachment 54963 [details] of bug 72505 is a Perl script that fixes a common problem in patch files (caused by a bug in our cvs server). You might try running it on your patch files and seeing if it fixes your problem. See my comment in the bug for more information.

New patch is same as before with the additions... - It registers additional JS arg formatters. These are helpers that get automatically called to handle specific types for JS_ConvertArguments and JS_PushArguments. We already have "%ip" for nsISupports conversions. This adds "%iv" for variant conversion (any jsval to an nsIVariant and back) and "%is" for AString conversions. Note that when converting a jsval to an AString the caller is expected to already have an AString instance whose pointer is passed and xpconnect will use the nsAString::Assign method to fill the string. This avoids unnecessary allocations. A wrinkle here is that there is not yet a generic 'voidable' string available. I filed bug 106827 about moving (and renaming) jst's XPCVoidableString to the string library. - Fixed crasher in TestXPC and extended it to test the above.

Comment on attachment 55165 [details] [diff] [review] more freshness, with JS arg formatter support added sr=vidur

Comment on attachment 55165 [details] [diff] [review] more freshness, with JS arg formatter support added r=dbradley Only thing I noticed was the xpcPropertyBagEnumerator. I know the one I ran into in the hash table it used a done condition while this was uses has more condition. Is there a precident one way or the other?

Thanks David. The code you looked at elsewhere is based on the evil old nsIEnumerator. This is based on the good new nsISimpleEnumerator.

FYI, XPCOM's "good, new" nsISimpleEnumerator is based on java.util.Enumeration (http://java.sun.com/j2se/1.3/docs/api/java/util/Enumeration.html). /be

I do plan to check this in today. I'll file bugs on the remaining work to be done.

The patch was checked into the trunk last week.

Trying to verify this check-in of the final patch (id=55165). However, I find a discrepancy in one file: the patch says Index: xpcom/reflect/xptinfo/src/xptiprivate.h =================================================================== RCS file: /cvsroot/mozilla/xpcom/reflect/xptinfo/src/xptiprivate.h,v retrieving revision 1.14 diff -u -r1.14 xptiprivate.h --- xptiprivate.h 2001/10/18 04:23:25 1.14 +++ xptiprivate.h 2001/10/26 00:19:54 @@ -62,6 +62,7 @@ #include "nsIDirectoryService.h" #include "nsDirectoryServiceDefs.h" #include "nsAppDirectoryServiceDefs.h" +#include "nsIWeakReference.h" #include "nsCRT.h" #include "nsMemory.h" @@ -702,11 +703,12 @@ /***************************************************************************/ class xptiInterfaceInfoManager - : public nsIInterfaceInfoManager, + : public nsIInterfaceInfoSuperManager, public xptiEntrySink { NS_DECL_ISUPPORTS NS_DECL_NSIINTERFACEINFOMANAGER + NS_DECL_NSIINTERFACEINFOSUPERMANAGER // implement xptiEntrySink PRBool @@ -799,9 +801,10 @@ PRFileDesc* mOpenLogFile; PRLock* mResolveLock; PRLock* mAutoRegLock; + PRLock* mAdditionalManagersLock; + nsSupportsArray mAdditionalManagers; nsCOMPtr<nsILocalFile> mManifestDir; nsCOMPtr<nsISupportsArray> mSearchPath; - }; /***************************************************************************/ But CVS indicates: $ cvs diff -r1.14 -rHEAD xptiprivate.h Index: xptiprivate.h =================================================================== RCS file: /cvsroot/mozilla/xpcom/reflect/xptinfo/src/xptiprivate.h,v retrieving revision 1.14 retrieving revision 1.15 diff -r1.14 -r1.15 609a610,611 > static PRBool Delete(xptiInterfaceInfoManager* aMgr); > Is this OK? Note: bonsai comments for version 1.15 say, "fix bug 108045, etc. ...", and do not refer to this bug -

Phil, I'm glad to see you're actually watching that close! The first chuck you cite is part of the fix to bug 103805 that I didn't mean to include in the patch I attached here. It is correct that it was not checked in. The second chunk is part of the fixed bug 108045. I believe this is all correct.

OK, thanks! Marking Verified -