95282 - Implement nsICollation interface on MacOS X by using Unicode Utility

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P4)

Description

[Frank Tang]

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]

Attachment: intl/locale/src/mac/nsCollationMacUC.cpp

Comment 2

[Frank Tang]

Attachment: intl/locale/src/mac/nsCollationMacUC.cpp

Comment 3

[Frank Tang]

The 2nd attachment is   intl/locale/src/mac/nsCollationMacUC.h not .cpp

Comment 4

[Frank Tang]

Attachment: use the UC version on Carbon build

Comment 5

[Frank Tang]

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)]

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]

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]

MacOS X run without any problem. pinkerton- can you code review this one so I
can land it for m94?

Comment 9

[Frank Tang]

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

Comment 10

[msanz]

nsbranch- since Frank moved it to 0.9.5

Comment 11

[Frank Tang]

nhotta, can you r= this one ?

Comment 12

[nhottanscp]

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]

Attachment: v2 of nsCollationMacUC.h

Comment 14

[Frank Tang]

Attachment: v2 of nsCollationMacUC.cpp

Comment 15

[Frank Tang]

Attachment: v2 of patch

Comment 16

[Frank Tang]

nhotta, can you r= this one ?

Comment 17

[nhottanscp]

Comment on attachment 53193 [details]
v2 of nsCollationMacUC.cpp

r=nhotta for this and other v2 patches.

Comment 18

[Simon Fraser [no longer active]]

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]

>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]

fixed and check in 

Comment 21

[Andreas Becker]

Switching qa contact to ftang for now. Frank, can this be verified within 
development or can you provide QA with a test case? Thanks.