418703 - reduce narrow windows API calls.

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Doug Turner (:dougt)]

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

Comment 1

[Doug Turner (:dougt)]

Attachment: ugly wip patch

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.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: minimal changes to build with unicode defined

more to come

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: works with or without unicode defined

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?

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: more changes in gfx, toolkit and others

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: xpcom changes

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fixes a linux build issue

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fixed some stuff that bsmedberg pointed out

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

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fixes errors that show up in debug build

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.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 11

[Doug Turner (:dougt)]

did you define MOZ_UNICODE?

Comment 12

[Doug Turner (:dougt)]

Attachment: patch to modules/plugin

need to verify if this builds without UNICODE defined.

Comment 13

[(not currently active) Ted Mielczarek]

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

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: builds without UNICODE defined

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: removed most of the #ifdef UNICODE's

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: changes based on stuart's comments

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: switched from wchar_t to PRUnichar and integrated plugin changes

Comment 18

[Benjamin Smedberg]

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.

Comment 19

[Benjamin Smedberg]

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.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: changes based on bsmedberg's comments

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: changes to nsGREGluw.cpp

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: nss changes

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: nspr changes

Comment 24

[Wan-Teh Chang]

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

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

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fixes some compiler warnings in nspr

Comment 26

[Benjamin Smedberg]

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

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

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

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

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a=beltzner

Comment 29

[Damon Sicore (:damons)]

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

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 32

[Stuart Parmenter]

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

Comment 33

[Stuart Parmenter]

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

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: same patch that I checked in, but it doesn't fail the mochitests now

Comment 36

[Doug Turner (:dougt)]

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)

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

>    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. 

Comment 38

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: updated based on dougt's comments [missed 1.9]

again, passes all mochitests

Comment 39

[Doug Turner (:dougt)]

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

discussed on aim; carrying flag forward.

Comment 40

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: necko changes

Comment 41

[Doug Turner (:dougt)]

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

carrying bsmedberg sr flag forward.

Comment 42

[Damon Sicore (:damons)]

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.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 44

[Doug Turner (:dougt)]

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).

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 46

[Doug Turner (:dougt)]

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

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 48

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 49

[Stuart Parmenter]

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

Comment 50

[Reed Loden [:reed]]

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

Comment 51

[Stuart Parmenter]

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.

Comment 52

[Doug Turner (:dougt)]

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

Comment 53

[Alfred Kayser]

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.

Comment 54

[Mike Shaver (:shaver emeritus)]

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.

Comment 55

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 56

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Removing approval1.9+ per schrep's request.

Comment 57

[Brian Crowder]

Attachment: necko patch updates

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

Comment 58

[Brian Crowder]

Attachment: v2: necko changes

Will request review after a round of testing.

Comment 59

[Brian Crowder]

Attachment: v3: necko changes

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

Comment 60

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 61

[Brian Crowder]

Attachment: v4: necko changes

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.

Comment 62

[Brian Crowder]

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

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

Comment 63

[Brian Crowder]

Attachment: v5: necko changes

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).

Comment 64

[Brian Crowder]

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.

Comment 65

[Brian Crowder]

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

Comment 66

[Brian Crowder]

changeset:   16626:45df6b91be5a

Relanded.

Comment 67

[Bernard Alleysson]

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.

Comment 68

[Brian Crowder]

And backed-out again.

changeset:   16627:5365113ba1fb

Comment 69

[Brian Crowder]

Bernard:  Thanks, I'll fix that.

Comment 70

[Brian Crowder]

Attachment: v6: necko changes

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.

Comment 71

[Brian Crowder]

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.

Comment 72

[Brian Crowder]

changeset:   16689:f134ac75b142

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

Comment 73

[[e-mail]]

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...

Comment 74

[Brian Crowder]

(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.

Comment 75

[Brian Crowder]

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.

Comment 76

[:deanis74]

Did this cause bug 453156?