162361 - Unicode file i/o in XPCOM/IO (cannot open files whose names contain characters outside the current locale: e.g. Japanese/Chinese on French Windows)

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[Roy Yokoyama]

This is a split from 58866.

We need to store filenames in Unicode (either in
UTF8 or UCS2) and call a set of new NSPR_UCS2 interfaces.

Comment 1

[Rui Xu]

code issue, QA to [e-mail] for now, please reassign for QA.

Comment 2

[Roy Yokoyama]

Attachment: Supporting NSPR-UCS2. Storing mWorkingPath and mResolvedPath to be in UTF8

Phew,  It's more than what I have initially thought; but I think
I covered the most of cases and ready for review.

dougt: can you review?
Here is the run down:
- store mWorkingPath and mResolvedPath in UTF8
- call new PR_fooUCS2() instead
- Get/SetNativeFoo() converts path by calling UTF8toFS and FStoUTF8
respectively

Comment 3

[Frank Tang]

===================================================================
RCS file: /cvsroot/mozilla/xpcom/io/nsDirectoryService.cpp,v
+  #ifdef MOZ_UNICODE
...
+    if ( ::GetModuleFileNameW(0, buf, sizeof(buf)) ) {
..
+  #else
...
     if ( ::GetModuleFileName(0, buf, sizeof(buf)) ) {

Does GetModuleFileNameW exist on Win95/98/ME ? 
If NO, will this put into #ifdef cause load time error with the #ifdef turn on
under Win95/98/ME ? 
Does it function on Win95/98/ME ? 
If No, will this put into #ifdef always return false under Win95/98/ME ? 

Same question for
 _wstat in   void nsFileSpec::GetModDate, nsFileSpec::GetFileSize(),
nsFileSpec::IsFile(), nsFileSpec::IsDirectory() 
::ShellExecuteW in nsLocalFile::Launch()

Comment 4

[Roy Yokoyama]

The strategy changed since I posted the patch on 09/04.
We intended to have a pure unicode application and use
MS Layer for Unicode for Win9x OS.  
However, we decided _not_to use MSLU. 

I'll post a new patch to accomodate the change of strategy.

Comment 5

[Roy Yokoyama]

Attachment: store mWorkingPath and mResolvedPath in UTF8 and call PR_fooUCS2()

Last patch was rotten so I need to redo the patch.....
I'd like to provide an incremental patch for supporting file i/o issues.

With this patch we can:
- open non-locale file 

dougt: can you review?	I want to check this as soon as we are open for moz 1.3

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 104906 [details] [diff] [review]
store mWorkingPath and mResolvedPath in UTF8 and call PR_fooUCS2()

r=dougt

Comment 7

[Roy Yokoyama]

bulk milestone change

Comment 8

[kinmoz]

Comment on attachment 104906 [details] [diff] [review]
store mWorkingPath and mResolvedPath in UTF8 and call PR_fooUCS2()

sr=[e-mail]


==== Put a space after the equals sign:


+    output =NS_ConvertUCS2toUTF8(input);


==== So how does this relate to bug 170852, where you are actually removing
MOZ_UNICODE ifdefs? Are you going to remove the MOZ_UNICODE ifdefs for this
patch, when you land those changes?

Comment 9

[Jungshik Shin]

Attachment: another patch

This patch is based on Roy's patch, but it checks the OS version at the
run-time and calls either UTF16'nized PR_File* APIS or non-UTF16nized PR_File*
APIs. This is not optimized, but is just to show how it can be done. Because
this patch relies on NSPR UTF-16 APIs, NSPR  has to be compiled with
MOZ_UNICODE defined.
Currently UTF-16 APIs are only compiled in with MOZ_UNICODE defined. Can we
turn them on by default on Windows? 

Perhaps, a better approach is to check the OS (9x/ME vs 2k/XP) in xpcom/io and
then to set function pointers accordingly to points to non-UTF16 calls and
UTF-16 calls as was done in widget. 

Whichever way we can take, we  can solve most problems in xpcom/io.

Below is a bit off-topic.

However, PR_*File* APIs are also used directly (not via xpcom/io).  
To fix those cases, we may have to modify NSPR file-related APIs so that on
Windows, file paths in C-string are always interpreted as in UTF-8.  With this
change,  NSPR can internally convert UTF-8 to UTF-16 and invoke 'W' APIs on
Win2k/XP (and on Win9x/ME with Microsoft layers for Unicode, the presence of
which has to be detected when NSPR is initialized) while on Win9x/ME with MSLU
UTF-8 has to be converted to the system code page and 'A' APIs have to be
invoked. It might not be very  realistic because NSPR is not only for Mozilla
but also for other projects. Nonetheless, this is something we have to think
about.

Comment 10

[Jungshik Shin]

Attachment: a new patch

I need to check this out on Win9x/ME, but on Win2k, it works fine. 
At the beginning of the patch is a small fix to downloadmanager.js (see bug
208113 comment #8). 

In SpecialSystemDirectory.cpp, I switched to 'W' APIs and added emulators for
them on Win 9x/ME. The approach is similar to what's done in widget/src/windows
by Roy.

This patch also exposes NS_IsWindowsNT() that is available once xpcom is
init'd. Its two applications outside xpcom/io are in xpinstall and
netwerk/base/src where	0x5c is special-cased on DBCS OS. Because with this
patch, native path will be in UTF-8 on Win2k/XP. I changed those checks done
only if the OS is Win 9x/ME. 

I can remove MOZ_UNICODE define in Makefile.in in xpcom if UTF16 APIs in NSPR
are turned on by default for XP_WIN.

Comment 11

[Jungshik Shin]

Attachment: another experimental patch (it's working)

This works rather well. I can make and read files of which names include Thai,
Devanagari, Greek, Cyrillic letters altogether !

However, this is still experimental (especially, 'extern nsNativeToUnicode() is
a hack I just played with that I'm gonna get rid of). In terms of actual
implementation, I still need to figure out what the best course is. I'm
wondering how good/bad an idea is to have something like nsWin32API::WinAPIName
(that is accessible  across the tree). This is to avoid some overlap with
what's done in widget/src/windows/nsToolkit.*

Comment 12

[[e-mail]]

Comment on attachment 125603 [details] [diff] [review]
another experimental patch (it's working)

Bah, this conflicts with an old (but unfortunately no sr) patch in bug 156422.

I wonder what the point of MakeUpperCase is.

Comment 13

[Jungshik Shin]

Neil, thanks for the note about bug 156422. It seems easy to make attachment
94582 [details] [diff] [review] (to bug 156422) 'Unicode-aware'. I can make a new patch with attachment
94582 [details] [diff] [review] incorporated if you want.  

As for |MakeUpperCase|, I have little idea other than it's probably to make
filenames case-insensitive on Windows. This bug is not for doing something new,
but for making Mozilla 'unicode-aware' on Win2k/XP so that I just recast
whatever is there in 'W" APIs. If we don't need it, that's nice

Comment 14

[:deanis74]

The Windows filesystem already is case-insensitive.

Comment 15

[Jungshik Shin]

I know that[1] and thought it's a bit strange to have MakeUpperCase. Anyway,
please don't ask me about MakeUpperCase. I just recast it  in W APIs without
thinking much. In this bug, I just want to focus on making Win32 file I/O in
Mozilla Unicode-aware. We can remove it later if it's not necessary. 

[1] Case-insensitivity beyond US-ASCII is not so simple as one may think.  

Comment 16

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

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

Comment 17

[timeless]

err. FAT is case preserving. NTFS can be case sensitive. Please don't ever
change the file name's case from the flavor the os offers.

Comment 18

[:deanis74]

Really?  Where is NTFS case-sensitive?

Comment 19

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

google found: http://techsupt.winbatch.com/TS/T000001036004F26.html
"Although NTFS does support case sensitive filenames, currently only the POSIX
subsystem uses case sensitive names."

Comment 20

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

also, there's a flag to CreateFile that allows to create two files with names
differing only in case:
FILE_FLAG_POSIX_SEMANTICS

Comment 21

[Jungshik Shin]

Is there anyway for us to move this forward? We need to make changes in NSPR and
I have yet to hear from wtc. It's embarrasing thta Mozilla on win32 is not yet
fully Unicode-aware. I have a patch and an idea, but without NSPR fix, not much
can be done.

Comment 22

[Jungshik Shin]

Adding leaf  for nspr, darin for xpcom and some drivers to Cc, retargetting and
assigning to myself.

Related threads:  (in n.p.m.nspr)
1.  Enabling NSPR UTF16 APIs

  news://news.mozilla.org:119/bfe3c5$[e-mail]

2. making |nsWindowsAPI| 
news://news.mozilla.org:119/bfr3cc$[e-mail]

I really love to fix this (and a bunch of other related bugs that will be fixed
or become very easy to fix once this is fixed), but I'm stuck because I don't
know how to move forward necessary NSPR changes. Any thought or help would be
appreciated. 

Comment 23

[Jungshik Shin]

It's easier to use google than 'news' URL.

http://groups.google.com/groups?threadm=bfe3c5%244ha2%40ripley.netscape.com
http://groups.google.com/groups?selm=bfr3cc%24nfg1%40ripley.netscape.com&rnum=2

btw, I wrote to wtc about NSPR Unicode file I/O and getenv/setenv vs
wgetenv/wsetenv (bug 227500)

Comment 24

[Jungshik Shin]

(In reply to comment #22)

> 2. making |nsWindowsAPI| 
> news://news.mozilla.org:119/bfr3cc$[e-mail]
> http://groups.google.com/groups?selm=bfr3cc%24nfg1%40ripley.netscape.com&rnum=2

I've just found |nsINativeAppSupportWin|. It seems that we can put all those W/A
API-related stuffs there including IsWindowsNT()?

Comment 25

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

(In reply to comment #24)
> I've just found |nsINativeAppSupportWin|. It seems that we can put all those W/A
> API-related stuffs there including IsWindowsNT()?

making large parts of the tree depend on xpfe sounds like no good idea to me

Comment 26

[Jungshik Shin]

I agree. I didn't realize that it's in xpfe. What I want to have/implement is
something akin to |nsCRT| for Windows APIs. See my news posting mentioned in
comment #23.

Comment 27

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

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

Comment 28

[Brodie]

Attachment: MZLU (proof of concept)

Here is a proof of concept library for wrapping Win32 calls. It is a very basic
implementation similar to MSLU but freely licenced for Mozilla and other open
source projects under the tri-licence. (I've thus called it MZLU).

Essentially what it does is define symbols like:
  GetTempPathMz 
  CreateFileMz
which are function pointers to functions which have the same signature as the W
version of the WIN32 or CRT API. A client program needs to link with the very
slim C static lib that defines these pointers. The only other thing the static
lib does is have an initialization function to set the pointers to the
appropriate implementation. Hard to tell exactly how much overhead in linked
code, but probably just a few Kb.

On Windows NT based OS the pointers are set to the real W functions.
  e.g. CreateFileMz = ::CreateFileW; 
Thus the penalty for using this library on NT/2K/XP is very low. Just the extra
few Kb for the static lib, plus one more indirection per OS call. 

On 9x/ME systems, these functions are set to wrapped versions of the ansi code
page based API. These wrapped functions don't exist in the C static lib, but
instead exist in a dynamically loaded DLL loaded only by 9x/ME systems. 

I thought about using direct calls to the W functions and on Win9x/ME rewriting
the import tables to point to our wrapped functions, which would save the
single indirection on NT, however the current method allows us to also support
functions which are only conditionally available. e.g. GetDiskFreeSpaceEx (not
available on Win95). On systems which doesn't have it available, the function
pointer will be left NULL. On systems which does support it the function
pointer is set. Thus simplifying the test for availability (although at the
cost of having the DLL loaded from startup, however since these are probably
all major system DLLs which are likely to be loaded anyhow, it is just a
mapping into our process that will be created).

The wrapper functions are implemented to convert between wide/narrow chars
while retaining the function specification. Sometimes this is simple (e.g.
DeleteFile), sometimes more difficult (e.g. _wgetdcwd or GetTempPath). See the
DLL implementation (mzlua) for details.

The functions currently implemented for proof of concept are CRT and WIN32
functions that are used in nsLocalFileWin.cpp ...

    _wgetdcwd
    CopyFile
    CreateDirectory
    CreateFile
    DeleteFile
    GetDiskFreeSpaceEx
    GetDiskFreeSpace
    GetFileAttributes
    GetLogicalDriveStrings
    GetTempPath
    MoveFile
    RemoveDirectory
    ShellExecute

In order to use this library to fully Unicode enable Mozilla on Windows. All
calls to non-Unicode API would need to be re-routed through this library. In
addition, all NSPR UCS2 functions would also need to use this library to allow
them to be called on all OS versions.

Is this acceptable to the Mozilla and NSPR philosophy?

Comment 29

[Darin Fisher]

Brodie: Thanks for taking the time to create MZLU.  I don't have a "final"
answer for you, but I think this may be the right direction.  The challenge is
really deciding what we want to do with NSPR.  I haven't heard a final decision
there.  I know that wtc is not thrilled about adding new APIs to NSPR, but
perhaps that is the best solution.  We need to hear from wtc on this matter. 
Jshin: what are your thoughts on this?  I know you spoke about wanting something
like MZLU... is this duplicating work you have already done, or is this the
missing puzzle piece?

Comment 30

[Brodie]

It seems strange to not use the MSLU when it provides the vendor recommended 
solution of Unicode on all platforms. I've looked at bug 118013 and bug 162362 
which asserts that there was discussion about this but doesn't note where. Can 
someone point me towards the problems with bundling MSLU with Mozilla? Licence-
wise it seems that one clause might be the problem...

"(c) you distribute your Application containing the Redistributable Components 
pursuant to an End-User License Agreement (which may be "break-the-
seal", "click-wrap" or signed), with terms no less protective than those 
contained herein;". 

Is it possible to create another product which is compatible with the terms of 
the MSLU licence, and then require users to install it in order for Mozilla to 
be used on the appropriate platforms? Not particularly nice, but MSLU is a good 
solution...

If not, then I am quite happy to move all of nsToolkit into MZLU, complete the 
implementation of all API that NSPR requires, and then get all of Moz using 
this. I really want to implement all of the Unicode support here. Working with 
MBCS is a major pain and makes things far more complex than it needs to be.

As for the problem of NSPR and it's API. Perhaps we should just use UTF-8 in 
all NSPR API on Windows (build option of ANSI or UTF-8 to maintain 
compatibility) and just wear the to/from UTF-8/UTF-16 conversions that it 
requires? No API change required, a new build option and different 
implementation internally. Wan-Teh -> what do you think? Others?

I know this doesn't make much difference to English speaking single byte "pizza 
is as international as I get" sort of people, but lets try and get this app 
fully internationalized for the rest of the world... Unicode isn't the future, 
it's already here. Mozilla has fallen behind.

Comment 31

[Jungshik Shin]

(In reply to comment #29)
 
Brodie, thank you for 'waking up' darin's interest in this bug :-)

> The challenge is
> really deciding what we want to do with NSPR.  I haven't heard a final decision
> there.  I know that wtc is not thrilled about adding new APIs to NSPR, but
> perhaps that is the best solution.  

I tend to agree about adding new APIs. An alternative (using UTF-8 on Win 2k/XP)
is a bit risky as discussed in the thread at 

http://groups.google.com/groups?threadm=bfe3c5%244ha2%40ripley.netscape.com

> We need to hear from wtc on this matter.

Unfortunately, he hasn't replied to my emails. I've written to him (and leaf,
the other owner of NSPR) at least a few times since summer 2003, but nothing
came back. I understand that he's busy, but it's frustrating not to hear
anything.  Anyway, I really hope this time around he can find some time to
resolve this long standing issue. 

  
> Jshin: what are your thoughts on this?  I know you spoke about wanting something
> like MZLU... is this duplicating work you have already done, or is this the
> missing puzzle piece?

 What's missing is, as you wrote, wtc's decision as to what to do with NSPR file
APIs.   MZLU can solve the problem I talked about in two news postings (comment
#23) and it can simplify a lot of things. I was approaching the problem from a
different angle (although equivalent) and didn't hit upon an idea of rolling out
our own version of MSLU.  

Then, a question arises why not just use MSLU. I asked the question before, but
no resolution has been reached yet. 

It's available on virtually all installations of MS Windows because MS IE comes
with it (we don't have to increase the code size to dupe what's already
available). There's an irony here in that we have to require MS IE be installed
to run Mozilla. However, some other parts of Mozilla already rely on DLLs that
come with MS IE so that relying on MSLU should not be a problem, IMHO.  Even if
MS IE is not installed, it's likely that some other applications (e.g. MS
Office) with MSLU are present. 

I'm adding some more people who may be at a better position to resolve this
issue (whether or not to make Mozilla depend on MSLU). 

Comment 32

[Jungshik Shin]

In bug 239279, we discussed the possibility of making Mozilla depend on MSLU.
There are quite a number of bugs nothing to do with file I/O  that can be easily
fixed by using 'W' APIs available for Win9x/ME via MSLU. (see bug 232969, bug
240272, bug 9449, bug 243618 for instance). All those bugs are independent of 
NSPR file I/O APIs.So, let's keep on discussing, in bug 239279, as to whether we
want to make Mozilla depend on MSLU or we want to implement our own (MZLU).

Comment 33

[Brodie]

Attachment: MZLU v0.1

This version of MZLU implements all of the functions that nsLocalFileWin uses
and that are implemented in nsToolkit/nsWindowsAPI. While we are arguing
over/waiting to hear whether or not to use the actual MSLU library or not,
let's start using MZLU instead. 

It will be very easy to move from calling MZLU functions to calling actual MSLU
wide functions (replace the Mz postfix from all functions with W).

There seems to be a number of places where people are actively adding new
wrappers for A/W, so we could at least move that to a centralized place.

On nsLocalFileWin.cpp, I found that I could get rid of nearly every call to
NSPR apart from PR_Open by going directly to the Win32 API. Since this is a
Windows only component I don't see any reason why we shouldn't do so. This
means that at least for the file handling, we only need PR_Open to support
unicode...

Comment 34

[Darin Fisher]

You could also implement PRFileDesc yourself in terms of the WIN32 API, but that
is probably not ideal ;-)

The biggest concerns I have with moving to a full Unicode backend is what to do
with the "native" character encoding defined by nsIFile/nsILocalFile.  If we
keep that using the ANSI codepage, then won't we have a lossy conversion from
nsIFile to file:// URL?  Remember, that file:// URLs are currently generated
from the "native" file path.

I think we might want to solve these problems by using UTF-8 as the "native"
character encoding under Win32 builds.  Or, perhaps we could even have an
optional runtime mode in which that is enabled.

We also have to keep in mind that changing the encoding of file:// URLs affects
interoperability with other applications when users drag-n-drop file:// URLs
from Mozilla to other applications.  We should choose an encoding that is most
compatible with Unicode aware applications.  E.g., how does Microsoft Office
encode non-ASCII file:// URLs?

NOTE: Under most Linux distros, UTF-8 is the default character encoding, and it
is therefore what we use for file:// URLs and nsIFile::nativePath under Linux. 
The same is true of OSX.

Comment 35

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

(In reply to comment #34)
> I think we might want to solve these problems by using UTF-8 as the "native"
> character encoding under Win32 builds.

the downside is that mozilla is not backwards-compatible wrt to its file urls
then. for example, sucks if you had a homepage (as a local file) in a directory
containing non-ascii characters before, because the url would no longer work.

Comment 36

[Darin Fisher]

Christian: I agree... that's a major concern.  Perhaps we need to utilize a
failover technique.  Or maybe we can unescape file:// URLs and test whether they
are UTF-8 or not (using IsUTF8).  If they are not UTF-8, then we try using the
ANSI codepage.  That might be the best solution.  We could probably do all of
this inside nsURLHelperWin.cpp.

Comment 37

[Brodie]

Comment on attachment 151998 [details]
MZLU v0.1

See bug 239279 for updates of MZLU.

Comment 38

[Jungshik Shin]

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

Comment 39

[chris hofmann]

jshin,  any progress on this one?  if there still is a chance of a low risk
patch renominate for 1.0

Comment 40

[Jungshik Shin]

Sorry I don't have enough time to fix this before 1.0. Even if I have a lot more
time, the patch may be too extensive to be regarded as safe for 1.0. 

Comment 41

[Jungshik Shin]

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

Comment 42

[OstGote!]

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

Comment 43

[Markus Essl]

I am using Mozilla code as a base for an application for e-learning, for 
already some years. Right now we are having troubles because customers in 
russia and polen have problems because of this issue, so I created an own class 
that converts (and creates) the unicode path and converts it to its alternate 
path, and uses this alternate short path to create a NS_NewLocalFile, so that 
PR_Open works correctly. Of course, this is far far away from what should be, 
but so I get my code working and customers satisfied. 

But what about the idea of converting the path to its short variant? The only 
downside is, as far as i can see, is the creating of new files, which will not 
be made with NSPR. But it would be possible to create a shortnamefile with 
nspr, and then rename it. 

BTW, the current solution of converting characters to "_" because "?" is not a 
valid filename is bad too - because of this, if you are going to save a page in 
mozilla (1.7.5), you can select a directory with russian characters, and it 
ends up in a totally different folder. Which is, in my opinion, even worse than 
disallowing such pathnames. 

Is there any way that I can be helpful to you? I would like to help out, just 
tell me what to do.

Comment 44

[timeless]

it's possible for shortfile variants to be disabled, which means you'll gain
nothing by trying.

Comment 45

[Markus Essl]

You are right; I was not aware of this. I just read
http://support.microsoft.com/kb/q121007/. 

Comment 46

[Roy Yokoyama]

I started fixing the unicode file i/o related bugs since 2002 and as far as 
I remember, only stuff waiting for supporting the _full_ unicode in Windows 
is to enable NSPR to call W APIs.  (except that non-ASCII Commandline issue is 
still outstanding, i believe)  I believe I have tested file:// URL and 
drag-drop of non-system filenames before I checked in the code (all with 
MOZ_UNICODE)

There appears to have new path to use MZLU; but bug 239279 turning up to a 
licensing talk.  We also discussed using MSLU years back and decided not to use
it simply because we don't want to introduce another dependency.

Can we turn on the flag (MOZ_UNICODE) in NSPR after we branch out 
for Gecko1.8? 

Darin, Chris? Is WTC still active in mozilla? 

Comment 47

[Jungshik Shin]

See comment #9, comment #10, comment #11 and comment #22 (two news postings and
responses to them in the newsgroup). Anyway, people don't seem to like
introducing new 'Wide' APIs to NSPR. 

Comment 48

[Jungshik Shin]

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

Comment 49

[Jungshik Shin]

darin, what's your opinion of introducing wide APIs to NSPR? 
Alternatives are: 
  1. implementing equivalents of NSPR APIs in xpcom with Windows W APIs
  2. pass UTF-8 (on Windows 2k/XP) to existing NSPR APIs and let NSPR do the   
     conversion 

I'm afraid the second alternative has a performance issue because we have to go
back and forth between UTF-8 and UTF-16 on Win 2k/XP.

 

Comment 50

[Darin Fisher]

I'm in favor of adding UTF-16 APIs to NSPR.  I'd like to see us move
nsNativeCharsetUtils into NSPR, and make those conversion routines publicly
available.  I spoke with WTC about this plan, and he agreed that it is the best
way to go.  Neither of us have time to implement it though.  The biggest
challenge, I think, is in supporting those UTF-16 APIs on non-Win32 platforms. 
That's where the code in nsNativeCharsetUtils (or some large part of it) will
come in handy.

Comment 51

[Darin Fisher]

One more point: I don't think NSPR should depend on MZLU or something like that
since there really aren't that many wide APIs to implement.

Comment 52

[Jungshik Shin]

(In reply to comment #50)
> I'm in favor of adding UTF-16 APIs to NSPR.  I'd like to see us move
> nsNativeCharsetUtils into NSPR, and make those conversion routines publicly
> available.  I spoke with WTC about this plan, and he agreed that it is 
> the best way to go.

 So, WTC has changed his mind since 2003. That's fine because I also agree with
you.    

> challenge, I think, is in supporting those UTF-16 APIs on non-Win32 platforms. 

  We don't need those APIs on platforms other than Windows at least for now. If
implementing NSPR 'W' APIs only on Windows for now is acceptable, I'll begin
with attachment 125603 [details] [diff] [review].

Comment 53

[Darin Fisher]

I would really like to see us try to provide the same NSPR API on all platforms.
 I think we can given the code in nsNativeCharsetUtils.cpp.

Comment 54

[Wan-Teh Chang]

Yes, let's make the current PR_xxxUTF16 functions official,
and avoid making NSPR depend on MZLU (at least in the first
implementation).

Comment 55

[Roy Yokoyama]

PR_xxxUTF16 functions are #ifdef'ed with MOZ_UNICODE.
All NSPR needs to do is to enable the flag.

From the comment in prdir.c : 
Bug 162358: added NSPR file I/O functions that take UTF16 pathnames. The patch
is contributed by Roy Yokoyama <[e-mail]>. Modified Files:
config/config.mk prio.h prtypes.h _win95.h primpl.h prdir.c prfile.c w95io.c ptio.c

Comment 56

[Jungshik Shin]

(In reply to comment #53)
> I would really like to see us try to provide the same NSPR API on all platforms.
>  I think we can given the code in nsNativeCharsetUtils.cpp.

As you implied, moving nsNativeCharsetUtils to NSPR is more involved than simple
copy'n'paste partly because of the language difference. For the sake of
completenetss, it's good to provide the same NSPR API on all platforms, but do
we really want to hold this Windows-specific bug just for that? How about just
adding a 'dummy' implementation for other platforms for now? (there's no
non-Windows consumer at the moment).  

Comment 57

[Darin Fisher]

I'm not against doing things in stages, but we need to decide what form we want
this to be in when Firefox 1.1 ships.  Maybe WTC has some thoughts on this?  I'd
love to see this bug fixed for Firefox 1.1, but we're talking about a pretty
significant API change for NSPR.

Comment 58

[Jungshik Shin]

(In reply to comment #57)
> but we're talking about a pretty significant API change for NSPR.

We're not gonna remove existing file APIs (char-based), are we? UTF-16 APIs are
not likely to be used by non-Windows consumers

Btw, what do you think of |if (isNT) use UTF-16 APIs else use 8bit APIs| in file
operations? We can move some of them to NSPR (and probably get rid of them there
using function pointer indirection), but I'm afraid we have to live with some of
them. File operations are  more critical than registry handling performance-wise
so that you might have some concern..
 

Comment 59

[Darin Fisher]

> We're not gonna remove existing file APIs (char-based), are we?

No, definitely not.


> UTF-16 APIs are not likely to be used by non-Windows consumers

I suspect they will be used by Mozilla.  Why not?  People work with UTF-16
strings a lot in Mozilla, and having a consistent (Portable!) API is what NSPR
is designed for.  It's pretty odd to have Windows-only APIs exported by NSPR.


> Btw, what do you think of |if (isNT) use UTF-16 APIs else use 8bit APIs| in 
> file operations? We can move some of them to NSPR (and probably get rid of 
> them there using function pointer indirection), but I'm afraid we have to live 
> with some of them. File operations are  more critical than registry handling 
> performance-wise so that you might have some concern..

I don't think it matters that much either way.  That said, I think the current
code that uses GetProcAddress to resolve CreateFileW and friends is wrong. 
Those symbols exist on Windows 9x, but they are simply not implemented.

The current NSPR implementation for these routines assumes that callers will
handle "not implemented" errors appropriately.  I'm not sure I like that.  I'd
prefer to see us implement the UTF-16 functions in all cases.  However, I
suppose we could entertain the idea of making all users of the UTF-16 routines
know how to failover to the ANSI versions.  But, that is the least desirable
solution IMO.

Comment 60

[Jungshik Shin]

(In reply to comment #59)
> > We're not gonna remove existing file APIs (char-based), are we?
> 
> No, definitely not.

 The question was rhetorical one because adding UTF16 APIs didn't seem to me as
significant as you think it is.

> > > UTF-16 APIs are not likely to be used by non-Windows consumers
> 
> I suspect they will be used by Mozilla.  Why not?  People work with UTF-16
> strings a lot in Mozilla, and having a consistent (Portable!) API is what NSPR
> is designed for.  It's pretty odd to have Windows-only APIs exported by NSPR.

I'm not saying we will never implement them on other platforms. We need to
implement them on all platforms to use them in cross-platform code which
directly invoke NSPR APIs instead of going through xpcom/io (so my statement
'not likely to be .... other platforms' is not quite right). However, there's no
consumer outside xpcom/io so that I think we can do it in stages adding a very
prominent warning that UTF-16 APIs should not be used on other platforms for now.

> I don't think it matters that much either way.  That said, I think the current
> code that uses GetProcAddress to resolve CreateFileW and friends is wrong. 
> Those symbols exist on Windows 9x, but they are simply not implemented.

I'll change it to check the OS version and do the 'right' thing depending on the
result. 

>  I'd prefer to see us implement the UTF-16 functions in all cases.

So would I. That means, xpcom/io can move some of |if (isNT) ...| over to NSPR.  

Comment 61

[Brodie]

It is quite simple to rewrite all of xpcom/io to use direct windows calls apart
from a single call to PROpen. I did this once when experimenting with a unicode
build of mozilla. Not really much point though because the requirement for
Unicode support in moz spreads out far more than just xpcom/io.

Part of the solution is that NSPR needs a wide-string version of the API. I
agree that it needs to be cross-platform, and that it should be implemented in
stages. However what we need to avoid is the case when only stage 1 is ever
done, and the other platforms are never coded up. Cross-platform must include
Windows 9x platforms.

At the higher level, I still think that any solution which involves doing
if(isNT) in application code is wrong. It makes the application more complicated
and clutters the logic. The handling of wide-string to legacy encoding on 9x
platforms should be handled seamlessly by a library. Ideally that library is MS
unicows, and I still believe that we can find a solution to distribute it with
the official mozilla build. Otherwise use opencow (nee MZLU,
http://opencow.sourceforge.net/).

Comment 62

[:Gavin Sharp [email: [e-mail]]]

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

Comment 63

[:Gavin Sharp [email: [e-mail]]]

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

Comment 64

[:Gavin Sharp [email: [e-mail]]]

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

Comment 65

[Adam Guthrie]

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

Comment 66

[Jungshik Shin]

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

Comment 67

[Elmar Ludwig]

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

Comment 68

[Jungshik Shin]

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

Comment 69

[Ray Booysen]

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

Comment 70

[:Mook]

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

Comment 71

[Syophone]

(In reply to comment #61)
> It is quite simple to rewrite all of xpcom/io to use direct windows calls apart
> from a single call to PROpen. I did this once when experimenting with a unicode
> build of mozilla. Not really much point though because the requirement for
> Unicode support in moz spreads out far more than just xpcom/io.
> 
> Part of the solution is that NSPR needs a wide-string version of the API. I
> agree that it needs to be cross-platform, and that it should be implemented in
> stages. However what we need to avoid is the case when only stage 1 is ever
> done, and the other platforms are never coded up. Cross-platform must include
> Windows 9x platforms.
> 
> At the higher level, I still think that any solution which involves doing
> if(isNT) in application code is wrong. It makes the application more complicated
> and clutters the logic. The handling of wide-string to legacy encoding on 9x
> platforms should be handled seamlessly by a library. Ideally that library is MS
> unicows, and I still believe that we can find a solution to distribute it with
> the official mozilla build. Otherwise use opencow (nee MZLU,
> http://opencow.sourceforge.net/).

But How to patch for Fx 1.5b1/2?

Comment 72

[石庭豐 (Seak, Teng-Fong)]

I'm sure people are going to say I'm just making noise here, but I still like to remind that Windows Vista is due to be out next year, and Win98 is to be abandoned even more.  Please, for the well-being of the whole humanity, or at least of the Mozilla community, forget about Win9x family.  The lastest versions of FF and TB (1.5 and 1.0.7 resp.) are stable enough for those who are forced to use Win9x.  We must move forward.

How many times I'm pissed off because TB can't attach a file and I'm forced to change the name to something totally non-sense to suit it.  I know that more and more TB users are switched to Outlook (and Outlook Express) because of the user-friendliness it offers.  The more we are stagnant in this stage, the more TB is getting worse.  We need to be more determined.

Comment 73

[Darin Fisher]

>  We need to be more determined.

I agree.  Are you volunteering to write code?

Comment 74

[石庭豐 (Seak, Teng-Fong)]

Sure.  But I'm mostly a Java and web programmer.  I can't say I'm very efficient in C++.

Anyway, I've tried to take a look at how to do get the source code and learnt that we need to have Cygwin and Visual C++ 6.  I could install VC6 but I would like not to do so if possible.  And then it talks about VS.NET.  I'd probably misunderstood something...

OTOH, in this bug as well as in other bugs, it seems that the Unicode support (NSLU) is already there for years but developpers are just reluctant to use them because that would need extensice testing (in Win9x and WinNT platforms).

Comment 75

[Jungshik Shin]

Teng-Fong, I'll try to resurrect my 2.5 year old patch (attachment 125603 [details] [diff] [review]), make changes to work with the current code and do  what darin suggested in bug 239279 comment #116. However, my time is limited and won't begin to work on it this year (well, I'm hoping I'll have some free time to work on this in the first few days of next year). However, if you're willing to work on this, you're free to go ahead. As to what compiler to use, see http://developer.mozilla.org/en/docs/Windows_Build_Prerequisites:Free_Microsoft_Compilers
and http://developer.mozilla.org/en/docs/Windows_Build_Prerequisites#Compiler_.26_Linker

Comment 76

[Jungshik Shin]

Attachment: patch (far far from review-ready)

This is a just wip (hodgepodge of various patches). However, I don't think I'll take the approach taken in this patch. I just began to write a totally different patch that almost exclusilvey uses 'W' APIs on Windows. What I wanna do is to make things work on Win 2k/XP first and see how much effort it takes to make it work on Win 9x/ME if we still want to support them in FF 3.0, which is not very likely.

Comment 77

[Brendan Eich [:brendan]]

Current plan is not to support Windows 98 in Firefox 3 / Gecko 1.9.  I'm not sure about ME -- cc'ing vlad.

/be

Comment 78

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

ME's in the same boat as 98 -- (its usage numbers are even lower than 98's)

Comment 79

[Roy Yokoyama]

I'm glad to see people working on this bug.

XPCOM/IO was the last item I needed to do to fully support Unicode filenames 
back then; but it never materialized for reasons I don't want to get in.

I look forward to your real patch, Jungshik. 

Comment 80

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

does "not support w9x in gecko 1.9" (it is more than firefox) mean "we won't spend extra effort on w9x" or "we will actively break it and don't want patches for it"?

Comment 81

[Darin Fisher]

> does "not support w9x in gecko 1.9" (it is more than firefox) mean "we won't
> spend extra effort on w9x" or "we will actively break it and don't want patches
> for it"?

In the discussions I've heard on this topic, the intention is the latter... to accept patches.

Comment 82

[Darin Fisher]

Sorry, I meant:  In the discussions I've heard on this topic, the intention is to accept patches.  In other words, if members of the community wish to support Win9x, then we should support them in their efforts.

Comment 83

[Brendan Eich [:brendan]]

Again, vlad should comment, but short of a lot of work, the rendering subsystem moving to Thebes/Cairo in 1.9 means Win98 will perform poorly in some cases.  From what I remember, very poorly.  If someone writes a patch to correct this, it may require a lot of work.  It may require graphics card level hacking.  It may not be possible at all.

/be

Comment 84

[Caleb]

I have no say in this, but I hope it's OK for me to express my opinion.

Even if someone is dedicated enough to do something as crazy as that, why should the code be littered with hacks/workarounds/etc... for a dying (dead) OS?

Microsoft has long dropped support for Win98SE (http://support.microsoft.com/lifecycle/?p1=6898) and will drop support for WinME in 2006.
More Windows support cycles at http://support.microsoft.com/gp/lifeselectwin.

I believe that the only reason people still use Win9x is because they're stuck with it on their ancient hardware. And once the old computer dies, it will be replaced with something that will run atleast Windows XP.

Comment 85

[Jungshik Shin]

Attachment: patch (stage 1)

This gets compiled, but my build hasn't yet finished so that I haven't tested it.

mResolvedPath and mWorkingPath are now in nsString and UTF-16 APIs directly invoke 'W' APIs while 'native' APIs call UTF-16 APIs with encoding conversions. Instead of using NSPR UTF-16 APIs, simplified and modified implementations were added to nsLocalFileWin.cpp. I added a flag, PR_LD_PATHW (0x4000) to  PR_LoadLibraryWithFlags to pass a UTF-16 string (casted into 'char *'). 

This doesn't work on Win 9x/ME, but I'm eager to see this fixed in FF 2.0 so that I'll add Win 9x/ME support. SpecialSystemDirectory needs to be patched as well (attachment 208200 [details] [diff] [review] has an unfinished patch for that). To do that, I also have to add an OS-detection code somewhere (perhaps in nsNativeCharsetUtils init. routine). I may or may not fix xpcom/obsolete/nsSpecialSystemDirectory.

Comment 86

[Jungshik Shin]

Attachment: another checkpoint (nsLocalFileWin)

With this patch applied, a trunk build works as well as a trunk build without it (except for a potential slow-down because 'native' methods are implemented in terms of UTF-16 methods and our codebase use 'native' methods more often than UTF-16 methods). However, there are a lot more to do to enable even a simple thing like opening a file whose name has characters not covered by the current 'legacy' codepage. That's because our code use 'native' (lossy) APIs all over the places. 
When I added a warning to GetNativePath, my console got bombarded with warnings.

In many cases, we need to do something like

#ifdef XP_WIN
 use UTF-16 APIs
#else
 use 'native' APIs
#endif

An alternative is to make 'native' on Win 2k/XP UTF-8 (as done in my 'hodgepodge' patch) while leaving as it is on Win 9x/ME. In addition, for non-file related use of 'native' (registry, cmd line, env. variables), we might introduce 'nativeA' (as opposed to 'nativeW'). However, this is not compatible with a long-standing convention that 'native' filename can be fed to '(f)open'-like functions so that it's not such a good idea. 

Another alternative is to add 'UTF8' methods to nsILocalFile....

Whichever option we take, we have a lot of files to 'fix'. Of course, that has to be done in a separate bug. 

 
BTW, this patch is a lot easier to read than attachment 208388 [details] [diff] [review] thanks to the way methods are ordered in nsLocalFileWin.cpp

Comment 87

[Jungshik Shin]

(In reply to comment #86)

> than UTF-16 methods). However, there are a lot more to do to enable even a
> simple thing like opening a file whose name has characters not covered by the
> current 'legacy' codepage. That's because our code use 'native' (lossy) APIs

Oops. Just with this patch, opening a file with non-'native' characters in its name works file if it's done via File|Open (For a moment, I forgot that nsFilePicker was made to deal with the full Unicode range a long time ago by Roy). I guess opening with a double clicking should work if I apply the latest patch in bug 278161.
 

Comment 88

[Jungshik Shin]

Attachment: yet another checkpoint (with support for Win 9x/ME)

It's still in WIP (especially when it comes to supporting Win 9x/ME)

Comment 89

[[e-mail]]

Would it be silly to use short names for 8-bit paths and long names for 16-bit?

Comment 90

[Jungshik Shin]

Attachment: patch confirmed to work on Windows ME

I've tested a debug build with this patch on Windows ME (en-US) as well as Windows 2k (ko) to find that it actually works as intended.

There are still some loose ends to tie up. They include potential errors (buffer overrun, memory leak, use of replacement char '?' vs '_' in W2M conversion, potential string API 'link' issue, xpcom/obsolete support, whether or not to expose emulated 'W' APIs globally and how to do that if necessary etc) in my 'W' API emulation on Windows 9x/ME. I also need to make sure that I can refer to the addresses of 'W' APIs on Windows 9x/ME (although they're not actually implemented) instead of using GetProcAddress(?). My test on Windows ME indicates that it's possible, but  Wi 9x/ME differ slightly from each other  in what APIs they have so that actual testing on Win 95/98 is ncessary.

Due to a problem described in this thread (http://groups.google.com/group/netscape.public.mozilla.builds/browse_thread/thread/736f3b8791ed959b), I can't make an optimized build (even without profile added, cvpack gives me the same error when linking gklayout). If anybody is interested in testing a debug build (~13MB zipped) on Win 95/98, I'll put it up somewhere.

Comment 91

[Jungshik Shin]

Attachment: 1.8.x branch patch

This is a patch for 1.8.x branch. With this patch, MS IE bookmarks with characters outside the system default codepage (Devanagari with Korean locale) were confirmed to be imported. Because this patch is a port of attachment 209325 [details] [diff] [review] to 1.8.x branch, it shares common issues with attachment 209325 [details] [diff] [review].

Comment 92

[Jungshik Shin]

Attachment: patch that really works on Windows ME

I have no idea what happened with attachment 209325 [details] [diff] [review]. There's no way it could have worked on Windows ME (was I hallucinating? ...) because a dozen of 'W' APIs (which would just lead to 'Not Implemented' error on Win ME) were called with that patch. I thought I had replaced them all with the corresponding nsWinAPIs, but I didn't. Moreover, in NSPR, LoadLibraryW was used, which resulted in the dll load failure on Win ME.

Anyway, after a number of rebootings between Win 2k and Win ME, I finally made this patch work on Windows ME as well as on Windows 2k. While working on that, I realized that nsAppRunner.cpp uses nsILocalFile before xpcom initialization (and nsWinAPIs initialization). As an ad-hoc measure, I exposed NS_StartupWinAPIs so that it can be called in XRE_Main() of nsAppRunner.cpp. If there's a better way than this (e.g. calling it in nsLocalFileWinConstructor...), I'm willing to change it.

Other remaining issues:
 - I didn't change xpcom/obsolete because I need to expose nsWinAPIs outside xpcom/io to fix xpcom/obsolete (alternatively, I have to duplicate a bunch of lines in xpcom/obsolete). I'm not sure what's the best way to expose 'W' API wrapper functions of nsWinAPIs. We may want to do that to avoid the code duplication anyway (even if we don't wanna fix xpcom/obsolete) because some other parts of our code directly call Windows APIs that are wrapped up (for Windows 9x/ME) in xpcom/io/nsWinAPIs (that I added in this patch)

  - A bit more simplification is possible if we abandon Windows 95 (before OSR2), but seamonkey is still supposed to work on Win 95 so that I guess I'll just have to keep them now.
  - I need to inspect the code for emulation of 'W' APIs more thoroughly for possible 'one-by-off' error and buffer overrun, etc.
  - Need to add more comments
  - Need to resolve bug 278161 before resolving this one (the patch uploaded here may be 'polluted' with my interim patch for bug 278161)
  - There may be some Windows CE issues (but given that Darin's nsIWindowsRegKey 
implementation works there, I don't expect many issues although there may be a few problems to work around/fix).

Darin, can you take a look at this patch and give me some feedback (perhaps not in details but in the overall approach)? 
 
wtc, can you also take a look at the NSPR part (the amount of change is relatively small)? 

Thanks.

Comment 93

[Wan-Teh Chang]

Comment on attachment 210834 [details] [diff] [review]
patch that really works on Windows ME

You should add new prlink.h functions that take
UTF-16 pathnames.  We shouldn't make NSPR users
cast a PRUnichar * string to a char * string
just so we can avoid adding new NSPR functions.

We should also convert all library pathnames to
UTF-8, rather than converting just the ones given
to NSPR in UTF-16.

Comment 94

[Darin Fisher]

Hrm.. perhaps we should split the NSPR changes out into a separate bug.

Comment 95

[Jungshik Shin]

(In reply to comment #94)
> Hrm.. perhaps we should split the NSPR changes out into a separate bug.

That's a good idea given that there's apparently a "conflict" of "interest" between NSPR and Firefox et al. ;-) I filed bug 326168.

(In reply to comment #93)
> (From update of attachment 210834 [details] [diff] [review] [edit])
> You should add new prlink.h functions that take
> UTF-16 pathnames.  We shouldn't make NSPR users
> cast a PRUnichar * string to a char * string
> just so we can avoid adding new NSPR functions.

The idea behind that was to minimize NSPR changes necessary to fix this bug, on which I thought we kinda agreed in our email discussion. Perhaps, I was mistaken  or went too far in that direction.

> We should also convert all library pathnames to
> UTF-8, rather than converting just the ones given
> to NSPR in UTF-16.

That's one of what I had in mind when I wrote I needed to add more comment. In pr_UnlockedFindLibrary,  it's only the leaf name (not the whole path) that matters, isn't it? That being the case, it should work either way (in 99.99% of cases) because virtually all DLL names are in ASCII only and the directory separator is the same in UTF-8, ASCII and legacy encodings. Anyway, that's certainly not bullet-proof because somebody may have a non-ASCII dll name.

Comment 96

[Mike Cowperthwaite]

Does this patch address bug 210445?

Comment 97

[Jungshik Shin]

(In reply to comment #96)
> Does this patch address bug 210445?

No, it doesn't because to fix that, we need to use 'wmain' for the command line handling instead of main, but we can't do that as long as we support Win 9x/ME. In FF 3.0, perhaps we will switch to wmain because Win 9x/ME support will be dropped.

Comment 98

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

GetCommandLineW?

Comment 99

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

remember that we don't currently use main, because mozilla is a gui app, so it uses WinMain.

Comment 100

[Jungshik Shin]

(In reply to comment #99)
> remember that we don't currently use main, because mozilla is a gui app, so it
> uses WinMain.

Aha. thanks. Anyway, that's beyond the scope of this bug. We already have a bug on that, don't we? 

Darin and others, do you have any comment on my latest patch (even if you haven't gone through it in details but just have had a cursory look) other than NSPR part? 
For PR_LoadLibraryWithFlags, it'd be nice to hear back some opinions in bug 326168 so that we can resolve this long standing bug before long.
 

Comment 101

[Darin Fisher]

jungshik: I just read over the patch, and I think it is looking really great!

Comment 102

[[e-mail]]

(In reply to comment #98)
>GetCommandLineW?
Note that Win 9x/Me doesn't support CommandLineToArgvW.

Comment 103

[Jungshik Shin]

Attachment: another update(getting closer)

Thanks, Darin, for taking a look. I'm getting closer. Updated my tree to sync with the trunk and cleaned up a bit. This patch also contains the latest patch for bug 326168 (with a typo fixed)

Comment 104

[Jungshik Shin]

Attachment: another update (should work on Win95)

includes the latest patch for bug 326168

Comment 105

[Jungshik Shin]

Attachment: patch for 1.8.x branch

With this patch and the necko part of the latest patch for bug 278161, I can open a file whose name has chars. outside the default codepage on Windows 2k. It also works fine on Windows ME. It should also work on Windows 95/98 (which are basically the same as Win ME), but I can't test it myself.

Comment 106

[Jungshik Shin]

Kimura-san, can you test my patches (for trunk and for branch) on Windows 95? 
Thanks tons in advance.

Comment 107

[Jungshik Shin]

Attachment: patch for trunk

attachment 212261 [details] [diff] [review] has a missing file. (prtypes.h)

Comment 108

[Jungshik Shin]

Attachment: a partial patch for bug 278161 necessary for testing my patch here

To test attachment 212312 [details] [diff] [review] or attachment 212322 [details] [diff] [review] on Win 2k/XP/Vista (to see if a filename with characters outside the default repertoire works), this partial patch for bug 278161 needs to be applied.

Comment 109

[Masatoshi Kimura [:emk]]

Comment on attachment 212322 [details] [diff] [review]
patch for trunk 

> nsGetFileVersionInfo       nsWinAPIs::mGetFileVersionInfo = GetFileVersionInfoW;
> nsGetFileVersionInfoSize   nsWinAPIs::mGetFileVersionInfoSize = GetFileVersionInfoSizeW;

VC6 sucks. winver.h bundled with VC6 defined GetFileVersionInfoW as
GetFileVersionInfoW(LPWSTR, DWORD, DWORD, LPVOID);
                    ^^^^^^
It should be
GetFileVersionInfoW(LPCWSTR, DWORD, DWORD, LPVOID);
GetFileVersionInfoW declaration does not also care about constness.

You should cast them to make VC6 happy.
Such as:
> nsGetFileVersionInfo       nsWinAPIs::mGetFileVersionInfo = (nsGetFileVersionInfo)GetFileVersionInfoW;
> nsGetFileVersionInfoSize   nsWinAPIs::mGetFileVersionInfoSize = (nsGetFileVersionInfoSize)GetFileVersionInfoSizeW;

And unfortunately, this didn't work on Win95 because SHGetPathFromIDListW is not exported from Win95 shell32.dll without IE. GetFileAttributesExW and GetDiskFreeSpaceExW are not also exported from Win95 kernel32.dll. You need GetProcAddress to assign function value to these functions.
Moreover, GetFileAttributesExA is not exported from Win95 kernel (even A function). You need to use an approach similar to GetDiskFreeSpaceExA.

With all the above errors resolved, I coudn't start on Win95 yet :-(
I'm digging into the reason.

Comment 110

[Jungshik Shin]

Thanks for testing on Windows 95.

(In reply to comment #109)
> (From update of attachment 212322 [details] [diff] [review] [edit])
> > nsGetFileVersionInfo       nsWinAPIs::mGetFileVersionInfo = GetFileVersionInfoW;
> > nsGetFileVersionInfoSize   nsWinAPIs::mGetFileVersionInfoSize = GetFileVersionInfoSizeW;
> 
> VC6 sucks. winver.h bundled with VC6 defined GetFileVersionInfoW as
> GetFileVersionInfoW(LPWSTR, DWORD, DWORD, LPVOID);
                     ^^^^^^

> It should be
> GetFileVersionInfoW(LPCWSTR, DWORD, DWORD, LPVOID);
........
> You should cast them to make VC6 happy.

I'm aware of the inconsistency between winver.h of VC++ 6 and that of MS Windows Platform SDK (and later version of VC++), but it doesn't matter as long as you have the include directory for Win PSDK *before* that of VC++ 6 as recommended by the mozilla build guide. I do use VC++ 6 because that's the *only* VC++ I have. 

> And unfortunately, this didn't work on Win95 because SHGetPathFromIDListW is
> not exported from Win95 shell32.dll without IE. GetFileAttributesExW and
> GetDiskFreeSpaceExW are not also exported from Win95 kernel32.dll. You need
> GetProcAddress to assign function value to these functions.
> Moreover, GetFileAttributesExA is not exported from Win95 kernel (even A
> function). You need to use an approach similar to GetDiskFreeSpaceExA.

Thanks. I wrote wrappers for all three of them and both trunk and 1.8.x branch build seem to work fine on Win 2k/ME. I also tried emulating GetFileAttributesExA on WinME to see how it would work on Win95 and it worked well. Anyway, none of these tests can substitute actual tests on Win95 so that your test on Win95 would be appreciated.  

Comment 111

[Jungshik Shin]

Attachment: trunk patch update addressing issues pointed out in comment #109

Masatoshi, can you try it on Win95? Testing on Win98 and other old Windows would be nice, too.

Comment 112

[Jungshik Shin]

Attachment: branch patch update

Comment 113

[Masatoshi Kimura [:emk]]

Comment on attachment 212367 [details] [diff] [review]
trunk patch update addressing issues pointed out in comment #109

"Program start error" does no longer occur, but it still fails with "This program has performed an illegal operation and will be shut down. If the problem persists, contact the program vendor."(actually in Japanese) We are the program vendor :-(
I'm building a debug version...

Comment 114

[Jungshik Shin]

Thanks again for testing. Hmm... that's tough. 
BTW, I put up my debug build (trunk) at 
http://i18nl10n.com/moztest/ff.debug.zip for others without a build set-up to test. 
(make a directory for it and unzip it in the directory and 'firefox' binary will be in the directory). On Windows 2k/XP, by setting the environment variable WINAPI_USE_API, one can sorta emulate Win 9x/ME. With that set, it would behave like ff 1.5/trunk. Without that set, it would be able to access the full unicode repertoire in file operations (e.g. File | open). Be aware that this patch alone does NOT fix all the bugs blocked by this bug. However, opening a file with Japanese name on French Windows 2k/XP should be possible. Saving a file to a desktop should work on Russian Windows 2k/XP whose default codepage is *changed* from the default (Windows-1251)  to one that cannot represent Russian (e.g. Windows-1252 - French, German, English, etc). 

Comment 115

[Masatoshi Kimura [:emk]]

OK. I found a crash reason.
On Win95 withoutIE, gGetSpecialPathProc will be initialized NS_GetSpecialFolderPath, but gGetSpecialFolderPathA aren't initialized because old shell32 doesn't export even A-version of GetSpecialFolderPath.
Therefore NS_GetSpecialFolderPath tries to call null pointer, then crash.
We should fall back to SHGetSpecialFolderLocation code path in this case.

Comment 116

[Masatoshi Kimura [:emk]]

That is,
+           gGetSpecialFolderPathA = (nsGetSpecialFolderPathA) 
+               GetProcAddress(gShell32DLLInst, "SHGetSpecialFolderPathA");
+           gGetSpecialFolderPath = NS_GetSpecialFolderPath;
should be like this.
+           gGetSpecialFolderPathA = (nsGetSpecialFolderPathA) 
+               GetProcAddress(gShell32DLLInst, "SHGetSpecialFolderPathA");
+           if (gGetSpecialFolderPathA)
+               gGetSpecialFolderPath = NS_GetSpecialFolderPath;
With this change, I could start, open Japanese directory, and open Japanese filename successfully on Win95!
Some minor problem remains:
I could no longer open local root directrory (e.g. file:///C:/).
This is a regression because I can open it without a patch.

Comment 117

[Benjamin Smedberg]

jshin, what is the target release for this work? On trunk we may and should use the W APIs directly because we're dropping support for anything less than win2k. The dynamic loading only makes sense if we're targeting ff2.

Comment 118

[Jungshik Shin]

(In reply to comment #117)

> win2k. The dynamic loading only makes sense if we're targeting ff2.

I'm targeting it at FF2. And even for FF3, there _might_ be some "non-GUI" consumers of XPCOM. BTW, whether we use W APIs directly or not, our codebase has tons of issues to deal with to take the full advantage of Unicode support on Win2k or later.

(In reply to comment #116)

> With this change, I could start, open Japanese directory, and open Japanese
> filename successfully on Win95!

Thanks !!

> Some minor problem remains:
> I could no longer open local root directrory (e.g. file:///C:/).
> This is a regression because I can open it without a patch.

Using GetFileAttributesEx rather FindFirstFile, I wanted to avoid the issue, but on Win95, I have to deal with it. See 

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/md/windows/w95io.c#803

I can port that code over here, but we may as well just forget about Win95 without IE (and possibly Win NT 3.5x).

Comment 119

[Jungshik Shin]

Benjamin, Darin, Wan-Teh and I all agreed, in an offline discussion, that in the long run, we need to implement NSPR UTF-16 APIs and call them in xpcom i/o. The following is my plan I sent to Darin and Wan-Teh offline. 

1. Finish what I've been doing and (try to) include it in FF 2.0 (and
there are many places throughout our codebase I need to fix so that
fixing bug 162361 has a real impact in mozilla)
2. Implement UTF-16 APIs on Win32 (it's relatively easy because we
don't have to worry about charset name variants and iconv/wchar_t
chaos on Unix) and possibly on Mac OS X. In this step, some lines of
code added to nsLocalFileWin will become redudant.
3. Implement UTF-16 APIs on Unix and other platforms

After step 2 is complete, we may want to make trunk  use NSPR for WINNT (where NSPR UTF16 APIs will use 'W' APIs) instead of WIN95. 

Anyway, I could have used/can use a lot of '#ifdef MOZILLA_1_8_BRANCH' to use 'W' APIs directly, but given the above plan, it's not worth bothering to...
 

Comment 120

[Masatoshi Kimura [:emk]]

(In reply to comment #110)
> I'm aware of the inconsistency between winver.h of VC++ 6 and that of MS
> Windows Platform SDK (and later version of VC++), but it doesn't matter as long
> as you have the include directory for Win PSDK *before* that of VC++ 6 as
> recommended by the mozilla build guide. I do use VC++ 6 because that's the
> *only* VC++ I have. 
But this may cause tbird tbox bustage. See bug 327675. Tbird tbox seems to not follow the build guide:-)
It doesn't matter if this patch land after upgrading the tinderboxes, of course.

Comment 121

[Jungshik Shin]

Attachment: patch updated (for a new patch for bug 326168)

bug 326168 is about to be fixed with a slightly different patch than included in the previous patch uploaded to this bug. This patch doesn't include NSPR patch any more, but was updated to work with the latest patch for bug 326168.

BTW, below is the result of the startup time measurement for my optimized trunk build without and with this patch. Because our tree uses nsILocalFile 'native' methods a lot, we have 5.9% performance loss [1][2]. Darin, offline, suggested that we replace them with 'UTF16' methods to gain performance on Windows 2k or later while sacrificing Linux and OS X a little (not very much because OS X uses UTF-8 and modern Linux uses UTF-8 as well).

00003.485
00003.164
00002.844
00002.804
00002.794
-----------
average: 3.0182


00003.725
00003.555
00002.794
00002.924
00002.984
-------------
average: 3.1964

[1] tinderbox startup numbers are about 500ms, but I'm getting several times larger numbers here assuming the unit is in second. I'm not sure why. 
[2] Variation is rather high so that 5.9% so that 5.9% may not be that much meaningful.

Comment 122

[Jungshik Shin]

Attachment: patch updated (for a new patch for bug 326168)

bug 326168 is about to be fixed with a slightly different patch than included in the previous patch uploaded to this bug. This patch doesn't include NSPR patch any more, but was updated to work with the latest patch for bug 326168.

BTW, below is the result of the startup time measurement for my optimized trunk build without and with this patch. Because our tree uses nsILocalFile 'native' methods a lot, we have 5.9% performance loss [1][2]. Darin, offline, suggested that we replace them with 'UTF16' methods to gain performance on Windows 2k or later while sacrificing Linux and OS X a little (not very much because OS X uses UTF-8 and modern Linux uses UTF-8 as well).

00003.485
00003.164
00002.844
00002.804
00002.794
-----------
average: 3.0182


00003.725
00003.555
00002.794
00002.924
00002.984
-------------
average: 3.1964

[1] tinderbox startup numbers are about 500ms, but I'm getting several times larger numbers here assuming the unit is in second. I'm not sure why. 
[2] Variation is rather high so that 5.9% so that 5.9% may not be that much meaningful.

Comment 123

[Darin Fisher]

jshin: the tinderbox startup tests record the lowest value of the five runs instead of the average.  unfortunately it is really hard to get consistent startup numbers, so i guess someone figured this was a better way to estimate startup time.  i think we might be better off throwing out the best and the worst and then averaging the middle three values or something like that ;-)

Comment 124

[Jungshik Shin]

(In reply to comment #121)

> BTW, below is the result of the startup time measurement for my optimized trunk

The numbers reported earlier turned out to be completely bogus. I wrote a shell script as following (to follow the instruction at
http://www.mozilla.org/performance/measureStartup.html )
and ran it from an xterm (under cygwin). 

-------------------
export NS_TIMELINE_ENABLE=1

for i in `seq 0 5`
do
./firefox -P "Default User" file:///c:/temp/quit.html > startup.log.$i 2>&1
done 
-------------

Somehow the lap time reported for 'main1' depends on when I move focus to firefox. (when firefox is started from an xterm under cygwin, the focus doesn't automatically move to firefox) That is, it can be made arbitrarily long. Using 'measure-simple.pl' didn't work either. I also tried it under a non-xterm cygwin console, but it has a similar but different problem. I'll try what tinderbox does (method 2). 

Incidentally, I'll  exclude two extreme points before averaging as suggested by Darin. 

Comment 125

[Jungshik Shin]

Here's the tally of 15 runs with and without my patch (optimized static trunk) on Windows 2k(P3 700Mhz, 512MB RAM). (btw, the numbers are sorted)

patched	not patched
------  ---------
1813	1812
1813	1882
1872	1892
1872	1893
1882	1903
1883	1903
1892	1903
1893	1903
1893	1913
1902	1913
1912	1913
1913	1913
1913	1923
1913	1933
1913	1943
-------------
1885	1902.8  : average (all 15 points)
32	29      : std. deviation
1888	1906    : average(excluding max/min points)

Two-sided Student t-test (assuming the equal variance) and Welch(sp?) t-test (not assuming the equal variance) gave me 0.135269 and 0.135387, which indicates that they're different. Apparently, we have some performance gain with the patch.

Comment 126

[Darin Fisher]

nice!

Comment 127

[Jungshik Shin]

I had to run before posting my previous comment and made a couple of mistakes. 

First, the comparison was made between my build with the latest patches for this and bug 278161 and my build with only the patch for bug 278161. The patch for bug 278161 is likely to give a perf. edge to the build with this patch applied so that the comparison is not fair. 

Second, my conclusion was wrong based on the p-value of 0.13(one-sided p-value is 0.065). With that high p-value, the null hypothesis (two builds are equal in terms of startup time) should be accepted. That is, there's no strong evidence that two builds have any perf. difference.

I made a new measurement (23 startups each) with fresh builds, one with only the patch for this bug and the other without any patch applied. One-sided t-test (with H_0 : patched is slower than unpatched) gave me p-value of 0.0074. With the significance level 0.01, H_0 is rejected so that my patched build is rather likely to be faster than unpatched. This is a little unexpected given my first point above.

Anyway, the bottom line here is that my patch here does NOT make startup time longer. That is, we don't have to worry about the startup performance.  

Comment 128

[Jungshik Shin]

Comment on attachment 214628 [details] [diff] [review]
patch updated (for a new patch for bug 326168)

Asking for review only for now.

Comment 129

[Darin Fisher]

Attachment: review comments from darin on attachment 214628

Comment 130

[Darin Fisher]

Comment on attachment 214628 [details] [diff] [review]
patch updated (for a new patch for bug 326168)

Please see my attached review comments.

Comment 131

[Jungshik Shin]

Attachment: trunk patch addressing Darin's review comment

Darin, thanks a lot for your thorough review. I think I addressed all of your concerns. 

I used 'MAX_PATH' (it seems it's virtually identical to '_MAX_PATH' so that I thought I'd rather 'save' space in the source code :-)). I changed some boundary checking parts and static buffer size because I found that MAX_PATH(=_MAX_PATH) includes the terminating null (it's for paths like "C:\<256 chars>NULL"). 

Also added are a few more error checkings in SpecialSystemDirectory.cpp. The current trunk  doesn't do that, but I thought it's better to be safe. 
 
I added nsWinAPIs::sDummy and initialized it with nsWinAPIs::GlobalInit(). I don't have to call NS_StartupWin in nsXREDir...cpp any more. I kept it in nsXPCOMInit.cpp just in case. Anyway, both static optimized build and non-static debug build worked fine. 

For OM check with SetLength, I added a template helper function |SetLengthAndCheck| to nsWinAPIs.cpp because the same pattern appears several times with nsAutoString and nsCAutoString. 

I deleted all the commented out codes and unncessary comments while adding a rather long comment about helper functions in nsLocalFileWin (OpenDir, ReadDir, OpenFile, etc.) that will eventually be removed once NSPR implements them. 

As for not bothering to take care of root path and paths ending with a slash on Win95 (nsWinAPIs.cpp : GetFileAttributesEx), it's very rare to see Win 95 without MS IE 4 or later. Even on such a machine, the only problem is that a user can't open a root path or a path ending with a slash. We can just release-note it instead of copying a raher big chunk of code from NSPR. Btw, MS's own emulation of GetFileAttributesEx in an SDK header file doesn't do that either.

I haven't yet tested this patch on real Windows ME (I'm building a non-cairo build now), but with WINAPI_USE_ANSI on Windows 2k, a non-static debug build worked fine.

Comment 132

[Jungshik Shin]

Attachment: updated trunk patch to make it work on real Win 9x/ME

attachment 215618 [details] [diff] [review] has a critical problem. It doesn't work on Win 9x/ME. 'WINAPI_USE_ANSI' cannot be a true substitute for testing it on an actual Win 9x/ME. |nsWinAPIs::sDummy| was defined _before_ function pointers for Win APIs are so that function pointers set to our emulated 'W' APIs in |GlobalInit| were reverted back to native 'W' APIs which are just stubs on Win 9x/ME. 

By initializing |sDummy| with |GlobalInit| _after_ function pointers for Win APIs, I was able to avoid the problem. I actually tested a non-cairo debug (non-static) on Windows ME and it worked well. I haven't yet tested an optimized static build (non-cairo), but I guess/hope a static build doesn't have any problem.

Still a question remains : Can we rely on the behavior of VC++ 8.0 that the order static variables are initialized is the same as the order they're defined in the source file? Does C++ standard say anything about it?

Comment 133

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

(In reply to comment #132)
> Still a question remains : Can we rely on the behavior of VC++ 8.0 that the
> order static variables are initialized is the same as the order they're defined
> in the source file? Does C++ standard say anything about it? 

If it works in VC6, then it's fine for the 1.8 branch; for the trunk, Win9x/ME are not supported, so it's a moot point.

Comment 134

[Benjamin Smedberg]

You should not rely on the order of initializing static vars. Can't we do lazy-init in the nsLocalFile constructor or a similar place?

Comment 135

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

the C++ standard does say that order of initialization is order of declaration.
(you can't do a similar thing in C)

Comment 136

[Jungshik Shin]

(In reply to comment #134)
> You should not rely on the order of initializing static vars. Can't we do
> lazy-init in the nsLocalFile constructor or a similar place?

I can (that's one of two alternatives Darin mentioned in his review comment), but because that's a hot spot, I was worried about the overload of a calling NS_StartupWinAPI(). It may not be a problem (can be buried in 'noise'. I need to measure it). If it's not a perf-hit, perhaps it's better to take this approach. 

(In reply to comment #135)
> the C++ standard does say that order of initialization is order of declaration.

The order of *declaration* (rather than the order of *definition*)?? Hmm... function pointers are public and declared before |sDummy| (in the defintion of |class nsWinAPIs| in nsWinAPIs.h) which is private, but |sDummy| was initialized *before* function pointers until I moved its definition *after* the definition of function pointers (in nsWinAPIs.cpp). I googled it (newsgroup search) and it seems that it's the order of *definitions* that matters, but it's not definitive . [1]
 
(In reply to comment #133)

> If it works in VC6, then it's fine for the 1.8 branch

I can't build 1.8 branch with VC6 any more because VC6 and VC8-express cannot coexist on a single machine. I had to switch to VC7 toolkit to build both 1.8 branch and trunk on a single Win2k box. Anyway, my 1.8 (optimized static) build with VC7 toolkit was just completed and I'm about to test it under Win ME.


> for the trunk, Win9x/ME are not supported, so it's a moot point.

I know.. that's why I built a non-cairo build. In an unlikely(rare)  case of non-GUI embedders,  it might not ;-)

[1] C++ draft standard (perhaps of C++ 98) has the following. 
<quote> 
3.6.2 Initialization of nonlocal objects [basic.start.init]
1 The storage for objects with static storage duration (3.7.1) shall be zeroinitialized (8.5) before any other
initialization takes place. Objects of POD types (3.9) with static storage duration initialized with constant expressions (5.19) shall be initialized before any dynamic initialization takes place. Objects of namespace
scope with static storage duration _defined_ in the _same_ translation unit and dynamically initialized shall be initialized in the _order_ in which their _definition_ appears in the translation unit.
</quote>

So, it's the order of definitions that counts, but it's about objects of 'namespace scope'

Comment 137

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

Oh... sorry, yeah, I think I actually mean definition

Comment 138

[Masatoshi Kimura [:emk]]

(In reply to comment #133)
> If it works in VC6, then it's fine for the 1.8 branch; for the trunk, Win9x/ME
> are not supported, so it's a moot point.
At least installer should start on Win9x to fix bug 330208. Do you mean bug 330208 should be WONTFIXed?

(In reply to comment #136)
> I can't build 1.8 branch with VC6 any more because VC6 and VC8-express cannot
> coexist on a single machine. 
Really? I can build trunk with VC8 and build 1.8 branch with VC6 on the same machine. But my VC8 is Standard Edition, so it may make a difference.

> > for the trunk, Win9x/ME are not supported, so it's a moot point.
> I know.. that's why I built a non-cairo build. In an unlikely(rare)  case of
> non-GUI embedders,  it might not ;-)
Moreover, some tinderboxen do not still enable cairo. Have SeaMonkey guys said they would migrate to cairo build?

Comment 139

[Stuart Parmenter]

(In reply to comment #138)
> > > for the trunk, Win9x/ME are not supported, so it's a moot point.
> > I know.. that's why I built a non-cairo build. In an unlikely(rare)  case of
> > non-GUI embedders,  it might not ;-)
> Moreover, some tinderboxen do not still enable cairo. Have SeaMonkey guys said
> they would migrate to cairo build?
> 
Gecko as a platform is dropping support for old versions of Windows (Win95,98,ME) for 1.9.  Whenever SeaMonkey wishes to release with this gecko version they'll also drop support for those.

I was told by the seamonkey council that they were going to work off the 1.8 branch for a long time and would migrate to 1.9 when they were ready.  Because of this, upgrading the SeaMonkey windows tinderboxes has not been a priority.

Comment 140

[Jungshik Shin]

(In reply to comment #136)

> I can't build 1.8 branch with VC6 any more because VC6 and VC8-express cannot
> coexist on a single machine. I had to switch to VC7 toolkit to build both 1.8
> branch and trunk on a single Win2k box. Anyway, my 1.8 (optimized static) build
> with VC7 toolkit was just completed and I'm about to test it under Win ME.

 I'm writing this using a 1.8 branch build (optimized, static) with the latest patch (slightly changed for the branch)  on Windows ME. I can't make a build with VC6, but I found a newsposting in VC++ newsgroup that it's compliant to the standard mentioned by biesi (comment #135 and comment #137) so that I guess we can go with this approach.
 

Comment 141

[Darin Fisher]

pav: this patch is being developed with the goal of creating something that can ship in FF2.  that may be a tall order, but it's why we are making the effort to support Win9x.

Comment 142

[Stuart Parmenter]

For ff2 that makes sense, but for the trunk it seems like we're better off removing all the ascii calls entirely..

Comment 143

[Jungshik Shin]

Attachment: patch for 1.8 branch

This is basically identical to attachment 215662 [details] [diff] [review] except for a few differences necessary for 1.8.x branch. An optimized static build with this patch was tested on a real WinME box.

Comment 144

[Darin Fisher]

> For ff2 that makes sense, but for the trunk it seems like we're better off
> removing all the ascii calls entirely..

Perhaps.  Maybe someone will want to port the trunk to Win9x?  Are we going to tell them "no" ?

Comment 145

[Stuart Parmenter]

(In reply to comment #144)
> Perhaps.  Maybe someone will want to port the trunk to Win9x?  Are we going to
> tell them "no" ?
> 

I'm not really sure what the right answer is there.  I think that developers should be able to ignore win9x and we should start moving our code away from it to cleaner and simpler code.  If someone did want to keep it working on win9x it might be better for them to do it as a compatibility library so that we can keep most of the code clean.

Comment 146

[Shane Caraveo]

(In reply to comment #143)
> Created an attachment (id=215753) [edit]
> patch for 1.8 branch
> 

That patch does not build on 1.8, it makes use of PR_LibSpec_PathnameU which is only on trunk, but that's not the only error.

Comment 147

[Darin Fisher]

Comment on attachment 215662 [details] [diff] [review]
updated trunk patch to make it work on real Win 9x/ME

>Index: xpcom/build/nsXPComInit.cpp

>@@ -501,14 +504,19 @@ NS_InitXPCOM3(nsIServiceManager* *result
...
>+#ifdef XP_WIN
>+    NS_StartupWinAPIs();
>+#endif

Is this still necessary?


>+// This is a dummy variable to make sure that WinAPI is initialized 
>+// at the very start. Note that |sDummy| must be defined AFTER
>+// all the function pointers for Win APIs are initialized. Otherwise,
>+// what's done in |GlobalInit| would have no effect.
>+// XXX: Can we rely on that |sDummy| is initialized after all
>+// the function pointers for Win APIs are? Does C/C++ standard anything to 
>+// say about the order of initializing static variables?  
>+PRBool nsWinAPIs::sDummy = nsWinAPIs::GlobalInit();

If we have decided that this works and is valid per the C++ spec,
then let's go ahead and change this comment to remove the XXX part.


Perhaps you should seek an additional review on this patch... maybe
bsmedberg would be willing? ;-)

Comment 148

[Shane Caraveo]

(In reply to comment #146)
> (In reply to comment #143)
> > Created an attachment (id=215753) [edit]
> > patch for 1.8 branch
> > 
> 
> That patch does not build on 1.8, it makes use of PR_LibSpec_PathnameU which is
> only on trunk, but that's not the only error.
> 

I see, it relies on bug 326168

Comment 149

[Jungshik Shin]

> 
> That patch does not build on 1.8, it makes use of PR_LibSpec_PathnameU which is
> only on trunk, but that's not the only error.

Yes, it relies on the patch for bug 326168 as you realized later. The patch for bug 326168 needs to be slightly changed (the diff context is a little different so that it can't be applied cleanly). 

(In reply to comment #145)
> (In reply to comment #144)
> > Perhaps.  Maybe someone will want to port the trunk to Win9x?  Are we going to
> > tell them "no" ?
> >  

> to cleaner and simpler code.  If someone did want to keep it working on win9x
> it might be better for them to do it as a compatibility library so that we can
> keep most of the code clean.

That's more or less what's done here. Almost everything for Win 9x/ME is confined to WinAPIs.{h,cpp}. Exceptions are that we use 'nsWinAPIs::mCopyFile" instead of  '::CopyFileW' in other files(the same is true of other Win32 APIs we use in xpcom i/o) and that there are a couple of |if NS_UseUnicode() ... else ...| in other files. NSPR needs some changes as well (in bug 326168), but it has its 'own life' so that it's not much relevant here.

Comment 150

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

when fixed, Bug 330276 would make it very hard to make trunk work on win9x w/o a compatibility library

Comment 151

[Jungshik Shin]

Attachment: updated trunk patch (NS_StartupWinAPIs is now gone)

I got rid of the unncessary NS_StartupWinAPIs and NS_ShutdownWinAPIs as pointed out by Darin. I also removed 'XXX' comment about the initialization order.

Comment 152

[Jungshik Shin]

Comment on attachment 215818 [details] [diff] [review]
updated trunk patch (NS_StartupWinAPIs is now gone)

Darin and Benjamin, thanks for review. 

Darin, would you sr or should someone else do it?

Comment 153

[Darin Fisher]

Comment on attachment 215818 [details] [diff] [review]
updated trunk patch (NS_StartupWinAPIs is now gone)

sr=darin

let me know if you need help getting this landed.  nice work on this patch, jshin!

Comment 154

[Jungshik Shin]

Thanks, everyone. I've just landed the patch. Without fixing bug 278161, opening a file with characters not covered by the default code page in filename wouldn't work. However, importing IE bookmarks with the same problem should work without fixing bug 278161. So, that should be taken as a test for this patch for now.

Comment 155

[Jungshik Shin]

Attachment: 1.8 branch patch updated (no more NS_StartupWinAPis)

This is basically the same as attachment 215818 [details] [diff] [review] with a few differences due to the difference between the trunk and the 1.8 branch. This patch relies on NSPR changes in bug 326168 so that the latest patch  uploaded  for 1.8.x branch in that bug also has to be applied. I'll ask for 1.8 approval after some baking of attachment 215818 [details] [diff] [review] in trunk. In the meantime, anybody is welcome to test it on her/his box (especially Win 9x/ME). On Win 9x/ME, some startup (and other) performance loss is expected because we now store file paths in UTF-16 and convert to and from the native encoding for file I/O on Win 9x/ME.

Comment 156

[Hans-Andreas Engel]

Attachment: Patch for mingw-header include/w32api/winver.h

The fix for this bug broke compilation with MinGW due to an error in the MinGW header.  This header can be corrected with the attached patch; I will also submit it to the MinGW project.

Comment 157

[Bill Gianopoulos [:WG9s]]

The code checked into the trunk for this bug appears to have broken the Windows Thunderbird build.  Patrocles is red in the Thunderbird tinderbox.

Comment 158

[Jungshik Shin]

(In reply to comment #157)
> The code checked into the trunk for this bug appears to have broken the Windows
> Thunderbird build.  Patrocles is red in the Thunderbird tinderbox.

See comment #110 and comment #120. I also filed bug 331433. Who can fix patrocles configuration? 

Comment 159

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

preed handles this sort of thing, i believe.

Comment 160

[Bill Gianopoulos [:WG9s]]

This checkin appears to have caused the regression in bug 331453.

Comment 161

[Jungshik Shin]

Attachment: fix a "typo" in attachment 215818

I'm sorry somehow this stupid mistake crept in. This has a remote chance of being the cause of the regression reported in 331453

Comment 162

[Darin Fisher]

Comment on attachment 216083 [details] [diff] [review]
fix a "typo" in attachment 215818 [details] [diff] [review]

r+sr=darin

Comment 163

[Jungshik Shin]

(In reply to comment #162)
> (From update of attachment 216083 [details] [diff] [review] [edit])
> r+sr=darin

thanks. this got landed on the trunk 

Comment 164

[Jungshik Shin]

Attachment: updated branch patch (with a 'typo' fixed, regression taken care of)

This incorporates attachment 216083 [details] [diff] [review] and the latst patch for bug 331453 (regression in file download)

Comment 165

[cls]

Attachment: fix mingw bustage v1

|const WCHAR| vs |WCHAR| vs |CHAR| again.

Comment 166

[Hans-Andreas Engel]

Probably it is better to fix MinGW directly and to use the proper function declarations using |const| (see Comment 156 and Attachment 215907 [details] [diff]), than to change the mozilla code (see Comment 165).

MinGW's CVS is now updated, so the mentioned MinGW bustage should disappear when updating to the most recent MinGW version; see Bug 328499, Comment 48 for instructions. 

Comment 167

[Jungshik Shin]

Attachment: branch patch with follow-up patches combined

This patch includes the patches for bug 331453, bug 332123, bug 331433 (a temporary workaround for misconfigured tinderbox) as well as attachment 216083 [details] [diff] [review]. It's been in the trunk for about 10 days and I guess there won't be any more regression, but it might need still more baking on the trunk. (say, 10 more days...)

Darin, can you approve for branch landing when you think this patch has been baked long enough? 

Once this patch is landed on the branch, we can remove nsWinAPIs on the trunk (as Win 9x/ME is not supported any more on trunk: bug 330276)

Comment 168

[Darin Fisher]

OK.  Let's shoot for later this week.

Comment 169

[Darin Fisher]

Comment on attachment 217241 [details] [diff] [review]
branch patch with follow-up patches combined

a=darin

Comment 170

[Peter van der Woude [:Peter6]]

checked in on branch on 2006-04-08 10:12

Comment 171

[[e-mail]]

Attachment: Fix my branch tinderbox

I would like this patch on the branch. It has also been tested on Windows XP.

Comment 172

[Jungshik Shin]

Neil landed his patch for NT 3.51 on 1.8 branch. I had landed my branch patch earlier as noted in comment #170. 

Comment 173

[Jungshik Shin]

Comment on attachment 217750 [details] [diff] [review]
Fix my branch tinderbox


Ooops. Sorry I was mistaken by Neil's tinderbox comment. He just applied the patch to his NT 3.51 tinderbox, but not yet landed this patch. 

Yeah, this is what I would have done. 

r=jshin

Comment 174

[Michael Osipov]

I can't believe that this is fixed because it's not working
using german windows w/ german locale and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060410 Firefox/2.0a1
with a simple test Отдых.html (cyrillic script)
fifox gave me: check my first attachment
it replaced the russian letters with all the same %3F==? thi cant me right
I created a second file: tö€ßн.html
2nd screenshot
firefox failed of course of the russian н bug also won't display in the browser the € symbol instead %80 is shows %AC
this test with some special characters but no russian ones
and it works
see 3rd screenshot
looks perfect even with the € symbol and correct %80

Can somebody explain this? I can't, that's why I don't understand why this is marked as resolved fixed!

Comment 175

[Michael Osipov]

Attachment: 1st screenshot

fails russian script

Comment 176

[Michael Osipov]

Attachment: 2nd screenshot

fails russian script and additionally € sign

Comment 177

[Michael Osipov]

Attachment: 3nd screenshot

renders perfectly

Comment 178

[Jungshik Shin]

(In reply to comment #174)
> I can't believe that this is fixed because it's not working
> using german windows w/ german locale and Mozilla/5.0 (Windows; U; Windows NT
> 5.1; en-US; rv:1.8) Gecko/20060410 Firefox/2.0a1
> with a simple test Отдых.html (cyrillic script)

Please, set View | Character Encoding to UTF-8 before posting any non-ASCII character. You don't need to attach three screenshots to show what's already well-known. Bug 278161 is not yet fixed on 1.8 branch, which is why you still have the problem. On the trunk, bug 278161 has been fixed so that it should work fine. (see comment #154).

Comment 179

[Michael Osipov]

Jungshik,

I just downloaded Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060415 Firefox/3.0a1

still fails!

what am I doing wrong?

Comment 180

[Jungshik Shin]

(In reply to comment #179)

> I just downloaded Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
> Gecko/20060415 Firefox/3.0a1

> still fails!

How did you try to open the file? Did you try to open it by double-clicking on a file? That doesn't work yet because our command line handling and parameter passing (for file types associated with firefox) don't yet use 'W' APIs (there are bugs for both issues, but I don't remember bug numbers at the moment). Drag&Drop doesn't work either, but will be fixed very soon on the trunk.

However, opening a file with 'File | Open' works fine. So does opening a file by typing 'file:///........' in the url bar (probably since the patch for bug 261929 was checked in.) I've just downloaded the latest trunk build and confirmed that both methods worked fine (opening a Hindi-named file)

Comment 181

[Michael Osipov]

Jungshik,

I used double-click only!
That's probably the reason.

Well, lets wait to have all opening ways patched!

Comment 182

[Jungshik Shin]

(In reply to comment #181)
> I used double-click only!
> That's probably the reason.

That is the reason :-). I should have figured that out from question marks in your screenshot. Why don't you try 'file | open' or typing 'file://..../<cyrilic file name>' in the url bar.
 
> Well, lets wait to have all opening ways patched!

I couldn't find a bug on 'double click and file opening'. The closest I found is bug 268290. I also filed bug 334282.
 

Comment 183

[Jungshik Shin]

(In reply to comment #182)

> I couldn't find a bug on 'double click and file opening'. The closest I found
> is bug 268290. I also filed bug 334282.
 
I meant bug 267989. Bug 334282 was duped to bug 282285

Comment 184

[Michael Osipov]

Jungshik,

just tried 1.9a1: 2006041604 trunk

works as you have described altough produced while handling there files high CPU load which lead to browser crash

Comment 185

[石庭豐 (Seak, Teng-Fong)]

I've just filed a bug relating to Unicode filename. Please see if it could be included in this bug's dependency tree:
https://bugzilla.mozilla.org/show_bug.cgi?id=359148

I know, I know, this bug is closed.

Comment 187

[[e-mail]]

Why is GetNativeCanonicalPath lossy? I thought short paths were always ASCII.

Comment 188

[Masatoshi Kimura [:emk]]

Short path names are not guaranteed to be present. If the short path is not exist, GetShortPathName will return the input path without modification which may contain non-ASCII characters.