Last Comment Bug 95282 - Implement nsICollation interface on MacOS X by using Unicode Utility
: Implement nsICollation interface on MacOS X by using Unicode Utility
Status: RESOLVED FIXED
MacOS X
:
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: PowerPC Mac OS X
: P4 normal (vote)
: mozilla0.9.6
Assigned To: Frank Tang
: Frank Tang
: Makoto Kato [:m_kato]
Mentors:
: 21066 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-08-14 14:22 PDT by Frank Tang
Modified: 2001-10-16 12:33 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
intl/locale/src/mac/nsCollationMacUC.cpp (7.09 KB, text/plain)
2001-08-17 16:56 PDT, Frank Tang
no flags Details
intl/locale/src/mac/nsCollationMacUC.cpp (2.37 KB, text/plain)
2001-08-17 16:57 PDT, Frank Tang
no flags Details
use the UC version on Carbon build (1.10 KB, patch)
2001-08-17 16:59 PDT, Frank Tang
no flags Details | Diff | Splinter Review
v2 of nsCollationMacUC.h (3.32 KB, text/plain)
2001-10-11 17:21 PDT, Frank Tang
no flags Details
v2 of nsCollationMacUC.cpp (8.13 KB, text/plain)
2001-10-11 17:22 PDT, Frank Tang
nhottanscp: review+
Details
v2 of patch (1.61 KB, patch)
2001-10-11 17:22 PDT, Frank Tang
no flags Details | Diff | Splinter Review

Description Frank Tang 2001-08-14 14:22:20 PDT
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
Comment 1 Frank Tang 2001-08-17 16:56:55 PDT
Created attachment 46293 [details]
intl/locale/src/mac/nsCollationMacUC.cpp
Comment 2 Frank Tang 2001-08-17 16:57:08 PDT
Created attachment 46294 [details]
intl/locale/src/mac/nsCollationMacUC.cpp
Comment 3 Frank Tang 2001-08-17 16:58:45 PDT
The 2nd attachment is   intl/locale/src/mac/nsCollationMacUC.h not .cpp
Comment 4 Frank Tang 2001-08-17 16:59:14 PDT
Created attachment 46295 [details] [diff] [review]
use the UC version on Carbon build
Comment 5 Frank Tang 2001-08-17 17:00:45 PDT
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 Mike Pinkerton (not reading bugmail) 2001-08-20 10:10:01 PDT
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?
Comment 7 Frank Tang 2001-08-20 14:02:14 PDT
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.
Comment 8 Frank Tang 2001-08-21 15:36:20 PDT
MacOS X run without any problem. pinkerton- can you code review this one so I
can land it for m94?
Comment 9 Frank Tang 2001-08-21 17:24:01 PDT
*** Bug 21066 has been marked as a duplicate of this bug. ***
Comment 10 msanz 2001-09-10 22:23:03 PDT
nsbranch- since Frank moved it to 0.9.5
Comment 11 Frank Tang 2001-10-08 10:38:49 PDT
nhotta, can you r= this one ?
Comment 12 nhottanscp 2001-10-08 13:08:03 PDT
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.

Comment 13 Frank Tang 2001-10-11 17:21:54 PDT
Created attachment 53192 [details]
v2 of nsCollationMacUC.h
Comment 14 Frank Tang 2001-10-11 17:22:33 PDT
Created attachment 53193 [details]
v2 of nsCollationMacUC.cpp
Comment 15 Frank Tang 2001-10-11 17:22:55 PDT
Created attachment 53194 [details] [diff] [review]
v2 of patch
Comment 16 Frank Tang 2001-10-11 17:23:22 PDT
nhotta, can you r= this one ?
Comment 17 nhottanscp 2001-10-11 17:37:48 PDT
Comment on attachment 53193 [details]
v2 of nsCollationMacUC.cpp

r=nhotta for this and other v2 patches.
Comment 18 Simon Fraser 2001-10-12 14:17:29 PDT
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
Comment 19 Frank Tang 2001-10-12 17:14:24 PDT
>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
Comment 20 Frank Tang 2001-10-16 05:38:29 PDT
fixed and check in 
Comment 21 Andreas Becker 2001-10-16 12:33:49 PDT
Switching qa contact to ftang for now. Frank, can this be verified within 
development or can you provide QA with a test case? Thanks.

Note You need to log in before you can comment on or make changes to this bug.