Closed
Bug 95282
Opened 23 years ago
Closed 23 years ago
Implement nsICollation interface on MacOS X by using Unicode Utility
Categories
(Core :: Internationalization, defect, P4)
Tracking
()
RESOLVED
FIXED
mozilla0.9.6
People
(Reporter: ftang, Assigned: ftang)
References
Details
(Whiteboard: MacOS X)
Attachments
(3 files, 3 obsolete files)
Our current implementation of nsICollation is not unicode based (See /intl/locale/src/mac/nsCollationMac.cpp /intl/locale/src/mac/nsCollationMac.h ) we should use the following APIs in the Unicode Utilities (see http://developer.apple.com/techpubs/macosx/Carbon/text/UnicodeUtilities/Unicode_ Utilities/index.html ) to implement them on MacOS X We should implement NS_IMETHOD Initialize(nsILocale* locale) by calling UCCreateCollator We should implment NS_IMETHOD CompareString(const nsCollationStrength strength, const nsString& string1, const nsString& string2, PRInt32* result) = 0; by using UCCompareText implement NS_IMETHOD GetSortKeyLen(const nsCollationStrength strength, const nsString& stringIn, PRUint32* outLen) = 0; and NS_IMETHOD CreateRawSortKey(const nsCollationStrength strength, const nsString& stringIn, PRUint8* key, PRUint32 *outLen) = 0; by calling UCGetCollationKey implement the destructor by calling UCDisposeCollator Implment NS_IMETHOD CompareRawSortKey(const PRUint8* key1, const PRUint32 len1, const PRUint8* key2, const PRUint32 len2, PRInt32* result) = 0; by calling UCCompareCollationKeys
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Whiteboard: MacOS X
Assignee | ||
Comment 1•23 years ago
|
||
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 3•23 years ago
|
||
The 2nd attachment is intl/locale/src/mac/nsCollationMacUC.h not .cpp
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 years ago
|
||
The patch work. However, I encounter bad crash (with corrupt stacktrack) with this patch while I select View:Character Set:Auto-Detect menu items. Don't check in untill we find out why it crash. I am sure it is related to this patch since I cannot crash without it.
Comment 6•23 years ago
|
||
frank, there is a known bug with carbonlib prior to 1.4 where if we make any changes to a submenu via the onCreate handler (thus causing the menu to be marked 'dirty' and rebuilt) then it corrupts the heap when the menu goes away. Could this be what you're running into? Are you seeing this on fizzilla running on os9 or only on osx?
Assignee | ||
Comment 7•23 years ago
|
||
I see the crash on MacOS 9, I have not try MacOS X yet. I will do it today. Thanks for your info. It sound's related.
Assignee | ||
Comment 8•23 years ago
|
||
MacOS X run without any problem. pinkerton- can you code review this one so I can land it for m94?
Assignee | ||
Updated•23 years ago
|
Comment 10•23 years ago
|
||
nsbranch- since Frank moved it to 0.9.5
Assignee | ||
Comment 11•23 years ago
|
||
nhotta, can you r= this one ?
Comment 12•23 years ago
|
||
1) In nsCollationMacUC::StrengthToOptions, avoid assign to *aOptions twice. Change as below then the initial assignment would not be needed. switch( aStrength) { case kCollationCaseSensitive: default: *aOptions = kUCCollateStandardOptions; break; 2) In nsCollationMacUC::ConvertLocale, Change to use NS_LITERAL_STRING for "NSILOCALE_COLLATE", collate.AssignWithConversion("NSILOCALE_COLLATE"); nsresult res = aNSLocale->GetCategory(collate.get(), &aLocaleString); 3) In nsCollationMacUC::GetSortKeyLen, // According to the document, the timension should typically be // at least 5 * textLength What is 'timension'? Also please coordinate with the nsString -> nsAString change (bug 103222) if possible.
No longer blocks: 103669
Assignee | ||
Updated•23 years ago
|
Attachment #46293 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #46294 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #46295 -
Attachment is obsolete: true
Assignee | ||
Comment 13•23 years ago
|
||
Assignee | ||
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 years ago
|
||
Assignee | ||
Comment 16•23 years ago
|
||
nhotta, can you r= this one ?
Comment 17•23 years ago
|
||
Comment on attachment 53193 [details]
v2 of nsCollationMacUC.cpp
r=nhotta for this and other v2 patches.
Attachment #53193 -
Flags: review+
Comment 18•23 years ago
|
||
Comments: class nsCollationMacUC : public nsICollation { ... nsCollationMacUC(); virtual ~nsCollationMacUC(); Minor nit: i prefer to see ctor and dtor first, before other interface methods. private: PRBool mInit; PRBool mHasCollator; Minor nit: use PRPackedBool (8 bits, not 32) to possibly save space. nsCollationMacUC::nsCollationMacUC() { NS_INIT_REFCNT(); mInit = PR_FALSE; mHasCollator = PR_FALSE; } Why do you need 'mHasCollator'? Isn't checking mCollator for null good enough? Minor nit: prefer initializors: nsCollationMacUC::nsCollationMacUC() : mInit(PR_FALSE) , mHasCollator(PR_FALSE) { NS_INIT_REFCNT(); } Messy: nsAutoString collate; PRUnichar* aLocaleString = nsnull; nsresult res = aNSLocale-> GetCategory(NS_LITERAL_STRING("NSILOCALE_COLLATE").get(), &aLocaleString); NS_ENSURE_SUCCESS(res, res); NS_ENSURE_ARG_POINTER(aLocaleString); nsCAutoString tmp; tmp.AssignWithConversion(aLocaleString); nsMemory::Free(aLocaleString); nsCAutoString changed; tmp.ReplaceChar('-', '_'); OSStatus err; err = ::LocaleRefFromLocaleString( tmp.get(), aMacLocale); NS_ENSURE_TRUE((err == noErr), NS_ERROR_FAILURE); here, PRUnichar* aLocaleString = nsnull; Use nsXPIDLString and getter_Copies. nsCAutoString changed; This is never used. Watch the compiler warnings! NS_ENSURE_TRUE((err == noErr), NS_ERROR_FAILURE); is this really clearer than: if (err != noErr) return NS_ERROR_FAILURE ? It's certainly harder to put breakpoints in. I recommend only using NS_ENSURE macros for argument checking at the top of a function. NS_ENSURE_TRUE(mInit, NS_ERROR_FAILURE); NS_ERROR_NOT_INITIALIZED? if (mHasCollator) { err = ::UCDisposeCollator( &mCollator ); mHasCollator = PR_FALSE; See comment above about mHasCollator. NS_ENSURE_TRUE((!mInit), NS_ERROR_FAILURE); NS_ERROR_ALREADY_INITIALIZED? // According to the document, the lenght of the key should typically be typo. Fix those, and sr=sfraser
Assignee | ||
Comment 19•23 years ago
|
||
>Why do you need 'mHasCollator'? Isn't checking mCollator for null good enough?
You are assuming mCollator (which is in type CollatorRef) is a pointer and nsnull
mean we don't have it. However, I cannot find any documentation say 0 value in a
CollatorRef mean it is an illegal value so I am not dare to make such assumption
Assignee | ||
Updated•23 years ago
|
Assignee | ||
Updated•23 years ago
|
Assignee | ||
Comment 20•23 years ago
|
||
fixed and check in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 21•23 years ago
|
||
Switching qa contact to ftang for now. Frank, can this be verified within development or can you provide QA with a test case? Thanks.
QA Contact: andreasb → ftang
You need to log in
before you can comment on or make changes to this bug.
Description
•