104305 - -P commandline option doesn't work with non-Native char profile names

Status: RESOLVED WORKSFORME

(Core :: Internationalization, defect, P3)

Description

[Rui Xu]

Build: 2001-10-10 094 branch.

Steps:
1. Pre-create a new profile with double-byte char name.
2. Using netscp6 -P <double-byte char name created at step 1> to launch 
browser.

Actual:
Profile Manager appears, cannot launch browser with both DBCS and unicode 
double-byte char names.

Comment 1

[Frank Tang]

m0.9.6 P2

Comment 2

[Roy Yokoyama]

Attachment: Convert commandline (platform locale) to Unicode + indentation fix

Comment 3

[Roy Yokoyama]

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

Comment 4

[Roy Yokoyama]

Attachment: Same as previous patch excluding the indentation to improve the readability

Comment 5

[Roy Yokoyama]

nhotta: can you review the patch? 
10/23/01 18:56 is the real patch.
10/23/01 19:08 is just to show you what's really fixed. 

Comment 6

[nhottanscp]

Please check the result of ConvertStringToUnicode.
I think this should be reviewed by module owner (ccarlen?).

Comment 7

[Roy Yokoyama]

Attachment: Adding ASSERT for GetPlatformCharset(),ConvertStringToUnicode()

Comment 8

[Roy Yokoyama]

ccarlen:  can you review?
======= ccing [e-mail]

Comment 9

[Conrad Carlen (not reading bugmail)]

The patch looks good. I know the whitespace in that file isn't good, but is it
bad enough to warrant wiping out all the CVS blame info?

Comment 10

[Roy Yokoyama]

ccarlen: I was asked few times to fix some of indentations within
the function by the super reviewers.  
In this patch, I correct only for nsProfile::ProcessArgs()
which turns out to be about 200 lines of code out of 2500 lines
in the nsProfile.cpp

If you feel it's not justifiable, then I'll post a new patch
just to include my changes.

Comment 11

[Conrad Carlen (not reading bugmail)]

It seems like most of this blame info which would be changed is fairly old, and
I don't know if I've looked at it often, so go ahead. r=ccarlen.

But still, it's something to think about - cleaning up whitespace is good but
has to be weighed against the loss of blame info. 

Comment 12

[Roy Yokoyama]

Comment on attachment 55090 [details] [diff] [review]
Adding ASSERT for GetPlatformCharset(),ConvertStringToUnicode()

r=ccarlen

Comment 13

[Roy Yokoyama]

brendan: can you super review?
The patch (10/25/01 10:40) includes bunch of tab/indentation corrections.
You may want to review the smaller patch (10/23/01 19:08) which only
displays what is really changed, execpt the assertions in case 
GetPlatformCharset() and ConvertStringToUnicode() fail.

Thanks

Comment 14

[Brendan Eich [:brendan]]

ccarlen: cvsblame lets you find blame for old revs, so you can "look past" a
whitespace cleanup checkin.  There's a webtools bug on file to make this easier
to use.

I still think we should not use nsAutoString& in formal parameter lists.  Cc'ing
jag for best advice, I think nsAWritableString& is good but I welcome the latest
wisdom from the mountain.

/be

Comment 15

[jag (Peter Annema)]

Brendan's correct. Instead, try this:

+extern nsresult ConvertStringToUnicode(const nsString& aCharset, const char*
inString, nsAString& outString);

+extern nsresult GetPlatformCharset(nsString& aCharset);

To use nsAString& all through you'd have to fix up nsIPlatformCharset and
nsICharsetConverterManager (or switch to using nsICharsetConverterManager2?).

Comment 16

[kill this account]

Since it is an out parameter and we expect the function to modify it would
a nsAWritable string be more declarative here?

  +extern nsresult ConvertStringToUnicode(..., nsAString& outString);

Comment 17

[Roy Yokoyama]

Comment on attachment 55090 [details] [diff] [review]
Adding ASSERT for GetPlatformCharset(),ConvertStringToUnicode()

post a new patch

Comment 18

[Roy Yokoyama]

Attachment: Use nsString and nsAWritableString in formal parameter lists

Comment 19

[Roy Yokoyama]

Comment on attachment 55757 [details] [diff] [review]
Use nsString and nsAWritableString in formal parameter lists

carry over r=ccarlen.
brendan: can you /sr= ?

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 55757 [details] [diff] [review]
Use nsString and nsAWritableString in formal parameter lists

Do you need to test 'if (cmdResult)' given the successful return values?

Don't add tabs:

+    if (cmdResult) {
+		  foundProfileCommandArg = PR_TRUE;
+
+      // get a platform charset

Lot of reformatting -- are you converting the file to two-space indentation?

See the Emacs modeline at the top, and notice the matching indentation (if
you treat tabs as four-space equivalent).

Either stick with 4-space for now and convert to 2-space
later, or do it all now.  How about a diff -wu for easier reviewing, in
addition to diff -u?

/be

Comment 21

[Roy Yokoyama]

Attachment: Removing reformating.

Comment 22

[Roy Yokoyama]

brendan: i didn't realize the Emacs modeline saids 4-space.
         Removing the reformat.

>Do you need to test 'if (cmdResult)' given the successful return values?
 'rv = ProfileExists(currProfileName.get(), &exists);'
 and 
 'rv = CreateNewProfile(currProfileName.get(), currProfilePath, 
            nsnull, PR_TRUE);'
 test the validity of the cmdResult. 

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 55785 [details] [diff] [review]
Removing reformating.

Ok, thanks.  Some frown on strtok, but I'm old-school.  Other threads
can't safely use it, no big loss.

/b

Comment 24

[Roy Yokoyama]

checked into the trunk

Comment 25

[Rui Xu]

Verified with build 2001-11-16 trunk.
The DBCS handling has been fixed! Still can repro with some unicode chars and need more fix on unicode. 
Reopen.

Comment 26

[Roy Yokoyama]

moving to Unicode Milestone.

Comment 27

[nhottanscp]

nsbeta1-
 the original problem is fixed, remaining issue is for non default locale support

Comment 28

[Rui Xu]

This bug also happened with CHT chars on CHT WinNT/Win2K, and KOR chars on KOR 
WinNT/Win2K.

Comment 29

[nhottanscp]

In comment #25,
>The DBCS handling has been fixed! Still can repro with some unicode chars and
need >more fix on unicode. 
Could you elaborate what is fixed and what is the remaining problem?

Comment 30

[Rui Xu]

From technical point, any char in locale default DBCS can be handled after this 
fix, and any char outside locale default DBCS still cannot be handled.

From user point, even if using double-byte chars with the system default 
standard Windows IME on a CHT or KOR WinNT/2K/XP, user cannot use certain 
double-byte chars on our product since we don't fully support Unicode but system 
IME and OS support Unicode.

Comment 31

[Roy Yokoyama]

Mozilla can't support char outside of its Native CodePage
unless we convert Moz to an Unicode app. ( calling W API won't help )
I believe those CHT chars on CHT WinNT/Win2K are not in the CHT code page.  

Comment 32

[Roy Yokoyama]

something went wrong.  Summary has changed... don't know why.

Comment 33

[Rui Xu]

I agree with Roy's comments. we really need Moz as an Unicode app.

Comment 34

[Rui Xu]

->nsbeta1

Comment 35

[Frank Tang]

nsbeta1- per triage meeting

Comment 36

[Rui Xu]

Can repro this bug with US N6.2.1 on SC WinXP under SC codepage.

Comment 37

[Frank Tang]

nsbeta1+ for m1.2final release.

Comment 38

[Roy Yokoyama]

changing summary to reflect the remaining issue

Comment 39

[Roy Yokoyama]

ftang: Unless we change ( //xpfe/bootstrap/nsAppRunner.cpp )
           int main(int argc, char* argv[])
       to 
           int wmain(int argc, wchar_t *argv[ ], wchar_t *envp[ ]) // MS specific
       We won't be able to fix this problem.

       As it stands right now, we _can_ support the native Profile name thru -P 
       the CmdLine argument; but unless we do the above, we are stuck.

I need your and others input for this. 

Comment 40

[Roy Yokoyama]

correction: "we _can_ support" -> "we _are_ supporting"

Comment 41

[timeless]

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/getcommandline.asp
GetCommandLine

The GetCommandLine function retrieves the command-line string for the current
process.

LPTSTR GetCommandLine(void);

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/commandlinetoargvw.asp
CommandLineToArgvW

The CommandLineToArgvW function parses a Unicode command-line string. It returns
a pointer to a set of Unicode argument strings and a count of arguments, similar
to the standard C run-time argv and argc values. The function provides a way to
obtain a Unicode set of argv and argc values from a Unicode command-line string.

LPWSTR* CommandLineToArgvW(
  LPCWSTR lpCmdLine,
  int* pNumArgs
);

Comment 42

[Roy Yokoyama]

bulk milestone change

Comment 43

[Roy Yokoyama]

timeless: thanks, I'll see what I can do with it.

Comment 44

[Samir Gehani]

i18n triage team: nsbeta1-

Comment 45

[Simon Montagu :smontagu]

Does this bug still exist? Seems like it should have been fixed by bug 396052.

Comment 46

[Makoto Kato [:m_kato]]

Yes.  this is already fixed at past year.  This is MacOS 9 issue, not MacOS X.

Comment 47

[Makoto Kato [:m_kato]]

Opps. this isn't MacOS.  But I think that this is already fixed by bug 396052.

Comment 48

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 49

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has severity 'major'.
:m_kato, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 50

[Makoto Kato [:m_kato]]

per comment #47