Xpinstall currently supports a number of Windows registry manipulation routines. One that is missing and quite important is the ability to enumerate a particular windows registry key for either subkeys or names (vars).

nominating and clearing future to jog the memory -- weren't some AOL/netscape partners needing this?

dveditz -- yes. we want installers to enumerate the Mozilla subkey, as documented here: http://mozilla.org/projects/plugins/install-scheme.html So having the ability to parse the Mozilla subkey is key, no pun intended.

plussing

--> dprice

Need to remember this for the doc update once it's done.

OK, so when this lands, dprice, can you let Ian and I know so that: 1. I can start hacking on it to make sure it answers the needs documented in http://www.mozilla.org/projects/plugins/install-scheme.html 2. Ian can document it? This is important because AOL/NSCP partners need this feature in order to successfully enumerate the registry via non-native code (e.g. via means other than an EXE installer).

Could you post your API spec/syntax for these two functions? That'll let QA get a head start on test cases and allow the embedding folks a chance to look it over and see if it does what they need. And then Ian can use it to update the docs. thanks.

The functions are String WinReg.enumKeys(String key, int index); String WinReg.enumValueNames(String key, int index); Both functions return null if the index is out of range.

Comment on attachment 75944 [details] [diff] [review] first round patch >+ // public int enumValueNames ( String keyName, >+ // Int index); The return value's a string, right? For consistency I'd prefer documenting the first argument as "subkey" instead of keyName. >+ if(!JS_ValueToInt32(cx, argv[1], (int32 *)&b1)) We don't really care about b1 in this routine, could you declare it with a type that doesn't require this ugly cast? >+ { >+ JS_ReportError(cx, "Parameter must be a number"); >+ return JS_FALSE; Really want to avoid returning JS_FALSE, which aborts the script. We should return true here and can probably content ourselves with the default null as the rval. Do we still want to report the error anyway? Dunno, I suppose the failure could be a little mysterious without that. If you do, though, you should probably use JS_ReportWarning() instead of JS_ReportError(). If you're going to warn, though, you need something less ambiguous because it'll show up on the js console with just a line number. >+ if(NS_OK != nativeThis->EnumValueNames(b0, b1, &nativeRet)) >+ { >+ return JS_FALSE; Again, no JS_FALSE. >+ } >+ >+ ConvertStrToJSVal(nativeRet, cx, rval); >+ } >+ else >+ { >+ JS_ReportError(cx, "WinReg.enumValueNames() parameters error"); >+ return JS_FALSE; And again. In fact, unless you want to issue the JS_ReportWarning(), you don't need this else block at all. You could even reverse the sense of the if's to make the error cases the exception, if (JS_ValueToInt32()) { if (NS_OK == nativeThis->Enum...()) { ConvertStrToJSVal(); } } Then stick in the else's if you want to issue the warnings. >+ // public int enumKeys ( String keyName, >+ // Int index); string return value, and "subkey" for arg name. Basically same thing here as above. > PRInt32 >+nsWinReg::EnumValueNames(const nsString& keyname, PRInt32 index, nsString* aReturn) >+{ >+ NativeEnumValueNames(keyname, index, aReturn); >+ return NS_OK; >+} >+ >+PRInt32 >+nsWinReg::EnumKeys(const nsString& keyname, PRInt32 index, nsString* aReturn) >+{ >+ NativeEnumKeys(keyname, index, aReturn); >+ return NS_OK; >+} These stubs are artifacts of the semi-automatic conversion Raman did from Java to C++ when we first ported the 4.x SmartUpdate code to Mozilla. We don't need to propogate this broken pattern into fresh code, just put the real code here rather than have this useless extra function call. I notice this always returns NS_OK, so there's not much point in checking for it not being NS_OK in the js gluecode layer. The return value would be better as an nsString reference rather than a pointer, and up in the js glue code layer you could make it an nsAutoString instead so that most of the time we keep the string on the stack rather than have to allocate. >+nsWinReg::NativeEnumValueNames(const nsString& keyname, PRInt32 index, nsString* aReturn) using our naming convention aSubkey and aIndex for args would help the readability of the code. >+{ >+ char valbuf[_MAXKEYVALUE_]; >+ unsigned char databuf[_MAXKEYVALUE_]; Since we're not doing anything with the actual data you can pass null for both the databuf and the pointer to the datasize. 8K seems overkill for a value name. >+ char* keynameCString = ToNewCString(keyname); This is a lossy conversion. I know it's all through this file, but it's starting to come back and bite us on the butt so let's start to fix this. At the very least I'd prefer we use NS_LossyConvertUCS2toASCII() to flag this for later fixing, but we should consider using the multibyte-to-wide string conversion routines and doing it right. Too bad we have to deal with Win9x systems, it if were all NT-based we could just use the Unicode forms of the registry routines and not have to deal with it. >+ result = RegOpenKeyEx( root, keynameCString, 0, KEY_READ, &newkey ); So this could be result = RegOpenKeyEx( (HKEY)mRootKey, NS_LossyConvertUCS2toASCII(aSubkey).get(), 0, KEY_READ /*or KEY_QUERY_VALUE*/, &newkey ); >+ if ( ERROR_SUCCESS == result ) { >+ result = RegEnumValue( newkey, index, (char*)valbuf, &valsize, nsnull, &type, (unsigned char*)databuf, &datasize ); last three args can be 0, we don't care about them. >+ if(type == REG_SZ) type refers to the data, the value name is always a string. >+ aReturn->AssignWithConversion((char*)valbuf); avoid "WithConversion" routines, the string guys are trying to stamp them out. Make the conversion explicit, in this case the broken aReturn = NS_ConvertASCIItoUCS2(valbuf); // or aReturn.Assign( NS_Convert... ) >+ if(ERROR_ACCESS_DENIED == result) >+ result = nsInstall::ACCESS_DENIED; You don't do anything with result, so why bother? >+void >+nsWinReg::NativeEnumKeys(const nsString& keyname, PRInt32 index, nsString* aReturn) Similar concerns here. If we're going to fix the conversion stuff, you can see the right way in http://bugzilla.mozilla.org/attachment.cgi?id=67303&action=view

I think I've got all of the suggestions in here. I tried to do the multibyte stuff the right way. I'm uncomfortable with it because this is kind of greek to me (no pun intended ;) Anyway NS_LossyConvertUCS2toASCII().get() returns a const char * and Recycle() wants a char *. Badness happens when you try forcing one to the other. I'm returning NS_OK in a lot of places. I figure the null return value will be enough tell the user something's wrong. If they pass in a null string, they'll get null back. In the horrible case where they can't allocate memory, they've got bigger problems. On the other hand I left in all of the warnings in the js glue code because the user wouldn't be able to tell the difference between a null return value (which is legal if they access the wrong index) and an error condition.

Comment on attachment 77405 [details] [diff] [review] patch >+ JS_ReportWarning(cx, "WinReg.enumValueNames() - Too many parameters"); Too few, right? (two places) >+ nativeThis->EnumValueNames(b0, b1, nativeRet); >+ ConvertStrToJSVal(nativeRet, cx, rval); In our API's that return objects instead of error codes we promise errors will return null; this looks like it'll return "" instead. You need to either return errors and check for them, or I suppose you could assume that Keys and Values can never actually be named "" and call ConvertStrToJSVal only if !nativeRet.IsEmpty(). In some WINAPIs the "default" value is referenced using "", so I guess you'd better make sure this value is not enumerated before taking the second tack. >+ size_t chars = ::WideCharToMultiByte(CP_ACP, 0, >+ aSubkey.get(), -1, >+ NULL, 0, NULL, NULL); >+ if ( chars == 0 ) >+ return NS_OK; >+ subkeyCString = (char*)nsMemory::Alloc(chars * sizeof(char)); You use a fixed 8192 byte buffer for the enumerated name, why not the same for the incoming key? 8K is way overkill, seems odd to do that one place and then allocate precisely elsewhere. How about two 4k buffers instead? >+ result = RegOpenKeyEx( root, subkeyCString, 0, KEY_READ, &newkey ); >+ >+ if ( ERROR_SUCCESS == result ) { Since you don't return any error codes (maybe you should, but see above) you should at least aReturn.Truncate() right off the bat to make sure the message is conveyed. >+ result = RegEnumValue( newkey, aIndex, (char*)namebuf, &namesize, nsnull, 0, 0, 0 ); >+ RegCloseKey( newkey ); >+ aReturn.Assign(NS_ConvertASCIItoUCS2(namebuf)); You do nothing with result, so in an error case (such as going past the last index) you'll happily return whatever uninitialize gunk is in namebuf. You'd probably not notice this in a debug build which zeros things out for you. You did the right conversion going in, but are using the broken ASCII conversion coming out. If you were going to skip one it wouldn't be this one, mangling things coming out of the registry is what's messing us up (see, for example, bug 125106). On the way out you need to use the other half of the WideCharToMultiByte pair: MultiByteToWideChar. You'll need a conversion buffer for that, too; you could re-use the one you're going to add for the key conversion on the way in since now you're done with that data. >+ return NS_OK; Don't return useless values. If you're not ever going to return an error make it a void function. >+nsWinReg::EnumKeys(const nsString& aSubkey, PRInt32 aIndex, nsString &aReturn) Same things here.

I think this will do it. The nsWinReg methods are now using the correct string conversions and they are passing error codes back to the js glue code which is making certain that null is returned on error.

Comment on attachment 77942 [details] [diff] [review] another patch >+nsWinReg::EnumValueNames(const nsString& aSubkey, PRInt32 aIndex, nsString &aReturn) >+{ >+ char namebuf[_MAXKEYVALUE_]; This should be MAX_BUF, that's plenty big enough. >+ char subkeyCString[MAX_BUF]; >+ ::WideCharToMultiByte(CP_ACP, 0, If you're not checking for errors here you need to make sure the buffer starts out empty. That way you are sure RegOpenKeyEx will fail on an empty string rather than hoping you don't have a leftover good string on the stack by accident. Same concern in the other routine. >+ if ( ::MultiByteToWideChar(CP_ACP, 0, namebuf, -1, returnBuf, MAX_BUF) ) >+ aReturn.Assign(returnBuf); >+ else >+ rv = NS_ERROR_FAILURE; >+ } >+ else >+ rv = NS_ERROR_ILLEGAL_VALUE; >+ } >+ else >+ rv = NS_ERROR_ILLEGAL_VALUE; Instead a string of errors you could have initialize rv to an error and then set it to NS_OK in the one success case. No need to change now, but it would have simplified the code. carrying r=curt over if you fix the first two comments then sr=dveditz

the final tweaks are in my tree. adding adt1.0.0 for approval request.

adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0.

Comment on attachment 77942 [details] [diff] [review] another patch a=rjesup@wgate.com for drivers

landed

Build: 2002-04-17-17-1.0.0(WIN), 2002-04-18-10-trunk(WIN) This fix got checked in before the 1.0.0 branch. It is working on both the branch and trunk. http://jimbob/jars/a_winreg_create_enum.xpi creates keys and value names from HKEY_LOCAL_MACHINE\Software http://jimbob/jars/a_winreg_enumkeys_enumvaluenames.xpi uses the enum functions and writes results to install.log Marking Verified!

Adding keyword verified1.0.0. This fix was made before the 1.0.0 branch. Still works on branch.