Doug Turner (:dougt) Reporter
	Description • 17 years ago

We want to be using wide API calls where ever possible.

	Doug Turner (:dougt) Reporter
	Comment 1 • 17 years ago

lots of problem with this patch, but it shows what we are attempting.

note that this also have windows mobile changes which are not for this bug.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 2 • 17 years ago

more to come

	Brad Lassey [:blassey] (use needinfo?)
	Comment 3 • 17 years ago

also note that I'm building with plugins and printing disabled for this patch

	Brad Lassey [:blassey] (use needinfo?)
	Comment 4 • 17 years ago

the question being how to enable building with unicode defined.  Right now there are 3 lines at the end of config.mk in config and nsprpub/config that add the define.  Should it be a mozconfig flag?

	Brad Lassey [:blassey] (use needinfo?)
	Comment 8 • 17 years ago

This patch seems to break the extension search in the add-on manager.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 9 • 17 years ago

The issue with the add-on manger appears to be a dependency that the add-on manager has on @mozilla.org/plugin/host;1.  There is going to need to be some configure magic to undefine UNICODE in plugins and printing.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 10 • 17 years ago

This shouldn't be "turned on" (i.e. actually defining UNICODE) yet.  Just found some places in nspr where char* is being passed to system calls, which for some reason is only a compiler warning and not an error.

	Doug Turner (:dougt) Reporter
	Comment 11 • 17 years ago

did you define MOZ_UNICODE?

	Doug Turner (:dougt) Reporter
	Comment 12 • 17 years ago

need to verify if this builds without UNICODE defined.

	(not currently active) Ted Mielczarek
	Comment 13 • 17 years ago

NSS isn't Unicode safe on Windows either, see bug 413171.

	Benjamin Smedberg
	Comment 18 • 17 years ago

Comment on attachment 305989 [details] [diff] [review]
changes based on stuart's comments

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp
>@@ -1410,7 +1410,7 @@
> {
>   HRESULT result;
> 
>-  IUniformResourceLocator* urlLink = nsnull;
>+  IUniformResourceLocatorW* urlLink = nsnull;
>   result = ::CoCreateInstance(CLSID_InternetShortcut, NULL, CLSCTX_INPROC_SERVER,
>                               IID_IUniformResourceLocator, (void**)&urlLink);

I think you need to modify IID_IUniformResourceLocator also.

>-          *aOutURL = PL_strdup(lpTemp);
>-
>+          *aOutURL = *aOutURL = (char*)ToNewUTF8String(nsDependentString(lpTemp));

Note, this changes the allocator for the outparam (was NSPR, now XPCOM)... but the old code was incorrect, and I've verified that the (one) caller handles the XPCOM allocator correctly (passed to getter_Copies).

>Index: browser/components/shell/src/nsWindowsShellService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.cpp,v
>retrieving revision 1.51
>diff -u -w -r1.51 nsWindowsShellService.cpp
>--- browser/components/shell/src/nsWindowsShellService.cpp	26 Nov 2007 19:56:52 -0000	1.51
>+++ browser/components/shell/src/nsWindowsShellService.cpp	27 Feb 2008 11:18:58 -0000
>@@ -409,7 +409,7 @@
>   STARTUPINFO si = {sizeof(si), 0};
>   PROCESS_INFORMATION pi = {0};
> 
>-  BOOL ok = CreateProcess(NULL, (LPSTR)appHelperPath.get(), NULL, NULL,
>+  BOOL ok = CreateProcess(NULL, (LPTSTR)appHelperPath.get(), NULL, NULL,

This is incorrect, you're casting something that's always a char* to LPTSTR, which could be a WCHAR*.

It should be pretty trivial to convert this code to use CreateProcessW.

>Index: embedding/browser/activex/src/plugin/XPCDocument.cpp

>@@ -1915,7 +1915,7 @@
>                 NS_SUCCEEDED(baseURI->GetSpec(spec)))
>             {
>                 USES_CONVERSION;
>-                if (FAILED(CreateURLMoniker(NULL, T2CW(spec.get()), &baseURLMoniker)))
>+                if (FAILED(CreateURLMoniker(NULL, A2CW((char*)spec.get()), &baseURLMoniker)))

T2CW is correct, but I don't like the cast. Why is it necessary?

>Index: gfx/src/thebes/nsSystemFontsWin.cpp

>-  if (aIsWide)
>     memcpy(name, ptrLogFont->lfFaceName, LF_FACESIZE*2);
>-  else {
>-    MultiByteToWideChar(CP_ACP, 0, ptrLogFont->lfFaceName,
>-      strlen(ptrLogFont->lfFaceName) + 1, name, sizeof(name)/sizeof(name[0]));
>-  }

The "aIsWide" parameter is now unused, and I'm pretty sure it should be removed altogether to avoid confusion.

>Index: gfx/thebes/src/gfxWindowsSurface.cpp

>+    wchar_t *title = (wchar_t*)(nsPromiseFlatString(titleStr).get());

This code won't work. The nsPromiseFlatString temporary goes out of scope immediately, while you're storing the pointer for later use. You need to make the temporary last as long as "title". Same thing below with "docName"

Secondly, .get() returns a const PRUnichar* and you're casting to wchar_t*... is that necessary? can't title be "const wchar_t*"? 

>Index: intl/uconv/src/nsWinCharset.cpp

>Index: modules/libpr0n/decoders/icon/win/nsIconChannel.cpp

>-  char fileNativePath[MAX_PATH];
>-  nsCAutoString fileNativePathStr;
>-  aFile->GetNativePath(fileNativePathStr);
>-  ::GetShortPathName(fileNativePathStr.get(), fileNativePath, sizeof(fileNativePath));
>+  wchar_t fileNativePath[MAX_PATH];
>+  nsAutoString fileNativePathStr;
>+  aFile->GetPath(fileNativePathStr);
>+  ::GetShortPathNameW(fileNativePathStr.get(), fileNativePath, sizeof(fileNativePath));

This code was already broken, so you don't need to fix it here, but on systems with 8.3 shortnames disabled the GetShortPathName function will fail... i'm not sure why we need the shortname here, but if we really do, we should have a fallback or error check. Can you file a bug about this?

>@@ -342,7 +342,9 @@
> 
>   // Not a special folder, or something else failed above.
>   if (!shellResult)
>-    shellResult = ::SHGetFileInfo(filePath.get(), FILE_ATTRIBUTE_ARCHIVE, &sfi, sizeof(sfi), infoFlags);
>+      shellResult = ::SHGetFileInfoW(
>+                                    NS_ConvertUTF8toUTF16(filePath).get(),
>+                                    FILE_ATTRIBUTE_ARCHIVE, &sfi, sizeof(sfi), infoFlags);

This hunk is incorrect. Either we should make "filePath" a unicode string, or we should be doing NS_ConvertNativeToUTF16... filePath is definitely not UTF8.

>Index: netwerk/base/src/nsSocketTransport2.cpp

>-            tryAgain = nsNativeConnectionHelper::OnConnectionFailed(SocketHost().get());
>+            tryAgain = nsNativeConnectionHelper::OnConnectionFailed(
>+		    NS_ConvertUTF8toUTF16(SocketHost()).get()
>+);

This change needs r=biesi or a necko peer... I'm not sure whether SocketHost is always UTF8.

>Index: nsprpub/pr/include/md/_win95.h

I am not a NSPR peer. Please separate out the NSPR changes into a separate patch for review by wtc.

>Index: security/nss/cmd/shlibsign/shlibsign.c

I am not an NSS peer, please separate out NSS changes for review by wtc/kaie/nelsonb.

>Index: toolkit/components/startup/src/nsUserInfoWin.cpp

>-    *aUsername = nsCRT::strdup(username);
>-    
>+    *aUsername = (char*)NS_ConvertUTF16toUTF8( nsCRT::strdup(username)).get();
>     if (*aUsername) return NS_OK;

This is incorrect. I think you want

*aUsername = ToNewUTF8String(username);

>Index: toolkit/xre/nsNativeAppSupportWin.cpp

> // Simple Win32 mutex wrapper.
> struct Mutex {
>-    Mutex( const char *name )
>+    Mutex( const wchar_t *name )
>         : mName( name ),
>           mHandle( 0 ),
>           mState( -1 ) {
>-        mHandle = CreateMutexA( 0, FALSE, mName.get() );
>+        mHandle = CreateMutex( 0, FALSE, mName.get() );

This is where we get into concerns about compatibility. This mutex is supposed to work with old versions of FF also. If an old version does CreateMutexA("foo") and a new version does CreateMutexW("foo"), are those the same mutex? If not, then we'll need an explicit WINCE ifdef and keep the narrow versions of this code on the desktop.

I'm going to need to review the toolkit/xre changes separately... can you create a patch just for them?


>Index: uriloader/exthandler/win/nsMIMEInfoWin.cpp

>-        sinfo.lpVerb   = (LPCSTR)&cmdVerb;
>+        sinfo.lpVerb   = (LPTSTR)&cmdVerb;

This cast is incorrect unless cmdVerb is WCHAR, and I'm a little confused by the use of & on a type that is already an array. const_cast<LPCSTR>(cmdVerb) is better code.

>Index: widget/src/windows/nsBidiKeyboard.cpp

>-  NS_ASSERTION((strlen(mCurrentLocaleName) < KL_NAMELENGTH), 
>+  NS_ASSERTION((_tcslen(mCurrentLocaleName) < KL_NAMELENGTH), 
>     "GetKeyboardLayoutName return string length >= KL_NAMELENGTH");

wcslen here

>-  NS_ASSERTION((strlen(mRTLKeyboard) < KL_NAMELENGTH), 
>+  NS_ASSERTION((_tcslen(mRTLKeyboard) < KL_NAMELENGTH), 
>     "mLTRKeyboard has string length >= KL_NAMELENGTH");
>-  NS_ASSERTION((strlen(mLTRKeyboard) < KL_NAMELENGTH), 
>+  NS_ASSERTION((_tcslen(mLTRKeyboard) < KL_NAMELENGTH), 
>     "mRTLKeyboard has string length >= KL_NAMELENGTH");

and here

>-  NS_ASSERTION((strlen(localeName) < KL_NAMELENGTH), 
>+  NS_ASSERTION((_tcslen(localeName) < KL_NAMELENGTH), 
>     "GetKeyboardLayout return string length >= KL_NAMELENGTH");

and here

>Index: widget/src/windows/nsClipboard.cpp

>     // Display the string.
>-    MessageBox( NULL, (const char *)lpMsgBuf, "GetLastError", MB_OK|MB_ICONINFORMATION );
>+    MessageBoxW( NULL, (LPCWSTR)lpMsgBuf, L"GetLastError", MB_OK|MB_ICONINFORMATION );

This change is incorrect without also changing to FormatMessageW directly above.

>Index: widget/src/windows/nsDeviceContextSpecWin.h

>-  void GetDriverName(char *&aDriverName) const   { aDriverName = mDriverName;     }
>-  void GetDeviceName(char *&aDeviceName) const   { aDeviceName = mDeviceName;     }
>+  void GetDriverName(TCHAR *&aDriverName) const   { aDriverName = mDriverName;     }
>+  void GetDeviceName(TCHAR *&aDeviceName) const   { aDeviceName = mDeviceName;     }

Can these and below be WCHAR?

>Index: widget/src/windows/nsSound.cpp

>+#include "nsString.h"
>+#include "nsXPCOMStrings.h"

You shouldn't need nsXPCOMStrings.h in internal code, see below

>-    nsCAutoString nativeSoundAlias;
>-    NS_CopyUnicodeToNative(aSoundAlias, nativeSoundAlias);
>-    ::PlaySound(nativeSoundAlias.get(), nsnull, SND_ALIAS | SND_ASYNC);
>+    const PRUnichar* data;
>+    NS_StringGetData(aSoundAlias, &data);
>+    ::PlaySoundW(data, nsnull, SND_ALIAS | SND_ASYNC);

::PlaySoundW(PromiseFlatString(aSoundAlias).get(), ...)

>Index: xpcom/glue/nsGREGlue.cpp

Pretty much all the code in xpcom/glue is incorrect: you are doing utf8->utf16 conversions instead of native->utf16 conversions, and you aren't allowed to use mozilla's string functions to find out where Mozilla is. Please separate this part out and we can discuss the proper solution.

>Index: xpcom/io/SpecialSystemDirectory.cpp

>+    gShell32DLLInst = LoadLibraryA("Shell32.dll");

I'll bet you meant LoadLibraryW(L"Shell32.dll");

>Index: xpcom/io/nsLocalFileWin.cpp

>Index: xpcom/stub/nsXPComStub.cpp

> EXPORT_XPCOM_API(nsresult)
>-NS_GetFrozenFunctions(XPCOMFunctions *functions, const char* /* libraryPath */)
>+NS_GetFrozenFunctions(XPCOMFunctions *functions, const char* libraryPath)

The old way prevents a strict warning about unused variables.

>Index: xpcom/threads/nsProcessCommon.cpp

More tchar stuff... we should convert all the way to unicode if we're going to. It's not clear what effect you're trying to achieve or what casts might end up bad from this change.

	Benjamin Smedberg
	Comment 19 • 17 years ago

Comment on attachment 306068 [details] [diff] [review]
switched from wchar_t to PRUnichar and integrated plugin changes 

Clearing review: I'd like to get the changes from the last patch separately in so I don't have to wade through a bunch of stuff I've already reviewed.

	Wan-Teh Chang
	Comment 24 • 17 years ago

Comment on attachment 306170 [details] [diff] [review]
nss changes

This patch contains only a whitespace change.  Did you attach the wrong patch?

	Benjamin Smedberg
	Comment 26 • 17 years ago

Comment on attachment 306167 [details] [diff] [review]
changes based on bsmedberg's comments

>Index: config/config.mk

Please just leave this out for now.

>Index: embedding/browser/activex/src/plugin/XPConnect.cpp

>-        HMODULE hlib = ::LoadLibrary("xpcom.dll");
>+      HMODULE hlib = ::LoadLibraryW(L"xpcom.dll");

nit, untabify

>Index: netwerk/base/src/nsSocketTransport2.cpp


>-        if (autodialEnabled)
>-            tryAgain = nsNativeConnectionHelper::OnConnectionFailed(SocketHost().get());
>+        if (autodialEnabled){
>+          // SocketHost returns a nsCString, which is 8-bit
>+          tryAgain = nsNativeConnectionHelper::OnConnectionFailed(
>+		    NS_ConvertUTF8toUTF16(SocketHost()).get());
>+	}

Please attach these bits as a separate patch for review by biesi or a necko peer.

>Index: netwerk/protocol/file/src/nsFileProtocolHandler.cpp


>-    IUniformResourceLocator* urlLink = nsnull;
>+    IUniformResourceLocatorW* urlLink = nsnull;
>     result = ::CoCreateInstance(CLSID_InternetShortcut, NULL, CLSCTX_INPROC_SERVER,
>                                 IID_IUniformResourceLocator, (void**)&urlLink);

Whoops, didn't catch this the first time: need to use the W form of the IID here.

>Index: rdf/datasource/src/nsFileSystemDataSource.cpp

>     PRInt32         driveType;
>-    char            drive[32];
>+    PRUnichar            drive[32];
>     PRInt32         volNum;

please line these up.


>Index: widget/src/windows/nsBidiKeyboard.cpp


>   if (IsRTLLanguage(locale)) {
>-    strncpy(mRTLKeyboard, localeName, KL_NAMELENGTH);
>+      swprintf(mRTLKeyboard, localeName, KL_NAMELENGTH);
>     mRTLKeyboard[KL_NAMELENGTH-1] = '\0'; // null terminate
>   }
>   else {
>-    strncpy(mLTRKeyboard, localeName, KL_NAMELENGTH);
>+      swprintf( mLTRKeyboard, localeName, KL_NAMELENGTH);
>     mLTRKeyboard[KL_NAMELENGTH-1] = '\0'; // null terminate

nit, these should be two-space indented

>Index: widget/src/windows/nsClipboard.cpp


>-  else
>-    format = ::RegisterClipboardFormat(aMimeStr);
>+  else{
>+
>+  format = ::RegisterClipboardFormatW(NS_ConvertASCIItoUTF16(aMimeStr).get());
>+}

nit, follow existing style, avoid braces around the single-line else clause, and indent the clause properly

>Index: widget/src/windows/nsDataObj.h

> #ifndef CFSTR_INETURLA
>-#define CFSTR_INETURLA    "UniformResourceLocator"
>+#define CFSTR_INETURLA    TEXT("UniformResourceLocator")
> #endif
> #ifndef CFSTR_INETURLW
>-#define CFSTR_INETURLW    "UniformResourceLocatorW"
>+#define CFSTR_INETURLW    TEXT("UniformResourceLocatorW")

No TEXT here... either "" or L""

>Index: widget/src/windows/nsDeviceContextSpecWin.cpp


>+      PRUnichar* deviceName;
>+      PRUNichar* driverName;

typo

>Index: widget/src/windows/nsDeviceContextSpecWin.h

>-  void GetDriverName(char *&aDriverName) const   { aDriverName = mDriverName;     }
>-  void GetDeviceName(char *&aDeviceName) const   { aDeviceName = mDeviceName;     }
>+  void GetDriverName(TCHAR *&aDriverName) const   { aDriverName = mDriverName;     }
>+  void GetDeviceName(TCHAR *&aDeviceName) const   { aDeviceName = mDeviceName;     }

no tchars

>Index: widget/src/windows/nsFilePicker.cpp


>-      MessageBox(ofn.hwndOwner,
>+      MessageBoxW(ofn.hwndOwner,
>                  0,
>-                 "The filepicker was unexpectedly closed by Windows.",
>+                 L"The filepicker was unexpectedly closed by Windows.",
>                  MB_ICONERROR);

nit, please re-indent the secondary lines


>Index: widget/src/windows/nsWindow.h
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/windows/nsWindow.h,v
>retrieving revision 3.246
>diff -u -1 -0 -r3.246 nsWindow.h
>--- widget/src/windows/nsWindow.h	19 Dec 2007 19:40:20 -0000	3.246
>+++ widget/src/windows/nsWindow.h	28 Feb 2008 01:08:25 -0000
>@@ -98,20 +98,27 @@
> const LPCWSTR kWClassNameContent      = L"MozillaContentWindowClass";
> const LPCWSTR kWClassNameContentFrame = L"MozillaContentFrameWindowClass";
> const LPCWSTR kWClassNameGeneral      = L"MozillaWindowClass";
> const LPCWSTR kWClassNameDialog       = L"MozillaDialogClass";
> const LPCSTR kClassNameHidden         = "MozillaHiddenWindowClass";
> const LPCSTR kClassNameUI             = "MozillaUIWindowClass";
> const LPCSTR kClassNameContent        = "MozillaContentWindowClass";
> const LPCSTR kClassNameContentFrame   = "MozillaContentFrameWindowClass";
> const LPCSTR kClassNameGeneral        = "MozillaWindowClass";
> const LPCSTR kClassNameDialog         = "MozillaDialogClass";
>+const LPCTSTR kTClassNameHidden       = TEXT("MozillaHiddenWindowClass");
>+const LPCTSTR kTClassNameUI           = TEXT("MozillaUIWindowClass");
>+const LPCTSTR kTClassNameContent      = TEXT("MozillaContentWindowClass");
>+const LPCTSTR kTClassNameContentFrame = TEXT("MozillaContentFrameWindowClass");
>+const LPCTSTR kTClassNameGeneral      = TEXT("MozillaWindowClass");
>+const LPCTSTR kTClassNameDialog       = TEXT("MozillaDialogClass");

I don't understand why we need both style here... if the explanation is complicated, then Neil or a widget peer should review all this code under widget/src/windows


>Index: xpcom/build/nsXPCOMPrivate.h

>+#define LXPCOM_SEARCH_KEY L"PATH"
>+#define LGRE_CONF_NAME    L"gre.config"
>+#define LGRE_WIN_REG_LOC  L"Software\\mozilla.org\\GRE"
>+#define LXPCOM_DLL        L"xpcom.dll"
>+#define LXUL_DLL          L"xul.dll"

Leave this change out until we understand the scope/plan of the xpcom/glue changes.

>Index: xpcom/glue/nsVersionComparator.cpp

as well as everything under xpcom/glue


>Index: xpcom/threads/nsProcessCommon.cpp

>-    retVal = CreateProcess(NULL,
>+    retVal = CreateProcessW(NULL,
>                            // const_cast<char*>(mTargetPath.get()),
>                            cmdLine,
>                            NULL,  /* security attributes for the new

nit, reindent

>Index: xpcom/windbgdlg/Makefile.in

>+OS_LIBS		+= Shell32.lib

all-lowercase so we don't break the mingw build

>Index: xpcom/windbgdlg/windbgdlg.cpp

>+#include <Shellapi.h>

Probably also lowercase for mingw

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 27 • 17 years ago

Comment on attachment 306167 [details] [diff] [review]
changes based on bsmedberg's comments

clearing this nom as there seems to be a new patch ...

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 28 • 17 years ago

Comment on attachment 306167 [details] [diff] [review]
changes based on bsmedberg's comments

a=beltzner

	Damon Sicore (:damons)
	Comment 29 • 17 years ago

Discussed this with Doug.  Not blocking 1.9, but wanted-1.9.0.x.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 30 • 17 years ago

This seems like a really, really bad thing to take this late in the 1.9 cycle.  Why did we want this?

	David Baron :dbaron:
	Comment 31 • 17 years ago

Comment on attachment 306167 [details] [diff] [review]
changes based on bsmedberg's comments

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp
>+          *aOutURL = *aOutURL = (char*)ToNewUTF8String(nsDependentString(lpTemp));

As pointed out on IRC, this has two problems:
 * you're assigning twice, unnecessarily
 * you have an unneeded cast that could just mask errors in the future

	Stuart Parmenter
	Comment 32 • 17 years ago

there are several files in here that aren't actually built anymore.  old gfx for example.

	Stuart Parmenter
	Comment 33 • 17 years ago

also, there are a lot of weird indention changes in here where it looks like you search and replaced without changing spacing.

	David Baron :dbaron:
	Comment 34 • 17 years ago

This caused compilation errors in debug builds:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1204755960.1204758040.12955.gz

Also, should windows.h really be included in nsNativeConnectionHelper.h ?

And, for the record, the test failure in
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1204754076.1204756364.8959.gz
was (when viewing the log as ISO-8859-1 and pasting it here, which is UTF-8):
*** 40118 ERROR FAIL | parse+compute+serialize(font-family) should be idempotent for 'font: caption' | got "serif", expected "ååŒ æ•¨æ±¬ä æ¬ïˆ€ïˆ¸ï‰ˆï‰ˆæ®á€€Ð¾ê”ྀá\\Ä" | /tests/layout/style/test/test_value_storage.html

	Doug Turner (:dougt) Reporter
	Comment 36 • 17 years ago

Comment on attachment 308503 [details] [diff] [review]
same patch that I checked in, but it doesn't fail the mochitests now

Index: db/mork/src/morkFile.cpp

do we even care about mork anymore?

Index: extensions/auth/nsAuthSSPI.cpp

 static HINSTANCE                 sspi_lib; 
-static PSecurityFunctionTable    sspi;
+static PSecurityFunctionTableW    sspi;

Match he spacing


Index: gfx/src/thebes/nsSystemFontsWin.cpp

   name[0] = 0;
-  if (aIsWide)
     memcpy(name, ptrLogFont->lfFaceName, LF_FACESIZE*2);
-  else {

fix spacing


Index: gfx/src/windows/nsFontMetricsWin.cpp

   memset(aGlyphMetrics, 0, sizeof(GLYPHMETRICS)); // UMR: bug 46438
-  if (eGlyphAgent_UNKNOWN == mState) { // first time we have been in this function
-    // see if this platform implements GetGlyphOutlineW()
-    DWORD len = GetGlyphOutlineW(aDC, aChar, GGO_METRICS, aGlyphMetrics, 0, nsnull, &mMat);
-    if (GetLastError() == ERROR_CALL_NOT_IMPLEMENTED) {
-      // next time, we won't bother trying GetGlyphOutlineW()
-      mState = eGlyphAgent_ANSI;
-    }
-    else {
-      // all is well with GetGlyphOutlineW(), we will be using it from now on
       mState = eGlyphAgent_UNICODE;
-      return len;



Index: intl/uconv/src/nsWinCharset.cpp

   nsAutoString acp_key(NS_LITERAL_STRING("acp."));
-  acp_key.AppendWithConversion(acp_name);
+  acp_key.Append(acp_name);


Is this the right conversion?  shoult it be NS_ConvertUTF8toUTF16 or something?  



Index: modules/libpr0n/decoders/icon/win/nsIconChannel.cpp


-    shellResult = ::SHGetFileInfo(filePath.get(), FILE_ATTRIBUTE_ARCHIVE, &sfi, sizeof(sfi), infoFlags);
+    shellResult = ::SHGetFileInfoW(
+                                    NS_ConvertUTF8toUTF16(filePath).get(),
+                                    FILE_ATTRIBUTE_ARCHIVE, &sfi, sizeof(sfi), infoFlags);




Index: modules/libpr0n/decoders/icon/win/nsIconChannel.h


Not sure why you made this change.  why isn't using the abstract class correct?


modules/plugin/*

Did not review since I authored most of it. :-)




Index: uriloader/exthandler/win/nsOSHelperAppService.cpp


      HKEY hKey;
-     LONG err = ::RegOpenKeyEx(HKEY_CLASSES_ROOT, aProtocolScheme, 0,
+     LONG err = ::RegOpenKeyExA(HKEY_CLASSES_ROOT, aProtocolScheme, 0,
                                KEY_QUERY_VALUE, &hKey);


we want wide here, me thinks.



Index: widget/src/windows/nsBidiKeyboard.cpp


 
   if (IsRTLLanguage(locale)) {
-    strncpy(mRTLKeyboard, localeName, KL_NAMELENGTH);
+      swprintf(mRTLKeyboard, localeName, KL_NAMELENGTH);
     mRTLKeyboard[KL_NAMELENGTH-1] = '\0'; // null terminate
   }
   else {
-    strncpy(mLTRKeyboard, localeName, KL_NAMELENGTH);
+      swprintf( mLTRKeyboard, localeName, KL_NAMELENGTH);
     mLTRKeyboard[KL_NAMELENGTH-1] = '\0'; // null terminate
   }



Spacing is off a bit



Index: widget/src/windows/nsLookAndFeel.cpp

-  gShell32DLLInst = LoadLibrary("Shell32.dll");
+  gShell32DLLInst = LoadLibraryW(L"Shell32.dll");

and

Index: xpcom/io/SpecialSystemDirectory.cpp
     // so that we need to use GetProcAddress to get the pointer.
-    gShell32DLLInst = LoadLibrary("Shell32.dll");
+    gShell32DLLInst = LoadLibraryW(L"Shell32.dll");


^^^
in other places we use shell32.dll -- lets clean it up while we are here.



Index: widget/src/windows/nsSound.cpp


-    ::PlaySound(reinterpret_cast<const char*>(data), 0, flags);
+    ::PlaySoundA(reinterpret_cast<const char*>(data), 0, flags);

Could you add a comment stating the reason why the A version is used here (because |data| can be a string or blob)

	Brad Lassey [:blassey] (use needinfo?)
	Comment 37 • 17 years ago

>    name[0] = 0;
> -  if (aIsWide)
>      memcpy(name, ptrLogFont->lfFaceName, LF_FACESIZE*2);
> -  else {
> 
> fix spacing

Fixed, as well as switching to LOGFONTW
 
> 
> Index: gfx/src/windows/nsFontMetricsWin.cpp
> 
>    memset(aGlyphMetrics, 0, sizeof(GLYPHMETRICS)); // UMR: bug 46438
> -  if (eGlyphAgent_UNKNOWN == mState) { // first time we have been in this
> function
> -    // see if this platform implements GetGlyphOutlineW()
> -    DWORD len = GetGlyphOutlineW(aDC, aChar, GGO_METRICS, aGlyphMetrics, 0,
> nsnull, &mMat);
> -    if (GetLastError() == ERROR_CALL_NOT_IMPLEMENTED) {
> -      // next time, we won't bother trying GetGlyphOutlineW()
> -      mState = eGlyphAgent_ANSI;
> -    }
> -    else {
> -      // all is well with GetGlyphOutlineW(), we will be using it from now on
>        mState = eGlyphAgent_UNICODE;
> -      return len;
> 

Is there a comment here?

> 
> Index: intl/uconv/src/nsWinCharset.cpp
> 
>    nsAutoString acp_key(NS_LITERAL_STRING("acp."));
> -  acp_key.AppendWithConversion(acp_name);
> +  acp_key.Append(acp_name);
> 
> Is this the right conversion?  shoult it be NS_ConvertUTF8toUTF16 or something? 
> 

I believe this is the right thing, no conversion is needed, we're appending a wide char string to a wide char string

> 
> 
> 
> Index: modules/libpr0n/decoders/icon/win/nsIconChannel.h
> 
> 
> Not sure why you made this change.  why isn't using the abstract class correct?
> 
> 
> modules/plugin/*
> 
> Did not review since I authored most of it. :-)
> 
> 
> 
> 
> Index: uriloader/exthandler/win/nsOSHelperAppService.cpp
> 
> 
>       HKEY hKey;
> -     LONG err = ::RegOpenKeyEx(HKEY_CLASSES_ROOT, aProtocolScheme, 0,
> +     LONG err = ::RegOpenKeyExA(HKEY_CLASSES_ROOT, aProtocolScheme, 0,
>                                 KEY_QUERY_VALUE, &hKey);
> we want wide here, me thinks.
> 

Eventually, yes.  Will need to do a conversion, for now we can do this in the shunt I think.


> 
> 
> Index: widget/src/windows/nsBidiKeyboard.cpp
> 
> 
> 
>    if (IsRTLLanguage(locale)) {
> -    strncpy(mRTLKeyboard, localeName, KL_NAMELENGTH);
> +      swprintf(mRTLKeyboard, localeName, KL_NAMELENGTH);
>      mRTLKeyboard[KL_NAMELENGTH-1] = '\0'; // null terminate
>    }
>    else {
> -    strncpy(mLTRKeyboard, localeName, KL_NAMELENGTH);
> +      swprintf( mLTRKeyboard, localeName, KL_NAMELENGTH);
>      mLTRKeyboard[KL_NAMELENGTH-1] = '\0'; // null terminate
>    }
> 
> 
> 
> Spacing is off a bit
> 
> 
> 
> Index: widget/src/windows/nsLookAndFeel.cpp
> 
> -  gShell32DLLInst = LoadLibrary("Shell32.dll");
> +  gShell32DLLInst = LoadLibraryW(L"Shell32.dll");
> 
> and
> 
> Index: xpcom/io/SpecialSystemDirectory.cpp
>      // so that we need to use GetProcAddress to get the pointer.
> -    gShell32DLLInst = LoadLibrary("Shell32.dll");
> +    gShell32DLLInst = LoadLibraryW(L"Shell32.dll");
> 
> 
> ^^^
> in other places we use shell32.dll -- lets clean it up while we are here.
> 

I assume you mean it should be lowercase, I think that's needed for mingw32.

> 
> Index: widget/src/windows/nsSound.cpp
> 
> 
> -    ::PlaySound(reinterpret_cast<const char*>(data), 0, flags);
> +    ::PlaySoundA(reinterpret_cast<const char*>(data), 0, flags);
> 
> Could you add a comment stating the reason why the A version is used here
> (because |data| can be a string or blob)
> 

no, data is a string.  I've added the conversion. 

	Brad Lassey [:blassey] (use needinfo?)
	Comment 38 • 17 years ago

again, passes all mochitests

	Doug Turner (:dougt) Reporter
	Comment 39 • 17 years ago

Comment on attachment 308560 [details] [diff] [review]
updated based on dougt's comments [missed 1.9]

discussed on aim; carrying flag forward.

	Doug Turner (:dougt) Reporter
	Comment 41 • 17 years ago

Comment on attachment 308560 [details] [diff] [review]
updated based on dougt's comments [missed 1.9]

carrying bsmedberg sr flag forward.

	Damon Sicore (:damons)
	Comment 42 • 17 years ago

Comment on attachment 308560 [details] [diff] [review]
updated based on dougt's comments [missed 1.9]

a1.9+=damons

/me runs for cover in the nearest ditch.  Pops an iodine pill and pulls on radiation suit.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 43 • 17 years ago

I skimmed through the patch and didn't see anything wrong.

But no-one in this bug ever mentioned *why* we're doing this and I'm curious why.

	Doug Turner (:dougt) Reporter
	Comment 44 • 17 years ago

roc:
currently we have a "shunt" library that provides an implementation for narrow APIs on Windows Mobile.  As you may know, there are no narrow windows apis defined on windows mobile.  In this shunt, most of the implementations are simple text conversion, but some more involved.  To reduce this extra layer of conversation, as well as avoid #ifdefs outside of the shunt layer, we would like to use the wide version of windows apis. this patch also fixes uses of tchar (whos type vary based on the define of UNICODE).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 45 • 17 years ago

OK, that makes sense, thanks. I did not know that Windows Mobile only defines the W APIs. Strange they don't ship Uniscribe then...

	Doug Turner (:dougt) Reporter
	Comment 46 • 17 years ago

don't get me started....  although FreeType+HarfBuzz might be the right way to go.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 47 • 17 years ago

Comment on attachment 308564 [details] [diff] [review]
necko changes

netwerk/base/src/nsAutodialWin.cpp
+    PRUnichar* key = nsnull;
+    PRUnichar* val = nsnull;

those should really be const, can you change that? (even though that's not your fault)

              ::GetProcAddress(mhRASapi32, "RasEnumConnectionsA");
              ::GetProcAddress(mhRASapi32, "RasEnumEntriesA");

shouldn't those get changed to the W versions?

you need to change the callers of GetDefaultEntryName. the size argument of RegQueryValueExW is in bytes, not in characters.

+++ netwerk/base/src/nsSocketTransport2.cpp	11 Mar 2008 05:26:57 -0000
+          // SocketHost returns a nsCString, which is 8-bit

that's kind of obvious...

Why are you including windows.h from nsNativeConnectionHelper.h?

+++ netwerk/protocol/file/src/nsFileProtocolHandler.cpp	11 Mar 2008 05:26:58 -0000
                                 IID_IUniformResourceLocator, (void**)&urlLink);

you have to make that IID_IUniformResourceLocatorW right?

	Brad Lassey [:blassey] (use needinfo?)
	Comment 48 • 17 years ago

(In reply to comment #47)
> (From update of attachment 308564 [details] [diff] [review])
> netwerk/base/src/nsAutodialWin.cpp
> +    PRUnichar* key = nsnull;
> +    PRUnichar* val = nsnull;
> 
> those should really be const, can you change that? (even though that's not your
> fault)

sure thing

> 
>               ::GetProcAddress(mhRASapi32, "RasEnumConnectionsA");
>               ::GetProcAddress(mhRASapi32, "RasEnumEntriesA");
> 
> shouldn't those get changed to the W versions?

There actually is no W version of this call on desktop windows.  as a side note, there is no A version of this call on windows mobile, we need to do some ugly undef'ing and def'ing as a work around....


> 
> you need to change the callers of GetDefaultEntryName. the size argument of
> RegQueryValueExW is in bytes, not in characters.
> 
> +++ netwerk/base/src/nsSocketTransport2.cpp     11 Mar 2008 05:26:57 -0000
> +          // SocketHost returns a nsCString, which is 8-bit
> 
> that's kind of obvious...
> 
> Why are you including windows.h from nsNativeConnectionHelper.h?
> 
> +++ netwerk/protocol/file/src/nsFileProtocolHandler.cpp 11 Mar 2008 05:26:58
> -0000
>                                  IID_IUniformResourceLocator,
> (void**)&urlLink);
> 
> you have to make that IID_IUniformResourceLocatorW right?
> 

good catch

	Stuart Parmenter
	Comment 49 • 17 years ago

Comment on attachment 308560 [details] [diff] [review]
updated based on dougt's comments [missed 1.9]

please get rid of the changes to files that are no longer built

Index: gfx/src/* for example

	Reed Loden [:reed]
	Comment 50 • 17 years ago

Get actual review and superreview next time, please, instead of carrying it over from several iterations ago.

	Stuart Parmenter
	Comment 51 • 17 years ago

Comment on attachment 308560 [details] [diff] [review]
updated based on dougt's comments [missed 1.9]

this also was checked in with various C casts that dbaron asked you to remove last time.

I also don't see other module owners reviewing this.

	Doug Turner (:dougt) Reporter
	Comment 52 • 17 years ago

stuart is seeing problems with the assertion dialog in debug.  I think the problem is that we need to build the windbgdlg with the UNICODE flag enabled so that the command line (which is the text to be display in the debug dialog) is wide.  

http://lxr.mozilla.org/mozilla/source/xpcom/windbgdlg/windbgdlg.cpp#48

CreateProcessW

	Alfred Kayser
	Comment 53 • 17 years ago

A little problem in nsDeviceContextWin.cpp (but this code is not used anyway):
nsresult nsDeviceContextWin::CopyLogFontToNSFont(HDC* aHDC, const LOGFONTW* ptrLogFont, nsFont* aFont) const
{
  name[0] = 0;
  if (aIsWide)
    memcpy(name, ptrLogFont->lfFaceName, LF_FACESIZE*sizeof(PRUnichar));
  else {
    MultiByteToWideChar(CP_ACP, 0, ptrLogFont->lfFaceName,
      strlen(ptrLogFont->lfFaceName) + 1, name, sizeof(name)/sizeof(name[0]));
  }

The parameter aIsWide is removed, so this code won't compile anymore.

Better is to first remove the old code in gfx/src/windows (bug 418104), and do this wide (and large) Windows patch.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 54 • 17 years ago

Nothing like this patch should go in until its changes are tested, which means actually _running_ the code that's being changed.  In some cases that'll be difficult because the code isn't used any more, but that should probably be a tip-off that the patch isn't ready to be committed.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 55 • 16 years ago

Comment on attachment 308560 [details] [diff] [review]
updated based on dougt's comments [missed 1.9]

It appears that this patch missed landing in 1.9.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 56 • 16 years ago

Comment on attachment 308560 [details] [diff] [review]
updated based on dougt's comments [missed 1.9]

Removing approval1.9+ per schrep's request.

	Brian Crowder Assignee
	Comment 57 • 16 years ago

This is an interdiff of the changes between attachment 308564 [details] [diff] [review]: necko changes, and the patch I am about to upload.

	Brian Crowder Assignee
	Comment 58 • 16 years ago

Will request review after a round of testing.

	Brian Crowder Assignee
	Comment 59 • 16 years ago

Another round of changes; mostly what biesi asked for and a few little tweaks.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 60 • 16 years ago

Comment on attachment 332983 [details] [diff] [review]
v3: necko changes

+++ mozilla/netwerk/base/src/nsAutodialWin.cpp	Fri Aug  8 12:37:40 2008
+    nsresult result = GetDefaultEntryName(mDefaultEntryName, sizeof(mDefaultEntryName));

can you limit your lines to 80 characters?

     LOGD(("Autodial: Added address %s to RAS autodial db for entry %s.",
-     hostName, autodialEntry.szEntry));
+         hostName, autodialEntry.szEntry));

szEntry is a wide string. this will not work.

+++ mozilla/netwerk/base/src/nsAutodialWin.h	Fri Aug  8 12:37:40 2008
 typedef struct tagRASAUTODIALENTRYA {
     DWORD dwSize;
     DWORD dwFlags;
     DWORD dwDialingLocation;
-    CHAR szEntry[RAS_MaxEntryName + 1];
+    PRUnichar szEntry[RAS_MaxEntryName + 1];
 } RASAUTODIALENTRYA, *LPRASAUTODIALENTRYA;
 typedef RASAUTODIALENTRYA RASAUTODIALENTRY, *LPRASAUTODIALENTRY;

you should change those names to *W instead of *A

+++ mozilla/netwerk/base/src/nsSocketTransport2.cpp	Fri Aug  8 12:37:40 2008
+        if (autodialEnabled){

missing space before {

+                    rv = NS_NewURI(aURI, NS_ConvertUTF16toUTF8(lpTemp).get());

you don't need the .get() here

	Brian Crowder Assignee
	Comment 61 • 16 years ago

Sorry about the 80-column violation; a lot of other code in this file breaks that rule, so I had figured it was a different width for this module.  I've restored this line to fitting, though.

Fixed the rest of your suggestions, asking for another round of review, despite your + on last patch, since this patch is large and largely generated.

	Brian Crowder Assignee
	Comment 62 • 16 years ago

Comment on attachment 333249 [details] [diff] [review]
v4: necko changes

Woops, found a buffer overflow here, one more go needed.

	Brian Crowder Assignee
	Comment 63 • 16 years ago

This fixes a potential buffer overflow in the previous version of nsRASAutodial::GetFirstEntryName, to do with the change from strncpy to wcsncpy.  All instances of changes like this should be audited (I hope you spot some I've missed).

	Brian Crowder Assignee
	Comment 64 • 16 years ago

For the landing of the necko/ fixes:

changeset:   16594:f788da9b2165

Thanks for the careful and fast reviews!

This bug remains open for now, to track its dependencies.

	Brian Crowder Assignee
	Comment 65 • 16 years ago

I had to back out 16594:f788da9b2165 this morning, because the tree was closed.  Will take another swing we it reopens.

	Brian Crowder Assignee
	Comment 66 • 16 years ago

changeset:   16626:45df6b91be5a

Relanded.

	Bernard Alleysson
	Comment 67 • 16 years ago

I took a look at the patch "V5 : necko changes":

 // Add the specified address to the autodial directory.
-PRBool nsRASAutodial::AddAddressToAutodialDirectory(const char* hostName)
+PRBool nsRASAutodial::AddAddressToAutodialDirectory(const PRUnichar* hostName)
 {
     // Need to load the DLL if not loaded yet.
     if (!LoadRASapi32DLL())
         return PR_FALSE;
 
     // First see if there is already a db entry for this address. 
-    RASAUTODIALENTRY autodialEntry;
+    RASAUTODIALENTRYW autodialEntry;
     autodialEntry.dwSize = sizeof(RASAUTODIALENTRY);
     DWORD size = sizeof(RASAUTODIALENTRY);
     DWORD entries = 0;
 

I looks like it should read "sizeof(RASAUTODIALENTRYW)", I don't know if the sizes are the same. I would recommend to use "sizeof(autodialEntry)" though.

	Brian Crowder Assignee
	Comment 68 • 16 years ago

And backed-out again.

changeset:   16627:5365113ba1fb

	Brian Crowder Assignee
	Comment 69 • 16 years ago

Bernard:  Thanks, I'll fix that.

	Brian Crowder Assignee
	Comment 70 • 16 years ago

Sorry to ask for review again, but I wanted to cleanup some sizeof() usages throughout the autodial module.  I'll check next to make sure bugzilla interdiff works.

	Brian Crowder Assignee
	Comment 71 • 16 years ago

Alas, bugzilla interdiff fails and I can't get a good interdiff otherwise, either...  Don't want to spend a lot of time on it, so I won't unless you need it.

	Brian Crowder Assignee
	Comment 72 • 16 years ago

changeset:   16689:f134ac75b142

Landed again (on a truly open tree this time!)

	neil@parkwaycc.co.uk
	Comment 73 • 16 years ago

Comment on attachment 333599 [details] [diff] [review]
v6: necko changes

>-                    rv = NS_NewURI(aURI, lpTemp);
>-
>+                    rv = NS_NewURI(aURI, NS_ConvertUTF16toUTF8(lpTemp));
NS_NewURI has an nsAString& overload, no change needed here...

	Brian Crowder Assignee
	Comment 74 • 16 years ago

(In reply to comment #73)
> NS_NewURI has an nsAString& overload, no change needed here...

This did need a change, the LPWSTR doesn't find an implicit conversion.  Instead I used an nsDependentString:

changeset:   18484:e3d348de9ee2

If the rest of this landing sticks, I'll attach the changeset info shortly.

	Brian Crowder Assignee
	Comment 75 • 16 years ago

http://hg.mozilla.org/mozilla-central/rev/9698ed0a3e43

This landing finally stuck, with the one fix mentioned in comment #74.  Still keeping the bug open to track the remaining fixes.

	Dean Tessman
	Comment 76 • 16 years ago

Did this cause bug 453156?