Open Bug 104305 Opened 20 years ago Updated 3 years ago

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

Categories

(Core :: Internationalization, defect, P3)

x86
Windows XP
defect

Tracking

()

ASSIGNED
mozilla1.3alpha

People

(Reporter: ruixu, Assigned: tetsuroy)

References

Details

(Keywords: intl)

Attachments

(1 file, 4 obsolete files)

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.
Keywords: intl
QA Contact: andreasb → ruixu
Blocks: 101606
m0.9.6 P2
Priority: -- → P2
Target Milestone: --- → mozilla0.9.6
Blocks: 104500
Status: NEW → ASSIGNED
*** Bug 101581 has been marked as a duplicate of this bug. ***
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. 

Please check the result of ConvertStringToUnicode.
I think this should be reviewed by module owner (ccarlen?).
Attachment #54821 - Attachment is obsolete: true
Attachment #54820 - Attachment is obsolete: true
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?
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. 
Comment on attachment 55090 [details] [diff] [review]
Adding ASSERT for GetPlatformCharset(),ConvertStringToUnicode()

r=ccarlen
Attachment #55090 - Flags: review+
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
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?).
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 on attachment 55090 [details] [diff] [review]
Adding ASSERT for GetPlatformCharset(),ConvertStringToUnicode()

post a new patch
Attachment #55090 - Attachment is obsolete: true
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
Attachment #55757 - Attachment is obsolete: true
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. 
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+
checked into the trunk
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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 → ---
moving to Unicode Milestone.
Status: REOPENED → ASSIGNED
Target Milestone: mozilla0.9.6 → mozilla1.1
Keywords: nsbeta1
nsbeta1-
 the original problem is fixed, remaining issue is for non default locale support
Keywords: nsbeta1nsbeta1-
This bug also happened with CHT chars on CHT WinNT/Win2K, and KOR chars on KOR 
WinNT/Win2K.
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?
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.
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=
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
I agree with Roy's comments. we really need Moz as an Unicode app.
->nsbeta1
Keywords: nsbeta1-nsbeta1
nsbeta1- per triage meeting
Keywords: nsbeta1nsbeta1-
Can repro this bug with US N6.2.1 on SC WinXP under SC codepage.
OS: Windows 2000 → Windows XP
Blocks: 157673
nsbeta1+ for m1.2final release.
Keywords: nsbeta1-nsbeta1+
Whiteboard: [eta:8/16/2002]
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
Depends on: 166735
Target Milestone: mozilla1.2alpha → mozilla1.2beta
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
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]
correction: "we _can_ support" -> "we _are_ supporting"
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
);
bulk milestone change
Target Milestone: mozilla1.2beta → mozilla1.3alpha
timeless: thanks, I'll see what I can do with it.
i18n triage team: nsbeta1-
Keywords: nsbeta1+nsbeta1-
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.
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
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.