162358 - NSPR needs UCS2 interface

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[Roy Yokoyama]

This is a split from 58866.

We need to have a set of new NSPR_UCS2 interfaces for 
accessing the Unicode file system.

Comment 1

[Roy Yokoyama]

Attachment: remove bunch of UCS2 defines in primpl.h

wtc: can you review?

Comment 2

[Rui Xu]

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

Comment 3

[Wan-Teh Chang]

Comment on attachment 94973 [details] [diff] [review]
remove bunch of UCS2 defines in primpl.h

Roy,

This patch looks very good.  I'll write more
detailed comments later.  Here are two quick
notes.

1. PRUnichar is now defined in the XPCOM header
nscore.h.  Since NSPR can be used stand alone,
PRUnichar also needs to be defined in an NSPR
header (the best choice is prtypes.h).

To avoid defining the type twice, we will need
to do something like

#ifndef __PRUNICHAR__
#define __PRUNICHAR__
typedef ... PRUnichar;
#endif

2. I am afraid that these UCS2 functions will
need to be conditionally compiled because they
are only implemented on Windows.  This doesn't
meet NSPR's minimum cross platform requirement.

Maybe put them in something like #ifdef MOZ_UNICODE?

Are you going to store the pathnames in UTF8 or
UCS2?  Does the conversion from UTF8 to UCS2
incur perceptible performance penalty?

Comment 4

[Roy Yokoyama]

Wan-Teh: so should I just get rid of all the changes 
         in non-windows (eg. mac, os2)?

Comment 5

[Roy Yokoyama]

Attachment: all the changes are wrapped in #ifdef MOZ_UNICODE and removed other platform code

nhotta/smontagu: can you build my patch on other platforms (nhotta: Mac and 
		 smontagu: Linux)?  Please tests with and without 
		 MOZ_UNICODE environment setting. (Note: I am not sure how
other
		 platforms do this thoug....)

Comment 6

[nhottanscp]

Roy, can we do that using your machine (my machine cannot be used currently)?

Comment 7

[Simon Montagu :smontagu]

The patch builds on Linux without errors.

Comment 8

[Roy Yokoyama]

With nhotta's help, I was able to compile the patch on mac without errors.

>Are you going to store the pathnames in UTF8 or
>UCS2?  Does the conversion from UTF8 to UCS2
>incur perceptible performance penalty?
As it stands, I am pushing for UTF8 for the pathnames.
However, it may change in the future.  Performance
from UTF8 to UCS2 should be minimum since the conversion
is a simple mathmatical algorithm.

Wan-Teh: can you review?

Comment 9

[Frank Tang]

>Wan-Teh: so should I just get rid of all the changes in non-windows (eg. mac, os2)?
Roy: I am not quit undersatand why you ask that question based on wtc's previous
comment. Are you should that is what wtc want to see ? 

wtc- can you take a look at the new patch by the end of this week and let roy
know if there are *major* issue asap. Thanks 

Comment 10

[Roy Yokoyama]

Frank: Wan-Teh didn't ask us to remove all other non-Windows platform code. 
       He asked we should #ifdef the code.  I wanted to clarify 
       what he meant.  (BTW, I put the ifdef'ed patch for Windows platform only) 

Comment 11

[Wan-Teh Chang]

Attachment: Proposed patch v3

I edited Roy's patch (v2) to match NSPR's coding style.
Only one change affected the exported API: the 'nameUCS2'
field of the PRDirEntryUCS2 structure is renamed 'name'.

Here are some other changes I made to Roy's patch.

1. I added a big "EXPERIMENTAL" warning to the PRUnichar type
and the new functions.	I know you WILL use these functions
in a shipping product soon.  However, there are two open issues
with these new functions.
    1) They are only implemented for Windows.  NSPR functions
       need to be implemented at least for Windows and Unix
       (including Linux and Mac OS X).
    2) I am still not sure whether the new functions should
       take UCS2 or UTF8 pathnames.
Therefore, it is possible that these new UCS2 I/O functions will
be removed or replaced by UTF8 I/O functions in a future NSPR
release.  Since NSPR maintains backward compatibility, I must
issue a warning on functions that may be removed.

2. The indentation in NSPR is four spaces, not two spaces.

3. The definition of the PRUnichar type is moved to prtypes.h.

Finally, I noticed the UCS2 versions of some functions were
missing in Roy's patch, such as PR_GetFileInfo64, PR_Delete,
PR_Access, PR_MakeDir, and PR_RmDir.

Comment 12

[Wan-Teh Chang]

Attachment: Proposed patch v4

Only one change from the previous version:
PR_DIRECTORY_SEPARATOR ('\\') is changed to
L'\\' in FlipSlashesW().

Comment 13

[Roy Yokoyama]

Wan-Teh: Thanks for putting the revised patch together.
         Should I give /r= for the patch v4?

Comment 14

[Roy Yokoyama]

Comment on attachment 100805 [details] [diff] [review]
Proposed patch v4

/r=yokoyama
This patch works with 
MOZ_UNICODE flag.

Please get this /sr'ed
check in at your earliest

Comment 15

[Roy Yokoyama]

Attachment: Support for Win95 platform

Identical to the previous patch except 
implicit call to FindFirstFileW(), FindNextFileW(), CreateFileW().
Otherwise doesn't run in Win95.

Comment 16

[Roy Yokoyama]

Comment on attachment 103751 [details] [diff] [review]
Support for Win95 platform

/r=yokoyama
carry over my review stamp

Comment 17

[Roy Yokoyama]

bulk milestone change

Comment 18

[Roy Yokoyama]

Attachment: Oops, we need to call FreeLibrary() after calling LoadLibrary()

wtc: can you review?

Comment 19

[Wan-Teh Chang]

Comment on attachment 105651 [details] [diff] [review]
Oops, we need to call FreeLibrary() after calling LoadLibrary()

Roy,

The basic idea of your patch is good.  I'm
going to suggest a few changes.

1. In mozilla/nsprpub/pr/src/md/windows/w95io.c,
the way you look up the function addresses
is not thread-safe.  The easiest way to make
it thread-safe is to look up the function
addresses during NSPR initialization.  You
can add a new function that looks up the
function addresses and have _PR_MD_INIT_IO
call that function.  Then, at the beginning of
each of the _PR_MD_XXX_UCS2 functions, you say
something like this (using _PR_MD_OPEN_FILE_UCS2
as an example):

    if (!createFileW) {
	PR_SetError(PR_NOT_IMPLEMENTED_ERROR, 0);
	return -1;
    }

2. In mozilla/nsprpub/pr/src/md/windows/w95io.c,
the return value of NS_GetDriveTypeW should be
UINT, not DWORD.

3. In mozilla/nsprpub/pr/src/md/windows/w95io.c,
do not use the C++ comment delimiter //.  Also,
the level of indentation is four spaces, not two.

4. In mozilla/nsprpub/pr/src/md/windows/w95io.c,
there are several comments like this:

  Moz doesn't run on Win95 if we have CreateFileW()

They should be changed to something like this:

  NSPR doesn't run on Win9x if we call CreateFileW().

5. In mozilla/nsprpub/pr/src/pthreads/ptio.c, the
PR_GetFileInfo64UCS2 function should simply say:

    PR_SetError(PR_NOT_IMPLEMENTED_ERROR, 0);
    return PR_FAILURE;

Comment 20

[Wan-Teh Chang]

Attachment: Proposed patch v7

In addition to the changes I suggested in comment #19, I
made the following changes.

1. Renamed _PR_IS_SLASH_UCS2 to _PR_IS_W_SLASH.

2. Renamed IsRootDirectoryUCS2 to IsRootDirectoryW.

3. In _PR_MD_GETFILEINFO64_UCS2, sizeof(pathbuf) needs
to be replaced by sizeof(pathbuf)/sizeof(pathbuf[0])
because it is the size of the pathbuf array in PRUnichars,
not bytes, that we want.

Roy, please review and test this patch.  Thanks.

4. I replaced isalpha(LOBYTE(fn[0])) by iswalpha(fn[0]).

Comment 21

[Roy Yokoyama]

Comment on attachment 106301 [details] [diff] [review]
Proposed patch v7

/r=yokoyama
I've reviewed and tested. 
It works fine.
Thanks.

Comment 22

[Wan-Teh Chang]

Attachment: Proposed patch v8

The only change from the v7 patch is that "UCS2" has been
changed to "UTF16".

Comment 23

[Roy Yokoyama]

It looks like the new patch is already checked in.
Thanks, Wan-Teh.