When you are about to download a file of a type that we don't have a default handler registration for, the Unknown Content Type dialog is presented, and the default handler for the specified type is filled in. nsOSHelperAppService on Windows determines this type by looking in: HKEY_CLASSES_ROOT\.fileExtension\<default value> It turned out that in many cases the app handler for things like .doc was Word.Document.Blah.Goat or similar, so it "seemed" that you could trim at the first "." and get the app name. Unfortunately that's not always true - the default value of the extension key is actually a key into another part of the registry, namely: HKEY_CLASSES_ROOT\<key from default value>\ e.g. HKEY_CLASSES_ROOT\Word.Document.Blah.Goat\ Because of this, not all default values in the extension keys are obliged to format in the AppName.SomethingElse.SomethingElse format, rather, they can use any string they like. In the case of things like png, the result is "pngfile", in the case of WMV movies, the key is "WMVFile" etc. The real name of the app handler is stored in the handler EXE's Version block in its resources section. We can only get at the handler's EXE path by delving a little deeper... the path is stored under: HKEY_CLASSES_ROOT\<key from extension default value>\shell\open\command and is formatted in one of the following ways: 1) it is either a path to an executable, formatted: a) "C:\Path with spaces\Inside\Quotes.exe" -P params ... in this case, the path is the first quoted string, b) C:\NOSPAC~1\INPATH.exe -P params ... in this case, the path is the first chunk before the space or the end of the string. 2) it is a DLL (usually an internal windows function like the Picture and Fax Viewer) invoked with rundll32.exe, e.g.: rundll32.exe "C:\Windows\System32\shimgvw.dll",Image_Something_Blah %1 ... in this case we need to chomp the rundll32.exe part, and the stuff after the last "," (Windows paths can contain ","s) Once the path is extracted, a file object can be constructed and metadata read from the Version block. Several things are needed to implement this. 1) a utility that reads parameters from the Version block. I created nsILocalFileWin to do this. I think this is probably best for this and several other usages I have in mind - this is all Windows specific code, but the 2) code in nsOSHelperAppService that looks for the path in the right location - i.e. does all the described parsing. In this order: - check to see if it's a rundll32.exe type path, and strip related matter out *first* before attempting to do anything else, since this type of URL can either be a long-path-with-spaces or a short-path-with-no-spaces, and contain embedded environment variables. - chomp off the parameters (optionally) the quotes from the first quoted string to obtain the handler path. - substitute environment variables - construct a file object (nsILocalFileWin) with the path and get the FileDescription property - Set THIS value as the defaultDescription property on the MIMEInfo object, not the parsed string (kill the old "characters before the first ." code since that only worked by chance).

Comment on attachment 153707 [details] [diff] [review] URILoader - nsOSHelperAppService.cpp section +static void SubstituteEnvironmentVariables(nsAString& aPath) windows offers ExpandEnvironmentStrings + nsCOMPtr<nsILocalFileWin> lfw(do_CreateInstance("@mozilla.org/file/local;1")); NS_NewLocalFile is maybe slightly better (less codesize, less rv to check) although, then you have to QI... + realPath = Substring(aPath, 1, aPath.Length() - 1); + PRInt32 nextQuote = realPath.FindChar(PRUnichar('"')); + if (nextQuote > 0) + realPath = Substring(realPath, 0, nextQuote); looks like you could use .Truncate() here + realPath = Substring(aPath, 0, firstSpace); and here. + // If all else fails, use the file type key name, which will be something like "pngfile" for + // .pngs, "WMVFile" for .wmvs, etc. + aDefaultDescription = aTypeName; should this continue to truncate at the first '.' like before? + if (prefix.EqualsIgnoreCase("rundll32.exe")) { prefix.LowerCaseEqualsLiteral("rundll32.exe") However, I think you want StringBeginsWith here. Are you going to update this patch? I'd like to have this on trunk.

The description is sometimes not too clear. A mangled real world example is "Windows xxx client", which doesn't actually tell the application name at all. I think the description should contain the actual executable name in addition to other descriptions. For example "Windows xxx client (winxxx.exe)".

I will update this patch for the trunk shortly. I may give up on the nsILocalFileWin segment and just build the code into nsOSHelperAppService.

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

Sadly not a priority. May pull back in if I have time.

this code here can probably also be used for a better implementation of (the new) nsOSHelperAppService::GetApplicationDescription.

This patch now does 3 things, part of a rollup of stuff I'm doing for preferences. 1) it makes defaultDescription work correctly (as in the original patch) + addresses biesi's review comments for the first patch. 2) it adds a defaultApplicationHandler property to nsIMIMEInfo to provide convenient access to the default handler (there's already access to the preferred handler). 3) fixes this bug: sometimes the windows registry can have metadata listed for a different mime type to that commonly sent by servers for a particular type, e.g. application/x-zip-compressed vs application/zip when the download process is under way, if the user chooses to make a particular action automatic, that action is saved into mimeTypes.rdf with the type specified by the server. when we list the overrides in the preferences UI we list the set of types shown in the overrides list, and when we try to get say an icon for one where there's a mismatch between the content type sent by servers and whatever happens to be in the windows registry, an empty mime info obj is returned. So, as a last ditch attempt we should look for appropriate extensions in the override ds, rather than just giving up when there is no match on the mime type specified by the override ds.

+ ::VerQueryValue(ver, subBlock, (void**)&value, &size); Can this fail? because if it does, it seems like value will be null, and creating an nsDependentCString from NULL will crash.

(In reply to comment #11) > + ::VerQueryValue(ver, subBlock, (void**)&value, &size); > > Can this fail? because if it does, it seems like value will be null, and > creating an nsDependentCString from NULL will crash. > Why yes, yes it can. I should change this to make both call sites check like this: if (::VerQueryValue(...) && size != 0) { // OK, proceed }

er, data != NULL

Comment on attachment 174202 [details] [diff] [review] patch >Index: netwerk/mime/public/nsIMIMEInfo.idl >+ /** >+ * Returns a nsIFile that points to the application the system has >+ * determined is the default handler for this content type. >+ */ >+ readonly attribute nsIFile defaultApplicationHandler; What if the default handler cannot be represented via an nsIFile (either because it includes command-line arguments, or because it requires PATH resolution, or because it's not an "application" at all)? Note that this attribute was removed from this interface for precisely those reasons. Why do we need to expose it, exactly? I don't see it being used anywhere in the patch, and it's not even implemented in a useful way on a lot of our platforms (will always return null). Note that we're aiming to freeze this interface soon (possibly 1.8 timeframe, if we can get away with it), so if we do need something like this on it now is the time to talk about it. >Index: uriloader/exthandler/win/nsOSHelperAppService.cpp >+static void RemoveParameters(nsAString& aPath) Why not just make this take nsString& ? Then you won't need the string-copy into realPath. >+ // 1) "C:\Program Files\Company Name\product.exe" -foo -bar (long version) Single quotes not allowed? >+ if (nextQuote > 0) != kNotFound, please. Also, what if it's not found? But we started with a '"', and have now stripped it off? Is that what we want to be doing? >+ if (firstSpace > 0) != kNotFound, please. >+nsOSHelperAppService::GetDefaultAppInfo(nsAString& aTypeName, nsAString& aDefaultDescription, >+ DWORD err = ::RegOpenKeyEx(HKEY_CLASSES_ROOT, handlerKeyName.get(), 0, KEY_QUERY_VALUE, &hHandlerKey); RegOpenKeyEx returns LONG, no? >+ if (lastCommaPos > 0) != kNotFound. >+ handlerFilePath = Substring(handlerCommand, 13, lastCommaPos - 13); Whence comes the number 13? Use NS_ARRAY_LENGTH on the appropriate char array as needed, or sizeof on a literal string, or at least document what the magic number means. >+ handlerFilePathCStr.AssignWithConversion(handlerFilePath); Why is this correct? Is handlerFilePath always ASCII? >+ if (!destination) >+ return NS_ERROR_OUT_OF_MEMORY; This leaks the open registry key. >+ handlerFilePath.AssignWithConversion(destination); Again, intl issue... >+ if (!lf) >+ return NS_ERROR_OUT_OF_MEMORY; This leaks the open key. >+ lfw->GetVersionInfoField(NS_LITERAL_CSTRING("FileDescription"), >+ aDefaultDescription); Weird indent. Tabs? >+ nsCOMPtr<nsIFile> file(do_QueryInterface(lfw)); >+ *aDefaultApplication = file; >+ NS_IF_ADDREF(*aDefaultApplication); Can't all this just be |lfw.swap(*aDefaultApplication);|? And at the end here we leak the open registry key. nsOSHelperAppService::GetMIMEInfoFromOS(const nsACString& aMIMEType, const nsACString& aFileExt, PRBool *aFound) >+ mi = GetByUsingOverrideDatabase(flatType.get()).get(); I'm not sure what this change has to do with this bug. If it doesn't, should it be in a separate bug?

(In reply to comment #14) > Why do we need to expose it, exactly? I don't see it being used anywhere in > the patch, and it's not even implemented in a useful way on a lot of our > platforms (will always return null). I just noticed this... I need it because in a new piece of UI I'm working on, I want to display the default application's icon. Basically the UI looks something like this: http://www.bengoodger.com/software/mb/options/prefwindowv/downloadactions-edit.png See the greyed out field at the top of the dialog (the icon to the left of PictureViewer comes from doing something like moz-icon://<path to default app>) ... it's possible for the custom handler (the second field in the dialog) because that file is available through the MIME info interface. I chose to expose it as a nsILocalFile because I was looking for symmetry with the customAppHandler property, and also thought that exposing the file would make it easy to get other properties of the file in the future even if the MIME Info interface froze and no new integrated properties were possible. Maybe expose the handler path with flags? Or expose a separate property containing the args? > >+ mi = GetByUsingOverrideDatabase(flatType.get()).get(); > > I'm not sure what this change has to do with this bug. > If it doesn't, should it be in a separate bug? Perhaps, I just rolled it up here since it was with other changes in this area of code from the branch I'm trying to land ;-) If you want me to separate it out and file a new bug, I can do that.

For Doug/Darin - I know we have had some discussion about this XPCOM patch before Christmas, and I know Doug was interested in an XP interface, the problem is there is a large amount of metadata stored in a proprietary fashion. Some of it is platform specific. The calling code in uriloader and in the browser is all #ifdef XP_WIN. It does not make sense to me to create a crippled generic interface at this point, primarily because we cannot guarantee what each platform might want to expose through it. What I think makes the most sense is a non-frozen platform specific interface, much like nsILocalFileMac, which is what my patch implements.

So the defaultApplicationHandler change really has nothing to do with this bug, basically. Please file a separate bug so we can figure out how to expose the necessary information on MIMEInfo? And yes, please file a separate bug on the RDF thing too.

> Single quotes not allowed? I don't believe I've ever seen single quotes used to "escape" paths in registry keys... > Also, what if it's not found? But we started with a '"', and have now > stripped it off? Is that what we want to be doing? I think in that case there would have been a very odd path... e.g. "foo bar we strip the ", leaving: foo bar which is at least as executable if not moreso than the former! > >+ nsCOMPtr<nsIFile> file(do_QueryInterface(lfw)); > >+ *aDefaultApplication = file; > >+ NS_IF_ADDREF(*aDefaultApplication); > > Can't all this just be |lfw.swap(*aDefaultApplication);|? No, because not all nsIFiles are nsILocalFileWins... despite the *aDefaultApplication = nsnull earlier on, the compiler barfs. Here's a new patch.

if you want that nsILocalFile attribute just for the icon, why not add an nsIURI iconURI attribute? or you could just do what the helper app dialog does (seamonkey's, at least) in fact, why don't you just use moz-icon://.<extension> to get the icon? (or does that not work for some reason?) I'll look at the patch a bit later today, probably. >>+ if (nextQuote > 0) >!= kNotFound, please. those would not be the same tests...

(In reply to comment #20) > if you want that nsILocalFile attribute just for the icon, why not add an nsIURI > iconURI attribute? or you could just do what the helper app dialog does > (seamonkey's, at least) Because I also need to do other things with the file, not just get an icon URI for it. Adding an iconURI attribute begs the question, why not add an iconURI for the custom app handler - the answer to which is "don't clutter the interface, you have the file property, use that and generate the Icon URI yourself". > in fact, why don't you just use moz-icon://.<extension> to get the icon? (or > does that not work for some reason?) I'm trying to get the icon for the application, not the file type.

So how come we can't use ExpandEnvironmentStrings anymore?

That is, what's with the SubstituteEnvironmentVariables function. Is it even used?

(In reply to comment #23) > That is, what's with the SubstituteEnvironmentVariables function. Is it even used? erm. oops. wrong patch.

Comment on attachment 174304 [details] [diff] [review] actual patch >Index: uriloader/exthandler/win/nsOSHelperAppService.cpp >+ nsresult rv = lf->QueryInterface(NS_GET_IID(nsILocalFileWin), (void**)&lfw); nsresult rv = CallQueryInterface(lf, &lfw); sr=bzbarsky with that.

Comment on attachment 174201 [details] [diff] [review] New nsILocalFileWin patch why does the interface have to be wide? Seams like the implementation just widens the data; is there really a need? why do you have to make a copy of mWorkingPath? As in other cases in this implementation, you might need to consider resolving file path before using mWorkingPath. According to msdn, when calling GetFileVersionInfoSize, you need to ensure that: The short path form of the specified file name must be less than 126 characters. VerQueryValue will take a A string -- you can drop the TEXT If VerQueryValue fails, you will be using translation without it being initalized. Check for failure both times it is called.

Comment on attachment 174304 [details] [diff] [review] actual patch + aPath = Substring(aPath, 1, aPath.Length() - 1); + PRInt32 nextQuote = aPath.FindChar(PRUnichar('"')); + if (nextQuote != kNotFound) + aPath.Truncate(nextQuote); this seems like an odd way to write this... why not first find the closing quote (passing an offset to FindChar), and only then assigning the Substring? (avoids the Truncate call, and would be a bit clearer, I think) +// executable file held in various registry keys) is stored n the VERSIONINFO typo here - should be "in the VERSIONINFO" hm... open may not be the default action... should you check HKCR\<type>\shell\<default value> to find it? + nsCAutoString handlerKeyName; + NS_CopyUnicodeToNative(aTypeName, handlerKeyName); Hm... so, much of this file tries to use wide APIs where possible. Could I get you to factor out the various duplicated RegOpenKeyExW/RegOpenKeyEx calls and create a helper function that takes a wide string and returns a HKEY, and use that here? (if you don't want to do it, please file a bug on that) (I guess non-ASCII handler key names are unusual, but still...) Hm... can't you close handlerKey directly after GetValueString? That way, you avoid having to call it various times in the function. + nsILocalFileWin* lfw = nsnull; + nsresult rv = lf->QueryInterface(NS_GET_IID(nsILocalFileWin), (void**)&lfw); + if (NS_SUCCEEDED(rv) && lfw) { you need not initialize lfw to null, and you only need to check one of rv and lfw... it looks like you no longer truncate the description at the dot, if lookup of detailed information failed... I guess that's ok. looks like GetDefaultAppInfo could be void. the whitespace change in uriloader/exthandler/win/nsOSHelperAppService.h seems unnecessary...

Comment on attachment 174201 [details] [diff] [review] New nsILocalFileWin patch hm... how about always exporting this IDL file? that way, callers (esp. JS) can just try to QI to it, and need not check whether they are running on windows)

Comment on attachment 174202 [details] [diff] [review] patch (I assume this review request is obsolete...)

Comment on attachment 174304 [details] [diff] [review] actual patch >+static void RemoveParameters(nsString& aPath) >+{ >+ // Command Strings stored in the Windows registry with parameters look like >+ // this: >+ // >+ // 1) "C:\Program Files\Company Name\product.exe" -foo -bar (long version) >+ // -- OR -- >+ // 2) C:\PROGRA~1\COMPAN~2\product.exe -foo -bar (short version) >+ // >+ // For 1), the path is the first "" quoted string. (quotes are used to >+ // prevent parameter parsers from choking) >+ // For 2), the path is the string up until the first space (spaces are >+ // illegal in short DOS-style paths) or like the following gems: [HKEY_CLASSES_ROOT\APHandler.Handler.1\shell\BlankCD\command] @=C:\Program Files\Ahead\nero\nero.exe /BlankCD [HKEY_CLASSES_ROOT\APHandler.Handler.1\shell\CDAudio\command] @=C:\Program Files\Ahead\nero\nero.exe /CDAudio [HKEY_CLASSES_ROOT\Applications\mstsc.exe\shell\Connect\command] @=mstsc.exe "%l" [HKEY_CLASSES_ROOT\Applications\PGPDISK.EXE\shell\open\command] @=C:\Program Files\PGP Corporation\PGP for Windows XP\PGPDISK.EXE open "%1" [HKEY_CLASSES_ROOT\Applications\wmplayer.exe\shell\open\command] @=C:\Program Files\Windows Media Player\wmplayer.exe /Open "%L" [HKEY_CLASSES_ROOT\ChannelFile\Shell\Edit\Command] @=notepad.exe %1 [HKEY_CLASSES_ROOT\clpfile\shell\open\command] @=clipbrd.exe %1 [HKEY_CLASSES_ROOT\helpfile\shell\open\command] @=winhlp32.exe %1 [HKEY_CLASSES_ROOT\MPlayer\shell\open\command] @=mplay32.exe /play /close "%L" [HKEY_CLASSES_ROOT\MSInfo.Document\Shell\Open\Command] @=C:\Program Files\Common Files\Microsoft Shared\MSInfo\MSInfo32.exe /msinfo_file %1 [HKEY_CLASSES_ROOT\regedit\shell\open\command] @=regedit.exe %1 [HKEY_CLASSES_ROOT\regfile\shell\open\command] @=regedit.exe "%1" [HKEY_CLASSES_ROOT\SoundRec\shell\record\command] @=sndrec32.exe "%L" please at least handle the space case, it's clearly not uncommon and happens for microsoft apps (WMP). you'll want to parse the string the way windows does, reading to each space and checking to see if you can find an app that would match that string :). at the very least include notes for the two cases (pathless and unquoted spaces with params)

(In reply to comment #27) > (From update of attachment 174201 [details] [diff] [review] [edit]) > why does the interface have to be wide? Seams like the implementation just > widens the data; is there really a need? The data can be multi-byte, in native charset... I figured it made it easier for the callers to do the conversion. > why do you have to make a copy of mWorkingPath? None of the Win API functions take const char*

Biesi's right, imo. The IDL should always be exported...

Comment on attachment 174366 [details] [diff] [review] newer nsILocalFileWin patch >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ xpcom/io/nsILocalFileWin.idl 15 Feb 2005 09:08:28 -0000 >@@ -0,0 +1,54 @@ >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. >+ * Portions created by the Initial Developer are Copyright (C) 2001 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * Conrad Carlen <ccarlen@netscape.com> this can't be right. please steal from the standard places instead. >+[scriptable, uuid(dc42f467-4094-437d-9e3e-8912a072aede)] >+interface nsILocalFileWin : nsILocalFile >+{ >+ AString getVersionInfoField(in ACString aField); >+}; Hrm. I'd kinda want access to stream properties if we're going to start exposing stuff onto an interface like this. >+NS_IMETHODIMP >+nsLocalFile::GetVersionInfoField(const nsACString &aField, nsAString& _retval) >+{ >+ nsresult rv = ResolveAndStat(); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ char *pathCopy = ToNewCString(mFollowSymlinks ? mResolvedPath : mWorkingPath); >+ nsCRT::free(pathCopy); http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsReadableUtils.h 102 * @return a new |char| buffer you must free with |nsMemory::Free|. _must_ 103 */ 104 NS_COM char* ToNewCString( const nsAString& aSource ); you're using the wrong free. >+ pathCopy = nsnull; And that's a useless assignment. >+ return NS_ERROR_FAILURE; >+ } >+ LPVOID value = NULL; >+ UINT size; >+ queryResult = ::VerQueryValue(ver, subBlock, (void**)&value, &size); >+ if (queryResult && (const char*)value) why the cast? >+ nsCRT::free(pathCopy); >+ pathCopy = nsnull;

Comment on attachment 174366 [details] [diff] [review] newer nsILocalFileWin patch i really hate that we use MOZ_WIDGET_TOOLKIT to get the right IDLs in the io/Makefile.in. extra points if you make it use OS_ARCH. You do not need to null the local variable here (and the one at the botton of the patch): + nsCRT::free(pathCopy); + pathCopy = nsnull; I think you can drop some of that casting as timeless mentioned. There are other comments about this patch. lets get those fixed, then attach a new patch.

Comment on attachment 174502 [details] [diff] [review] patch looks fine. thanks for the patch.

Comment on attachment 174502 [details] [diff] [review] patch >Index: xpcom/io/nsLocalFileWin.cpp >+ char *pathCopy = ToNewCString(mFollowSymlinks ? mResolvedPath : mWorkingPath); >+ if (!pathCopy) >+ return NS_ERROR_OUT_OF_MEMORY; do you really need to malloc a copy of the buffer? i don't see any uses below that modify the string. how about this instead: const char *path = mFollowSymlinks ? mResolvedPath.get() : mWorkingPath.get(); or, did i miss something? >+ char shortPath[MAX_PATH]; >+ ::GetShortPathName(pathCopy, shortPath, sizeof(shortPath)); >+ if (strlen(shortPath) >= 126) perhaps shortPath can be |char shortPath[126];| then, right? >+ nsCAutoString field(aField); >+ TCHAR subBlock[MAX_PATH]; >+ wsprintf(subBlock, "\\StringFileInfo\\%04x%04x\\%s", >+ (i == 0 ? translate[0].wLanguage : ::GetUserDefaultLangID()), >+ translate[0].wCodePage, field.get()); I think you should use PR_snprintf here instead. Also, TCHAR is char in Mozilla. So, you should just assume that. not having to worry about freeing pathCopy should help you reduce overall indenting too :) people have historically been resistant to adding nsILocalFileWin, but I think this is a fine use case for adding it.

Comment on attachment 175052 [details] [diff] [review] patch >Index: xpcom/build/Makefile.in >-EXTRA_DSO_LDOPTS += $(call EXPAND_LIBNAME,shell32 ole32 uuid) >+EXTRA_DSO_LDOPTS += $(call EXPAND_LIBNAME,shell32 ole32 uuid Version) nit: file names are case insensitive, and style seems to be to prefer lowercase. i'm not sure anyone cares though. >Index: xpcom/io/nsLocalFileWin.cpp >+NS_IMPL_THREADSAFE_ISUPPORTS3(nsLocalFile, nsILocalFileWin, nsILocalFile, nsIFile) nit: I'd put nsILocalFileWin last in the list. Most consumers QI to nsILocalFile. Optimize for it. >+ // Cast away const-ness here because WinAPI functions don't understand it, >+ // the path is used for [in] parameters only however so it's safe. >+ char *path = NS_CONST_CAST(char*, mFollowSymlinks ? mResolvedPath.get() : mWorkingPath.get()); nit: wrap long lines to 80 chars >+ DWORD size = ::GetFileVersionInfoSize(path, &dummy); >+ if (size) >+ { why not early return if |size == 0|? also, it looks like you'll return a successful rv in this case. is that intended? if you return early then you can reduce indenting, which is a good thing :) >+ void* ver = malloc(size); >+ if (ver) >+ { >+ memset(ver, 0, size); nit: use calloc instead if you want the buffer initialized to zero. >+ >+ if (::GetFileVersionInfo(path, 0, size, ver)) >+ { >+ LANGANDCODEPAGE* translate; >+ UINT pageCount; >+ BOOL queryResult = ::VerQueryValue(ver, "\\VarFileInfo\\Translation", >+ (void**)&translate, &pageCount); >+ if (queryResult && translate) >+ { >+ for (PRInt32 i = 0; i < 2; ++i) >+ { >+ nsCAutoString field(aField); perhaps aField should be |const char *| instead so you don't have to copy it here. i presume that aField is always ASCII-valued, so this should be fine. if you want to stick with in ACString, then please change this to use PromiseFlatCString instead, like so: const nsPromiseFlatCString &flat = PromiseFlatCString(aField); const char *field = flat.get(); >+ } >+ else >+ rv = NS_ERROR_OUT_OF_MEMORY; >+ } >+ >+ return rv; >+} the error handling seems fishy in this code. it seems like there are several places where you could end up returning NS_OK without setting the out param. is that intended? should it throw an exception in all "failure" cases?

Comment on attachment 175064 [details] [diff] [review] patch, revised >+ free(ver); >+ ver = nsnull; >+ >+ return rv; >+} nit: no need to null out |ver| since you are leaving this function scope. i'm sure all compilers would eliminate that line of code, so it doesn't really matter. sr=darin

(In reply to comment #41) > nit: file names are case insensitive, and style seems to be to > prefer lowercase. i'm not sure anyone cares though. That probably matters when cross-compiling from linux to windows...

Comment on attachment 175064 [details] [diff] [review] patch, revised >Index: xpcom/io/nsILocalFileWin.idl >Index: xpcom/io/nsLocalFileWin.cpp >@@ -990,16 +991,87 @@ nsLocalFile::SetNativeLeafName(const nsA indentation is wrong: >+typedef struct { >+ WORD wLanguage; >+ WORD wCodePage; >+} LANGANDCODEPAGE;