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

ASSIGNED
Assigned to

Status

()

P2
major
ASSIGNED
17 years ago
10 years ago

People

(Reporter: ruixu, Assigned: tetsuroy)

Tracking

({intl})

Trunk
mozilla1.3alpha
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Updated

17 years ago
Keywords: intl
QA Contact: andreasb → ruixu
(Reporter)

Updated

17 years ago
Blocks: 101606

Comment 1

17 years ago
m0.9.6 P2
Priority: -- → P2
Target Milestone: --- → mozilla0.9.6

Updated

17 years ago
Blocks: 104500
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

17 years ago
Created attachment 54820 [details] [diff] [review]
Convert commandline (platform locale) to Unicode + indentation fix
(Assignee)

Comment 3

17 years ago
*** Bug 101581 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 4

17 years ago
Created attachment 54821 [details] [diff] [review]
Same as previous patch excluding the indentation to improve the readability
(Assignee)

Comment 5

17 years ago
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

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

Updated

17 years ago
Attachment #54821 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #54820 - Attachment is obsolete: true
(Assignee)

Comment 7

17 years ago
Created attachment 55090 [details] [diff] [review]
Adding ASSERT for GetPlatformCharset(),ConvertStringToUnicode()
(Assignee)

Comment 8

17 years ago
ccarlen:  can you review?
======= ccing ccarlen@netscape.com
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?
(Assignee)

Comment 10

17 years ago
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.
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. 
(Assignee)

Comment 12

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

r=ccarlen
Attachment #55090 - Flags: review+
(Assignee)

Comment 13

17 years ago
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
Whiteboard: Need /sr=
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

17 years ago
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

17 years ago
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);
(Assignee)

Comment 17

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

post a new patch
Attachment #55090 - Attachment is obsolete: true
(Assignee)

Comment 18

17 years ago
Created attachment 55757 [details] [diff] [review]
Use nsString and nsAWritableString in formal parameter lists
(Assignee)

Comment 19

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

carry over r=ccarlen.
brendan: can you /sr= ?
Attachment #55757 - Flags: review+
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
(Assignee)

Comment 21

17 years ago
Created attachment 55785 [details] [diff] [review]
Removing reformating.
(Assignee)

Updated

17 years ago
Attachment #55757 - Attachment is obsolete: true
(Assignee)

Comment 22

17 years ago
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. 
(Assignee)

Updated

17 years ago
Blocks: 107664
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
Attachment #55785 - Flags: superreview+
(Assignee)

Comment 24

17 years ago
checked into the trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 25

17 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 26

17 years ago
moving to Unicode Milestone.
Status: REOPENED → ASSIGNED
Target Milestone: mozilla0.9.6 → mozilla1.1
(Reporter)

Updated

17 years ago
Keywords: nsbeta1

Comment 27

17 years ago
nsbeta1-
 the original problem is fixed, remaining issue is for non default locale support
Keywords: nsbeta1 → nsbeta1-
(Reporter)

Comment 28

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

Comment 29

17 years ago
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?
(Reporter)

Comment 30

17 years ago
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.
(Assignee)

Comment 31

17 years ago
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.  


Blocks: 86948, 104166
No longer blocks: 107664
QA Contact: ruixu → ylong
Summary: P option doesn't work with double-byte char profile names → Files are not opened, if the path or file name contains some high-ascii or double-byte characters
Whiteboard: Need /sr=
(Assignee)

Comment 32

17 years ago
something went wrong.  Summary has changed... don't know why.
QA Contact: ylong → ruixu
Summary: Files are not opened, if the path or file name contains some high-ascii or double-byte characters → P option doesn't work with double-byte char profile names
(Reporter)

Comment 33

17 years ago
I agree with Roy's comments. we really need Moz as an Unicode app.
(Reporter)

Comment 34

17 years ago
->nsbeta1
Keywords: nsbeta1- → nsbeta1

Comment 35

17 years ago
nsbeta1- per triage meeting
Keywords: nsbeta1 → nsbeta1-
(Reporter)

Comment 36

17 years ago
Can repro this bug with US N6.2.1 on SC WinXP under SC codepage.
OS: Windows 2000 → Windows XP

Updated

17 years ago
Blocks: 157673

Comment 37

17 years ago
nsbeta1+ for m1.2final release.
Keywords: nsbeta1- → nsbeta1+
Whiteboard: [eta:8/16/2002]
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
(Assignee)

Updated

16 years ago
Depends on: 166735
(Assignee)

Updated

16 years ago
Target Milestone: mozilla1.2alpha → mozilla1.2beta
(Assignee)

Comment 38

16 years ago
changing summary to reflect the remaining issue
Summary: P option doesn't work with double-byte char profile names → P option doesn't work with non-Native char profile names
(Assignee)

Comment 39

16 years ago
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. 
Whiteboard: [eta:8/16/2002]
(Assignee)

Comment 40

16 years ago
correction: "we _can_ support" -> "we _are_ supporting"

Comment 41

16 years ago
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
);
(Assignee)

Comment 42

16 years ago
bulk milestone change
Target Milestone: mozilla1.2beta → mozilla1.3alpha
(Assignee)

Comment 43

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

Comment 44

16 years ago
i18n triage team: nsbeta1-
Keywords: nsbeta1+ → nsbeta1-

Updated

10 years ago
Severity: critical → major
QA Contact: ruixu → i18n
Summary: P option doesn't work with non-Native char profile names → -P commandline option doesn't work with non-Native char profile names
Does this bug still exist? Seems like it should have been fixed by bug 396052.
Yes.  this is already fixed at past year.  This is MacOS 9 issue, not MacOS X.
Opps. this isn't MacOS.  But I think that this is already fixed by bug 396052.
You need to log in before you can comment on or make changes to this bug.