Converted implicit char API calls to explicit wide character API calls. Added explicit PRUnichar CheckVersion() function and PRUnichar version of NS_CompareVersions Added NextChar() function if MOZILLA_INTERNAL_API is defined

Comment on attachment 338711 [details] [diff] [review] WinCE XPCOM Explicit Unicode API changes why this: +#if defined(XP_WIN) +#include <tchar.h> +#include "nsString.h" +#endif + Why the explicit cast here: + if(GetModuleFileNameW(GetModuleHandleW(L"xpcom.dll"), (LPWCH)executable, MAX_PATH) && avoid the casts as they can hide problems. CreateProcessW has the same casts. Does the stack walking code actually work on wince -- or are these just narrow->wide changes. bsmedberg needs to review these changes after you post another patch.

Same attachment as before, removed WCHAR casts as per dougt request.

Comment on attachment 339982 [details] [diff] [review] Same attachment, with some WCHAR casts removed doug said pass this on to stuart for review

Can't we move completely to unicode versions? Why do we need to keep the narrow versions around too?

Comment on attachment 339982 [details] [diff] [review] Same attachment, with some WCHAR casts removed this looks ok to me... you should be able to get rid of the +#include <tchar.h> and remove the ifdef around the nsString.h includes i believe

Comment on attachment 339982 [details] [diff] [review] Same attachment, with some WCHAR casts removed >+static PRBool >+CheckVersion(const PRUnichar* toCheck, >+ const GREVersionRange *versions, >+ PRUint32 versionsLength); > static PRBool > CheckVersion(const char* toCheck, > const GREVersionRange *versions, > PRUint32 versionsLength); Duplicating this code seems bad... can you just template it instead? >diff --git a/xpcom/glue/nsVersionComparator.cpp b/xpcom/glue/nsVersionComparator.cpp >+#include <wchar.h> >+#ifdef MOZILLA_INTERNAL_API >+#include "nsString.h" >+#else >+#include "nsStringAPI.h" >+#endif replace this with #include "nsStringGlue.h" > > struct VersionPart { > PRInt32 numA; >@@ -49,7 +55,24 @@ > PRInt32 numC; > > char *extraD; // null-terminated >+ > }; >+ >+ >+struct VersionPartW { more templates >diff --git a/xpcom/glue/standalone/nsGlueLinkingWin.cpp b/xpcom/glue/standalone/nsGlueLinkingWin.cpp >+ const PRUnichar* xpcomFile = >+ NS_ConvertUTF8toUTF16(aXpcomFile).get(); 1) It's hard to believe that aXpcomFile is UTF8... I'm pretty sure in most cases it won't be. If it is, we need to re-document a bunch of stuff in nsXPCOMGlue.h, and fix the relevant callers. 2) We can't use mozilla string code here, because we haven't loaded XPCOM yet and so all string functions (and even basic functions like NS_Alloc) will fail. >diff --git a/xpcom/obsolete/component/Makefile.in b/xpcom/obsolete/component/Makefile.in >--- a/xpcom/obsolete/component/Makefile.in >+++ b/xpcom/obsolete/component/Makefile.in >@@ -49,7 +49,9 @@ > SHORT_LIBNAME = xpcomctc > endif > >+ifndef MINIMO > EXPORT_LIBRARY = 1 >+endif > IS_COMPONENT = 1 > GRE_MODULE = 1 > LIBXUL_LIBRARY = 1 I can't see why you'd touch this code... only Thunderbird if anyone is still building it... and wasn't MINIMO removed in any case? >diff --git a/xpcom/string/public/nsUTF8Utils.h b/xpcom/string/public/nsUTF8Utils.h I don't understand what the changes in this file are for. >diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp >-static int assembleCmdLine(char *const *argv, char **cmdLine) >+static int assembleCmdLine(char *const *argv, PRUnichar **cmdLine) You're zero-extending the characters in argv. This is incorrect.

(In reply to comment #6) > (From update of attachment 339982 [details] [diff] [review]) > >+static PRBool > >+CheckVersion(const PRUnichar* toCheck, > >+ const GREVersionRange *versions, > >+ PRUint32 versionsLength); > > static PRBool > > CheckVersion(const char* toCheck, > > const GREVersionRange *versions, > > PRUint32 versionsLength); > > Duplicating this code seems bad... can you just template it instead? Agreed, but I don't see how to use templates without a pretty major rewrite. > >-static int assembleCmdLine(char *const *argv, char **cmdLine) > >+static int assembleCmdLine(char *const *argv, PRUnichar **cmdLine) > > You're zero-extending the characters in argv. This is incorrect. not sure I understand this comment, can you elaborate?

You're assigning a char* to a PRUnichar* without any conversion at all (in assembleCmdLine). The character set of the char* isn't documented but is by convention the "native" character set. Please split the nsprocess change to a different bug, because it's going to be a more involved problem to fix.

Comment on attachment 346946 [details] [diff] [review] removed nsCommonProcess diff --git a/xpcom/glue/nsVersionComparator.cpp b/xpcom/glue/nsVersionComparator.cpp #include <stdlib.h> #include <string.h> +#include <wchar.h> +#include "nsStringGlue.h" You'll need #ifdef XP_WIN here and elsewhere in this file. The wide version of NS_CompareVersion should only be available on windows.

Comment on attachment 347231 [details] [diff] [review] nsCommonProcess changes >diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp Please document that the wideCmdLine parameter in assembleCmdLine must be freed with PR_Free. >-static int assembleCmdLine(char *const *argv, char **cmdLine) >+static int assembleCmdLine(char *const *argv, PRUnichar **wideCmdLine) >- retVal = CreateProcess(NULL, >+ retVal = CreateProcessW(NULL, > // const_cast<char*>(mTargetPath.get()), > cmdLine, nit, please re-indent

pushed http://hg.mozilla.org/mozilla-central/rev/ce8fbd7d222e and http://hg.mozilla.org/mozilla-central/rev/0f1b8ab7cd8e

I backed out http://hg.mozilla.org/mozilla-central/rev/ce8fbd7d222e because it caused unittest and mochitest failures: Error: uncaught exception: [Exception... "Component returned failure code: 0x80520003 (NS_ERROR_FILE_EXECUTION_FAILED) [nsILocalHandlerApp.launchWithURI]" nsresult: "0x80520003 (NS_ERROR_FILE_EXECUTION_FAILED)" location: "JS frame :: http://localhost:8888/tests/uriloader/exthandler/tests/mochitest/handlerApps.js :: test :: line 110" data: no] while running http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml

Comment on attachment 351546 [details] [diff] [review] fixed mochitest failure >diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp > #if defined(XP_WIN) >-static int assembleCmdLine(char *const *argv, char **cmdLine) >+// out param must be freed by caller >+static int assembleCmdLine(char *const *argv, PRUnichar **wideCmdLine) Please be specific in the comment: Out param `wideCmdLine` must be PR_Freed by the caller. >+ char *p, *q, *tmpCmdLine, **cmdLine; As discussed on IRC, the *tmpCmdLine, **cmdLine change is just confusing. Please just use *cmdLine and remove the indirection below.

Comment on attachment 346946 [details] [diff] [review] removed nsCommonProcess a191=beltzner

Comment on attachment 351591 [details] [diff] [review] gets rid of tmpCmdLine a191=beltzner

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/665fb5dfd8be http://hg.mozilla.org/releases/mozilla-1.9.1/rev/66c7e78d564e

(In reply to comment #21) > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/66c7e78d564e This broke runtime GRE discovery on Windows. Details in bug 492948.